Re: [XTF PATCH 06/16] vvmx: test vmxon with CR4.VMXE cleared

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 16/12/16 13:43, Haozhong Zhang wrote:
> diff --git a/tests/vvmx/util.c b/tests/vvmx/util.c
> index 74b4d01..f30fc26 100644
> --- a/tests/vvmx/util.c
> +++ b/tests/vvmx/util.c
> @@ -1,7 +1,31 @@
>  #include <xtf.h>
> +#include <arch/x86/msr-index.h>
>  #include <arch/x86/hvm/vmx/vmcs.h>
>  #include "util.h"
>  
> +#define INVALID_VMCS_REVID  (~(uint32_t)0)
> +
> +static uint32_t vmcs_revid = INVALID_VMCS_REVID;
> +
> +uint32_t get_vmcs_revid(void)
> +{
> +    if ( vmcs_revid != INVALID_VMCS_REVID )
> +        goto out;
> +
> +    uint64_t vmx_basic = rdmsr(MSR_IA32_VMX_BASIC);
> +
> +    vmcs_revid = (uint32_t)vmx_basic & VMX_BASIC_REVISION_MASK;
> +
> +out:
> +    return vmcs_revid;
> +}
> +
> +void clear_vmcs(void *vmcs, uint32_t revid)

Strictly speaking, this should be setup_vmcs() as it does more than just
clearing it.

> +{
> +    memset(vmcs, 0, PAGE_SIZE);
> +    *((uint32_t *)vmcs) = revid;
> +}
> +
>  #define vvmx_failure(prefix, fmt, ...)                       \
>      do {                                                     \
>          xtf_failure("Fail: %s: "fmt, prefix, ##__VA_ARGS__); \
> diff --git a/tests/vvmx/vmxon.c b/tests/vvmx/vmxon.c
> index e69de29..31f074c 100644
> --- a/tests/vvmx/vmxon.c
> +++ b/tests/vvmx/vmxon.c
> @@ -0,0 +1,47 @@
> +#include <xtf.h>
> +
> +#include "util.h"
> +
> +static uint8_t vmxon_region[PAGE_SIZE] __aligned(PAGE_SIZE);
> +
> +/**
> + * vmxon with CR4.VMXE cleared
> + *
> + * Expect: #UD
> + */
> +static bool test_vmxon_novmxe(void)
> +{
> +    uint8_t ret;
> +    exinfo_t fault;
> +
> +    if ( read_cr4() & X86_CR4_VMXE )
> +    {
> +        xtf_skip("Skip: CR4.VMXE is already set, "
> +                 "skip testing vmxon w/ CR4.VMXE=0\n");
> +        return true;

Tests should, wherever possible, not depend on the starting state of the
microkernel.  They are also entirely free to mess with any state they
want to construct a test scenario.

In this case, I would suggest...

> +    }
> +
> +    clear_vmcs(vmxon_region, get_vmcs_revid());
> +    ret = vmxon((uint64_t)vmxon_region, &fault);
> +
> +    return handle_vmxinsn_err(__func__, ret, fault,
> +                              VMXERR_FAULT, EXINFO_SYM(UD, 0), 0);
> +}
> +
> +bool test_vmxon(void)
> +{

unsigned long cr4 = read_cr4();

if ( cr4 & X86_CR4_VMXE )
    write_cr4(cr4 & ~X86_CR4_VMXE);

to explicitly set up state as intended.

~Andrew

> +    if ( !test_vmxon_novmxe() )
> +        return false;
> +
> +    return true;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel