On Fri, Nov 22, 2019 at 3:57 PM Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>
>
>
> > On 23 Nov 2019, at 1:43, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > Commit 37e4c997dadf ("KVM: VMX: validate individual bits of guest
> > MSR_IA32_FEATURE_CONTROL") broke the KVM_SET_MSRS ABI by instituting
> > new constraints on the data values that kvm would accept for the guest
> > MSR, IA32_FEATURE_CONTROL. Perhaps these constraints should have been
> > opt-in via a new KVM capability, but they were applied
> > indiscriminately, breaking at least one existing hypervisor.
> >
> > Relax the constraints to allow either or both of
> > FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX and
> > FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX to be set when nVMX is
> > enabled. This change is sufficient to fix the aforementioned breakage.
> >
> > Fixes: 37e4c997dadf ("KVM: VMX: validate individual bits of guest MSR_IA32_FEATURE_CONTROL")
> > Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
>
> I suggest to also add a comment in code to clarify why we allow setting
> FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX even though we expose a vCPU that doesn’t support Intel TXT.
> (I think the compatibility to existing workloads that sets this blindly on boot is a legit reason. Just recommend documenting it.)
>
> In addition, if the nested hypervisor which relies on this is public, please also mention it in commit message for reference.
It's not an L1 hypervisor that's the problem. It's Google's L0
hypervisor. We've been incorrectly reporting IA32_FEATURE_CONTROL as 7
to nested guests for years, and now we have thousands of running VMs
with the bogus value. I've thought about just changing it to 5 on the
fly (on real hardware, one could almost blame it on SMM, but the MSR
is *locked*, after all).
> Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
>
> > ---
> > arch/x86/kvm/vmx/vmx.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 04a8212704c17..9f46023451810 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -7097,10 +7097,12 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
> >
> > if (nested_vmx_allowed(vcpu))
> > to_vmx(vcpu)->msr_ia32_feature_control_valid_bits |=
> > + FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX |
> > FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> > else
> > to_vmx(vcpu)->msr_ia32_feature_control_valid_bits &=
> > - ~FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX;
> > + ~(FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX |
> > + FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX);
> >
> > if (nested_vmx_allowed(vcpu)) {
> > nested_vmx_cr_fixed1_bits_update(vcpu);
> > --
> > 2.24.0.432.g9d3f5f5b63-goog
> >
>
