Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 7 Jul 2022 16:17:58 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Anirudh Rayabharam <anrayabh@linux.microsoft.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 06/28] KVM: nVMX: Introduce
 KVM_CAP_HYPERV_ENLIGHTENED_VMCS2
Message-ID: <YscHNur0OsViyyDJ@google.com>
References: <20220629150625.238286-1-vkuznets@redhat.com>
 <20220629150625.238286-7-vkuznets@redhat.com>
 <YsYAPL1UUKJB3/MJ@google.com>
 <87o7y1qm5t.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87o7y1qm5t.fsf@redhat.com>
Precedence: bulk

On Thu, Jul 07, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Jun 29, 2022, Vitaly Kuznetsov wrote:
> >> Turns out Enlightened VMCS can gain new fields without version change
> >> and KVM_CAP_HYPERV_ENLIGHTENED_VMCS which KVM currently has cant's
> >> handle this reliably. In particular, just updating the current definition
> >> of eVMCSv1 with the new fields and adjusting the VMX MSR filtering will
> >> inevitably break live migration to older KVMs. Note: enabling eVMCS and
> >> setting VMX feature MSR can happen in any order.
> >> 
> >> Introduce a notion of KVM internal "Enlightened VMCS revision" and add
> >> a new capability allowing to add fields to Enlightened VMCS while keeping
> >> its version.
> >
> > Bumping a "minor" version number in KVM is going to be a nightmare.  KVM is going
> > to be stuck "supporting" old revisions in perpetuity, and userspace will be forced
> > to keep track of which features are available with which arbitrary revision (is
> > that information even communicated to userspace?).
> 
> My brain is certainly tainted with how we enable this in QEMU but why
> would userspace be interested in which features are actually filtered
> out?

For all the same reasons userspace wants to know what hardware features are
supported by hardware and KVM.

> Currently (again, by QEMU), eVMCS is treated as a purely software
> feature. When enabled, certain controls are filtered out "under the
> hood" as VMX MSRs reported to VMM remain unfiltered (see
> '!msr_info->host_initiated' in vmx_get_msr()). Same stays true with any
> new revision: VMM's job is just to check that a) all hardware features
> are supported on both source and destination and b) the requested 'eVMCS
> revision' is supported by both. No need to know what's filtered out and
> what isn't.

But users will inevitably want to know exactly what features a platform supports
for a given configuration.  E.g. if KVM only supported pre-configured CPU models,
KVM would need to document what features are supported by each model, and userspace
would have very little flexibility in terms of what features are exposed to the
guest.  That's an exaggerated example as there are far more CPUID features than
eVMCS features, but it's the same underlying concept.

> > I think a more maintainable approach would be to expose the "filtered" VMX MSRs to
> > userspace, e.g. add KVM_GET_EVMCS_VMX_MSRS.  Then KVM just needs to document what
> > the "filters" are for KVM versions that don't support KVM_GET_EVMCS_VMX_MSRS.
> > KVM itself doesn't need to maintain version information because it's userspace's
> > responsibility to ensure that userspace doesn't try to migrate to a KVM that doesn't
> > support the desired feature set.
> 
> That would be a reasonable (but complex for VMM) approach too but I
> don't think we need this (and this patch introducing 'eVMCS revisions'
> to this matter):

But userspace already has to deal with that complexity in raw VMX MSRs.  The
filtered values will always be a subset of the unfiltered values, so if eVMCS
will be exposed to the guest, userspace can simply treat the filtered set as the
baseline supported set, i.e. the userspace logic could simply be:

	if (expose_evmcs)
		get_evmcs_vmx_msrs(&msrs);
	else
		get_vmx_msrs(&msrs);


Userspace will need to take on more complexity if userspace wants to expose features
that are supported in hardware but not with eVMCS active, but I don't see the point
in doing so because AFAICT KVM will just override and filter the VMX MSRs anyways
when eVMCS is enabled, i.e. userspace _can't_ expose the unfiltered VMX MSRs to the
guest when eVMCS is enabled.

> luckily, Microsoft added a new PV CPUID feature bit inidicating the support
> for the new features in eVMCSv1 so KVM can just observe whether the bit was
> set by VMM or not and filter accordingly.

If there's a CPUID feature bit, why does KVM need to invent its own revision scheme?

> > That also avoids messes like unnecessarily blocking migration from "incompatible"
> > revisions when running on hardware that doesn't even support the control.
> 
> Well yea, in case the difference between 'eVMCS revisions' is void
> because the hardware doesn't support these, it would still be possible
> to migrate to an older KVM which doesn't support the new revision but
> I'd stay strict: if a newer revision was requested it must be supported,
> no matter the hardware.