On Wed, Feb 13, 2019 at 07:17:55PM -0800, Krish Sadhukhan wrote:
>
>
> On 02/12/2019 03:04 PM, Sean Christopherson 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. */
> >-static int setup_ept(bool enable_ad)
> >+static int setup_eptp(u64 hpa, bool enable_ad)
>
> I think the new function names don't reflect accurately what they are
> doing. setup_eptp() is doing more than just setting up the EPTP; it is
> checking the EPT bits as well as setting the "Enable EPT" Secondary
> Execution Control. A cleaner approach is to completely separate the
> EPT-related code to setup_ept() while putting only the EPTP-related code in
> setup_eptp().
Another name I considered was setup_fake_ept(). Maybe setup_dummy_ept()?
Then the common helper can be renamed __setup_ept().
> If the goal is to use a dummy EPTP address for some tests, you can also add
> a parameter to the existing setup_ept() instead of creating these wrappers.
I strongly dislike passing multiple booleans, e.g. "setup_ept(false, true)",
especially when one of the booleans drastically changes the function's
behavior. It all but requires me to check function definition to remember
which bool controls which behavior. Passing the address explicitly then
requires callers to allocate the page or pass a dummy value, which isn't
the end of the world but IMO has a higher maintenance cost.
> >@@ -4678,8 +4693,7 @@ static void test_ept_eptp(void)
> > report_prefix_pop();
> > secondary |= CPU_EPT;
>
> This line can be removed as the existing setup_ept() does it anyway.
No, secondary needs to be kept up-to-date as its used to toggle CPU_URG
later in the test. Note that this is just writing a local variable,
setup_etp() writes the VMCS.
>
> >- setup_ept(false);
> >- vmcs_write(CPU_EXEC_CTRL1, secondary);
> >+ enable_ept();
> > 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;
>
> This line can be removed as the existing setup_ept() does it anyway.
Same here, except its CPU_PML this time.
> >- setup_ept(false);
> >- vmcs_write(CPU_EXEC_CTRL1, secondary);
> >+ enable_ept();
> > report_prefix_pushf("enable-PML enabled, enable-EPT enabled");
> > test_vmx_controls(true, false);
> > report_prefix_pop();
>
