Re: [PATCH RFC] i386/kvm: fix enlightened VMCS with fine-grained VMX feature enablement

[Paolo Bonzini]

On 02/01/20 21:39, Vitaly Kuznetsov wrote:
> When enlightened VMCS is enabled, certain VMX controls disappear, e.g.
> posted interrupts for PINBASED_CTLS. With fine-grained VMX feature
> enablement QEMU tries to do KVM_SET_MSRS with default (matching CPU
> model) values and fails as KVM doesn't allow to set now-unsupported
> controls.
> 
> The ideal solution for the issue would probably be to re-read VMX
> feature MSRs after enabling KVM_CAP_HYPERV_ENLIGHTENED_VMCS, however,
> this doesn't seem to be possible: currently, KVM returns global
> &vmcs_config.nested values for VMX MSRs when userspace does KVM_GET_MSR.
> 
> It is also possible to modify KVM to apply 'evmcs filtering' to VMX
> MSRs when userspace tries to set them and hide the issue but this doesn't
> seem to be entirely correct.
> 
> It is unfortunate that we now need to support the list of VMX features
> disabled by enlightened VMCS in QEMU. When (and if) enlightened VMCS v2
> arrives we'll need to fix QEMU and allow previously disabled features.
> 
> Signed-off-by: Vitaly Kuznetsov <address@hidden>
> ---
> - I don't quite like this workaround myself, thus RFC. I'm sure someone
>  will suggest a better alternative.

The patch itself is not a big deal, the only thing is that we should
probably check that we get warnings if the "forbidden" features are
explicitly requested by the user.  On the other hand, for something like
"-cpu Haswell,vmx,hv_evmcs" there should be no warnings.

That said, I'm not sure about the whole idea of disabling features, even
in the kernel.  I would prefer if the guest knew that it cannot use
these features if using eVMCS.  Is this the case for Windows?  If so, we
should teach guest-side KVM about this, not QEMU.

Paolo

> ---
>  target/i386/kvm.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index 0b511906e3fe..1b0589b79358 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1198,6 +1198,30 @@ static int hyperv_handle_properties(CPUState *cs,
>          }
>  
>          if (!r) {
> +            /*
> +             * Certain VMX controls are unsupported when enlightened VMCS is
> +             * enabled, filter them out here so we don't attempt to set them
> +             * with KVM_SET_MSR even if they are supported by CPU model.
> +             * The list below is for eVMCS version 1.
> +             */
> +            env->features[FEAT_VMX_PINBASED_CTLS] &=
> +                ~(VMX_PIN_BASED_VMX_PREEMPTION_TIMER |
> +                  VMX_PIN_BASED_POSTED_INTR);
> +            env->features[FEAT_VMX_SECONDARY_CTLS] &=
> +                ~(VMX_SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
> +                  VMX_SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> +                  VMX_SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +                  VMX_SECONDARY_EXEC_ENABLE_PML |
> +                  VMX_SECONDARY_EXEC_ENABLE_VMFUNC |
> +                  VMX_SECONDARY_EXEC_SHADOW_VMCS |
> +                  /* VMX_SECONDARY_EXEC_TSC_SCALING | */
> +                  VMX_SECONDARY_EXEC_PAUSE_LOOP_EXITING);
> +            env->features[FEAT_VMX_ENTRY_CTLS] &=
> +                ~VMX_VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
> +            env->features[FEAT_VMX_EXIT_CTLS] &=
> +                ~VMX_VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
> +            env->features[FEAT_VMX_VMFUNC] &= ~MSR_VMX_VMFUNC_EPT_SWITCHING;
> +
>              env->features[FEAT_HV_RECOMM_EAX] |=
>                  HV_ENLIGHTENED_VMCS_RECOMMENDED;
>              env->features[FEAT_HV_NESTED_EAX] = evmcs_version;
>