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

[Sean Christopherson <sean.j.christopherson@intel.com>]

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.