> On May 15, 2019, at 9:45 AM, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> 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@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. 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”.
