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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

Jan