[PATCH 0/2 v2] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers

[PATCH 0/2 v2] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers

[Krish Sadhukhan]

v1 -> v2:
	1. A KVM fix has been added via patch# 1. This fix is required to test
	   the unsetting of "unrestricted guest" VM-execution control in
	   vmcs12.
	2. The kvm-unit-test has been enhanced by adding more combinations of
	   the conditions mentioned in the vmentry checks being tested here.
   

[PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution
[PATCH 2/2 v2] kvm-unit-tests: nVMX: Test Selector and Base Address fields of Guest Segment

 arch/x86/kvm/vmx/nested.c | 3 +++
 1 file changed, 3 insertions(+)

Krish Sadhukhan (1):
      nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

 Makefile            |   2 +-
 lib/x86/processor.h |   1 +
 x86/vmx_tests.c     | 201 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+), 1 deletion(-)

Krish Sadhukhan (1):
      nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Krish Sadhukhan]

Currently, prepare_vmcs02_early() does not check if the "unrestricted guest"
VM-execution control in vmcs12 is turned off and leaves the corresponding
bit on in vmcs02. Due to this setting, vmentry checks which are supposed to
render the nested guest state as invalid when this VM-execution control is
not set, are passing in hardware.

This patch turns off the "unrestricted guest" VM-execution control in vmcs02
if vmcs12 has turned it off.

Suggested-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index cbc9ea2de28f..4fa5f8b51c82 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2224,6 +2224,9 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
 			vmcs_write16(GUEST_INTR_STATUS,
 				vmcs12->guest_intr_status);
 
+		if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST))
+			exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;
+
 		secondary_exec_controls_set(vmx, exec_control);
 	}
 
-- 
2.20.1

[PATCH 2/2 v2] kvm-unit-tests: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Guest Segment Registers" in Intel SDM vol 3C,
the following checks are performed on the Guest Segment Registers on vmentry
of nested guests:

    Selector fields:
	— TR. The TI flag (bit 2) must be 0.
	— LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
	— SS. If the guest will not be virtual-8086 and the "unrestricted
	  guest" VM-execution control is 0, the RPL (bits 1:0) must equal
	  the RPL of the selector field for CS.1

    Base-address fields:
	— CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the
	  address must be the selector field shifted left 4 bits (multiplied
	  by 16).
	— The following checks are performed on processors that support Intel
	  64 architecture:
		TR, FS, GS. The address must be canonical.
		LDTR. If LDTR is usable, the address must be canonical.
		CS. Bits 63:32 of the address must be zero.
		SS, DS, ES. If the register is usable, bits 63:32 of the
		address must be zero.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 Makefile            |   2 +-
 lib/x86/processor.h |   1 +
 x86/vmx_tests.c     | 201 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 203 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 767b6c6..159eb64 100644
--- a/Makefile
+++ b/Makefile
@@ -51,7 +51,7 @@ cc-option = $(shell if $(CC) -Werror $(1) -S -o /dev/null -xc /dev/null \
 
 COMMON_CFLAGS += -g $(autodepend-flags) -fno-strict-aliasing
 COMMON_CFLAGS += -Wall -Wwrite-strings -Wempty-body -Wuninitialized
-COMMON_CFLAGS += -Wignored-qualifiers -Werror
+COMMON_CFLAGS += -Wignored-qualifiers -Werror -Wno-unused-function
 
 frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer
 fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "")
diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 3adddee..fe4f482 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -60,6 +60,7 @@
 #define X86_EFLAGS_OF    0x00000800
 #define X86_EFLAGS_IOPL  0x00003000
 #define X86_EFLAGS_NT    0x00004000
+#define X86_EFLAGS_VM    0x00020000
 #define X86_EFLAGS_AC    0x00040000
 
 #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 2014e54..e8b0e0a 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7672,6 +7672,203 @@ static void test_load_guest_bndcfgs(void)
 	vmcs_write(GUEST_BNDCFGS, bndcfgs_saved);
 }
 
+#define	GUEST_SEG_UNUSABLE_MASK	(1u << 16)
+#define	GUEST_SEG_SEL_TI_MASK	(1u << 2)
+#define	TEST_SEGMENT_SEL(xfail, sel, sel_name, val)			\
+	vmcs_write(sel, val);						\
+	test_guest_state("Test Guest Segment Selector",	xfail, val,	\
+			 sel_name);
+
+/*
+ * The following checks are done on the Selector field of the Guest Segment
+ * Registers:
+ *    — TR. The TI flag (bit 2) must be 0.
+ *    — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
+ *    — SS. If the guest will not be virtual-8086 and the "unrestricted
+ *	guest" VM-execution control is 0, the RPL (bits 1:0) must equal
+ *	the RPL of the selector field for CS.
+ *
+ *  [Intel SDM]
+ */
+static void test_guest_segment_sel_fields(void)
+{
+	u16 sel_saved;
+	u32 ar_saved;
+	u32 cpu_ctrl0_saved;
+	u32 cpu_ctrl1_saved;
+	u16 cs_rpl_bits;
+
+	/*
+	 * Test for GUEST_SEL_TR
+	 */
+	sel_saved = vmcs_read(GUEST_SEL_TR);
+	TEST_SEGMENT_SEL(true, GUEST_SEL_TR, "GUEST_SEL_TR",
+			 sel_saved | GUEST_SEG_SEL_TI_MASK);
+	vmcs_write(GUEST_SEL_TR, sel_saved);
+
+	/*
+	 * Test for GUEST_SEL_LDTR
+	 */
+	sel_saved = vmcs_read(GUEST_SEL_LDTR);
+	ar_saved = vmcs_read(GUEST_AR_LDTR);
+	/* LDTR is set unusable */
+	vmcs_write(GUEST_AR_LDTR, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_SEL(false, GUEST_SEL_LDTR, "GUEST_SEL_LDTR",
+			 sel_saved | GUEST_SEG_SEL_TI_MASK);
+	TEST_SEGMENT_SEL(false, GUEST_SEL_LDTR, "GUEST_SEL_LDTR",
+			 sel_saved & ~GUEST_SEG_SEL_TI_MASK);
+	/* LDTR is set usable */
+	vmcs_write(GUEST_AR_LDTR, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_SEL(true, GUEST_SEL_LDTR, "GUEST_SEL_LDTR",
+			 sel_saved | GUEST_SEG_SEL_TI_MASK);
+
+	TEST_SEGMENT_SEL(false, GUEST_SEL_LDTR, "GUEST_SEL_LDTR",
+			 sel_saved & ~GUEST_SEG_SEL_TI_MASK);
+
+	vmcs_write(GUEST_AR_LDTR, ar_saved);
+	vmcs_write(GUEST_SEL_LDTR, sel_saved);
+
+	/*
+	 * Test for GUEST_SEL_SS
+	 */
+	cpu_ctrl0_saved = vmcs_read(CPU_EXEC_CTRL0);
+	cpu_ctrl1_saved = vmcs_read(CPU_EXEC_CTRL1);
+	ar_saved = vmcs_read(GUEST_AR_SS);
+	/* Turn off "unrestricted guest" vm-execution control */
+	vmcs_write(CPU_EXEC_CTRL1, cpu_ctrl1_saved & ~CPU_URG);
+	cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3;
+	sel_saved = vmcs_read(GUEST_SEL_SS);
+	TEST_SEGMENT_SEL(true, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (~cs_rpl_bits & 0x3)));
+	TEST_SEGMENT_SEL(false, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (cs_rpl_bits & 0x3)));
+	/* Make SS usable if it's unusable or vice-versa */
+	if (ar_saved & GUEST_SEG_UNUSABLE_MASK)
+		vmcs_write(GUEST_AR_SS, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	else
+		vmcs_write(GUEST_AR_SS, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_SEL(true, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (~cs_rpl_bits & 0x3)));
+	TEST_SEGMENT_SEL(false, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (cs_rpl_bits & 0x3)));
+
+	/* Turn on "unrestricted guest" vm-execution control */
+	vmcs_write(CPU_EXEC_CTRL0, cpu_ctrl0_saved | CPU_SECONDARY);
+	vmcs_write(CPU_EXEC_CTRL1, cpu_ctrl1_saved | CPU_URG);
+	/* EPT and EPTP must be setup when "unrestricted guest" is on */
+	setup_ept(false);
+	TEST_SEGMENT_SEL(false, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (~cs_rpl_bits & 0x3)));
+	TEST_SEGMENT_SEL(false, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (cs_rpl_bits & 0x3)));
+	/* Make SS usable if it's unusable or vice-versa */
+	if (vmcs_read(GUEST_AR_SS) & GUEST_SEG_UNUSABLE_MASK)
+		vmcs_write(GUEST_AR_SS, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	else
+		vmcs_write(GUEST_AR_SS, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_SEL(false, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (~cs_rpl_bits & 0x3)));
+	TEST_SEGMENT_SEL(false, GUEST_SEL_SS, "GUEST_SEL_SS",
+				 ((sel_saved & ~0x3) | (cs_rpl_bits & 0x3)));
+
+	vmcs_write(GUEST_AR_SS, ar_saved);
+	vmcs_write(GUEST_SEL_SS, sel_saved);
+	vmcs_write(CPU_EXEC_CTRL0, cpu_ctrl0_saved);
+	vmcs_write(CPU_EXEC_CTRL1, cpu_ctrl1_saved);
+}
+
+#define	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(xfail, seg_base, seg_base_name)\
+	addr_saved = vmcs_read(seg_base);				\
+	for (i = 32; i < 63; i = i + 4) {				\
+		addr = addr_saved | 1ull << i;				\
+		vmcs_write(seg_base, addr);				\
+		test_guest_state(seg_base_name,	xfail, addr,		\
+				seg_base_name);				\
+	}								\
+	vmcs_write(seg_base, addr_saved);
+
+#define	TEST_SEGMENT_BASE_ADDR_CANONICAL(xfail, seg_base, seg_base_name)\
+	addr_saved = vmcs_read(seg_base);				\
+	vmcs_write(seg_base, NONCANONICAL);				\
+	test_guest_state(seg_base_name,	xfail, NONCANONICAL,		\
+			seg_base_name);					\
+	vmcs_write(seg_base, addr_saved);
+
+/*
+ * The following checks are done on the Base Address field of the Guest
+ * Segment Registers on processors that support Intel 64 architecture:
+ *    - TR, FS, GS : The address must be canonical.
+ *    - LDTR : If LDTR is usable, the address must be canonical.
+ *    - CS : Bits 63:32 of the address must be zero.
+ *    - SS, DS, ES : If the register is usable, bits 63:32 of the address
+ *	must be zero.
+ *
+ *  [Intel SDM]
+ */
+static void test_guest_segment_base_addr_fields(void)
+{
+	u64 addr_saved;
+	u64 addr;
+	u32 ar_saved;
+	int i;
+
+	/*
+	 * The address of TR, FS, GS and LDTR must be canonical.
+	 */
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(true, GUEST_BASE_TR, "GUEST_BASE_TR");
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(true, GUEST_BASE_FS, "GUEST_BASE_FS");
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(true, GUEST_BASE_GS, "GUEST_BASE_GS");
+	ar_saved = vmcs_read(GUEST_AR_LDTR);
+	/* Make LDTR unusable */
+	vmcs_write(GUEST_AR_LDTR, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(false, GUEST_BASE_LDTR,
+					"GUEST_BASE_LDTR");
+	/* Make LDTR usable */
+	vmcs_write(GUEST_AR_LDTR, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(true, GUEST_BASE_LDTR,
+					"GUEST_BASE_LDTR");
+
+	vmcs_write(GUEST_AR_LDTR, ar_saved);
+
+	/*
+	 * Bits 63:32 in CS, SS, DS and ES base address must be zero
+	 */
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(true, GUEST_BASE_CS,
+					 "GUEST_BASE_CS");
+	ar_saved = vmcs_read(GUEST_AR_SS);
+	/* Make SS unusable */
+	vmcs_write(GUEST_AR_SS, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(false, GUEST_BASE_SS,
+					 "GUEST_BASE_SS");
+	/* Make SS usable */
+	vmcs_write(GUEST_AR_SS, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(true, GUEST_BASE_SS,
+					 "GUEST_BASE_SS");
+	vmcs_write(GUEST_AR_SS, ar_saved);
+
+	ar_saved = vmcs_read(GUEST_AR_DS);
+	/* Make DS unusable */
+	vmcs_write(GUEST_AR_DS, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(false, GUEST_BASE_DS,
+					 "GUEST_BASE_DS");
+	/* Make DS usable */
+	vmcs_write(GUEST_AR_DS, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(true, GUEST_BASE_DS,
+					 "GUEST_BASE_DS");
+	vmcs_write(GUEST_AR_DS, ar_saved);
+
+	ar_saved = vmcs_read(GUEST_AR_ES);
+	/* Make ES unusable */
+	vmcs_write(GUEST_AR_ES, ar_saved | GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(false, GUEST_BASE_ES,
+					 "GUEST_BASE_ES");
+	/* Make ES usable */
+	vmcs_write(GUEST_AR_ES, ar_saved & ~GUEST_SEG_UNUSABLE_MASK);
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(true, GUEST_BASE_ES,
+					 "GUEST_BASE_ES");
+	vmcs_write(GUEST_AR_ES, ar_saved);
+}
+
 /*
  * Check that the virtual CPU checks the VMX Guest State Area as
  * documented in the Intel SDM.
@@ -7685,6 +7882,7 @@ static void vmx_guest_state_area_test(void)
 	 * The IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field
 	 * must each contain a canonical address.
 	 */
+/*
 	test_canonical(GUEST_SYSENTER_ESP, "GUEST_SYSENTER_ESP", false);
 	test_canonical(GUEST_SYSENTER_EIP, "GUEST_SYSENTER_EIP", false);
 
@@ -7693,6 +7891,9 @@ static void vmx_guest_state_area_test(void)
 	test_guest_efer();
 	test_load_guest_perf_global_ctrl();
 	test_load_guest_bndcfgs();
+*/
+	test_guest_segment_sel_fields();
+	test_guest_segment_base_addr_fields();
 
 	/*
 	 * Let the guest finish execution
-- 
2.20.1

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Sean Christopherson]

On Wed, Apr 15, 2020 at 02:30:46PM -0400, Krish Sadhukhan wrote:
> Currently, prepare_vmcs02_early() does not check if the "unrestricted guest"
> VM-execution control in vmcs12 is turned off and leaves the corresponding
> bit on in vmcs02. Due to this setting, vmentry checks which are supposed to
> render the nested guest state as invalid when this VM-execution control is
> not set, are passing in hardware.
> 
> This patch turns off the "unrestricted guest" VM-execution control in vmcs02
> if vmcs12 has turned it off.
> 
> Suggested-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index cbc9ea2de28f..4fa5f8b51c82 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2224,6 +2224,9 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
>  			vmcs_write16(GUEST_INTR_STATUS,
>  				vmcs12->guest_intr_status);
>  
> +		if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST))
> +			exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;

Has anyone worked through all the flows to verify this won't break any
assumptions with respect to enable_unrestricted_guest?  I would be
(pleasantly) surprised if this was sufficient to run L2 without
unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
suspect.

> +
>  		secondary_exec_controls_set(vmx, exec_control);
>  	}
>  
> -- 
> 2.20.1
> 

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Jim Mattson]

On Wed, Apr 15, 2020 at 12:30 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Apr 15, 2020 at 02:30:46PM -0400, Krish Sadhukhan wrote:
> > Currently, prepare_vmcs02_early() does not check if the "unrestricted guest"
> > VM-execution control in vmcs12 is turned off and leaves the corresponding
> > bit on in vmcs02. Due to this setting, vmentry checks which are supposed to
> > render the nested guest state as invalid when this VM-execution control is
> > not set, are passing in hardware.
> >
> > This patch turns off the "unrestricted guest" VM-execution control in vmcs02
> > if vmcs12 has turned it off.
> >
> > Suggested-by: Jim Mattson <jmattson@google.com>
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index cbc9ea2de28f..4fa5f8b51c82 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2224,6 +2224,9 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct vmcs12 *vmcs12)
> >                       vmcs_write16(GUEST_INTR_STATUS,
> >                               vmcs12->guest_intr_status);
> >
> > +             if (!nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST))
> > +                     exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;

Better, I think, would be to add SECONDARY_EXEC_UNRESTRICTED_GUEST to
the mask here:

/* Take the following fields only from vmcs12 */
exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
  SECONDARY_EXEC_ENABLE_INVPCID |
  SECONDARY_EXEC_RDTSCP |
  SECONDARY_EXEC_XSAVES |
  SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
  SECONDARY_EXEC_APIC_REGISTER_VIRT |
  SECONDARY_EXEC_ENABLE_VMFUNC);

> Has anyone worked through all the flows to verify this won't break any
> assumptions with respect to enable_unrestricted_guest?  I would be
> (pleasantly) surprised if this was sufficient to run L2 without
> unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
> suspect.

I think you're right to be concerned.

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Paolo Bonzini]

On 15/04/20 22:18, Jim Mattson wrote:
>> Has anyone worked through all the flows to verify this won't break any
>> assumptions with respect to enable_unrestricted_guest?  I would be
>> (pleasantly) surprised if this was sufficient to run L2 without
>> unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
>> suspect.
> 
> I think you're right to be concerned.

Thirded, but it shouldn't be too hard.  Basically,
enable_unrestricted_guest must be moved into loaded_vmcs for this to
work.  It may be more work to write the test cases for L2 real mode <->
protected mode switch, which do not entirely fit into the vmx_tests.c
framework (but with the v2 tests it should not be hard to adapt).

Paolo

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Krish Sadhukhan]

On 4/16/20 2:18 AM, Paolo Bonzini wrote:
> On 15/04/20 22:18, Jim Mattson wrote:
>>> Has anyone worked through all the flows to verify this won't break any
>>> assumptions with respect to enable_unrestricted_guest?  I would be
>>> (pleasantly) surprised if this was sufficient to run L2 without
>>> unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
>>> suspect.
>> I think you're right to be concerned.
> Thirded, but it shouldn't be too hard.  Basically,
> enable_unrestricted_guest must be moved into loaded_vmcs for this to
> work.  It may be more work to write the test cases for L2 real mode <->
> protected mode switch, which do not entirely fit into the vmx_tests.c
> framework (but with the v2 tests it should not be hard to adapt).


OK, I will move enable_unrestricted_guest  to loaded_vmcs.

I also see that enable_ept controls the setting of 
enable_unrestricted_guest. Perhaps both need to be moved to loaded_vmcs ?

About testing, I am thinking the test will first vmlaunch L2 in real 
mode or in protected mode, then vmexit on vmcall and then vmresume in 
the other mode. Is that how the test should flow ?

>
> Paolo
>

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Sean Christopherson]

On Fri, Apr 17, 2020 at 06:29:23PM -0700, Krish Sadhukhan wrote:
> 
> On 4/16/20 2:18 AM, Paolo Bonzini wrote:
> >On 15/04/20 22:18, Jim Mattson wrote:
> >>>Has anyone worked through all the flows to verify this won't break any
> >>>assumptions with respect to enable_unrestricted_guest?  I would be
> >>>(pleasantly) surprised if this was sufficient to run L2 without
> >>>unrestricted guest when it's enabled for L1, e.g. vmx_set_cr0() looks
> >>>suspect.
> >>I think you're right to be concerned.
> >Thirded, but it shouldn't be too hard.  Basically,
> >enable_unrestricted_guest must be moved into loaded_vmcs for this to
> >work.  It may be more work to write the test cases for L2 real mode <->
> >protected mode switch, which do not entirely fit into the vmx_tests.c
> >framework (but with the v2 tests it should not be hard to adapt).
> 
> 
> OK, I will move enable_unrestricted_guest  to loaded_vmcs.

Hmm, enable_unrestricted_guest doesn't _need_ to be moved to loaded_vmcs,
L1 can never diverge from enable_unrestricted_guest.  E.g. the main control
variable can stay global, we just need a flag in nested_vmx to override the
main control.  A simple wrapper can then take care of the check, e.g.

  static inline bool is_unrestricted_guest(struct kvm_vcpu *vcpu)
  {
	return enable_unrestricted_guest && (!is_guest_mode(vcpu) ||
	       to_vmx(vcpu)->nested.unrestricted_guest);
  }

Putting the flag in loaded_vmcs might be more performant?  My guess is it'd
be in the noise, at which point I'd rather have it be clear the override is
only possible/necessary for nested guests.

> I also see that enable_ept controls the setting of
> enable_unrestricted_guest. Perhaps both need to be moved to loaded_vmcs ?

No, letting L1 disable EPT in L0 would be pure insanity, and the overall
paging mode of L2 is already reflected in the MMU.

The dependency on EPT is that VMX requires paging of some form and
unrestricted guest allows entering non-root with CR0.PG=0, i.e. requires EPT
be enabled.

> About testing, I am thinking the test will first vmlaunch L2 in real mode or
> in protected mode, then vmexit on vmcall and then vmresume in the other
> mode. Is that how the test should flow ?
> 
> >
> >Paolo
> >

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Paolo Bonzini]

On 18/04/20 03:55, Sean Christopherson wrote:
> 
>   static inline bool is_unrestricted_guest(struct kvm_vcpu *vcpu)
>   {
> 	return enable_unrestricted_guest && (!is_guest_mode(vcpu) ||
> 	       to_vmx(vcpu)->nested.unrestricted_guest);
>   }
>
> Putting the flag in loaded_vmcs might be more performant?  My guess is it'd
> be in the noise, at which point I'd rather have it be clear the override is
> only possible/necessary for nested guests.

Even better: you can use secondary_exec_controls_get, which does get the
flag from the loaded_vmcs :) but without actually having to add one.

>> I also see that enable_ept controls the setting of
>> enable_unrestricted_guest. Perhaps both need to be moved to loaded_vmcs ?
>
> No, letting L1 disable EPT in L0 would be pure insanity, and the overall
> paging mode of L2 is already reflected in the MMU.

Absolutely.  Unrestricted guest requires EPT, but EPT is invisible to
the guest.  (Currently EPT requires guest MAXPHYADDR = host MAXPHYADDR,
in the sense that the guest can detect that the host is lying about
MAXPHYADDR; but that is really a bug that I hope will be fixed in 5.8,
relaxing the requirement to guest MAXPHYADDR <= host PHYADDR).

Paolo

> The dependency on EPT is that VMX requires paging of some form and
> unrestricted guest allows entering non-root with CR0.PG=0, i.e. requires EPT
> be enabled.

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Sean Christopherson]

On Sat, Apr 18, 2020 at 11:53:36AM +0200, Paolo Bonzini wrote:
> On 18/04/20 03:55, Sean Christopherson wrote:
> > 
> >   static inline bool is_unrestricted_guest(struct kvm_vcpu *vcpu)
> >   {
> > 	return enable_unrestricted_guest && (!is_guest_mode(vcpu) ||
> > 	       to_vmx(vcpu)->nested.unrestricted_guest);
> >   }
> >
> > Putting the flag in loaded_vmcs might be more performant?  My guess is it'd
> > be in the noise, at which point I'd rather have it be clear the override is
> > only possible/necessary for nested guests.
> 
> Even better: you can use secondary_exec_controls_get, which does get the
> flag from the loaded_vmcs :) but without actually having to add one.

I keep forgetting we have those shadows.  Definitely the best solution.

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Krish Sadhukhan]

On 4/18/20 2:53 AM, Paolo Bonzini wrote:
> On 18/04/20 03:55, Sean Christopherson wrote:
>>    static inline bool is_unrestricted_guest(struct kvm_vcpu *vcpu)
>>    {
>> 	return enable_unrestricted_guest && (!is_guest_mode(vcpu) ||
>> 	       to_vmx(vcpu)->nested.unrestricted_guest);
>>    }
>>
>> Putting the flag in loaded_vmcs might be more performant?  My guess is it'd
>> be in the noise, at which point I'd rather have it be clear the override is
>> only possible/necessary for nested guests.
> Even better: you can use secondary_exec_controls_get, which does get the
> flag from the loaded_vmcs :) but without actually having to add one.
>
>>> I also see that enable_ept controls the setting of
>>> enable_unrestricted_guest. Perhaps both need to be moved to loaded_vmcs ?
>> No, letting L1 disable EPT in L0 would be pure insanity, and the overall
>> paging mode of L2 is already reflected in the MMU.
> Absolutely.  Unrestricted guest requires EPT, but EPT is invisible to
> the guest.  (Currently EPT requires guest MAXPHYADDR = host MAXPHYADDR,
> in the sense that the guest can detect that the host is lying about
> MAXPHYADDR; but that is really a bug that I hope will be fixed in 5.8,
> relaxing the requirement to guest MAXPHYADDR <= host PHYADDR).


Should EPT for the nested guest be set up in the normal way (PML4E -> 
PDPTE-> PDE -> PTE) when GUEST_CR0.PE is zero ? Or does it have to be a 
special set up like only the PTEs are needed because no protection and 
no paging are used ?

> Paolo
>
>> The dependency on EPT is that VMX requires paging of some form and
>> unrestricted guest allows entering non-root with CR0.PG=0, i.e. requires EPT
>> be enabled.

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Paolo Bonzini]

On 28/04/20 09:25, Krish Sadhukhan wrote:
>>>
>> Absolutely.  Unrestricted guest requires EPT, but EPT is invisible to
>> the guest.  (Currently EPT requires guest MAXPHYADDR = host MAXPHYADDR,
>> in the sense that the guest can detect that the host is lying about
>> MAXPHYADDR; but that is really a bug that I hope will be fixed in 5.8,
>> relaxing the requirement to guest MAXPHYADDR <= host PHYADDR).
> 
> Should EPT for the nested guest be set up in the normal way (PML4E ->
> PDPTE-> PDE -> PTE) when GUEST_CR0.PE is zero ? Or does it have to be a
> special set up like only the PTEs are needed because no protection and
> no paging are used ?

I don't understand.  When EPT is in use, the vmcs02 CR3 is simply set to
the vmcs12 CR3.

Paolo

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Krish Sadhukhan]

On 4/28/20 1:14 AM, Paolo Bonzini wrote:
> On 28/04/20 09:25, Krish Sadhukhan wrote:
>>> Absolutely.  Unrestricted guest requires EPT, but EPT is invisible to
>>> the guest.  (Currently EPT requires guest MAXPHYADDR = host MAXPHYADDR,
>>> in the sense that the guest can detect that the host is lying about
>>> MAXPHYADDR; but that is really a bug that I hope will be fixed in 5.8,
>>> relaxing the requirement to guest MAXPHYADDR <= host PHYADDR).
>> Should EPT for the nested guest be set up in the normal way (PML4E ->
>> PDPTE-> PDE -> PTE) when GUEST_CR0.PE is zero ? Or does it have to be a
>> special set up like only the PTEs are needed because no protection and
>> no paging are used ?
> I don't understand.  When EPT is in use, the vmcs02 CR3 is simply set to
> the vmcs12 CR3.


Sorry, I should have framed my question in a better way.

My question is  how should L1 in the test code set up EPTP for L2 when 
L2 is an unrestricted guest with no protection (GUEST_CR0.PE = 0) and no 
paging (GUEST_CR0.PG = 0) ? Should EPTP in test code be set up in the 
same way as when L2 is an unrestricted guest with protection and paging 
enabled ?

Getting confused by legacy 16-bit Real Mode and an unrestricted guest in 
Real Mode. :-)
> Paolo
>

Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it

[Jim Mattson]

On Tue, Apr 28, 2020 at 10:39 AM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
>
> On 4/28/20 1:14 AM, Paolo Bonzini wrote:
> > On 28/04/20 09:25, Krish Sadhukhan wrote:
> >>> Absolutely.  Unrestricted guest requires EPT, but EPT is invisible to
> >>> the guest.  (Currently EPT requires guest MAXPHYADDR = host MAXPHYADDR,
> >>> in the sense that the guest can detect that the host is lying about
> >>> MAXPHYADDR; but that is really a bug that I hope will be fixed in 5.8,
> >>> relaxing the requirement to guest MAXPHYADDR <= host PHYADDR).
> >> Should EPT for the nested guest be set up in the normal way (PML4E ->
> >> PDPTE-> PDE -> PTE) when GUEST_CR0.PE is zero ? Or does it have to be a
> >> special set up like only the PTEs are needed because no protection and
> >> no paging are used ?
> > I don't understand.  When EPT is in use, the vmcs02 CR3 is simply set to
> > the vmcs12 CR3.
>
>
> Sorry, I should have framed my question in a better way.
>
> My question is  how should L1 in the test code set up EPTP for L2 when
> L2 is an unrestricted guest with no protection (GUEST_CR0.PE = 0) and no
> paging (GUEST_CR0.PG = 0) ? Should EPTP in test code be set up in the
> same way as when L2 is an unrestricted guest with protection and paging
> enabled ?

The EPT maps guest physical addresses to host physical addresses. That
mapping is the same regardless of whether the guest is using paging or
not.