Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 21 Jun 2021 17:08:13 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: nVMX: Dynamically compute max VMCS index for vmcs12
Message-ID: <YNDHfX0cntj72sk6@google.com>
References: <20210618214658.2700765-1-seanjc@google.com>
 <c847e00a-e422-cdc9-3317-fbbd82b6e418@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c847e00a-e422-cdc9-3317-fbbd82b6e418@redhat.com>
Precedence: bulk

On Mon, Jun 21, 2021, Paolo Bonzini wrote:
> On 18/06/21 23:46, Sean Christopherson wrote:
> > Calculate the max VMCS index for vmcs12 by walking the array to find the
> > actual max index.  Hardcoding the index is prone to bitrot, and the
> > calculation is only done on KVM bringup (albeit on every CPU, but there
> > aren't _that_ many null entries in the array).
> > 
> > Fixes: 3c0f99366e34 ("KVM: nVMX: Add a TSC multiplier field in VMCS12")
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > 
> > Note, the vmx test in kvm-unit-tests will still fail using stock QEMU,
> > as QEMU also hardcodes and overwrites the MSR.  The test passes if I
> > hack KVM to ignore userspace (it was easier than rebuilding QEMU).
> 
> Queued, thanks.  Without having checked the kvm-unit-tests sources very
> thoroughly, this might be a configuration issue in kvm-unit-tests; in theory
> "-cpu host" (unlike "-cpu host,migratable=no") should not enable TSC
> scaling.

As noted in the code comments, KVM allows VMREAD/VMWRITE to all defined fields,
whether or not the field should actually exist for the vCPU model doesn't enter
into the equation.  That's technically wrong as there are a number of fields
that the SDM explicitly states exist iff a certain feature is supported.  To fix
that we'd need to add a "feature flag" to vmcs_field_to_offset_table that is
checked against the vCPU model, though updating the MSR would probably fall onto
userspace's shoulders?

And FWIW, this is the QEMU code:

  #define VMCS12_MAX_FIELD_INDEX (0x17)

  static void kvm_msr_entry_add_vmx(X86CPU *cpu, FeatureWordArray f)
  {
      ...

      /*
       * Just to be safe, write these with constant values.  The CRn_FIXED1
       * MSRs are generated by KVM based on the vCPU's CPUID.
       */
      kvm_msr_entry_add(cpu, MSR_IA32_VMX_CR0_FIXED0,
                        CR0_PE_MASK | CR0_PG_MASK | CR0_NE_MASK);
      kvm_msr_entry_add(cpu, MSR_IA32_VMX_CR4_FIXED0,
                        CR4_VMXE_MASK);
      kvm_msr_entry_add(cpu, MSR_IA32_VMX_VMCS_ENUM,
                        VMCS12_MAX_FIELD_INDEX << 1);
  }