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

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Wed, Feb 21, 2024 at 3:23 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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".

Right, and when I said "let me know" I meant, "I'm going to ignore
this unless you say something, feel free to say nothing". :-D

I understood that you weren't asking for anything, but maybe coming
back to this after a few days you'd've had a simple fix.  I wouldn't
mind changing the text of the message, but I didn't feel like finding
a better text.  Reorganizing the checks (which seems closer to the
Right Thing) is off-topic for this patch of course.

 -George