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;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.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 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check
In-Reply-To: <Y1nAThjeMlMFFrAi@google.com>
References: <20221018101000.934413-1-vkuznets@redhat.com>
 <20221018101000.934413-3-vkuznets@redhat.com>
 <Y1nAThjeMlMFFrAi@google.com>
Date:   Thu, 27 Oct 2022 13:14:09 +0200
Message-ID: <87a65htt6m.fsf@ovpn-194-52.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sean Christopherson <seanjc@google.com> writes:

> On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
>> When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
>> need to be adjusted to avoid the situation when the feature is exposed
>> to the guest but there's no corresponding eVMCS field[s] for it. This
>> is not obvious and fragile.
>
> Eh, either way is fragile, the only difference is what goes wrong when it breaks.
>
> At the risk of making this overly verbose, what about requiring developers to
> explicitly define whether or not a new control is support?  E.g. keep the
> EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every
> feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED.
>
> That way the eVMCS "supported" controls don't need to include the ALWAYSON
> controls, and anytime someone adds a new control, they'll have to stop and think
> about eVMCS.

Is this a good thing or a bad one? :-) I'm not against being extra
verbose but adding a new feature to EVMCS1_SUPPORTED_* (even when there
is a corresponding field) requires testing or a
evmcs_has_perf_global_ctrl()-like story may happen and such testing
would require access to Windows/Hyper-V images. This sounds like an
extra burden for contributors. IMO it's OK if new features are
mechanically added to EVMCS1_UNSUPPORTED_* on the grounds that it
wasn't tested but then it's not much different from "unsupported by
default" (my approach). So I'm on the fence here.

>
> I think we'll still want (need?) the runtime sanitization, but this might allow
> catching at least some cases without needing to wait until a control actually gets
> exposed.
>
> E.g. possibly with more macro magic to reduce the boilerplate
>
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index d8b23c96d627..190932edcc02 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -422,6 +422,10 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
>         u32 ctl_high = (u32)(*pdata >> 32);
>         u32 unsupported_ctrls;
>  
> +       BUILD_BUG_ON((EVMCS1_SUPPORTED_PINCTRL | EVMCS1_UNSUPPORTED_PINCTRL) !=
> +                    (KVM_REQUIRED_VMX_PIN_BASED_VM_EXEC_CONTROL |
> +                     KVM_OPTIONAL_VMX_PIN_BASED_VM_EXEC_CONTROL));
> +
>         /*
>          * Hyper-V 2016 and 2019 try using these features even when eVMCS
>          * is enabled but there are no corresponding fields.
> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
> index 6f746ef3c038..58d77afe9d57 100644
> --- a/arch/x86/kvm/vmx/evmcs.h
> +++ b/arch/x86/kvm/vmx/evmcs.h
> @@ -48,6 +48,11 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
>   */
>  #define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \
>                                     PIN_BASED_VMX_PREEMPTION_TIMER)
> +#define EVMCS1_SUPPORTED_PINCTRL                                       \
> +       (PIN_BASED_EXT_INTR_MASK |                                      \
> +        PIN_BASED_NMI_EXITING |                                        \
> +        PIN_BASED_VIRTUAL_NMIS)
> +
>  #define EVMCS1_UNSUPPORTED_EXEC_CTRL (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
>  #define EVMCS1_UNSUPPORTED_2NDEXEC                                     \
>         (SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |                         \
>

-- 
Vitaly