| Message ID | 20190212230402.26851-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Fix unrestricted guest toggling | expand |
On Tue, Feb 12, 2019 at 3:04 PM Sean Christopherson <sean.j.christopherson@intel.com> 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@oracle.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > 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 >
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@oracle.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > 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(). 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. > { > - 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"); > 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. */ > +static int setup_ept(bool enable_ad) > +{ > + unsigned long end_of_memory; > + > pml4 = alloc_page(); > + > + setup_eptp(virt_to_phys(pml4), enable_ad); > + > 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; This line can be removed as the existing setup_ept() does it anyway. > - 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. > - 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();
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@oracle.com> > >Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > >--- > > 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(); >
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@intel.com> 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@oracle.com> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > 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 > >
On Thu, Feb 14, 2019 at 7:14 AM Sean Christopherson <sean.j.christopherson@intel.com> 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@intel.com> 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@oracle.com> > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > --- > > > 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 :-)!
On Thu, Feb 14, 2019 at 07:25:15AM -0800, Marc Orr wrote: > On Thu, Feb 14, 2019 at 7:14 AM Sean Christopherson > <sean.j.christopherson@intel.com> 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@intel.com> 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@oracle.com> > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > --- > > > > 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)) { No, the code is correct. The unit test framework doesn't require both UC and WB to be supported, rather it requires one of UC *or* 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; Blech, this code is backwards, we should prefer WB, not UC. Maybe a less confusing way to write everything would be: if (ept_vpid.val & EPT_CAP_WB) { eptp = EPT_MEM_TYPE_WB; } else if (ept_vpid.val & EPT_CAP_UC) { eptp = EPT_MEM_TYPE_UC; } else { printf("No usable EPT memtype (UC or WB) supported\n"); return 1; }
On Thu, Feb 14, 2019 at 7:43 AM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > On Thu, Feb 14, 2019 at 07:25:15AM -0800, Marc Orr wrote: > > On Thu, Feb 14, 2019 at 7:14 AM Sean Christopherson > > <sean.j.christopherson@intel.com> 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@intel.com> 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@oracle.com> > > > > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > > > > --- > > > > > 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)) { > > No, the code is correct. The unit test framework doesn't require both > UC and WB to be supported, rather it requires one of UC *or* 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; > > Blech, this code is backwards, we should prefer WB, not UC. > > Maybe a less confusing way to write everything would be: > > if (ept_vpid.val & EPT_CAP_WB) { > eptp = EPT_MEM_TYPE_WB; > } else if (ept_vpid.val & EPT_CAP_UC) { > eptp = EPT_MEM_TYPE_UC; > } else { > printf("No usable EPT memtype (UC or WB) supported\n"); > return 1; > } I agree, this code looks like a huge improvement. It's much easier to understand.
On 02/14/2019 07:05 AM, Sean Christopherson wrote: > 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@oracle.com> >>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >>> --- >>> 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()? setup_dummy_ept() seems a much better choice. > 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();
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@intel.com> 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@oracle.com> >>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> >>> --- 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 >>>
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) { - 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"); 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. */ +static int setup_ept(bool enable_ad) +{ + unsigned long end_of_memory; + pml4 = alloc_page(); + + setup_eptp(virt_to_phys(pml4), enable_ad); + 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(); 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(); report_prefix_pushf("enable-PML enabled, enable-EPT enabled"); test_vmx_controls(true, false); report_prefix_pop();
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@oracle.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- x86/vmx_tests.c | 51 +++++++++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 19 deletions(-)