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

[Haozhong Zhang <haozhong.zhang@intel.com>]

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@lists.xen.org
https://lists.xen.org/xen-devel