Re: [XEN PATCH v11 5/8] x86/domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.06.2024 14:33, Jiqian Chen wrote:
> @@ -237,6 +238,38 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        int irq;
> +        uint8_t mask = 1;
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        bool allow = domctl->u.gsi_permission.allow_access;
> +
> +        /* Check all bits and pads are zero except lowest bit */
> +        ret = -EINVAL;
> +        if ( domctl->u.gsi_permission.allow_access & ( !mask ) )
> +            goto gsi_permission_out;

I'm pretty sure that if you had, as would have been expected, added a
#define to the public header for just the low bit you assign meaning to,
you wouldn't have caused yourself problems here. For one, the
initializer for "allow" will be easy to miss if meaning is assigned to
another of the bits. It sadly _still_ takes the full 8 bits and
converts those to a boolean. And then the check here won't work either.
I don't see what use the local variable "mask" is, but at the very
least I expect in place of ! you mean ~ really.

> +        for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
> +            if ( domctl->u.gsi_permission.pad[i] )
> +                goto gsi_permission_out;
> +
> +        if ( gsi >= nr_irqs_gsi || ( irq = gsi_2_irq(gsi) ) < 0 )

nr_irqs_gsi is the upper bound on IRQs representing a GSI; as said before,
GSIs and IRQs are different number spaces, and hence you can't compare
gsi against nr_irqs_gsi. The (inclusive) upper bound on (valid) GSIs is
mp_ioapic_routing[nr_ioapics - 1].gsi_end, or the return value of
highest_gsi().

Also, style nit: Blanks belong immediately inside parentheses only for the
outer pair of control statements; no inner expressions should have them this
way.

Finally I'd like to ask that you use "<= 0", as we do in various places
elsewhere. IRQ0 is the timer interrupt; we never want to have that used by
any entity outside of Xen itself.

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -464,6 +464,12 @@ struct xen_domctl_irq_permission {
>      uint8_t pad[3];
>  };
>  
> +/* XEN_DOMCTL_gsi_permission */
> +struct xen_domctl_gsi_permission {
> +    uint32_t gsi;
> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi 
> access */

See above. It's not the field that serves as a flag for the purpose you
state, but just the low bit. You want to rename the field (flags?) and
#define a suitable constant.

Jan