Re: [RFC XEN PATCH v4 4/5] domctl: Use gsi to grant/revoke irq permission

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

On 2024/1/6 09:08, Stefano Stabellini wrote:
> On Fri, 5 Jan 2024, Jiqian Chen wrote:
>> Some type of domain don't have PIRQ, like PVH, current
>> implementation is not suitable for those domain.
>>
>> When passthrough a device to guest on PVH dom0, this
>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
>> at domain_pirq_to_irq.
>>
>> So, change it to use gsi to grant/revoke irq permission.
>> And change the caller of XEN_DOMCTL_irq_permission to
>> pass gsi to it instead of pirq.
>>
>> Co-developed-by: Huang Rui <ray.huang@xxxxxxx>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> 
> I realize there is no explanation or comment right now, but I think this
> change would benefit from a in-code comment in xen/include/public/domctl.h
> E.g.:
> 
> /* XEN_DOMCTL_irq_permission */
> struct xen_domctl_irq_permission {
>     uint32_t pirq;           /* pirq is the GSI on x86 */
>     uint8_t allow_access;    /* flag to specify enable/disable of IRQ access 
> */
>     uint8_t pad[3];
> };
Will add this comment in next version, thanks.

> 
> 
>> ---
>>  tools/libs/light/libxl_pci.c |  6 ++++--
>>  tools/libs/light/libxl_x86.c |  5 ++++-
>>  xen/common/domctl.c          | 12 ++++++++++--
>>  3 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da0794e..e1341d1e9850 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1418,6 +1418,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      unsigned long long start, end, flags, size;
>>      int irq, i;
>>      int r;
>> +    int gsi;
>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>      uint32_t domainid = domid;
>>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
>> @@ -1486,6 +1487,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +        gsi = irq;
> 
> A question for Roger and Jan: are we always guaranteed that gsi == irq
> (also in the PV case)?
> 
> Also I don't think we necessarily need the gsi local variable, I would
> just add an in-code comment below...
Here pass the pointer of irq to xc_physdev_map_pirq, and that function will 
assign the value of pirq to irq on Xen side, so after calling 
xc_physdev_map_pirq, the value of irq isn't the original value, we need a local 
variable to record the original value.

> 
> 
>>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
>> @@ -1494,10 +1496,10 @@ static void pci_add_dm_done(libxl__egc *egc,
>>              rc = ERROR_FAIL;
>>              goto out;
>>          }
> 
> ... like this:
> 
> /* xc_domain_irq_permission takes a gsi, here we assume irq == gsi */
> r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
Will add this comment in next version, thanks.

> 
> 
>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid,
>> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
>> +                  "xc_domain_irq_permission irq=%d (error=%d)", gsi, r);
>>              fclose(f);
>>              rc = ERROR_FAIL;
>>              goto out;
>> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
>> index d16573e72cd4..88ad50cf6360 100644
>> --- a/tools/libs/light/libxl_x86.c
>> +++ b/tools/libs/light/libxl_x86.c
>> @@ -654,12 +654,15 @@ out:
>>  int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)
> 
> you could just rename the int irq parameter to int gsi ?
No, the same reason as above.

> 
> 
>>  {
>>      int ret;
>> +    int gsi;
>> +
>> +    gsi = irq;
>>  
>>      ret = xc_physdev_map_pirq(CTX->xch, domid, irq, &irq);
>>      if (ret)
>>          return ret;
>>  
>> -    ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1);
>> +    ret = xc_domain_irq_permission(CTX->xch, domid, gsi, 1);
>>      return ret;
>>  }
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index f5a71ee5f78d..eeb975bd0194 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -653,12 +653,20 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
>> u_domctl)
>>          unsigned int pirq = op->u.irq_permission.pirq, irq;
>>          int allow = op->u.irq_permission.allow_access;
> 
> here we could benefit from renaming pirq to gsi, at least it becomes
> clear:
> 
> unsigned int gsi = op->u.irq_permission.pirq, irq;
Thank you, I will rename it in next version.

> 
> 
>> -        if ( pirq >= current->domain->nr_pirqs )
>> +        if ( pirq >= nr_irqs_gsi )
>>          {
>>              ret = -EINVAL;
>>              break;
>>          }
>> -        irq = pirq_access_permitted(current->domain, pirq);
>> +
>> +        if ( irq_access_permitted(current->domain, pirq) )
>> +            irq = pirq;
>> +        else
>> +        {
>> +            ret = -EPERM;
>> +            break;
>> +        }
>> +
>>          if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, allow) )
>>              ret = -EPERM;
>>          else if ( allow )
>> -- 
>> 2.34.1
>>

-- 
Best regards,
Jiqian Chen.