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

[George Dunlap <george.dunlap@xxxxxxxxx>]

On Mon, Feb 19, 2024 at 9:36 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 06.02.2024 02:20, George Dunlap wrote:
> > hvm_function_table is an internal structure; rather than manually
> > |-ing and &-ing bits, just make it a boolean bitfield and let the
> > compiler do all the work.  This makes everything easier to read, and
> > presumably allows the compiler more flexibility in producing efficient
> > code.
> >
> > No functional change intended.
> >
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxx>
>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> > ---
> > Questions:
> >
> > * Should hap_superpage_2m really be set unconditionally, or should we
> >   condition it on cpu_has_svm_npt?
>
> That's HAP capabilities; there's not going to be any use of HAP when
> there's no NPT (on an AMD system). IOW - all is fine as is, imo.

Basically there are two stances to take:

1. hap_superpage_2m always has meaning.  If there's no HAP, then there
are no HAP superpages, so we should say that there are no superpages.

2. hap_superpage_2m only has meaning if hap is enabled.  If there's no
HAP, then the question "are there superpages" is moot; nobody should
be paying attention to it.

The second is not without precedent, but is it really the best stance?
 It means that before reading hap_superpage_2m, you have to first
check hap; and it introduces a risk (no matter how small) that someone
will forget to check, and end up doing the wrong thing.

Not a huge deal, but overall my vote would be #1.  I may send a patch
at some point in the future.

> > * Do we really need to "!!cpu_has_svm_npt"?  If so, wouldn't it be
> >   better to put the "!!"  in the #define, rather than requiring the
> >   user to know that it's needed?
>
> Considering that hap_supported is bool now, the !! can simply be
> dropped. We've been doing so as code was touched anyway, not in a
> concerted effort.

Right, forgot to actually delete this para after adding a patch to address it.

> > --- 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?)

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

> > @@ -104,8 +96,11 @@ struct hvm_function_table {
> >      /* Hardware virtual interrupt delivery enable? */
> >      bool virtual_intr_delivery_enabled;
> >
> > -    /* Indicate HAP capabilities. */
> > -    unsigned int hap_capabilities;
> > +    struct {
> > +        /* Indicate HAP capabilities. */
> > +        bool hap_superpage_1gb:1,
> > +            hap_superpage_2mb:1;
>
> Nit: Would be nice imo if the two identifiers aligned vertically with
> one another.

Done.

 -George