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
