On 02.08.2024 05:10, Chen, Jiqian wrote:
> On 2024/8/1 19:06, Roger Pau Monné wrote:
>> On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote:
>>> --- a/xen/include/public/domctl.h
>>> +++ b/xen/include/public/domctl.h
>>> @@ -464,6 +464,13 @@ struct xen_domctl_irq_permission {
>>> uint8_t pad[3];
>>> };
>>>
>>> +/* XEN_DOMCTL_gsi_permission */
>>> +struct xen_domctl_gsi_permission {
>>> + uint32_t gsi;
>>> +#define XEN_DOMCTL_GSI_PERMISSION_MASK 1
>>
>> IMO this would be better named GRANT or similar, maybe something like:
>>
>> /* Low bit used to signal grant/revoke action. */
>> #define XEN_DOMCTL_GSI_REVOKE 0
>> #define XEN_DOMCTL_GSI_GRANT 1
>>
>>> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi
>>> access */
>>> + uint8_t pad[3];
>>
>> We might as well declare the flags field as uint32_t and avoid the
>> padding field.
> So, should this struct be like below? Then I just need to check whether
> everything except the lowest bit is 0.
> struct xen_domctl_gsi_permission {
> uint32_t gsi;
> /* Lowest bit used to signal grant/revoke action. */
> #define XEN_DOMCTL_GSI_REVOKE 0
> #define XEN_DOMCTL_GSI_GRANT 1
> #define XEN_DOMCTL_GSI_PERMISSION_MASK 1
> uint32_t access_flag; /* flag to specify enable/disable of x86 gsi
> access */
> };
Yet then why "access_flags"? You can't foresee what meaning the other bits may
gain. That meaning may (and likely will) not be access related at all.
Jan
