Re: [RFC XEN PATCH v4 4/5] domctl: Use gsi to grant/revoke irq permission

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 5 Jan 2024, Jiqian Chen wrote:
> Some type of domain don't have PIRQ, like PVH, current
> implementation is not suitable for those domain.
> 
> When passthrough a device to guest on PVH dom0, this
> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed
> at domain_pirq_to_irq.
> 
> So, change it to use gsi to grant/revoke irq permission.
> And change the caller of XEN_DOMCTL_irq_permission to
> pass gsi to it instead of pirq.
> 
> Co-developed-by: Huang Rui <ray.huang@xxxxxxx>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>

I realize there is no explanation or comment right now, but I think this
change would benefit from a in-code comment in xen/include/public/domctl.h
E.g.:

/* XEN_DOMCTL_irq_permission */
struct xen_domctl_irq_permission {
    uint32_t pirq;           /* pirq is the GSI on x86 */
    uint8_t allow_access;    /* flag to specify enable/disable of IRQ access */
    uint8_t pad[3];
};


> ---
>  tools/libs/light/libxl_pci.c |  6 ++++--
>  tools/libs/light/libxl_x86.c |  5 ++++-
>  xen/common/domctl.c          | 12 ++++++++++--
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index 96cb4da0794e..e1341d1e9850 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1418,6 +1418,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>      unsigned long long start, end, flags, size;
>      int irq, i;
>      int r;
> +    int gsi;
>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>      uint32_t domainid = domid;
>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
> @@ -1486,6 +1487,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>          goto out_no_irq;
>      }
>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
> +        gsi = irq;

A question for Roger and Jan: are we always guaranteed that gsi == irq
(also in the PV case)?

Also I don't think we necessarily need the gsi local variable, I would
just add an in-code comment below...


>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>          if (r < 0) {
>              LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
> @@ -1494,10 +1496,10 @@ static void pci_add_dm_done(libxl__egc *egc,
>              rc = ERROR_FAIL;
>              goto out;
>          }

... like this:

/* xc_domain_irq_permission takes a gsi, here we assume irq == gsi */
r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);


> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> +        r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1);
>          if (r < 0) {
>              LOGED(ERROR, domainid,
> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> +                  "xc_domain_irq_permission irq=%d (error=%d)", gsi, r);
>              fclose(f);
>              rc = ERROR_FAIL;
>              goto out;
> diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
> index d16573e72cd4..88ad50cf6360 100644
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -654,12 +654,15 @@ out:
>  int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq)

you could just rename the int irq parameter to int gsi ?


>  {
>      int ret;
> +    int gsi;
> +
> +    gsi = irq;
>  
>      ret = xc_physdev_map_pirq(CTX->xch, domid, irq, &irq);
>      if (ret)
>          return ret;
>  
> -    ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1);
> +    ret = xc_domain_irq_permission(CTX->xch, domid, gsi, 1);
>      return ret;
>  }
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index f5a71ee5f78d..eeb975bd0194 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -653,12 +653,20 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) 
> u_domctl)
>          unsigned int pirq = op->u.irq_permission.pirq, irq;
>          int allow = op->u.irq_permission.allow_access;

here we could benefit from renaming pirq to gsi, at least it becomes
clear:

unsigned int gsi = op->u.irq_permission.pirq, irq;


> -        if ( pirq >= current->domain->nr_pirqs )
> +        if ( pirq >= nr_irqs_gsi )
>          {
>              ret = -EINVAL;
>              break;
>          }
> -        irq = pirq_access_permitted(current->domain, pirq);
> +
> +        if ( irq_access_permitted(current->domain, pirq) )
> +            irq = pirq;
> +        else
> +        {
> +            ret = -EPERM;
> +            break;
> +        }
> +
>          if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, allow) )
>              ret = -EPERM;
>          else if ( allow )
> -- 
> 2.34.1
>