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.
