| Message ID | 157262961597.2838.16953618909905259198.stgit@naples-babu.amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Emulate and enable UMIP feature on AMD | expand |
On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote: > > The UMIP (User-Mode Instruction Prevention) feature bit should be > set if the emulation (kvm_x86_ops->umip_emulated) is supported > which is done already. > > Remove the unconditional setting of this bit. > > Fixes: ae3e61e1c28338d0 ("KVM: x86: add support for UMIP") > > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > arch/x86/kvm/cpuid.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index f68c0c753c38..5b81ba5ad428 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -364,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index) > /* cpuid 7.0.ecx*/ > const u32 kvm_cpuid_7_0_ecx_x86_features = > F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | > - F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | > + F(AVX512_VPOPCNTDQ) | F(AVX512_VBMI2) | F(GFNI) | > F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | > F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/; > This isn't unconditional. This is masked by the features on the boot CPU. Since UMIP can be virtualized (without emulation) on CPUs that support UMIP, you should leave this alone.
On 11/1/19 1:35 PM, Jim Mattson wrote: > On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote: >> >> The UMIP (User-Mode Instruction Prevention) feature bit should be >> set if the emulation (kvm_x86_ops->umip_emulated) is supported >> which is done already. >> >> Remove the unconditional setting of this bit. >> >> Fixes: ae3e61e1c28338d0 ("KVM: x86: add support for UMIP") >> >> Signed-off-by: Babu Moger <babu.moger@amd.com> >> --- >> arch/x86/kvm/cpuid.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >> index f68c0c753c38..5b81ba5ad428 100644 >> --- a/arch/x86/kvm/cpuid.c >> +++ b/arch/x86/kvm/cpuid.c >> @@ -364,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index) >> /* cpuid 7.0.ecx*/ >> const u32 kvm_cpuid_7_0_ecx_x86_features = >> F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | >> - F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | >> + F(AVX512_VPOPCNTDQ) | F(AVX512_VBMI2) | F(GFNI) | >> F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | >> F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/; >> > > This isn't unconditional. This is masked by the features on the boot > CPU. Since UMIP can be virtualized (without emulation) on CPUs that > support UMIP, you should leave this alone. > There is some inconstancy here. static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0; ... case 7: { .. entry->ecx |= f_umip; .. } and static bool svm_umip_emulated(void) { return false; } static inline bool vmx_umip_emulated(void) { return vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_DESC; } It appears that intention was to enable the UMIP if SVM or VMX support umip emulation. But that is not how it is working now. Now it is enabled if boot CPU supports UMIP.
On Fri, Nov 1, 2019 at 12:39 PM Moger, Babu <Babu.Moger@amd.com> wrote: > > > > On 11/1/19 1:35 PM, Jim Mattson wrote: > > On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote: > >> > >> The UMIP (User-Mode Instruction Prevention) feature bit should be > >> set if the emulation (kvm_x86_ops->umip_emulated) is supported > >> which is done already. > >> > >> Remove the unconditional setting of this bit. > >> > >> Fixes: ae3e61e1c28338d0 ("KVM: x86: add support for UMIP") > >> > >> Signed-off-by: Babu Moger <babu.moger@amd.com> > >> --- > >> arch/x86/kvm/cpuid.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > >> index f68c0c753c38..5b81ba5ad428 100644 > >> --- a/arch/x86/kvm/cpuid.c > >> +++ b/arch/x86/kvm/cpuid.c > >> @@ -364,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index) > >> /* cpuid 7.0.ecx*/ > >> const u32 kvm_cpuid_7_0_ecx_x86_features = > >> F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | > >> - F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | > >> + F(AVX512_VPOPCNTDQ) | F(AVX512_VBMI2) | F(GFNI) | > >> F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | > >> F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/; > >> > > > > This isn't unconditional. This is masked by the features on the boot > > CPU. Since UMIP can be virtualized (without emulation) on CPUs that > > support UMIP, you should leave this alone. > > > > There is some inconstancy here. > > static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 > > unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0; > ... > > case 7: { > .. > entry->ecx |= f_umip; > .. > } > > and > static bool svm_umip_emulated(void) > { > return false; > } > > static inline bool vmx_umip_emulated(void) > { > return vmcs_config.cpu_based_2nd_exec_ctrl & > SECONDARY_EXEC_DESC; > } > > > It appears that intention was to enable the UMIP if SVM or VMX support > umip emulation. But that is not how it is working now. Now it is enabled > if boot CPU supports UMIP. No. The code enumerates UMIP in the guest if *either* it can be virtualized (i.e. the host supports it natively), *or* it can be emulated.
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index f68c0c753c38..5b81ba5ad428 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -364,7 +364,7 @@ static inline void do_cpuid_7_mask(struct kvm_cpuid_entry2 *entry, int index) /* cpuid 7.0.ecx*/ const u32 kvm_cpuid_7_0_ecx_x86_features = F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ | F(RDPID) | - F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) | + F(AVX512_VPOPCNTDQ) | F(AVX512_VBMI2) | F(GFNI) | F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) | F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/;
The UMIP (User-Mode Instruction Prevention) feature bit should be set if the emulation (kvm_x86_ops->umip_emulated) is supported which is done already. Remove the unconditional setting of this bit. Fixes: ae3e61e1c28338d0 ("KVM: x86: add support for UMIP") Signed-off-by: Babu Moger <babu.moger@amd.com> --- arch/x86/kvm/cpuid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)