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)) {
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;
> >
> > > return 1;
> > > }
> > > + if (!(ept_vpid.val & EPT_CAP_PWL4)) {
> > > + printf("\tPWL4 is not supported\n");
> > > + return 1;
> > > + }
> > > +
> > > if (ept_vpid.val & EPT_CAP_UC)
> > > eptp = EPT_MEM_TYPE_UC;
> > > else
> > > eptp = EPT_MEM_TYPE_WB;
> > > - if (!(ept_vpid.val & EPT_CAP_PWL4)) {
> > > - printf("\tPWL4 is not supported\n");
> > > - return 1;
> > > - }
> > > + eptp |= (3 << EPTP_PG_WALK_LEN_SHIFT);
> > > + eptp |= hpa;
> > > + if (enable_ad)
> > > + eptp |= EPTP_AD_FLAG;
> > > +
> > > + vmcs_write(EPTP, eptp);
> > > vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0)| CPU_SECONDARY);
> > > vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1)| CPU_EPT);
> > > - eptp |= (3 << EPTP_PG_WALK_LEN_SHIFT);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* Enables EPT and sets up the identity map. */
> >
> > If you add the comment above, it would also be nice to extend this
> > comment to summarize the return value and enable_ad arg.
> >
> > > +static int setup_ept(bool enable_ad)
> > > +{
> > > + unsigned long end_of_memory;
> > > +
> > > pml4 = alloc_page();
> > > +
> > > + setup_eptp(virt_to_phys(pml4), enable_ad);
> >
> > Should you check the return value of setup_eptp() here?
>
> Doh, yes.
>
> > > +
> > > memset(pml4, 0, PAGE_SIZE);
> >
> > I'd move pml4 = alloc_page() above this memset.
>
> I'm confused, it's already above memset.
>
I mean, can we put the allocation and memset adjacent to each other,
like this? Or does setup_eptp() expect pml4 to be allocated and not
memset()'d?
setup_eptp(virt_to_phys(pml4), enable_ad);
pml4 = alloc_page();
memset(pml4, 0, PAGE_SIZE);
> >
> > > - eptp |= virt_to_phys(pml4);
> > > - if (enable_ad)
> > > - eptp |= EPTP_AD_FLAG;
> > > - vmcs_write(EPTP, eptp);
> > > +
> > > end_of_memory = fwcfg_get_u64(FW_CFG_RAM_SIZE);
> > > if (end_of_memory < (1ul << 32))
> > > end_of_memory = (1ul << 32);
> > > @@ -1052,6 +1062,11 @@ static int setup_ept(bool enable_ad)
> > > return 0;
> > > }
> > >
> > > +static int enable_ept(void)
> > > +{
> > > + return setup_eptp(0, false);
> > > +}
> > > +
> > > static void ept_enable_ad_bits(void)
> > > {
> > > eptp |= EPTP_AD_FLAG;
> > > @@ -4678,8 +4693,7 @@ static void test_ept_eptp(void)
> > > report_prefix_pop();
> > >
> > > secondary |= CPU_EPT;
> > > - setup_ept(false);
> > > - vmcs_write(CPU_EXEC_CTRL1, secondary);
> > > + enable_ept();
> >
> > Should you check the return value here?
>
> Technically yes? The test explicitly checks that EPT is support, i.e.
> if enable_ept() then the test will fail anyways. Probably makes more
> sense to change enable_ept() to spaz out on failure since I'm pretty
> sure all callers assume it will succeed.
Whatever you think is best. If it's best and easiest to ignore the
return value, I'd add a comment to make that clear. Otherwise,
modifying enable_ept() to blow up SGTM.
>
> >
> > > report_prefix_pushf("Enable-EPT enabled, unrestricted-guest enabled");
> > > test_vmx_controls(true, false);
> > > report_prefix_pop();
> > > @@ -4734,8 +4748,7 @@ static void test_pml(void)
> > > report_prefix_pop();
> > >
> > > secondary |= CPU_EPT;
> > > - setup_ept(false);
> > > - vmcs_write(CPU_EXEC_CTRL1, secondary);
> > > + enable_ept();
> >
> > Should you check the return value here?
>
> Same as above.
>
> > > report_prefix_pushf("enable-PML enabled, enable-EPT enabled");
> > > test_vmx_controls(true, false);
> > > report_prefix_pop();
> > > --
> > > 2.20.1
> > >
Also, as Paolo said, thanks for fixing this up :-)!
