Re: [XEN PATCH v10 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.06.2024 10:23, Chen, Jiqian wrote:
> On 2024/6/17 23:32, Jan Beulich wrote:
>> On 17.06.2024 11:00, Jiqian Chen wrote:
>>> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc,
>>>              rc = ERROR_FAIL;
>>>              goto out;
>>>          }
>>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>>> +#ifdef CONFIG_X86
>>> +        /* If dom0 doesn't have PIRQs, need to use 
>>> xc_domain_gsi_permission */
>>> +        r = xc_domain_getinfo_single(ctx->xch, 0, &info);
>>
>> Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but
>> I didn't check if that can be used with the underlying hypercall(s). 
>> Otherwise
>> you want to pass the actual domid of the local domain here.
> But the action of granting permission is from dom0 to domU, what I need to 
> get is the infomation of dom0,
> The actual domid here is domU's id I think, it is not useful.

Note how I said DOMID_SELF and "local domain". There's no talk of using the
DomU's domid. But what you apparently neglect is the fact that the hardware
domain isn't necessarily Dom0 (see CONFIG_LATE_HWDOM in the hypervisor).
While benign in most cases, this is relevant when it comes to referencing
the hardware domain by domid. And it is the hardware domain which is going
to drive the device re-assignment, as that domain is who's in possession of
all the devices not yet assigned to any DomU.

>>> @@ -237,6 +238,48 @@ long arch_do_domctl(
>>>          break;
>>>      }
>>>  
>>> +    case XEN_DOMCTL_gsi_permission:
>>> +    {
>>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>>> +        int irq;
>>> +        bool allow = domctl->u.gsi_permission.allow_access;
>>
>> See my earlier comments on this conversion of 8 bits into just one.
> Do you mean that I need to check allow_access is >= 0?
> But allow_access is u8, it can't be negative.

Right. What I can only re-iterate from earlier commenting is that you
want to check for 0 or 1 (can be viewed as looking at just the low bit),
rejecting everything else. It is only this way that down the road we
could assign meaning to the other bits, without risking to break existing
callers. That's the same as the requirement to check padding fields to be
zero.

Jan