Re: [PATCH 1/6] xen/hvm: Convert hap_capabilities into a bitfield

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2024 08:02, George Dunlap wrote:
> On Mon, Feb 19, 2024 at 9:36 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 06.02.2024 02:20, George Dunlap wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>> @@ -113,8 +113,8 @@ static int cf_check parse_ept_param_runtime(const char 
>>> *s)
>>>      int val;
>>>
>>>      if ( !cpu_has_vmx_ept || !hvm_funcs.hap_supported ||
>>> -         !(hvm_funcs.hap_capabilities &
>>> -           (HVM_HAP_SUPERPAGE_2MB | HVM_HAP_SUPERPAGE_1GB)) )
>>> +         !(hvm_funcs.caps.hap_superpage_2mb ||
>>> +           hvm_funcs.caps.hap_superpage_1gb) )
>>>      {
>>>          printk("VMX: EPT not available, or not in use - ignoring\n");
>>
>> Just to mention it: The conditional and the log message don't really
>> fit together. (I was first wondering what the 2mb/1gb checks had to
>> do here at all, but that's immediately clear when seeing that the
>> only sub-option here is "exec-sp".)
> 
> So you mean basically that the checks & error message are poorly
> factored, because there's only a single sub-option?  (i.e., if there
> were options which didn't rely on superpages, the check would be
> incorrect?)

Right.

> Let me know if there's something concrete you'd like me to do here.

Nothing. I meant to express this by starting with "Just to mention it".
If you were eager, you might deal with this right away, but there's no
expectation to this effect. If and when it becomes a problem, it'll
then need sorting. And the message being potentially misleading is,
well, just an annoyance - as with many other things, finding it confusing
will likely lead to looking up where it's issued, then realizing that
what it says isn't what it means.

Jan