| Message ID | 158923999440.20128.4859351750654993810.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arch/x86: Enable MPK feature on AMD | expand |
On Mon, May 11, 2020 at 4:33 PM Babu Moger <babu.moger@amd.com> wrote: > > Both Intel and AMD support (MPK) Memory Protection Key feature. > Move the feature detection from VMX to the common code. It should > work for both the platforms now. > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > arch/x86/kvm/cpuid.c | 4 +++- > arch/x86/kvm/vmx/vmx.c | 4 ---- > 2 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 901cd1fdecd9..3da7d6ea7574 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -278,6 +278,8 @@ void kvm_set_cpu_caps(void) > #ifdef CONFIG_X86_64 > unsigned int f_gbpages = F(GBPAGES); > unsigned int f_lm = F(LM); > + /* PKU is not yet implemented for shadow paging. */ > + unsigned int f_pku = tdp_enabled ? F(PKU) : 0; I think we still want to require that OSPKE be set on the host before exposing PKU to the guest.
On 5/11/20 6:51 PM, Jim Mattson wrote: > On Mon, May 11, 2020 at 4:33 PM Babu Moger <babu.moger@amd.com> wrote: >> >> Both Intel and AMD support (MPK) Memory Protection Key feature. >> Move the feature detection from VMX to the common code. It should >> work for both the platforms now. >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> arch/x86/kvm/cpuid.c | 4 +++- >> arch/x86/kvm/vmx/vmx.c | 4 ---- >> 2 files changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index 901cd1fdecd9..3da7d6ea7574 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -278,6 +278,8 @@ void kvm_set_cpu_caps(void) >> #ifdef CONFIG_X86_64 >> unsigned int f_gbpages = F(GBPAGES); >> unsigned int f_lm = F(LM); >> + /* PKU is not yet implemented for shadow paging. */ >> + unsigned int f_pku = tdp_enabled ? F(PKU) : 0; > > I think we still want to require that OSPKE be set on the host before > exposing PKU to the guest. > Ok I can add this check. + unsigned int f_pku = tdp_enabled && F(OSPKE)? F(PKU) : 0;
On Tue, May 12, 2020 at 8:12 AM Babu Moger <babu.moger@amd.com> wrote: > > > > On 5/11/20 6:51 PM, Jim Mattson wrote: > > On Mon, May 11, 2020 at 4:33 PM Babu Moger <babu.moger@amd.com> wrote: > >> > >> Both Intel and AMD support (MPK) Memory Protection Key feature. > >> Move the feature detection from VMX to the common code. It should > >> work for both the platforms now. > >> > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > >> arch/x86/kvm/cpuid.c | 4 +++- > >> arch/x86/kvm/vmx/vmx.c | 4 ---- > >> 2 files changed, 3 insertions(+), 5 deletions(-) > >> > >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > >> index 901cd1fdecd9..3da7d6ea7574 100644 > >> --- a/arch/x86/kvm/cpuid.c > >> +++ b/arch/x86/kvm/cpuid.c > >> @@ -278,6 +278,8 @@ void kvm_set_cpu_caps(void) > >> #ifdef CONFIG_X86_64 > >> unsigned int f_gbpages = F(GBPAGES); > >> unsigned int f_lm = F(LM); > >> + /* PKU is not yet implemented for shadow paging. */ > >> + unsigned int f_pku = tdp_enabled ? F(PKU) : 0; > > > > I think we still want to require that OSPKE be set on the host before > > exposing PKU to the guest. > > > > Ok I can add this check. > > + unsigned int f_pku = tdp_enabled && F(OSPKE)? F(PKU) : 0; That doesn't do what you think it does. F(OSPKE) is a non-zero constant, so that conjunct is always true.
On Tue, May 12, 2020 at 09:58:19AM -0700, Jim Mattson wrote: > On Tue, May 12, 2020 at 8:12 AM Babu Moger <babu.moger@amd.com> wrote: > > > > > > > > On 5/11/20 6:51 PM, Jim Mattson wrote: > > > On Mon, May 11, 2020 at 4:33 PM Babu Moger <babu.moger@amd.com> wrote: > > >> > > >> Both Intel and AMD support (MPK) Memory Protection Key feature. > > >> Move the feature detection from VMX to the common code. It should > > >> work for both the platforms now. > > >> > > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > > >> --- > > >> arch/x86/kvm/cpuid.c | 4 +++- > > >> arch/x86/kvm/vmx/vmx.c | 4 ---- > > >> 2 files changed, 3 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > >> index 901cd1fdecd9..3da7d6ea7574 100644 > > >> --- a/arch/x86/kvm/cpuid.c > > >> +++ b/arch/x86/kvm/cpuid.c > > >> @@ -278,6 +278,8 @@ void kvm_set_cpu_caps(void) > > >> #ifdef CONFIG_X86_64 > > >> unsigned int f_gbpages = F(GBPAGES); > > >> unsigned int f_lm = F(LM); > > >> + /* PKU is not yet implemented for shadow paging. */ > > >> + unsigned int f_pku = tdp_enabled ? F(PKU) : 0; > > > > > > I think we still want to require that OSPKE be set on the host before > > > exposing PKU to the guest. > > > > > > > Ok I can add this check. > > > > + unsigned int f_pku = tdp_enabled && F(OSPKE)? F(PKU) : 0; > > That doesn't do what you think it does. F(OSPKE) is a non-zero > constant, so that conjunct is always true. My vote would be to omit f_pku and adjust the cap directly, e.g. diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 6828be99b9083..998c902df9e57 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -326,7 +326,7 @@ void kvm_set_cpu_caps(void) ); kvm_cpu_cap_mask(CPUID_7_ECX, - F(AVX512VBMI) | F(LA57) | 0 /*PKU*/ | 0 /*OSPKE*/ | F(RDPID) | + F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/ @@ -334,6 +334,8 @@ void kvm_set_cpu_caps(void) /* Set LA57 based on hardware capability. */ if (cpuid_ecx(7) & F(LA57)) kvm_cpu_cap_set(X86_FEATURE_LA57); + if (!tdp_enabled || !boot_cpu_has(OSPKE)) + kvm_cpu_cap_clear(X86_FEATURE_PKU); kvm_cpu_cap_mask(CPUID_7_EDX, F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) |
On 5/12/20 12:28 PM, Sean Christopherson wrote: > On Tue, May 12, 2020 at 09:58:19AM -0700, Jim Mattson wrote: >> On Tue, May 12, 2020 at 8:12 AM Babu Moger <babu.moger@amd.com> wrote: >>> >>> >>> >>> On 5/11/20 6:51 PM, Jim Mattson wrote: >>>> On Mon, May 11, 2020 at 4:33 PM Babu Moger <babu.moger@amd.com> wrote: >>>>> >>>>> Both Intel and AMD support (MPK) Memory Protection Key feature. >>>>> Move the feature detection from VMX to the common code. It should >>>>> work for both the platforms now. >>>>> >>>>> Signed-off-by: Babu Moger <babu.moger@amd.com> >>>>> --- >>>>> arch/x86/kvm/cpuid.c | 4 +++- >>>>> arch/x86/kvm/vmx/vmx.c | 4 ---- >>>>> 2 files changed, 3 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>>>> index 901cd1fdecd9..3da7d6ea7574 100644 >>>>> --- a/arch/x86/kvm/cpuid.c >>>>> +++ b/arch/x86/kvm/cpuid.c >>>>> @@ -278,6 +278,8 @@ void kvm_set_cpu_caps(void) >>>>> #ifdef CONFIG_X86_64 >>>>> unsigned int f_gbpages = F(GBPAGES); >>>>> unsigned int f_lm = F(LM); >>>>> + /* PKU is not yet implemented for shadow paging. */ >>>>> + unsigned int f_pku = tdp_enabled ? F(PKU) : 0; >>>> >>>> I think we still want to require that OSPKE be set on the host before >>>> exposing PKU to the guest. >>>> >>> >>> Ok I can add this check. >>> >>> + unsigned int f_pku = tdp_enabled && F(OSPKE)? F(PKU) : 0; >> >> That doesn't do what you think it does. F(OSPKE) is a non-zero >> constant, so that conjunct is always true. > > My vote would be to omit f_pku and adjust the cap directly, e.g. Sure. I am fine with this. Thanks > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 6828be99b9083..998c902df9e57 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -326,7 +326,7 @@ void kvm_set_cpu_caps(void) > ); > > kvm_cpu_cap_mask(CPUID_7_ECX, > - F(AVX512VBMI) | F(LA57) | 0 /*PKU*/ | 0 /*OSPKE*/ | F(RDPID) | > + F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | > F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | > F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | > F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/ > @@ -334,6 +334,8 @@ void kvm_set_cpu_caps(void) > /* Set LA57 based on hardware capability. */ > if (cpuid_ecx(7) & F(LA57)) > kvm_cpu_cap_set(X86_FEATURE_LA57); > + if (!tdp_enabled || !boot_cpu_has(OSPKE)) > + kvm_cpu_cap_clear(X86_FEATURE_PKU); > > kvm_cpu_cap_mask(CPUID_7_EDX, > F(AVX512_4VNNIW) | F(AVX512_4FMAPS) | F(SPEC_CTRL) | >
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 901cd1fdecd9..3da7d6ea7574 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -278,6 +278,8 @@ void kvm_set_cpu_caps(void) #ifdef CONFIG_X86_64 unsigned int f_gbpages = F(GBPAGES); unsigned int f_lm = F(LM); + /* PKU is not yet implemented for shadow paging. */ + unsigned int f_pku = tdp_enabled ? F(PKU) : 0; #else unsigned int f_gbpages = 0; unsigned int f_lm = 0; @@ -326,7 +328,7 @@ void kvm_set_cpu_caps(void) ); kvm_cpu_cap_mask(CPUID_7_ECX, - F(AVX512VBMI) | F(LA57) | 0 /*PKU*/ | 0 /*OSPKE*/ | F(RDPID) | + F(AVX512VBMI) | F(LA57) | f_pku | 0 /*OSPKE*/ | F(RDPID) | F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/ diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 46898a476ba7..d153732ed88f 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7136,10 +7136,6 @@ static __init void vmx_set_cpu_caps(void) if (vmx_pt_mode_is_host_guest()) kvm_cpu_cap_check_and_set(X86_FEATURE_INTEL_PT); - /* PKU is not yet implemented for shadow paging. */ - if (enable_ept && boot_cpu_has(X86_FEATURE_OSPKE)) - kvm_cpu_cap_check_and_set(X86_FEATURE_PKU); - if (vmx_umip_emulated()) kvm_cpu_cap_set(X86_FEATURE_UMIP);
Both Intel and AMD support (MPK) Memory Protection Key feature. Move the feature detection from VMX to the common code. It should work for both the platforms now. Signed-off-by: Babu Moger <babu.moger@amd.com> --- arch/x86/kvm/cpuid.c | 4 +++- arch/x86/kvm/vmx/vmx.c | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-)