* [PATCH 0/2 v2] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers @ 2020-04-15 18:30 Krish Sadhukhan 2020-04-15 18:30 ` [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 2020-04-15 18:30 ` [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 0 siblings, 2 replies; 14+ messages in thread From: Krish Sadhukhan @ 2020-04-15 18:30 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-15 18:30 [PATCH 0/2 v2] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Krish Sadhukhan @ 2020-04-15 18:30 ` Krish Sadhukhan 2020-04-15 19:30 ` Sean Christopherson 2020-04-15 18:30 ` [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 1 sibling, 1 reply; 14+ messages in thread From: Krish Sadhukhan @ 2020-04-15 18:30 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-15 18:30 ` [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 @ 2020-04-15 19:30 ` Sean Christopherson 2020-04-15 20:18 ` Jim Mattson 0 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2020-04-15 19:30 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-15 19:30 ` Sean Christopherson @ 2020-04-15 20:18 ` Jim Mattson 2020-04-16 9:18 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Jim Mattson @ 2020-04-15 20:18 UTC (permalink / raw) To: Sean Christopherson; +Cc: Krish Sadhukhan, kvm list, Paolo Bonzini 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-15 20:18 ` Jim Mattson @ 2020-04-16 9:18 ` Paolo Bonzini 2020-04-18 1:29 ` Krish Sadhukhan 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2020-04-16 9:18 UTC (permalink / raw) To: Jim Mattson, Sean Christopherson; +Cc: Krish Sadhukhan, kvm list 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-16 9:18 ` Paolo Bonzini @ 2020-04-18 1:29 ` Krish Sadhukhan 2020-04-18 1:55 ` Sean Christopherson 0 siblings, 1 reply; 14+ messages in thread From: Krish Sadhukhan @ 2020-04-18 1:29 UTC (permalink / raw) To: Paolo Bonzini, Jim Mattson, Sean Christopherson; +Cc: kvm list 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-18 1:29 ` Krish Sadhukhan @ 2020-04-18 1:55 ` Sean Christopherson 2020-04-18 9:53 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Sean Christopherson @ 2020-04-18 1:55 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: Paolo Bonzini, Jim Mattson, kvm list 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 > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-18 1:55 ` Sean Christopherson @ 2020-04-18 9:53 ` Paolo Bonzini 2020-04-20 15:12 ` Sean Christopherson 2020-04-28 7:25 ` Krish Sadhukhan 0 siblings, 2 replies; 14+ messages in thread From: Paolo Bonzini @ 2020-04-18 9:53 UTC (permalink / raw) To: Sean Christopherson, Krish Sadhukhan; +Cc: Jim Mattson, kvm list 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-18 9:53 ` Paolo Bonzini @ 2020-04-20 15:12 ` Sean Christopherson 2020-04-28 7:25 ` Krish Sadhukhan 1 sibling, 0 replies; 14+ messages in thread From: Sean Christopherson @ 2020-04-20 15:12 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Krish Sadhukhan, Jim Mattson, kvm list 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-18 9:53 ` Paolo Bonzini 2020-04-20 15:12 ` Sean Christopherson @ 2020-04-28 7:25 ` Krish Sadhukhan 2020-04-28 8:14 ` Paolo Bonzini 1 sibling, 1 reply; 14+ messages in thread From: Krish Sadhukhan @ 2020-04-28 7:25 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson; +Cc: Jim Mattson, kvm list 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-28 7:25 ` Krish Sadhukhan @ 2020-04-28 8:14 ` Paolo Bonzini 2020-04-28 17:38 ` Krish Sadhukhan 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2020-04-28 8:14 UTC (permalink / raw) To: Krish Sadhukhan, Sean Christopherson; +Cc: Jim Mattson, kvm list 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-28 8:14 ` Paolo Bonzini @ 2020-04-28 17:38 ` Krish Sadhukhan 2020-04-28 18:00 ` Jim Mattson 0 siblings, 1 reply; 14+ messages in thread From: Krish Sadhukhan @ 2020-04-28 17:38 UTC (permalink / raw) To: Paolo Bonzini, Sean Christopherson; +Cc: Jim Mattson, kvm list 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2 v2] KVM: nVMX: KVM needs to unset "unrestricted guest" VM-execution control in vmcs02 if vmcs12 doesn't set it 2020-04-28 17:38 ` Krish Sadhukhan @ 2020-04-28 18:00 ` Jim Mattson 0 siblings, 0 replies; 14+ messages in thread From: Jim Mattson @ 2020-04-28 18:00 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: Paolo Bonzini, Sean Christopherson, kvm list 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2 v2] kvm-unit-tests: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests 2020-04-15 18:30 [PATCH 0/2 v2] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Krish Sadhukhan 2020-04-15 18:30 ` [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 @ 2020-04-15 18:30 ` Krish Sadhukhan 1 sibling, 0 replies; 14+ messages in thread From: Krish Sadhukhan @ 2020-04-15 18:30 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-04-28 18:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-15 18:30 [PATCH 0/2 v2] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers Krish Sadhukhan 2020-04-15 18:30 ` [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 2020-04-15 19:30 ` Sean Christopherson 2020-04-15 20:18 ` Jim Mattson 2020-04-16 9:18 ` Paolo Bonzini 2020-04-18 1:29 ` Krish Sadhukhan 2020-04-18 1:55 ` Sean Christopherson 2020-04-18 9:53 ` Paolo Bonzini 2020-04-20 15:12 ` Sean Christopherson 2020-04-28 7:25 ` Krish Sadhukhan 2020-04-28 8:14 ` Paolo Bonzini 2020-04-28 17:38 ` Krish Sadhukhan 2020-04-28 18:00 ` Jim Mattson 2020-04-15 18:30 ` [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
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.