On Tue, Feb 12, 2019 at 3:04 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> wrote:
>
> The hardware exception injection test toggles unrestricted guest so that
> it can test the case where an event is injected into real mode with and
> without an error code (exception error codes don't exist in real mode).
> Unrestricted guest has its own requirements, specifically that EPT is
> also enabled (since IA32 paging could be disabled).
>
> Unfortunately, the enable_unrestricted_guest() helper fails to ensure
> EPT is enabled, which causes all subsequent VMLAUNCH instructions to
> fail with "invalid control field". Use the new added enable_ept() to
> configure unrestricted guest. In addition, assert that unrestricted
> guest is disabled at the beginning of the relevant section as things
> will likely go sideways if unrestricted guest is already enabled, e.g.
> odds are good it was enabled in order to muck with CR0. This allows
> for the removal of disable_unrestricted_guest() entirely. And finally,
> clean up the control fields after finishing the unrestricted guest
> section (instead of invoking the defunct disable_unrestricted_guest()).
>
> Note that it's not the unrestricted guest tests that fail, since there
> is no "vmlaunch succeeds" variant, rather its the following tests that
> expect success that end up failing (because the shoddy enabling of URG
> isn't undone).
>
> Fixes: 8d2cdb3 ("x86: Add test for nested VM entry prereqs")
> Cc: Marc Orr <marcorr@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> x86/vmx.h | 18 ------------------
> x86/vmx_tests.c | 32 +++++++++++++++++++++++++++++---
> 2 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 8a00f73..bcfaa79 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -779,24 +779,6 @@ static inline bool invvpid(unsigned long type, u64 vpid, u64 gla)
> return ret;
> }
>
> -static inline int enable_unrestricted_guest(void)
> -{
> - if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY))
> - return -1;
> -
> - if (!(ctrl_cpu_rev[1].clr & CPU_URG))
> - return -1;
> -
> - vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY);
> - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG);
> - return 0;
> -}
> -
> -static inline void disable_unrestricted_guest(void)
> -{
> - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) & ~CPU_URG);
> -}
> -
> const char *exit_reason_description(u64 reason);
> void print_vmexit_info(void);
> void print_vmentry_failure_info(struct vmentry_failure *failure);
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 4cfb55f..ff51185 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1067,6 +1067,21 @@ static int enable_ept(void)
> return setup_eptp(0, false);
> }
>
> +static int enable_unrestricted_guest(void)
> +{
> + if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) ||
> + !(ctrl_cpu_rev[1].clr & CPU_URG))
> + return 1;
> +
> + if (enable_ept())
> + return 1;
> +
> + vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY);
> + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG);
> +
> + return 0;
> +}
> +
> static void ept_enable_ad_bits(void)
> {
> eptp |= EPTP_AD_FLAG;
> @@ -4081,12 +4096,16 @@ static void test_invalid_event_injection(void)
> * (a) the "unrestricted guest" VM-execution control is 0
> * (b) CR0.PE is set.
> */
> +
> + /* Assert that unrestricted guest is disabled or unsupported */
> + assert(!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) ||
> + !(secondary_save & CPU_URG));
> +
> ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION |
> GP_VECTOR;
> report_prefix_pushf("%s, VM-entry intr info=0x%x",
> "error code <-> (!URG || prot_mode) [-]",
> ent_intr_info);
> - disable_unrestricted_guest();
> vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG);
> vmcs_write(ENT_INTR_INFO, ent_intr_info);
> test_vmx_controls(false, false);
> @@ -4097,18 +4116,19 @@ static void test_invalid_event_injection(void)
> report_prefix_pushf("%s, VM-entry intr info=0x%x",
> "error code <-> (!URG || prot_mode) [+]",
> ent_intr_info);
> - disable_unrestricted_guest();
> vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG);
> vmcs_write(ENT_INTR_INFO, ent_intr_info);
> test_vmx_controls(true, false);
> report_prefix_pop();
>
> + if (enable_unrestricted_guest())
> + goto skip_unrestricted_guest;
> +
> ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK |
> INTR_TYPE_HARD_EXCEPTION | GP_VECTOR;
> report_prefix_pushf("%s, VM-entry intr info=0x%x",
> "error code <-> (!URG || prot_mode) [-]",
> ent_intr_info);
> - enable_unrestricted_guest();
> vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG);
> vmcs_write(ENT_INTR_INFO, ent_intr_info);
> test_vmx_controls(false, false);
> @@ -4124,6 +4144,12 @@ static void test_invalid_event_injection(void)
> test_vmx_controls(false, false);
> report_prefix_pop();
>
> + vmcs_write(CPU_EXEC_CTRL1, secondary_save);
> + vmcs_write(CPU_EXEC_CTRL0, primary_save);
Writing these inside of a helper, enable_unrestricted_guest(), and
restoring them explicitly here, seems oddly asymmetric to me. I guess
I'd prefer to see these vmcs_write() operations in a helper that
corresponds to enable_unrestricted_guest(). At a minimum, I think we
should put a comment here, to say that these vmcs_write() operations
are to restore these fields to their prior state, before
enable_unrestricted_guest().
> +
> +skip_unrestricted_guest:
> + vmcs_write(GUEST_CR0, guest_cr0_save);
> +
> /* deliver-error-code is 1 iff the interruption type is HW exception */
> report_prefix_push("error code <-> HW exception");
> for (cnt = 0; cnt < 8; cnt++) {
> --
> 2.20.1
>
Reviewed-by: Marc Orr <marcorr@xxxxxxxxxx>
