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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Thu, Mar 28, 2024 at 02:34:02PM +0800, Jiqian Chen wrote:
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 2cec83e0b734..debf6ec6ddc7 100644
> @@ -1500,13 +1510,25 @@ 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 (r < 0) {
> -            LOGED(ERROR, domainid,
> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> -            fclose(f);
> -            rc = ERROR_FAIL;
> -            goto out;
> +        if (is_gsi) {
> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +            if (r < 0 && r != -EOPNOTSUPP) {

Is it correct to check `r` for "-EOPNOTSUPP" ? Because `man ioctl` and
"xenctrl.h:105" says that `r` is 0 on success or -1 on error with `errno`
set.

> +                LOGED(ERROR, domainid,
> +                      "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, r);

Is the `r` value useful to print? Because LOGED() already prints
strerror(errno).

> +                fclose(f);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
> +        if (!is_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);
> +                fclose(f);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
>          }
>      }
>      fclose(f);
> @@ -2180,6 +2202,7 @@ static void pci_remove_detached(libxl__egc *egc,
>      FILE *f;
>      uint32_t domainid = prs->domid;
>      bool isstubdom;
> +    bool is_gsi = true;
>  
>      /* Convenience aliases */
>      libxl_device_pci *const pci = &prs->pci;
> @@ -2243,6 +2266,7 @@ skip_bar:
>          /* To compitable with old version of kernel, still need to use irq */
>          sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>                                 pci->bus, pci->dev, pci->func);
> +        is_gsi = false;
>      }
>      f = fopen(sysfs_path, "r");
>      if (f == NULL) {
> @@ -2261,9 +2285,17 @@ skip_bar:
>               */
>              LOGED(ERROR, domid, "xc_physdev_unmap_pirq irq=%d", irq);
>          }
> -        rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> -        if (rc < 0) {
> -            LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +        if (is_gsi) {
> +            rc = xc_domain_gsi_permission(ctx->xch, domid, irq, 0);

Could you store the return code of this system call into `r` instead?
`rc` is supposed to be use exclusively for libxl error code, so the
current code is wrong, but we can partially fixed that in your patch.

> +            if (rc < 0 && rc != -EOPNOTSUPP) {

Shouldn't you check EOPNOTSUPP in `errno` instead?

> +                LOGED(ERROR, domid, "xc_domain_gsi_permission gsi=%d", irq);
> +            }
> +        }
> +        if (!is_gsi || rc == -EOPNOTSUPP) {
> +            rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);

Same here, use `r` instead of `rc`.

> +            if (rc < 0) {
> +                LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +            }
>          }
>      }
>  

Thanks,

-- 
Anthony PERARD