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

["Huang, Kai" <kai.huang@intel.com>]

>>>>> +{
>>>>> +	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.

Yeah I saw vmcs_checkNN(), but I think __builtin_constant_p() makes 
sense for vmcs_checkNN()s because they are widely called.  But 
is_kvm_hc_exit_enabled() doesn't seem so.  But no hard opinion here.  As 
you said, it's kinda overkill (or abused to use) but zero-generated code.

> 
> 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.

Agreed, given @nr is from guest.