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

From: Anthony PERARD <anthony.perard@cloud.com>
To: Jiqian Chen <Jiqian.Chen@amd.com>
Cc: xen-devel@lists.xenproject.org, "Jan Beulich" <jbeulich@suse.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Juergen Gross" <jgross@suse.com>,
	"Daniel P . Smith" <dpsmith@apertussolutions.com>,
	"Stewart Hildebrand" <Stewart.Hildebrand@amd.com>,
	"Huang Rui" <Ray.Huang@amd.com>
Subject: Re: [RFC XEN PATCH v5 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi
Date: Wed, 21 Feb 2024 15:03:04 +0000	[thread overview]
Message-ID: <09290ba4-6915-4f0d-8e16-3e14713d02ba@perard> (raw)
In-Reply-To: <20240112061317.418658-6-Jiqian.Chen@amd.com>

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