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@xxxxxxxxx> 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.
