Re: [PATCH 2/2][kvm-unit-test] nVMX x86: Check PML and EPT on vmentry of L2 guests

[Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>]

On 09/26/2018 12:24 PM, Jim Mattson wrote:
> On Wed, Sep 26, 2018 at 11:18 AM, Krish Sadhukhan
> <krish.sadhukhan@xxxxxxxxxx> wrote:
> > According to section "Checks on VMX Controls" in Intel SDM vol 3C, the
> > following check needs to be enforced on vmentry of L2 guests:
> >
> >      If the "enable PML" VM-execution control is 1, the "enable EPT"
> >      VM-execution control must also be 1. In addition, the PML address
> >      must satisfy the following checks:
> >
> >        — Bits 11:0 of the address must be 0.
> >        — The address should not set any bits beyond the processor’s
> >          physical-address width.
> >
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
> > Reviewed-by: Mark Kanda <mark.kanda@xxxxxxxxxx>
> > ---
> >   x86/vmx.h       |  3 ++
> >   x86/vmx_tests.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> >   2 files changed, 91 insertions(+), 11 deletions(-)
> >
> > diff --git a/x86/vmx.h b/x86/vmx.h
> > index 22b2892..78a786e 100644
> > --- a/x86/vmx.h
> > +++ b/x86/vmx.h
> > @@ -448,6 +448,9 @@ enum Intr_type {
> >   #define INTR_TYPE_SOFT_EXCEPTION       (6 << 8) /* software exception */
> >   #define INTR_TYPE_OTHER_EVENT           (7 << 8) /* other event */
> >
> > +
> > +#define        PMLADDR_RESV_BITS               0xfff
> > +
> This is unnecessary, since these are the same bits that are reserved
> for all page references.

Agreed.

> >   /*
> >    * VM-instruction error numbers
> >    */
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > index 0e9d900..37db793 100644
> > --- a/x86/vmx_tests.c
> > +++ b/x86/vmx_tests.c
> > @@ -3452,14 +3452,18 @@ static void test_vmcs_page_addr(const char *name,
> >   static void test_vmcs_page_values(const char *name,
> >                                    enum Encoding encoding,
> >                                    bool ignored,
> > -                                 bool xfail_beyond_mapped_ram)
> > +                                 bool xfail_beyond_mapped_ram,
> > +                                 u64 resv_bits)
> >   {
> >          unsigned i;
> >          u64 orig_val = vmcs_read(encoding);
> >
> > -       for (i = 0; i < 64; i++)
> > +       for (i = 0; i < 64; i++) {
> > +               if (i & resv_bits)
> > +                       continue;
> >                  test_vmcs_page_addr(name, encoding, ignored,
> >                                      xfail_beyond_mapped_ram, 1ul << i);
> > +       }
> >
> >          test_vmcs_page_addr(name, encoding, ignored,
> >                              xfail_beyond_mapped_ram, PAGE_SIZE - 1);
> > @@ -3483,7 +3487,7 @@ static void test_vmcs_page_reference(u32 control_bit, enum Encoding field,
> >                                       const char *field_name,
> >                                       const char *control_name,
> >                                       bool xfail_beyond_mapped_ram,
> > -                                    bool control_primary)
> > +                                    bool control_primary, u64 resv_bits)
> >   {
> >          u32 primary = vmcs_read(CPU_EXEC_CTRL0);
> >          u32 secondary = vmcs_read(CPU_EXEC_CTRL1);
> > @@ -3506,7 +3510,8 @@ static void test_vmcs_page_reference(u32 control_bit, enum Encoding field,
> >                  vmcs_write(CPU_EXEC_CTRL0, primary | CPU_SECONDARY);
> >                  vmcs_write(CPU_EXEC_CTRL1, secondary | control_bit);
> >          }
> > -       test_vmcs_page_values(field_name, field, false, xfail_beyond_mapped_ram);
> > +       test_vmcs_page_values(field_name, field, false,
> > +           xfail_beyond_mapped_ram, resv_bits);
> >          report_prefix_pop();
> >
> >          report_prefix_pushf("%s disabled", control_name);
> > @@ -3516,7 +3521,7 @@ static void test_vmcs_page_reference(u32 control_bit, enum Encoding field,
> >                  vmcs_write(CPU_EXEC_CTRL0, primary & ~CPU_SECONDARY);
> >                  vmcs_write(CPU_EXEC_CTRL1, secondary & ~control_bit);
> >          }
> > -       test_vmcs_page_values(field_name, field, true, false);
> > +       test_vmcs_page_values(field_name, field, true, false, resv_bits);
> >          report_prefix_pop();
> >
> >          vmcs_write(field, page_addr);
> > @@ -3533,10 +3538,10 @@ static void test_io_bitmaps(void)
> >   {
> >          test_vmcs_page_reference(CPU_IO_BITMAP, IO_BITMAP_A,
> >                                   "I/O bitmap A", "Use I/O bitmaps", false,
> > -                                true);
> > +                                true, 0);
> >          test_vmcs_page_reference(CPU_IO_BITMAP, IO_BITMAP_B,
> >                                   "I/O bitmap B", "Use I/O bitmaps", false,
> > -                                true);
> > +                                true, 0);
> >   }
> >
> >   /*
> > @@ -3549,7 +3554,7 @@ static void test_msr_bitmap(void)
> >   {
> >          test_vmcs_page_reference(CPU_MSR_BITMAP, MSR_BITMAP,
> >                                   "MSR bitmap", "Use MSR bitmaps", false,
> > -                                true);
> > +                                true, 0);
> >   }
> >
> >   /*
> > @@ -3564,7 +3569,7 @@ static void test_apic_virt_addr(void)
> >   {
> >          test_vmcs_page_reference(CPU_TPR_SHADOW, APIC_VIRT_ADDR,
> >                                   "virtual-APIC address", "Use TPR shadow",
> > -                                true, true);
> > +                                true, true, 0);
> >   }
> >
> >   /*
> > @@ -3583,7 +3588,7 @@ static void test_apic_access_addr(void)
> >
> >          test_vmcs_page_reference(CPU_VIRT_APIC_ACCESSES, APIC_ACCS_ADDR,
> >                                   "APIC-access address",
> > -                                "virtualize APIC-accesses", false, false);
> > +                                "virtualize APIC-accesses", false, false, 0);
> >   }
> >
> >   static bool set_bit_pattern(u8 mask, u32 *secondary)
> > @@ -3869,7 +3874,8 @@ static void test_posted_intr(void)
> >          test_pi_desc_addr(0x00, true);
> >          test_pi_desc_addr(0xc000, true);
> >
> > -       test_vmcs_page_values("process-posted interrupts", POSTED_INTR_DESC_ADDR, false, false);
> > +       test_vmcs_page_values("process-posted interrupts",
> > +           POSTED_INTR_DESC_ADDR, false, false, 0);
> >
> >          vmcs_write(CPU_EXEC_CTRL0, saved_primary);
> >          vmcs_write(CPU_EXEC_CTRL1, saved_secondary);
> > @@ -4408,6 +4414,76 @@ done:
> >          vmcs_write(PIN_CONTROLS, pin_ctrls);
> >   }
> >
> > +/*
> > + * If the “enable PML” VM-execution control is 1, the “enable EPT”
> > + * VM-execution control must also be 1. In addition, the PML address
> > + * must satisfy the following checks:
> > + *
> > + *    — Bits 11:0 of the address must be 0.
> > + *    — The address should not set any bits beyond the processor’s
> > + *     physical-address width.
> > +
> > + *  [Intel SDM]
> > + */
> > +static void test_pml(void)
> > +{
> > +       u32 primary_saved = vmcs_read(CPU_EXEC_CTRL0);
> > +       u32 secondary_saved = vmcs_read(CPU_EXEC_CTRL1);
> > +       u32 primary = primary_saved;
> > +       u32 secondary = secondary_saved;
> > +       u64 pml_addr;
> > +       int i;
> > +
> > +       if (!((ctrl_cpu_rev[0].clr & CPU_SECONDARY) &&
> > +           (ctrl_cpu_rev[1].clr & CPU_EPT) && (ctrl_cpu_rev[1].clr & CPU_PML))) {
> > +               test_skip("\"Secondary execution\" control or \"enable EPT\" control or \"enable PML\" control is not supported !");
> > +               return;
> > +       }
> > +
> > +       primary |= CPU_SECONDARY;
> > +       vmcs_write(CPU_EXEC_CTRL0, primary);
> > +       secondary &= ~(CPU_PML | CPU_EPT);
> > +       vmcs_write(CPU_EXEC_CTRL1, secondary);
> > +       report_prefix_pushf("enable-PML disabled, enable-EPT disabled");
> > +       test_vmx_controls(true, false);
> > +       report_prefix_pop();
> > +
> > +       secondary |= CPU_PML;
> > +       vmcs_write(CPU_EXEC_CTRL1, secondary);
> > +       report_prefix_pushf("enable-PML enabled, enable-EPT disabled");
> > +       test_vmx_controls(false, false);
> > +       report_prefix_pop();
> > +
> > +       secondary |= CPU_EPT;
> > +       vmcs_write(CPU_EXEC_CTRL1, secondary);
> > +       report_prefix_pushf("enable-PML enabled, enable-EPT enabled");
> > +       test_vmx_controls(true, false);
> > +       report_prefix_pop();
> > +
> > +       secondary &= ~CPU_PML;
> > +       vmcs_write(CPU_EXEC_CTRL1, secondary);
> > +       report_prefix_pushf("enable-PML disabled, enable EPT enabled");
> > +       test_vmx_controls(true, false);
> > +       report_prefix_pop();
> > +
> > +       secondary |= CPU_PML;
> > +       vmcs_write(CPU_EXEC_CTRL1, secondary);
> > +
> > +       pml_addr = (u64) phys_to_virt(vmcs_read(PMLADDR));
> > +       for (i = 0; i < 12; i++) {
> > +               pml_addr |= 1ull << i;
> > +               vmcs_write(PMLADDR, virt_to_phys((void *) pml_addr));
> > +               report_prefix_pushf("PML address 0x%lx", pml_addr);
> > +               test_vmx_controls(false, false);
> > +               report_prefix_pop();
> > +       }
> > +
> > +       test_vmcs_page_reference(CPU_PML, PMLADDR, "PML address",
> > +           "PML", false, false, PMLADDR_RESV_BITS);
> > +
> > +       vmcs_write(CPU_EXEC_CTRL0, primary_saved);
> > +       vmcs_write(CPU_EXEC_CTRL1, secondary_saved);
> > +}
> Except for the "enable EPT" requirement, this can all be done with a
> variant of test_vmcs_page_reference that takes a secondary
> processor-based control bit rather than a primary processor-based
> control-bit.

It seems that for this patch, the existing test_vmcs_page_reference() is 
sufficient. Also, test_vmcs_page_addr() already covers the cases for 
reserved bits (sorry, I missed it earlier). So all I need to do is just 
call test_vmcs_page_reference() after the "enable EPT" part.