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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

Hi Daniel,

On 2024/6/17 17:00, Jiqian Chen wrote:
> Some type of domain don't have PIRQs, like PVH, it doesn't do
> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
> to guest base on PVH dom0, callstack
> pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq and
> irq on Xen side.
> What's more, current hypercall XEN_DOMCTL_irq_permission requires
> passing in pirq, it is not suitable for dom0 that doesn't have
> PIRQs.
> 
> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant the
> permission of irq(translate from gsi) to dumU when dom0 has no
> PIRQs.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> ---
> RFC: it needs review and needs to wait for the corresponding third patch on 
> linux kernel side to be merged.
> ---
>  tools/include/xenctrl.h            |  5 +++
>  tools/libs/ctrl/xc_domain.c        | 15 +++++++
>  tools/libs/light/libxl_pci.c       | 67 +++++++++++++++++++++++++++---
>  xen/arch/x86/domctl.c              | 43 +++++++++++++++++++
>  xen/arch/x86/include/asm/io_apic.h |  2 +
>  xen/arch/x86/io_apic.c             | 17 ++++++++
>  xen/arch/x86/mpparse.c             |  3 +-
>  xen/include/public/domctl.h        |  8 ++++
>  xen/xsm/flask/hooks.c              |  1 +
>  9 files changed, 153 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index a0381f74d24b..f3feb6848e25 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch,
>                               uint32_t pirq,
>                               bool allow_access);
>  
> +int xc_domain_gsi_permission(xc_interface *xch,
> +                             uint32_t domid,
> +                             uint32_t gsi,
> +                             bool allow_access);
> +
>  int xc_domain_iomem_permission(xc_interface *xch,
>                                 uint32_t domid,
>                                 unsigned long first_mfn,
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index f2d9d14b4d9f..8540e84fda93 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 = {
> +        .cmd = XEN_DOMCTL_gsi_permission,
> +        .domain = domid,
> +        .u.gsi_permission.gsi = gsi,
> +        .u.gsi_permission.allow_access = allow_access,
> +    };
> +
> +    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 376f91759ac6..f027f22c0028 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1431,6 +1431,9 @@ 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);
> +#ifdef CONFIG_X86
> +    xc_domaininfo_t info;
> +#endif
>  
>      /* Convenience aliases */
>      bool starting = pas->starting;
> @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc,
>              rc = ERROR_FAIL;
>              goto out;
>          }
> -        r = xc_domain_irq_permission(ctx->xch, domid, irq, 1);
> +#ifdef CONFIG_X86
> +        /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission 
> */
> +        r = xc_domain_getinfo_single(ctx->xch, 0, &info);
>          if (r < 0) {
> -            LOGED(ERROR, domainid,
> -                  "xc_domain_irq_permission irq=%d (error=%d)", irq, r);
> +            LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", errno);
>              fclose(f);
>              rc = ERROR_FAIL;
>              goto out;
>          }
> +        if (info.flags & XEN_DOMINF_hvm_guest &&
> +            !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) &&
> +            gsi > 0) {
> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 1);
> +            if (r < 0) {
> +                LOGED(ERROR, domainid,
> +                    "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, 
> errno);
> +                fclose(f);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
> +        else
> +#endif
> +        {
> +            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, 
> errno);
> +                fclose(f);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
>      }
>      fclose(f);
>  
> @@ -2200,6 +2228,10 @@ static void pci_remove_detached(libxl__egc *egc,
>  #endif
>      uint32_t domainid = prs->domid;
>      bool isstubdom;
> +#ifdef CONFIG_X86
> +    int r;
> +    xc_domaininfo_t info;
> +#endif
>  
>      /* Convenience aliases */
>      libxl_device_pci *const pci = &prs->pci;
> @@ -2287,9 +2319,32 @@ 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);
> +#ifdef CONFIG_X86
> +        /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission 
> */
> +        r = xc_domain_getinfo_single(ctx->xch, 0, &info);
> +        if (r < 0) {
> +            LOGED(ERROR, domid, "getdomaininfo failed (error=%d)", errno);
> +            fclose(f);
> +            rc = ERROR_FAIL;
> +            goto skip_legacy_irq;
> +        }
> +        if (info.flags & XEN_DOMINF_hvm_guest &&
> +            !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) &&
> +            gsi > 0) {
> +            r = xc_domain_gsi_permission(ctx->xch, domid, gsi, 0);
> +            if (r < 0) {
> +                LOGED(ERROR, domid,
> +                    "xc_domain_gsi_permission gsi=%d (error=%d)", gsi, 
> errno);
> +                rc = ERROR_FAIL;
> +            }
> +        }
> +        else
> +#endif
> +        {
> +            rc = xc_domain_irq_permission(ctx->xch, domid, irq, 0);
> +            if (rc < 0) {
> +                LOGED(ERROR, domid, "xc_domain_irq_permission irq=%d", irq);
> +            }
>          }
>      }
>  
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 335aedf46d03..6b465bbc6ec0 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -36,6 +36,7 @@
>  #include <asm/xstate.h>
>  #include <asm/psr.h>
>  #include <asm/cpu-policy.h>
> +#include <asm/io_apic.h>
>  
>  static int update_domain_cpu_policy(struct domain *d,
>                                      xen_domctl_cpu_policy_t *xdpc)
> @@ -237,6 +238,48 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        int irq;
> +        bool allow = domctl->u.gsi_permission.allow_access;
> +
> +        /* Check all pads are zero */
> +        ret = -EINVAL;
> +        for ( i = 0;
> +              i < sizeof(domctl->u.gsi_permission.pad) /
> +                  sizeof(domctl->u.gsi_permission.pad[0]);
> +              ++i )
> +            if ( domctl->u.gsi_permission.pad[i] )
> +                goto out;
> +
> +        /*
> +         * If current domain is PV or it has PIRQ flag, it has a mapping
> +         * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission
> +         * to grant irq permission.
> +         */
> +        ret = -EOPNOTSUPP;
> +        if ( is_pv_domain(currd) || has_pirq(currd) )
> +            goto out;
> +
> +        ret = -EINVAL;
> +        if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 )
> +            goto out;
> +
> +        ret = -EPERM;
> +        if ( !irq_access_permitted(currd, irq) ||
> +             xsm_irq_permission(XSM_HOOK, d, irq, allow) )
Copy the question of Jan from the previous version to here:
Is it okay to issue the XSM check using the translated value, not the one that 
was originally passed into the hypercall?

> +            goto out;
> +
> +        if ( allow )
> +            ret = irq_permit_access(d, irq);
> +        else
> +            ret = irq_deny_access(d, irq);
> +
> +    out:
> +        break;
> +    }
> +
>      case XEN_DOMCTL_getpageframeinfo3:
>      {
>          unsigned int num = domctl->u.getpageframeinfo3.num;
> diff --git a/xen/arch/x86/include/asm/io_apic.h 
> b/xen/arch/x86/include/asm/io_apic.h
> index 78268ea8f666..7e86d8337758 100644
> --- a/xen/arch/x86/include/asm/io_apic.h
> +++ b/xen/arch/x86/include/asm/io_apic.h
> @@ -213,5 +213,7 @@ unsigned highest_gsi(void);
>  
>  int ioapic_guest_read( unsigned long physbase, unsigned int reg, u32 *pval);
>  int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val);
> +int mp_find_ioapic(int gsi);
> +int gsi_2_irq(int gsi);
>  
>  #endif
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index b48a64246548..23845c8cb11f 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -955,6 +955,23 @@ static int pin_2_irq(int idx, int apic, int pin)
>      return irq;
>  }
>  
> +int gsi_2_irq(int gsi)
> +{
> +    int ioapic, pin, irq;
> +
> +    ioapic = mp_find_ioapic(gsi);
> +    if ( ioapic < 0 )
> +        return -EINVAL;
> +
> +    pin = gsi - io_apic_gsi_base(ioapic);
> +
> +    irq = apic_pin_2_gsi_irq(ioapic, pin);
> +    if ( irq <= 0 )
> +        return -EINVAL;
> +
> +    return irq;
> +}
> +
>  static inline int IO_APIC_irq_trigger(int irq)
>  {
>      int apic, idx, pin;
> diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
> index d8ccab2449c6..c95da0de5770 100644
> --- a/xen/arch/x86/mpparse.c
> +++ b/xen/arch/x86/mpparse.c
> @@ -841,8 +841,7 @@ static struct mp_ioapic_routing {
>  } mp_ioapic_routing[MAX_IO_APICS];
>  
>  
> -static int mp_find_ioapic (
> -     int                     gsi)
> +int mp_find_ioapic(int gsi)
>  {
>       unsigned int            i;
>  
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 2a49fe46ce25..f7ae8b19d27d 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -464,6 +464,12 @@ struct xen_domctl_irq_permission {
>      uint8_t pad[3];
>  };
>  
> +/* XEN_DOMCTL_gsi_permission */
> +struct xen_domctl_gsi_permission {
> +    uint32_t gsi;
> +    uint8_t allow_access;    /* flag to specify enable/disable of x86 gsi 
> access */
> +    uint8_t pad[3];
> +};
>  
>  /* XEN_DOMCTL_iomem_permission */
>  struct xen_domctl_iomem_permission {
> @@ -1306,6 +1312,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_get_paging_mempool_size       85
>  #define XEN_DOMCTL_set_paging_mempool_size       86
>  #define XEN_DOMCTL_dt_overlay                    87
> +#define XEN_DOMCTL_gsi_permission                88
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> @@ -1328,6 +1335,7 @@ struct xen_domctl {
>          struct xen_domctl_setdomainhandle   setdomainhandle;
>          struct xen_domctl_setdebugging      setdebugging;
>          struct xen_domctl_irq_permission    irq_permission;
> +        struct xen_domctl_gsi_permission    gsi_permission;
>          struct xen_domctl_iomem_permission  iomem_permission;
>          struct xen_domctl_ioport_permission ioport_permission;
>          struct xen_domctl_hypercall_init    hypercall_init;
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 5e88c71b8e22..a5b134c91101 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -685,6 +685,7 @@ static int cf_check flask_domctl(struct domain *d, int 
> cmd)
>      case XEN_DOMCTL_shadow_op:
>      case XEN_DOMCTL_ioport_permission:
>      case XEN_DOMCTL_ioport_mapping:
> +    case XEN_DOMCTL_gsi_permission:
>  #endif
>  #ifdef CONFIG_HAS_PASSTHROUGH
>      /*

-- 
Best regards,
Jiqian Chen.