Re: [kvm-unit-tests PATCH] x86: nVMX: Dynamically calculate and check max VMCS field encoding index

Re: [kvm-unit-tests PATCH] x86: nVMX: Dynamically calculate and check max VMCS field encoding index

[Nadav Amit]

> On Apr 15, 2019, at 6:38 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> Per Intel's SDM:
> 
>  IA32_VMX_VMCS_ENUM indicates to software the highest index value used
>  in the encoding of any field supported by the processor:
>    - Bits 9:1 contain the highest index value used for any VMCS encoding.
>    - Bit 0 and bits 63:10 are reserved and are read as 0
> 
> KVM correctly emulates this behavior, in no small part due to the VMX
> preemption timer being unconditionally emulated *and* having the highest
> index of any field supported in vmcs12.  Given that the maximum control
> field index is already above the VMX preemption timer (0x32 vs 0x2E),
> odds are good that the max index supported in vmcs12 will change in the
> not-too-distant future.
> 
> Unfortunately, the only unit test coverage for IA32_VMX_VMCS_ENUM is in
> test_vmx_caps(), which simply checks that the max index is >= 0x2a, i.e.
> won't catch any future breakage of KVM's IA32_VMX_VMCS_ENUM emulation,
> especially if the max index depends on underlying hardware support.
> 
> Instead of playing whack-a-mole with a hardcoded max index test,
> piggyback the exhaustive VMWRITE/VMREAD test and dynamically calculate
> the max index based on which fields can be VMREAD.  Leave the existing
> hardcoded check in place as it won't hurt anything and test_vmx_caps()
> is a better location for checking the reserved bits of the MSR.

[ Yes, I know this patch was already accepted. ]

This patch causes me problems.

I think that probing using the known VMCS fields gives you a minimum for the
maximum index. There might be VMCS fields that the test does not know about.
Otherwise it would require to update kvm-unit-tests for every fields that is
added to kvm.

One option is just to change the max index, as determined by the probing to
be required to smaller or equal to IA32_VMX_VMCS_ENUM.MAX_INDEX. A second
option is to run additional probing, using IA32_VMX_VMCS_ENUM.MAX_INDEX and
see if it is supported.

What do you say?

Re: [kvm-unit-tests PATCH] x86: nVMX: Dynamically calculate and check max VMCS field encoding index

[Sean Christopherson]

On Mon, May 13, 2019 at 03:43:18PM -0700, Nadav Amit wrote:
> > On Apr 15, 2019, at 6:38 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > Per Intel's SDM:
> > 
> >  IA32_VMX_VMCS_ENUM indicates to software the highest index value used
> >  in the encoding of any field supported by the processor:
> >    - Bits 9:1 contain the highest index value used for any VMCS encoding.
> >    - Bit 0 and bits 63:10 are reserved and are read as 0
> > 
> > KVM correctly emulates this behavior, in no small part due to the VMX
> > preemption timer being unconditionally emulated *and* having the highest
> > index of any field supported in vmcs12.  Given that the maximum control
> > field index is already above the VMX preemption timer (0x32 vs 0x2E),
> > odds are good that the max index supported in vmcs12 will change in the
> > not-too-distant future.
> > 
> > Unfortunately, the only unit test coverage for IA32_VMX_VMCS_ENUM is in
> > test_vmx_caps(), which simply checks that the max index is >= 0x2a, i.e.
> > won't catch any future breakage of KVM's IA32_VMX_VMCS_ENUM emulation,
> > especially if the max index depends on underlying hardware support.
> > 
> > Instead of playing whack-a-mole with a hardcoded max index test,
> > piggyback the exhaustive VMWRITE/VMREAD test and dynamically calculate
> > the max index based on which fields can be VMREAD.  Leave the existing
> > hardcoded check in place as it won't hurt anything and test_vmx_caps()
> > is a better location for checking the reserved bits of the MSR.
> 
> [ Yes, I know this patch was already accepted. ]
> 
> This patch causes me problems.
> 
> I think that probing using the known VMCS fields gives you a minimum for the
> maximum index. There might be VMCS fields that the test does not know about.
> Otherwise it would require to update kvm-unit-tests for every fields that is
> added to kvm.
> 
> One option is just to change the max index, as determined by the probing to
> be required to smaller or equal to IA32_VMX_VMCS_ENUM.MAX_INDEX. A second
> option is to run additional probing, using IA32_VMX_VMCS_ENUM.MAX_INDEX and
> see if it is supported.
> 
> What do you say?

Argh, I thought the test I was piggybacking was exhaustively probing all
theoretically possible fields, but that's the VMCS shadowing test.  That
was my intent: probe all possible fields to find the max index and compare
it against IA32_VMX_VMCS_ENUM.MAX_INDEX.

To fix the immediate issue, going with the "smaller or equal" check makes
sense.  To get the coverage I originally intended, I'll work on a test to
find the max non-failing field and compare it against MAX_INDEX.

Re: [kvm-unit-tests PATCH] x86: nVMX: Dynamically calculate and check max VMCS field encoding index

[Nadav Amit]

> On May 15, 2019, at 9:45 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Mon, May 13, 2019 at 03:43:18PM -0700, Nadav Amit wrote:
>>> On Apr 15, 2019, at 6:38 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>> 
>>> Per Intel's SDM:
>>> 
>>> IA32_VMX_VMCS_ENUM indicates to software the highest index value used
>>> in the encoding of any field supported by the processor:
>>>   - Bits 9:1 contain the highest index value used for any VMCS encoding.
>>>   - Bit 0 and bits 63:10 are reserved and are read as 0
>>> 
>>> KVM correctly emulates this behavior, in no small part due to the VMX
>>> preemption timer being unconditionally emulated *and* having the highest
>>> index of any field supported in vmcs12.  Given that the maximum control
>>> field index is already above the VMX preemption timer (0x32 vs 0x2E),
>>> odds are good that the max index supported in vmcs12 will change in the
>>> not-too-distant future.
>>> 
>>> Unfortunately, the only unit test coverage for IA32_VMX_VMCS_ENUM is in
>>> test_vmx_caps(), which simply checks that the max index is >= 0x2a, i.e.
>>> won't catch any future breakage of KVM's IA32_VMX_VMCS_ENUM emulation,
>>> especially if the max index depends on underlying hardware support.
>>> 
>>> Instead of playing whack-a-mole with a hardcoded max index test,
>>> piggyback the exhaustive VMWRITE/VMREAD test and dynamically calculate
>>> the max index based on which fields can be VMREAD.  Leave the existing
>>> hardcoded check in place as it won't hurt anything and test_vmx_caps()
>>> is a better location for checking the reserved bits of the MSR.
>> 
>> [ Yes, I know this patch was already accepted. ]
>> 
>> This patch causes me problems.
>> 
>> I think that probing using the known VMCS fields gives you a minimum for the
>> maximum index. There might be VMCS fields that the test does not know about.
>> Otherwise it would require to update kvm-unit-tests for every fields that is
>> added to kvm.
>> 
>> One option is just to change the max index, as determined by the probing to
>> be required to smaller or equal to IA32_VMX_VMCS_ENUM.MAX_INDEX. A second
>> option is to run additional probing, using IA32_VMX_VMCS_ENUM.MAX_INDEX and
>> see if it is supported.
>> 
>> What do you say?
> 
> Argh, I thought the test I was piggybacking was exhaustively probing all
> theoretically possible fields, but that's the VMCS shadowing test.  That
> was my intent: probe all possible fields to find the max index and compare
> it against IA32_VMX_VMCS_ENUM.MAX_INDEX.
> 
> To fix the immediate issue, going with the "smaller or equal" check makes
> sense.  To get the coverage I originally intended, I'll work on a test to
> find the max non-failing field and compare it against MAX_INDEX.

FYI: This test is really annoying since it fails on bare-metal due to the
erratum "IA32_VMX_VMCS_ENUM MSR (48AH) Does Not Properly Report The Highest
Index Value Used For VMCS Encoding”.