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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On 12/16/16 16:17 +0000, Andrew Cooper wrote:
> 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?
same as my understanding

> 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).
OK, then TODO is not necessary at all. My concern was that Intel SDM
says MSR_IA32_FEATURE_CONTROL is cleared to zero when a logical
processor is reset that is not consistent to what Xen currently
presents in HVM domains.

> > + */
> > +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.
will change

> > +        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.
Thanks for reminding me of this which I didn't notice.

> > +        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.
I'll add case to test vmxon w/ VMX is turned off by hypervisor.

> > +    }
> > +
> > +    if ( (msr_feat_ctrl & IA32_FEATURE_CONTROL_ENABLE_VMXON_INSIDE_SMX) &&
> > +         !(cpuid_feat_ecx & X86_FEATURE_SMX) )
> cpu_has_smx please.
Will change.

Thanks,
Haozhong

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