Re: [XTF PATCH 02/16] vvmx: test whether MSR_IA32_FEATURE_CONTROL is set correctly

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

On 16/12/16 13:43, Haozhong Zhang wrote:
> diff --git a/tests/vvmx/msr.c b/tests/vvmx/msr.c
> new file mode 100644
> index 0000000..100491d
> --- /dev/null
> +++ b/tests/vvmx/msr.c
> @@ -0,0 +1,67 @@
> +#include <xtf.h>
> +
> +#include <arch/x86/msr-index.h>
> +
> +/*
> + * NB. Guest MSR_IA32_FEATURE_CONTROL is set by Xen hypervisor instead
> + * by guest firmware or hvmloader, so this test checks whether bits in
> + * MSR_IA32_FEATURE_CONTROL are set correctly and does not require they
> + * are all zero.
> + *
> + * TODO: If the behavior in above NB is changed in future, remember to
> + * adjust this test.

What are the purposes of these lock bits on real hardware?  I presume it
is to allow a user choice in the BIOS, but to force the choice to be
immutable once the OS loads?

If so, I don't expect Xen's behaviour to ever change.  We'd prefer to do
all configuration like that at the toolstack/hypervisor level, rather
than the in-guest firmware (if any).

> + */
> +static bool test_msr_feature_control(void)
> +{
> +    bool passed = true;
> +    uint32_t cpuid_feat_ecx = cpuid_ecx(1);
> +    uint64_t msr_feat_ctrl;
> +
> +    if ( rdmsr_safe(MSR_IA32_FEATURE_CONTROL, &msr_feat_ctrl) )
> +    {
> +        xtf_failure("Fail: fault when rdmsr MSR_IA32_FEATURE_CONTROL\n");

"Fail: Fault when reading MSR_IA32_FEATURE_CONTROL\n" would be slightly
clearer.

> +        passed = false;

The status functions including xtf_failure and xtf_success are sticky,
worst-takes-priority.  Therefore, you don't need the xtf_failure(NULL)
case in test_main().  You can also even leave via the xtf_success(NULL)
case and the overall report will be FAILURE.

> +        goto out;

In the past, I have found it very useful to proceed with as many checks
as reasonable, even in the face of a failure.

Sometime, a subtle change in Xen can have a cascade failure for the
guest, and seeing all the failures at once can help identify which area
went wrong.

> +    }
> +
> +    if ( (msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX) &&
> +         !(cpuid_feat_ecx & X86_FEATURE_SMX) )

cpu_has_smx please.

~Andrew

> +    {
> +        xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[1] is set "
> +                    "but SMX is not supported\n");
> +        passed = false;
> +    }
> +
> +    if ( !(msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_OUTSIDE_SMX) )
> +    {
> +        xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[2] is not set\n");
> +        passed = false;
> +    }
> +
> +    if ( !(msr_feat_ctrl & IA32_FEATURE_CONTROL_LOCK) )
> +    {
> +        xtf_failure("Fail: MSR_IA32_FEATURE_CONTROL[0] is not set\n");
> +        passed = false;
> +    }
> +
> +out:
> +    return passed;
> +}
> +
> +bool test_msr_vmx(void)
> +{
> +    if ( !test_msr_feature_control() )
> +        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