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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

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,
    };

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?

So, gsi can only be positive integer, right? Could we forgo `has_gsi` and
just set "gsi = -1" when there's no gsi?

>      /* 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?

>          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.

> @@ -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() ?

> +        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,

-- 
Anthony PERARD