On 14/02/19 16:14, Sean Christopherson 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>
>>> ---
I pushed this already, since having a clean run for the tests is nice.
You can send further cleanups as a follow-up.
Thanks,
Paolo
>>> 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.
>
>>
>>> 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.
>
>>
>>> - 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.
>
>>
>>> 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
>>>
