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

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

On 2024/6/18 17:23, Jan Beulich wrote:
> 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
From the commit 10ef7a91b5a8cb8c58903c60e2dd16ed490b3bcf, DOMID_SELF is not 
allowed for XEN_DOMCTL_getdomaininfo.
And now XEN_DOMCTL_getdomaininfo gets domain through rcu_lock_domain_by_id.

>>> you want to pass the actual domid of the local domain here.
What is the local domain here?
What is method for me to get its domid?

>> 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.
OK, I need to get the information of hardware domain here?

> 
>>>> @@ -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.
OK, I will add check the other bit is zero except the lowest one bit.

> 
> Jan

-- 
Best regards,
Jiqian Chen.