[PATCH 0/4] Emulate and enable UMIP feature on AMD

[PATCH 0/4] Emulate and enable UMIP feature on AMD

[Moger, Babu]

AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
Prevention) feature. Emulate and enable the UMIP feature if bare metal
supports it. Emulation of UMIP instructions (sgdt, sidt, sldt, smsw and
str) requires the hypervisor to read and write the guest memory. Guest
memory is encrypted on SEV guest. Hypervisor cannot successfully read or
write the guest memory. So disable emulation on SEV guest. Enable the
emulation only on non SEV guest.

Tested on EPYC/EPYC-Rome VMs and works as expected. Please review.

Babu Moger (4):
  kvm: x86: Dont set UMIP feature bit unconditionally
  kvm: svm: Enable UMIP feature on AMD
  kvm: svm: Emulate UMIP instructions on non SEV guest
  x86/Kconfig: Rename UMIP config parameter

 arch/x86/Kconfig                         |  8 +++----
 arch/x86/include/asm/disabled-features.h |  2 +-
 arch/x86/include/asm/umip.h              |  4 ++--
 arch/x86/kernel/Makefile                 |  2 +-
 arch/x86/kvm/cpuid.c                     |  2 +-
 arch/x86/kvm/svm.c                       | 30 ++++++++++++++++++++----
 6 files changed, 34 insertions(+), 14 deletions(-)

-- 
2.20.1

[PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally

[Moger, Babu]

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*/;
 

[PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
Prevention) feature. The UMIP feature prevents the execution of certain
instructions if the Current Privilege Level (CPL) is greater than 0.
If any of these instructions are executed with CPL > 0 and UMIP
is enabled, then kernel reports a #GP exception.

The idea is taken from articles:
https://lwn.net/Articles/738209/
https://lwn.net/Articles/694385/

Enable the feature if supported on bare metal and emulate instructions
to return dummy values for certain cases.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4153ca8cddb7..79abbdeca148 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
 {
 }
 
+static bool svm_umip_emulated(void)
+{
+	return boot_cpu_has(X86_FEATURE_UMIP);
+}
+
 static void update_cr0_intercept(struct vcpu_svm *svm)
 {
 	ulong gcr0 = svm->vcpu.arch.cr0;
@@ -4438,6 +4443,13 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
 	return 1;
 }
 
+static int umip_interception(struct vcpu_svm *svm)
+{
+	struct kvm_vcpu *vcpu = &svm->vcpu;
+
+	return kvm_emulate_instruction(vcpu, 0);
+}
+
 static int pause_interception(struct vcpu_svm *svm)
 {
 	struct kvm_vcpu *vcpu = &svm->vcpu;
@@ -4775,6 +4787,10 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_SMI]				= nop_on_interception,
 	[SVM_EXIT_INIT]				= nop_on_interception,
 	[SVM_EXIT_VINTR]			= interrupt_window_interception,
+	[SVM_EXIT_IDTR_READ]			= umip_interception,
+	[SVM_EXIT_GDTR_READ]			= umip_interception,
+	[SVM_EXIT_LDTR_READ]			= umip_interception,
+	[SVM_EXIT_TR_READ]			= umip_interception,
 	[SVM_EXIT_RDPMC]			= rdpmc_interception,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
@@ -5976,11 +5992,6 @@ static bool svm_xsaves_supported(void)
 	return boot_cpu_has(X86_FEATURE_XSAVES);
 }
 
-static bool svm_umip_emulated(void)
-{
-	return false;
-}
-
 static bool svm_pt_supported(void)
 {
 	return false;

[PATCH 3/4] kvm: svm: Emulate UMIP instructions on non SEV guest

[Moger, Babu]

Emulation of UMIP instructions (sgdt, sidt, sldt, smsw and str) requires
the hypervisor to read and write the guest memory. Guest memory is
encrypted on SEV guest. Hypervisor cannot successfully read or write the
guest memory. So disable emulation on SEV guest. Enable the emulation only
on non SEV guest.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kvm/svm.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 79abbdeca148..267dae94e5ca 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1535,6 +1535,15 @@ static void init_vmcb(struct vcpu_svm *svm)
 	set_intercept(svm, INTERCEPT_NMI);
 	set_intercept(svm, INTERCEPT_SMI);
 	set_intercept(svm, INTERCEPT_SELECTIVE_CR0);
+
+	/* Enable interception only on non-sev guest */
+	if (!sev_guest(svm->vcpu.kvm)) {
+		set_intercept(svm, INTERCEPT_STORE_IDTR);
+		set_intercept(svm, INTERCEPT_STORE_GDTR);
+		set_intercept(svm, INTERCEPT_STORE_LDTR);
+		set_intercept(svm, INTERCEPT_STORE_TR);
+	}
+
 	set_intercept(svm, INTERCEPT_RDPMC);
 	set_intercept(svm, INTERCEPT_CPUID);
 	set_intercept(svm, INTERCEPT_INVD);

[PATCH 4/4] x86/Kconfig: Rename UMIP config parameter

[Moger, Babu]

AMD 2nd generation EPYC processors support the UMIP (User-Mode
Instruction Prevention) feature. So, rename X86_INTEL_UMIP to
generic X86_UMIP and modify the text to cover both Intel and AMD.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/Kconfig                         |    8 ++++----
 arch/x86/include/asm/disabled-features.h |    2 +-
 arch/x86/include/asm/umip.h              |    4 ++--
 arch/x86/kernel/Makefile                 |    2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index d6e1faa28c58..821b7cebff31 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1880,13 +1880,13 @@ config X86_SMAP
 
 	  If unsure, say Y.
 
-config X86_INTEL_UMIP
+config X86_UMIP
 	def_bool y
-	depends on CPU_SUP_INTEL
-	prompt "Intel User Mode Instruction Prevention" if EXPERT
+	depends on X86 && (CPU_SUP_INTEL || CPU_SUP_AMD)
+	prompt "User Mode Instruction Prevention" if EXPERT
 	---help---
 	  The User Mode Instruction Prevention (UMIP) is a security
-	  feature in newer Intel processors. If enabled, a general
+	  feature in newer x86 processors. If enabled, a general
 	  protection fault is issued if the SGDT, SLDT, SIDT, SMSW
 	  or STR instructions are executed in user mode. These instructions
 	  unnecessarily expose information about the hardware state.
diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
index a5ea841cc6d2..8e1d0bb46361 100644
--- a/arch/x86/include/asm/disabled-features.h
+++ b/arch/x86/include/asm/disabled-features.h
@@ -22,7 +22,7 @@
 # define DISABLE_SMAP	(1<<(X86_FEATURE_SMAP & 31))
 #endif
 
-#ifdef CONFIG_X86_INTEL_UMIP
+#ifdef CONFIG_X86_UMIP
 # define DISABLE_UMIP	0
 #else
 # define DISABLE_UMIP	(1<<(X86_FEATURE_UMIP & 31))
diff --git a/arch/x86/include/asm/umip.h b/arch/x86/include/asm/umip.h
index db43f2a0d92c..aeed98c3c9e1 100644
--- a/arch/x86/include/asm/umip.h
+++ b/arch/x86/include/asm/umip.h
@@ -4,9 +4,9 @@
 #include <linux/types.h>
 #include <asm/ptrace.h>
 
-#ifdef CONFIG_X86_INTEL_UMIP
+#ifdef CONFIG_X86_UMIP
 bool fixup_umip_exception(struct pt_regs *regs);
 #else
 static inline bool fixup_umip_exception(struct pt_regs *regs) { return false; }
-#endif  /* CONFIG_X86_INTEL_UMIP */
+#endif  /* CONFIG_X86_UMIP */
 #endif  /* _ASM_X86_UMIP_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 3578ad248bc9..52ce1e239525 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -134,7 +134,7 @@ obj-$(CONFIG_EFI)			+= sysfb_efi.o
 obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o
 obj-$(CONFIG_TRACING)			+= tracepoint.o
 obj-$(CONFIG_SCHED_MC_PRIO)		+= itmt.o
-obj-$(CONFIG_X86_INTEL_UMIP)		+= umip.o
+obj-$(CONFIG_X86_UMIP)			+= umip.o
 
 obj-$(CONFIG_UNWINDER_ORC)		+= unwind_orc.o
 obj-$(CONFIG_UNWINDER_FRAME_POINTER)	+= unwind_frame.o

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Andy Lutomirski]

On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>
> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
> Prevention) feature. The UMIP feature prevents the execution of certain
> instructions if the Current Privilege Level (CPL) is greater than 0.
> If any of these instructions are executed with CPL > 0 and UMIP
> is enabled, then kernel reports a #GP exception.
>
> The idea is taken from articles:
> https://lwn.net/Articles/738209/
> https://lwn.net/Articles/694385/
>
> Enable the feature if supported on bare metal and emulate instructions
> to return dummy values for certain cases.

What are these cases?

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Jim Mattson]

On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>
> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
> Prevention) feature. The UMIP feature prevents the execution of certain
> instructions if the Current Privilege Level (CPL) is greater than 0.
> If any of these instructions are executed with CPL > 0 and UMIP
> is enabled, then kernel reports a #GP exception.
>
> The idea is taken from articles:
> https://lwn.net/Articles/738209/
> https://lwn.net/Articles/694385/
>
> Enable the feature if supported on bare metal and emulate instructions
> to return dummy values for certain cases.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 4153ca8cddb7..79abbdeca148 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>  {
>  }
>
> +static bool svm_umip_emulated(void)
> +{
> +       return boot_cpu_has(X86_FEATURE_UMIP);
> +}

This makes no sense to me. If the hardware actually supports UMIP,
then it doesn't have to be emulated.

To the extent that kvm emulates UMIP on Intel CPUs without hardware
UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
emulation on AMD, because SVM has always offered intercepts of sgdt,
sidt, sldt, and str. So, if you really want to offer this emulation on
pre-EPYC 2 CPUs, this function should just return true. But, I have to
ask, "why?"

*Virtualization* of UMIP on EPYC 2 already works without any of these changes.

>  static void update_cr0_intercept(struct vcpu_svm *svm)
>  {
>         ulong gcr0 = svm->vcpu.arch.cr0;
> @@ -4438,6 +4443,13 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
>         return 1;
>  }
>
> +static int umip_interception(struct vcpu_svm *svm)
> +{
> +       struct kvm_vcpu *vcpu = &svm->vcpu;
> +
> +       return kvm_emulate_instruction(vcpu, 0);
> +}
> +
>  static int pause_interception(struct vcpu_svm *svm)
>  {
>         struct kvm_vcpu *vcpu = &svm->vcpu;
> @@ -4775,6 +4787,10 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>         [SVM_EXIT_SMI]                          = nop_on_interception,
>         [SVM_EXIT_INIT]                         = nop_on_interception,
>         [SVM_EXIT_VINTR]                        = interrupt_window_interception,
> +       [SVM_EXIT_IDTR_READ]                    = umip_interception,
> +       [SVM_EXIT_GDTR_READ]                    = umip_interception,
> +       [SVM_EXIT_LDTR_READ]                    = umip_interception,
> +       [SVM_EXIT_TR_READ]                      = umip_interception,
>         [SVM_EXIT_RDPMC]                        = rdpmc_interception,
>         [SVM_EXIT_CPUID]                        = cpuid_interception,
>         [SVM_EXIT_IRET]                         = iret_interception,
> @@ -5976,11 +5992,6 @@ static bool svm_xsaves_supported(void)
>         return boot_cpu_has(X86_FEATURE_XSAVES);
>  }
>
> -static bool svm_umip_emulated(void)
> -{
> -       return false;
> -}
> -
>  static bool svm_pt_supported(void)
>  {
>         return false;
>

Re: [PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally

[Jim Mattson]

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.

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

On 11/1/19 1:24 PM, Andy Lutomirski wrote:
> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>>
>> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
>> Prevention) feature. The UMIP feature prevents the execution of certain
>> instructions if the Current Privilege Level (CPL) is greater than 0.
>> If any of these instructions are executed with CPL > 0 and UMIP
>> is enabled, then kernel reports a #GP exception.
>>
>> The idea is taken from articles:
>> https://lwn.net/Articles/738209/
>> https://lwn.net/Articles/694385/
>>
>> Enable the feature if supported on bare metal and emulate instructions
>> to return dummy values for certain cases.
> 
> What are these cases?

It is mentioned in the article https://lwn.net/Articles/738209/

=== How does it impact applications?

When enabled, however, UMIP will change the behavior that certain
applications expect from the operating system. For instance, programs
running on WineHQ and DOSEMU2 rely on some of these instructions to
function. Stas Sergeev found that Microsoft Windows 3.1 and dos4gw use the
instruction SMSW when running in virtual-8086 mode [4]. SGDT and SIDT can
also be used on virtual-8086 mode.

I have not tested these cases. I just followed the same code that was done
for Intel UMIP.

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Andy Lutomirski]

On Fri, Nov 1, 2019 at 11:38 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>
>
>
> On 11/1/19 1:24 PM, Andy Lutomirski wrote:
> > On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
> >>
> >> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
> >> Prevention) feature. The UMIP feature prevents the execution of certain
> >> instructions if the Current Privilege Level (CPL) is greater than 0.
> >> If any of these instructions are executed with CPL > 0 and UMIP
> >> is enabled, then kernel reports a #GP exception.
> >>
> >> The idea is taken from articles:
> >> https://lwn.net/Articles/738209/
> >> https://lwn.net/Articles/694385/
> >>
> >> Enable the feature if supported on bare metal and emulate instructions
> >> to return dummy values for certain cases.
> >
> > What are these cases?
>
> It is mentioned in the article https://lwn.net/Articles/738209/
>
> === How does it impact applications?
>
> When enabled, however, UMIP will change the behavior that certain
> applications expect from the operating system. For instance, programs
> running on WineHQ and DOSEMU2 rely on some of these instructions to
> function. Stas Sergeev found that Microsoft Windows 3.1 and dos4gw use the
> instruction SMSW when running in virtual-8086 mode [4]. SGDT and SIDT can
> also be used on virtual-8086 mode.
>

What does that have to do with your series?  Your series is about
enabling UMIP (or emulating UMIP -- your descriptions are quite
unclear) on AMD hardware, and the hypervisor should *not* be emulating
instructions to return dummy values.  The *guest kernel* already knows
how to emulate userspace instructions as needed.

--Andy

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

On 11/1/19 1:29 PM, Jim Mattson wrote:
> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>>
>> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
>> Prevention) feature. The UMIP feature prevents the execution of certain
>> instructions if the Current Privilege Level (CPL) is greater than 0.
>> If any of these instructions are executed with CPL > 0 and UMIP
>> is enabled, then kernel reports a #GP exception.
>>
>> The idea is taken from articles:
>> https://lwn.net/Articles/738209/
>> https://lwn.net/Articles/694385/
>>
>> Enable the feature if supported on bare metal and emulate instructions
>> to return dummy values for certain cases.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 4153ca8cddb7..79abbdeca148 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>>  {
>>  }
>>
>> +static bool svm_umip_emulated(void)
>> +{
>> +       return boot_cpu_has(X86_FEATURE_UMIP);
>> +}
> 
> This makes no sense to me. If the hardware actually supports UMIP,
> then it doesn't have to be emulated.
My understanding..

If the hardware supports the UMIP, it will generate the #GP fault when
these instructions are executed at CPL > 0. Purpose of the emulation is to
trap the GP and return a dummy value. Seems like this required in certain
legacy OSes running in protected and virtual-8086 modes. In long mode no
need to emulate. Here is the bit explanation https://lwn.net/Articles/738209/

If we don't care about those legacy cases we don't need to emulate.

> 
> To the extent that kvm emulates UMIP on Intel CPUs without hardware
> UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
> emulation on AMD, because SVM has always offered intercepts of sgdt,
> sidt, sldt, and str. So, if you really want to offer this emulation on
> pre-EPYC 2 CPUs, this function should just return true. But, I have to
> ask, "why?"


Trying to support UMIP feature only on EPYC 2 hardware. No intention to
support pre-EPYC 2.

> 
> *Virtualization* of UMIP on EPYC 2 already works without any of these changes.
> 
>>  static void update_cr0_intercept(struct vcpu_svm *svm)
>>  {
>>         ulong gcr0 = svm->vcpu.arch.cr0;
>> @@ -4438,6 +4443,13 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
>>         return 1;
>>  }
>>
>> +static int umip_interception(struct vcpu_svm *svm)
>> +{
>> +       struct kvm_vcpu *vcpu = &svm->vcpu;
>> +
>> +       return kvm_emulate_instruction(vcpu, 0);
>> +}
>> +
>>  static int pause_interception(struct vcpu_svm *svm)
>>  {
>>         struct kvm_vcpu *vcpu = &svm->vcpu;
>> @@ -4775,6 +4787,10 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>>         [SVM_EXIT_SMI]                          = nop_on_interception,
>>         [SVM_EXIT_INIT]                         = nop_on_interception,
>>         [SVM_EXIT_VINTR]                        = interrupt_window_interception,
>> +       [SVM_EXIT_IDTR_READ]                    = umip_interception,
>> +       [SVM_EXIT_GDTR_READ]                    = umip_interception,
>> +       [SVM_EXIT_LDTR_READ]                    = umip_interception,
>> +       [SVM_EXIT_TR_READ]                      = umip_interception,
>>         [SVM_EXIT_RDPMC]                        = rdpmc_interception,
>>         [SVM_EXIT_CPUID]                        = cpuid_interception,
>>         [SVM_EXIT_IRET]                         = iret_interception,
>> @@ -5976,11 +5992,6 @@ static bool svm_xsaves_supported(void)
>>         return boot_cpu_has(X86_FEATURE_XSAVES);
>>  }
>>
>> -static bool svm_umip_emulated(void)
>> -{
>> -       return false;
>> -}
>> -
>>  static bool svm_pt_supported(void)
>>  {
>>         return false;
>>

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Andy Lutomirski]

On Fri, Nov 1, 2019 at 12:20 PM Moger, Babu <Babu.Moger@amd.com> wrote:
>
>
>
> On 11/1/19 1:29 PM, Jim Mattson wrote:
> > On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
> >>
> >> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
> >> Prevention) feature. The UMIP feature prevents the execution of certain
> >> instructions if the Current Privilege Level (CPL) is greater than 0.
> >> If any of these instructions are executed with CPL > 0 and UMIP
> >> is enabled, then kernel reports a #GP exception.
> >>
> >> The idea is taken from articles:
> >> https://lwn.net/Articles/738209/
> >> https://lwn.net/Articles/694385/
> >>
> >> Enable the feature if supported on bare metal and emulate instructions
> >> to return dummy values for certain cases.
> >>
> >> Signed-off-by: Babu Moger <babu.moger@amd.com>
> >> ---
> >>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
> >>  1 file changed, 16 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index 4153ca8cddb7..79abbdeca148 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
> >>  {
> >>  }
> >>
> >> +static bool svm_umip_emulated(void)
> >> +{
> >> +       return boot_cpu_has(X86_FEATURE_UMIP);
> >> +}
> >
> > This makes no sense to me. If the hardware actually supports UMIP,
> > then it doesn't have to be emulated.
> My understanding..
>
> If the hardware supports the UMIP, it will generate the #GP fault when
> these instructions are executed at CPL > 0. Purpose of the emulation is to
> trap the GP and return a dummy value. Seems like this required in certain
> legacy OSes running in protected and virtual-8086 modes. In long mode no
> need to emulate. Here is the bit explanation https://lwn.net/Articles/738209/
>

Indeed.  Again, what does this have to do with your patch?

>
> >
> > To the extent that kvm emulates UMIP on Intel CPUs without hardware
> > UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
> > emulation on AMD, because SVM has always offered intercepts of sgdt,
> > sidt, sldt, and str. So, if you really want to offer this emulation on
> > pre-EPYC 2 CPUs, this function should just return true. But, I have to
> > ask, "why?"
>
>
> Trying to support UMIP feature only on EPYC 2 hardware. No intention to
> support pre-EPYC 2.
>

I think you need to totally rewrite your changelog to explain what you
are doing.

As I understand it, there are a couple of things KVM can do:

1. If the underlying hardware supports UMIP, KVM can expose UMIP to
the guest.  SEV should be irrelevant here.

2. Regardless of whether the underlying hardware supports UMIP, KVM
can try to emulate UMIP in the guest.  This may be impossible if SEV
is enabled.

Which of these are you doing?

Re: [PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally

[Moger, Babu]

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.

Re: [PATCH 1/4] kvm: x86: Dont set UMIP feature bit unconditionally

[Jim Mattson]

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.

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

On 11/1/19 2:24 PM, Andy Lutomirski wrote:
> On Fri, Nov 1, 2019 at 12:20 PM Moger, Babu <Babu.Moger@amd.com> wrote:
>>
>>
>>
>> On 11/1/19 1:29 PM, Jim Mattson wrote:
>>> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
>>>>
>>>> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
>>>> Prevention) feature. The UMIP feature prevents the execution of certain
>>>> instructions if the Current Privilege Level (CPL) is greater than 0.
>>>> If any of these instructions are executed with CPL > 0 and UMIP
>>>> is enabled, then kernel reports a #GP exception.
>>>>
>>>> The idea is taken from articles:
>>>> https://lwn.net/Articles/738209/
>>>> https://lwn.net/Articles/694385/
>>>>
>>>> Enable the feature if supported on bare metal and emulate instructions
>>>> to return dummy values for certain cases.
>>>>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
>>>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>> index 4153ca8cddb7..79abbdeca148 100644
>>>> --- a/arch/x86/kvm/svm.c
>>>> +++ b/arch/x86/kvm/svm.c
>>>> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>>>>  {
>>>>  }
>>>>
>>>> +static bool svm_umip_emulated(void)
>>>> +{
>>>> +       return boot_cpu_has(X86_FEATURE_UMIP);
>>>> +}
>>>
>>> This makes no sense to me. If the hardware actually supports UMIP,
>>> then it doesn't have to be emulated.
>> My understanding..
>>
>> If the hardware supports the UMIP, it will generate the #GP fault when
>> these instructions are executed at CPL > 0. Purpose of the emulation is to
>> trap the GP and return a dummy value. Seems like this required in certain
>> legacy OSes running in protected and virtual-8086 modes. In long mode no
>> need to emulate. Here is the bit explanation https://lwn.net/Articles/738209/
>>
> 
> Indeed.  Again, what does this have to do with your patch?
> 
>>
>>>
>>> To the extent that kvm emulates UMIP on Intel CPUs without hardware
>>> UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
>>> emulation on AMD, because SVM has always offered intercepts of sgdt,
>>> sidt, sldt, and str. So, if you really want to offer this emulation on
>>> pre-EPYC 2 CPUs, this function should just return true. But, I have to
>>> ask, "why?"
>>
>>
>> Trying to support UMIP feature only on EPYC 2 hardware. No intention to
>> support pre-EPYC 2.
>>
> 
> I think you need to totally rewrite your changelog to explain what you
> are doing.
> 
> As I understand it, there are a couple of things KVM can do:
> 
> 1. If the underlying hardware supports UMIP, KVM can expose UMIP to
> the guest.  SEV should be irrelevant here.
> 
> 2. Regardless of whether the underlying hardware supports UMIP, KVM
> can try to emulate UMIP in the guest.  This may be impossible if SEV
> is enabled.
> 
> Which of these are you doing?
> 
My intention was to do 1.  Let me go back and think about this again.

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Jim Mattson]

On Fri, Nov 1, 2019 at 1:04 PM Moger, Babu <Babu.Moger@amd.com> wrote:
>
>
>
> On 11/1/19 2:24 PM, Andy Lutomirski wrote:
> > On Fri, Nov 1, 2019 at 12:20 PM Moger, Babu <Babu.Moger@amd.com> wrote:
> >>
> >>
> >>
> >> On 11/1/19 1:29 PM, Jim Mattson wrote:
> >>> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com> wrote:
> >>>>
> >>>> AMD 2nd generation EPYC processors support UMIP (User-Mode Instruction
> >>>> Prevention) feature. The UMIP feature prevents the execution of certain
> >>>> instructions if the Current Privilege Level (CPL) is greater than 0.
> >>>> If any of these instructions are executed with CPL > 0 and UMIP
> >>>> is enabled, then kernel reports a #GP exception.
> >>>>
> >>>> The idea is taken from articles:
> >>>> https://lwn.net/Articles/738209/
> >>>> https://lwn.net/Articles/694385/
> >>>>
> >>>> Enable the feature if supported on bare metal and emulate instructions
> >>>> to return dummy values for certain cases.
> >>>>
> >>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
> >>>> ---
> >>>>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
> >>>>  1 file changed, 16 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >>>> index 4153ca8cddb7..79abbdeca148 100644
> >>>> --- a/arch/x86/kvm/svm.c
> >>>> +++ b/arch/x86/kvm/svm.c
> >>>> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
> >>>>  {
> >>>>  }
> >>>>
> >>>> +static bool svm_umip_emulated(void)
> >>>> +{
> >>>> +       return boot_cpu_has(X86_FEATURE_UMIP);
> >>>> +}
> >>>
> >>> This makes no sense to me. If the hardware actually supports UMIP,
> >>> then it doesn't have to be emulated.
> >> My understanding..
> >>
> >> If the hardware supports the UMIP, it will generate the #GP fault when
> >> these instructions are executed at CPL > 0. Purpose of the emulation is to
> >> trap the GP and return a dummy value. Seems like this required in certain
> >> legacy OSes running in protected and virtual-8086 modes. In long mode no
> >> need to emulate. Here is the bit explanation https://lwn.net/Articles/738209/
> >>
> >
> > Indeed.  Again, what does this have to do with your patch?
> >
> >>
> >>>
> >>> To the extent that kvm emulates UMIP on Intel CPUs without hardware
> >>> UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
> >>> emulation on AMD, because SVM has always offered intercepts of sgdt,
> >>> sidt, sldt, and str. So, if you really want to offer this emulation on
> >>> pre-EPYC 2 CPUs, this function should just return true. But, I have to
> >>> ask, "why?"
> >>
> >>
> >> Trying to support UMIP feature only on EPYC 2 hardware. No intention to
> >> support pre-EPYC 2.
> >>
> >
> > I think you need to totally rewrite your changelog to explain what you
> > are doing.
> >
> > As I understand it, there are a couple of things KVM can do:
> >
> > 1. If the underlying hardware supports UMIP, KVM can expose UMIP to
> > the guest.  SEV should be irrelevant here.
> >
> > 2. Regardless of whether the underlying hardware supports UMIP, KVM
> > can try to emulate UMIP in the guest.  This may be impossible if SEV
> > is enabled.
> >
> > Which of these are you doing?
> >
> My intention was to do 1.  Let me go back and think about this again.

(1) already works.

RE: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

> -----Original Message-----
> From: kvm-owner@vger.kernel.org <kvm-owner@vger.kernel.org> On Behalf
> Of Jim Mattson
> Sent: Friday, November 1, 2019 3:08 PM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: Andy Lutomirski <luto@kernel.org>; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de; hpa@zytor.com; pbonzini@redhat.com;
> rkrcmar@redhat.com; sean.j.christopherson@intel.com; vkuznets@redhat.com;
> wanpengli@tencent.com; x86@kernel.org; joro@8bytes.org;
> zohar@linux.ibm.com; yamada.masahiro@socionext.com;
> nayna@linux.ibm.com; linux-kernel@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD
> 
> On Fri, Nov 1, 2019 at 1:04 PM Moger, Babu <Babu.Moger@amd.com> wrote:
> >
> >
> >
> > On 11/1/19 2:24 PM, Andy Lutomirski wrote:
> > > On Fri, Nov 1, 2019 at 12:20 PM Moger, Babu <Babu.Moger@amd.com>
> wrote:
> > >>
> > >>
> > >>
> > >> On 11/1/19 1:29 PM, Jim Mattson wrote:
> > >>> On Fri, Nov 1, 2019 at 10:33 AM Moger, Babu <Babu.Moger@amd.com>
> wrote:
> > >>>>
> > >>>> AMD 2nd generation EPYC processors support UMIP (User-Mode
> Instruction
> > >>>> Prevention) feature. The UMIP feature prevents the execution of certain
> > >>>> instructions if the Current Privilege Level (CPL) is greater than 0.
> > >>>> If any of these instructions are executed with CPL > 0 and UMIP
> > >>>> is enabled, then kernel reports a #GP exception.
> > >>>>
> > >>>> The idea is taken from articles:
> > >>>> https://lwn.net/Articles/738209/
> > >>>> https://lwn.net/Articles/694385/
> > >>>>
> > >>>> Enable the feature if supported on bare metal and emulate instructions
> > >>>> to return dummy values for certain cases.
> > >>>>
> > >>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
> > >>>> ---
> > >>>>  arch/x86/kvm/svm.c |   21 ++++++++++++++++-----
> > >>>>  1 file changed, 16 insertions(+), 5 deletions(-)
> > >>>>
> > >>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > >>>> index 4153ca8cddb7..79abbdeca148 100644
> > >>>> --- a/arch/x86/kvm/svm.c
> > >>>> +++ b/arch/x86/kvm/svm.c
> > >>>> @@ -2533,6 +2533,11 @@ static void
> svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
> > >>>>  {
> > >>>>  }
> > >>>>
> > >>>> +static bool svm_umip_emulated(void)
> > >>>> +{
> > >>>> +       return boot_cpu_has(X86_FEATURE_UMIP);
> > >>>> +}
> > >>>
> > >>> This makes no sense to me. If the hardware actually supports UMIP,
> > >>> then it doesn't have to be emulated.
> > >> My understanding..
> > >>
> > >> If the hardware supports the UMIP, it will generate the #GP fault when
> > >> these instructions are executed at CPL > 0. Purpose of the emulation is to
> > >> trap the GP and return a dummy value. Seems like this required in certain
> > >> legacy OSes running in protected and virtual-8086 modes. In long mode no
> > >> need to emulate. Here is the bit explanation
> https://lwn.net/Articles/738209/
> > >>
> > >
> > > Indeed.  Again, what does this have to do with your patch?
> > >
> > >>
> > >>>
> > >>> To the extent that kvm emulates UMIP on Intel CPUs without hardware
> > >>> UMIP (i.e. smsw is still allowed at CPL>0), we can always do the same
> > >>> emulation on AMD, because SVM has always offered intercepts of sgdt,
> > >>> sidt, sldt, and str. So, if you really want to offer this emulation on
> > >>> pre-EPYC 2 CPUs, this function should just return true. But, I have to
> > >>> ask, "why?"
> > >>
> > >>
> > >> Trying to support UMIP feature only on EPYC 2 hardware. No intention to
> > >> support pre-EPYC 2.
> > >>
> > >
> > > I think you need to totally rewrite your changelog to explain what you
> > > are doing.
> > >
> > > As I understand it, there are a couple of things KVM can do:
> > >
> > > 1. If the underlying hardware supports UMIP, KVM can expose UMIP to
> > > the guest.  SEV should be irrelevant here.
> > >
> > > 2. Regardless of whether the underlying hardware supports UMIP, KVM
> > > can try to emulate UMIP in the guest.  This may be impossible if SEV
> > > is enabled.
> > >
> > > Which of these are you doing?
> > >
> > My intention was to do 1.  Let me go back and think about this again.
> 
> (1) already works.

That’s right. Thanks Jim and Andy.
How about updating the Kconfig (patch #4) and update it to CONFIG_X86_UMIP (instead of CONFIG_X86_INTEL_UMIP).
Right now, it appears it is supported on only Intel.

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Borislav Petkov]

On Sat, Nov 02, 2019 at 07:23:45PM +0000, Moger, Babu wrote:
> How about updating the Kconfig (patch #4) and update it to
> CONFIG_X86_UMIP (instead of CONFIG_X86_INTEL_UMIP).

Yes, pls do that and make it depend on CPU_SUP_AMD too.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Paolo Bonzini]

On 01/11/19 18:33, Moger, Babu wrote:
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 4153ca8cddb7..79abbdeca148 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>  {
>  }
>  
> +static bool svm_umip_emulated(void)
> +{
> +	return boot_cpu_has(X86_FEATURE_UMIP);
> +}

For hardware that supports UMIP, this is only needed because of your
patch 1.  Without it, X86_FEATURE_UMIP should already be enabled on
processors that natively support UMIP.

If you want UMIP *emulation* instead, this should become "return true".

>  static void update_cr0_intercept(struct vcpu_svm *svm)
>  {
>  	ulong gcr0 = svm->vcpu.arch.cr0;
> @@ -4438,6 +4443,13 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
>  	return 1;
>  }
>  
> +static int umip_interception(struct vcpu_svm *svm)
> +{
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +
> +	return kvm_emulate_instruction(vcpu, 0);
> +}
> +
>  static int pause_interception(struct vcpu_svm *svm)
>  {
>  	struct kvm_vcpu *vcpu = &svm->vcpu;
> @@ -4775,6 +4787,10 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>  	[SVM_EXIT_SMI]				= nop_on_interception,
>  	[SVM_EXIT_INIT]				= nop_on_interception,
>  	[SVM_EXIT_VINTR]			= interrupt_window_interception,
> +	[SVM_EXIT_IDTR_READ]			= umip_interception,
> +	[SVM_EXIT_GDTR_READ]			= umip_interception,
> +	[SVM_EXIT_LDTR_READ]			= umip_interception,
> +	[SVM_EXIT_TR_READ]			= umip_interception,

This is missing enabling the intercepts.  Also, this can be just
emulate_on_interception instead of a new function.

Paolo

>  	[SVM_EXIT_RDPMC]			= rdpmc_interception,
>  	[SVM_EXIT_CPUID]			= cpuid_interception,
>  	[SVM_EXIT_IRET]                         = iret_interception,
> @@ -5976,11 +5992,6 @@ static bool svm_xsaves_supported(void)
>  	return boot_cpu_has(X86_FEATURE_XSAVES);
>  }
>  
> -static bool svm_umip_emulated(void)
> -{
> -	return false;
> -}
> -
>  static bool svm_pt_supported(void)
>  {
>  	return false;
> 

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

On 11/4/19 5:54 AM, Paolo Bonzini wrote:
> On 01/11/19 18:33, Moger, Babu wrote:
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 4153ca8cddb7..79abbdeca148 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2533,6 +2533,11 @@ static void svm_decache_cr4_guest_bits(struct kvm_vcpu *vcpu)
>>  {
>>  }
>>  
>> +static bool svm_umip_emulated(void)
>> +{
>> +	return boot_cpu_has(X86_FEATURE_UMIP);
>> +}
> 
> For hardware that supports UMIP, this is only needed because of your
> patch 1.  Without it, X86_FEATURE_UMIP should already be enabled on
> processors that natively support UMIP.

Yes, That is correct. Will remove the patch #1. Intention was to enable
UMIP for the hardware that supports it. Will send out only the config
changes(Patch #4).  Also there is a complexity with supporting emulation
on SEV guest.

> 
> If you want UMIP *emulation* instead, this should become "return true".
> 
>>  static void update_cr0_intercept(struct vcpu_svm *svm)
>>  {
>>  	ulong gcr0 = svm->vcpu.arch.cr0;
>> @@ -4438,6 +4443,13 @@ static int interrupt_window_interception(struct vcpu_svm *svm)
>>  	return 1;
>>  }
>>  
>> +static int umip_interception(struct vcpu_svm *svm)
>> +{
>> +	struct kvm_vcpu *vcpu = &svm->vcpu;
>> +
>> +	return kvm_emulate_instruction(vcpu, 0);
>> +}
>> +
>>  static int pause_interception(struct vcpu_svm *svm)
>>  {
>>  	struct kvm_vcpu *vcpu = &svm->vcpu;
>> @@ -4775,6 +4787,10 @@ static int (*const svm_exit_handlers[])(struct vcpu_svm *svm) = {
>>  	[SVM_EXIT_SMI]				= nop_on_interception,
>>  	[SVM_EXIT_INIT]				= nop_on_interception,
>>  	[SVM_EXIT_VINTR]			= interrupt_window_interception,
>> +	[SVM_EXIT_IDTR_READ]			= umip_interception,
>> +	[SVM_EXIT_GDTR_READ]			= umip_interception,
>> +	[SVM_EXIT_LDTR_READ]			= umip_interception,
>> +	[SVM_EXIT_TR_READ]			= umip_interception,
> 
> This is missing enabling the intercepts.  Also, this can be just
> emulate_on_interception instead of a new function.
> 
> Paolo
> 
>>  	[SVM_EXIT_RDPMC]			= rdpmc_interception,
>>  	[SVM_EXIT_CPUID]			= cpuid_interception,
>>  	[SVM_EXIT_IRET]                         = iret_interception,
>> @@ -5976,11 +5992,6 @@ static bool svm_xsaves_supported(void)
>>  	return boot_cpu_has(X86_FEATURE_XSAVES);
>>  }
>>  
>> -static bool svm_umip_emulated(void)
>> -{
>> -	return false;
>> -}
>> -
>>  static bool svm_pt_supported(void)
>>  {
>>  	return false;
>>
> 

Re: [PATCH 2/4] kvm: svm: Enable UMIP feature on AMD

[Moger, Babu]

On 11/3/19 5:45 AM, Borislav Petkov wrote:
> On Sat, Nov 02, 2019 at 07:23:45PM +0000, Moger, Babu wrote:
>> How about updating the Kconfig (patch #4) and update it to
>> CONFIG_X86_UMIP (instead of CONFIG_X86_INTEL_UMIP).
> 
> Yes, pls do that and make it depend on CPU_SUP_AMD too.

Sure. Will send the patch soon. Thanks
> 
> Thx.
>