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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

On 2024/7/4 21:33, Jan Beulich wrote:
> 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().
Will change to highest_gsi in next version.

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

You mean?

struct xen_domctl_gsi_permission {
    uint32_t gsi;
#define GSI_PERMISSION_MASK    1
#define GSI_PERMISSION_DISABLE 0
#define GSI_PERMISSION_ENABLE  1
    uint8_t access_flag;    /* flag to specify enable/disable of x86 gsi access 
*/
    uint8_t pad[3];
};

> 
> Jan

-- 
Best regards,
Jiqian Chen.