Re: [PATCH RFC 2/3] x86/kvm/hyper-v: move VMX controls sanitization out of nested_enable_evmcs()

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
	kvm@vger.kernel.org, Jim Mattson <jmattson@google.com>,
	linux-kernel@vger.kernel.org, Liran Alon <liran.alon@oracle.com>,
	Roman Kagan <rkagan@virtuozzo.com>
Subject: Re: [PATCH RFC 2/3] x86/kvm/hyper-v: move VMX controls sanitization out of nested_enable_evmcs()
Date: Tue, 21 Jan 2020 21:47:24 -0800	[thread overview]
Message-ID: <20200122054724.GD18513@linux.intel.com> (raw)
In-Reply-To: <6c4bdb57-08fb-2c2d-9234-b7efffeb72ed@redhat.com>

On Sun, Jan 19, 2020 at 09:54:44AM +0100, Paolo Bonzini wrote:
> On 15/01/20 18:10, Vitaly Kuznetsov wrote:
> > With fine grained VMX feature enablement QEMU>=4.2 tries to do KVM_SET_MSRS
> > with default (matching CPU model) values and in case eVMCS is also enabled,
> > fails.
> > 
> > It would be possible to drop VMX feature filtering completely and make
> > this a guest's responsibility: if it decides to use eVMCS it should know
> > which fields are available and which are not. Hyper-V mostly complies to
> > this, however, there is at least one problematic control:
> > SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES
> > which Hyper-V enables. As there is no 'apic_addr_field' in eVMCS, we
> > fail to handle this properly in KVM. It is unclear how this is supposed
> > to work, genuine Hyper-V doesn't expose the control so it is possible that
> > this is just a bug (in Hyper-V).
> 
> Yes, it most likely is and it would be nice if Microsoft fixed it, but I
> guess we're stuck with it for existing Windows versions.  Well, for one
> we found a bug in Hyper-V and not the converse. :)
> 
> There is a problem with this approach, in that we're stuck with it
> forever due to live migration.  But I guess if in the future eVMCS v2
> adds an apic_address field we can limit the hack to eVMCS v1.  Another
> possibility is to use the quirks mechanism but it's overkill for now.
> 
> Unless there are objections, I plan to apply these patches.

Doesn't applying this patch contradict your earlier opinion?  This patch
would still hide the affected controls from the guest because the host
controls enlightened_vmcs_enabled.

On Sat, Jan 18, 2020 at 10:42:31PM +0100, Paolo Bonzini wrote:
> IMHO the features should stay available in case the guest chooses not to
> use eVMCS.  A guest that uses eVMCS should know which features it cannot
> use and not enable them.

Makes sense, wasn't thinking about the scenario where the guest doesn't
support eVMCS or doesn't want to use it for whatever reason.

Rather than update vmx->nested.msrs or filter vmx_get_msr(), what about
manually adding eVMCS consistency checks on the disallowed bits and handle
SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES as a one-off case by simply
clearing it from the eVMCS?  Or alternatively, squashing all the disallowed
bits.