Re: [PATCH v2 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu <binbin.wu@linux.intel.com>]

On 8/14/2024 9:31 AM, Sean Christopherson wrote:
> On Tue, Aug 13, 2024, Isaku Yamahata wrote:
>> On Wed, Aug 14, 2024 at 11:11:29AM +1200,
>> Kai Huang <kai.huang@intel.com> wrote:
>>
>>>
>>> On 14/08/2024 5:50 am, Isaku Yamahata wrote:
>>>> On Tue, Aug 13, 2024 at 01:12:55PM +0800,
>>>> Binbin Wu <binbin.wu@linux.intel.com> wrote:
>>>>
>>>>> Check whether a KVM hypercall needs to exit to userspace or not based on
>>>>> hypercall_exit_enabled field of struct kvm_arch.
>>>>>
>>>>> Userspace can request a hypercall to exit to userspace for handling by
>>>>> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
>>>>> hypercall_exit_enabled.  Make the check code generic based on it.
>>>>>
>>>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>>>>> ---
>>>>>    arch/x86/kvm/x86.c | 4 ++--
>>>>>    arch/x86/kvm/x86.h | 7 +++++++
>>>>>    2 files changed, 9 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>>> index af6c8cf6a37a..6e16c9751af7 100644
>>>>> --- a/arch/x86/kvm/x86.c
>>>>> +++ b/arch/x86/kvm/x86.c
>>>>> @@ -10226,8 +10226,8 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>>>    	cpl = kvm_x86_call(get_cpl)(vcpu);
>>>>>    	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>>>>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>>>>> -		/* MAP_GPA tosses the request to the user space. */
>>>>> +	if (!ret && is_kvm_hc_exit_enabled(vcpu->kvm, nr))
>>>>> +		/* The hypercall is requested to exit to userspace. */
>>>>>    		return 0;
>>>>>    	if (!op_64_bit)
>>>>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>>>>> index 50596f6f8320..0cbec76b42e6 100644
>>>>> --- a/arch/x86/kvm/x86.h
>>>>> +++ b/arch/x86/kvm/x86.h
>>>>> @@ -547,4 +547,11 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>>>>>    			 unsigned int port, void *data,  unsigned int count,
>>>>>    			 int in);
>>>>> +static inline bool is_kvm_hc_exit_enabled(struct kvm *kvm, unsigned long hc_nr)
> I would rather have "hypercall" in the name, "hc" never jumps out to me as being
> "hypercall". Maybe is_hypercall_exit_enabled(), user_exit_on_hypercall(), or just
> exit_on_hypercall()?
>
> I'd probably vote for user_exit_on_hypercall(), as that clarifies it's all about
> exiting to userspace, not from the guest.
user_exit_on_hypercall() looks good to me.
Thanks!


>
>>>>> +{
>>>>> +	if(WARN_ON_ONCE(hc_nr >= sizeof(kvm->arch.hypercall_exit_enabled) * 8))
>>>>> +		return false;
>>>> Is this to detect potential bug? Maybe
>>>> BUILD_BUG_ON(__builtin_constant_p(hc_nr) &&
>>>>                !(BIT(hc_nr) & KVM_EXIT_HYPERCALL_VALID_MASK));
>>>> Overkill?
>>> I don't think this is the correct way to use __builtin_constant_p(), i.e. it
>>> doesn't make sense to use __builtin_constant_p() in BUILD_BUG_ON().
> KVM does use __builtin_constant_p() to effectively disable some assertions when
> it's allowed (by KVM's arbitrary rules) to pass in a non-constant value.  E.g.
> see all the vmcs_checkNN() helpers.  If we didn't waive the assertion for values
> that aren't constant at compile-time, all of the segmentation code would need to
> be unwound into switch statements.
>
> But for things like guest_cpuid_has(), the rule is that the input must be a
> compile-time constant.
>
>>> IIUC you need some build time guarantee here, but __builtin_constant_p() can
>>> return false, in which case the above BUILD_BUG_ON() does nothing, which
>>> defeats the purpose.
>> It depends on what we'd like to detect.  BUILT_BUG_ON(__builtin_constant_p())
>> can detect the usage in the patch 2/2,
>> is_kvm_hc_exit_enabled(vcpu->kvm, KVM_HC_MAP_GPA_RANGE).  The potential
>> future use of is_kvm_hc_exit_enabled(, KVM_HC_MAP_future_hypercall).
>>
>> Although this version doesn't help for the one in kvm_emulate_hypercall(),
>> !ret check is done first to avoid WARN_ON_ONCE() to hit here.
>>
>> Maybe we can just drop this WARN_ON_ONCE().
> Yeah, I think it makes sense to drop the WARN, otherwise I suspect we'll end up
> dancing around the helper just to avoid the warning.
>
> I'm 50/50 on the BUILD_BUG_ON().  One one hand, it's kinda overkill.  On the other
> hand, it's zero generated code.
>
Will remove the WARN_ON_ONCE().