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

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

Hi Anthony,

On 2024/2/21 23:03, Anthony PERARD wrote:
> On Fri, Jan 12, 2024 at 02:13:17PM +0800, Jiqian Chen wrote:
>> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
>> index f2d9d14b4d9f..448ba2c59ae1 100644
>> --- a/tools/libs/ctrl/xc_domain.c
>> +++ b/tools/libs/ctrl/xc_domain.c
>> @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch,
>>      return do_domctl(xch, &domctl);
>>  }
>>  
>> +int xc_domain_gsi_permission(xc_interface *xch,
>> +                             uint32_t domid,
>> +                             uint32_t gsi,
>> +                             bool allow_access)
>> +{
>> +    struct xen_domctl domctl = {};
>> +
>> +    domctl.cmd = XEN_DOMCTL_gsi_permission;
>> +    domctl.domain = domid;
>> +    domctl.u.gsi_permission.gsi = gsi;
>> +    domctl.u.gsi_permission.allow_access = allow_access;
> 
> This could be written as:
>     struct xen_domctl domctl = {
>         .cmd = XEN_DOMCTL_gsi_permission,
>         .domain = domid,
>         .u.gsi_permission.gsi = gsi,
>         .u.gsi_permission.allow_access = allow_access,
>     };
> 
That's fine, I will change to this in next version.

> but your change is fine too.
> 
> 
>> +
>> +    return do_domctl(xch, &domctl);
>> +}
>> +
>>  int xc_domain_iomem_permission(xc_interface *xch,
>>                                 uint32_t domid,
>>                                 unsigned long first_mfn,
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index a1c6e82631e9..4136a860a048 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1421,6 +1421,8 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>      uint32_t domainid = domid;
>>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
>> +    int gsi;
>> +    bool has_gsi = true;
> 
> Why is this function has "gsi" variable, and "gsi = irq" but the next
> function pci_remove_detached() does only have "irq" variable?
Because in pci_add_dm_done, it calls " r = xc_physdev_map_pirq(ctx->xch, domid, 
irq, &irq); ", it passes the pointer of irq in, and then irq will be changed, 
so I need to use gsi to save the origin value.

> 
> So, gsi can only be positive integer, right? Could we forgo `has_gsi` and
> just set "gsi = -1" when there's no gsi?
No, we can't forgo 'has_gsi', because we need to set gsi = irq to save the 
original val.

> 
>>      /* Convenience aliases */
>>      bool starting = pas->starting;
>> @@ -1482,6 +1484,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>                                  pci->bus, pci->dev, pci->func);
>>  
>>      if ( access(sysfs_path, F_OK) != 0 ) {
>> +        has_gsi = false;
>>          if ( errno == ENOENT )
>>              sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", 
>> pci->domain,
>>                                  pci->bus, pci->dev, pci->func);
>> @@ -1497,6 +1500,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +        gsi = irq;
> 
> Why do you do this, that that mean that `gsi` and `irq` are the same? Or
> does `irq` happen to sometime contain a `gsi` value?
Above reason.

> 
>>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
>> @@ -1505,7 +1509,10 @@ static void pci_add_dm_done(libxl__egc *egc,
>>              rc = ERROR_FAIL;
>>              goto out;
>>          }
>> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>> +        if ( has_gsi )
>> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
>> +        if ( !has_gsi || r == -EOPNOTSUPP )
>> +            r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid,
>>                    "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> 
> Looks like this error message could be wrong, I think we want to know if
> which call between xc_domain_irq_permission() and
> xc_domain_gsi_permission() has failed.
You are right, I will separate them in next version.

> 
>> @@ -2185,6 +2192,7 @@ static void pci_remove_detached(libxl__egc *egc,
>>      FILE *f;
>>      uint32_t domainid = prs->domid;
>>      bool isstubdom;
>> +    bool has_gsi = true;
>>  
>>      /* Convenience aliases */
>>      libxl_device_pci *const pci = &prs->pci;
>> @@ -2244,6 +2252,7 @@ skip_bar:
>>                             pci->bus, pci->dev, pci->func);
>>  
>>      if ( access(sysfs_path, F_OK) != 0 ) {
>> +        has_gsi = false;
>>          if ( errno == ENOENT )
>>              sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", 
>> pci->domain,
>>                                  pci->bus, pci->dev, pci->func);
>> @@ -2270,7 +2279,10 @@ skip_bar:
>>               */
>>              LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>>          }
>> -        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
>> +        if ( has_gsi )
>> +            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);
> 
> Why do you use the xc_domain_gsi_permission() with "irq" here, but not
> in pci_add_dm_done() ?
Because xc_physdev_unmap_pirq doesn't change the value of irq, but in 
pci_add_dm_done, the value of irq is changed by xc_physdev_map_pirq.

> 
>> +        if ( !has_gsi || rc == -EOPNOTSUPP )
>> +            rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
>>          if (rc < 0) {
>>              LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
>>          }
> 
> 
> These changes to libxl are quite confusing, there's a mixed up between
> "gsi" and "irq", it's hard to follow.
> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.