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.
> -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.
> 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?
> +
> memset(pml4, 0, PAGE_SIZE);
I'd move pml4 = alloc_page() above this memset.
> - 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?
> 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?
> report_prefix_pushf("enable-PML enabled, enable-EPT enabled");
> test_vmx_controls(true, false);
> report_prefix_pop();
> --
> 2.20.1
>
