Re: [kvm-unit-tests PATCH 2/3] KVM: nVMX: Add enable_ept() helper to configure legal EPTP

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 14, 2019 at 7:43 AM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> On Thu, Feb 14, 2019 at 07:25:15AM -0800, Marc Orr wrote:
> > On Thu, Feb 14, 2019 at 7:14 AM Sean Christopherson
> > <sean.j.christopherson@xxxxxxxxx> wrote:
> > >
> > > On Wed, Feb 13, 2019 at 07:07:34PM -0800, Marc Orr wrote:
> > > > On Tue, Feb 12, 2019 at 3:04 PM Sean Christopherson
> > > > <sean.j.christopherson@xxxxxxxxx> wrote:
> > > > >
> > > > > Enabling EPT requires a valid EPTP, but that only means the EPTP itself
> > > > > must satisfy the VM-Enter consistency checks.  Split out the EPTP setup
> > > > > to a separate helper and wrap it with a new helper, enable_ept(), that
> > > > > uses a dummy top-level EPT table, i.e. address 0.  This skips allocating
> > > > > a page and setting up the EPT tables for tests that just want to set
> > > > > EPT=1 to satisfy a dependent consistency check, e.g. unrestricted guest.
> > > > >
> > > > > Fixes: b57936c ("If "enable EPT" is enabled in a test, EPT pointer must also be set up")
> > > > > Cc: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > > > ---
> > > > >  x86/vmx_tests.c | 51 +++++++++++++++++++++++++++++++------------------
> > > > >  1 file changed, 32 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > > > > index a990081..4cfb55f 100644
> > > > > --- a/x86/vmx_tests.c
> > > > > +++ b/x86/vmx_tests.c
> > > > > @@ -1004,42 +1004,52 @@ static int insn_intercept_exit_handler(void)
> > > > >         return VMX_TEST_RESUME;
> > > > >  }
> > > > >
> > > > > -
> > > > > -/* Enables EPT and sets up the identity map. */
> > > >
> > > > I think a comment before the function, similar to setup_ept() would be
> > > > nice. In particular, it would be helpful to say that setup_eptp()
> > > > returns 0 upon success and also summarize the function's arguments,
> > > > hpa and enable_ad.
> > >
> > > Comments would be nice.
> > >
> > > > > -static int setup_ept(bool enable_ad)
> > > > > +static int setup_eptp(u64 hpa, bool enable_ad)
> > > > >  {
> > > > > -       unsigned long end_of_memory;
> > > > > -
> > > > >         if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) ||
> > > > >             !(ctrl_cpu_rev[1].clr & CPU_EPT)) {
> > > > >                 printf("\tEPT is not supported");
> > > > >                 return 1;
> > > > >         }
> > > > >
> > > > > -
> > > > >         if (!(ept_vpid.val & EPT_CAP_UC) &&
> > > > >                         !(ept_vpid.val & EPT_CAP_WB)) {
> > > > >                 printf("\tEPT paging-structure memory type "
> > > > >                                 "UC&WB are not supported\n");
> > > >
> > > > Is the text in this print statement consistent with the check? It
> > > > looks like the check is saying that ept_vpid should have the
> > > > EPT_CAP_UC or EPT_CAP_WB set---not that it shouldn't have both set.
> > >
> > > Heh, it's correct, just poorly worded.  The "&" is trying to convey
> > > that both UC and WB are both unsupported.  A better wording might be:
> > >
> > >     No usable EPT paging-structure memory type (UC or WB) supported.
> > >
> >
> > Should the if condition be like this then (I removed the bangs)?
> >          if ((ept_vpid.val & EPT_CAP_UC) &&
> >                          (ept_vpid.val & EPT_CAP_WB)) {
>
> No, the code is correct.  The unit test framework doesn't require both
> UC and WB to be supported, rather it requires one of UC *or* WB.
>
> >
> > Below, having the bits set seems to dictate the caching behavior, not
> > clearing them, as is checked in this if statement:
> >         if (ept_vpid.val & EPT_CAP_UC)
> >                 eptp = EPT_MEM_TYPE_UC;
> >         else
> >                 eptp = EPT_MEM_TYPE_WB;
>
> Blech, this code is backwards, we should prefer WB, not UC.
>
> Maybe a less confusing way to write everything would be:
>
>         if (ept_vpid.val & EPT_CAP_WB) {
>                 eptp = EPT_MEM_TYPE_WB;
>         } else if (ept_vpid.val & EPT_CAP_UC) {
>                 eptp = EPT_MEM_TYPE_UC;
>         } else {
>                 printf("No usable EPT memtype (UC or WB) supported\n");
>                 return 1;
>         }

I agree, this code looks like a huge improvement. It's much easier to
understand.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux