Re: [XEN PATCH v15 2/4] x86/irq: allow setting IRQ permissions from GSI instead of pIRQ

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

Hi Daniel,

On 2024/9/11 14:58, Chen, Jiqian wrote:
> Some domains are not aware of the pIRQ abstraction layer that maps
> interrupt sources into Xen space interrupt numbers.  pIRQs values are
> only exposed to domains that have the option to route physical
> interrupts over event channels.
> 
> This creates issues for PCI-passthrough from a PVH domain, as some of
> the passthrough related hypercalls use pIRQ as references to physical
> interrupts on the system.  One of such interfaces is
> XEN_DOMCTL_irq_permission, used to grant or revoke access to
> interrupts, takes a pIRQ as the reference to the interrupt to be
> adjusted.
> 
> Since PVH doesn't manage interrupts in terms of pIRQs, introduce a new
> hypercall that allows setting interrupt permissions based on GSI value
> rather than pIRQ.
> 
> Note the GSI hypercall parameters is translated to an IRQ value (in
> case there are ACPI overrides) before doing the checks.
> 
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> ---
> CC: Daniel P . Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> Remaining comment @Daniel P . Smith:
> +        ret = -EPERM;
> +        if ( !irq_access_permitted(currd, irq) ||
> +             xsm_irq_permission(XSM_HOOK, d, irq, flags) )
> +            break;
> Is it okay to issue the XSM check using the translated value(irq),
> not the one(gsi) that was originally passed into the hypercall?

Could you please take a time to answer at this question?

> ---
> v13->v15 changes:
> Change to use the commit message wrote by Roger.
> Change the code comment from "Check all bits are zero except lowest bit" to 
> "Check only valid bits are set".
> Change the end return sentence of gsi_2_irq to "return irq ?: -EINVAL;" to 
> preserve the error code from apic_pin_2_gsi_irq().
> 
> v12->v13 changes:
> For struct xen_domctl_gsi_permission, rename "access_flag" to "flags", change 
> its type from uint8_t to uint32_t, delete "pad", add XEN_DOMCTL_GSI_REVOKE 
> and XEN_DOMCTL_GSI_GRANT macros.
> Move "gsi > highest_gsi()" into function gsi_2_irq.
> Modify parameter gsi in function gsi_2_irq and mp_find_ioapic to unsigned int 
> type.
> Delete unnecessary spaces and brackets around "~XEN_DOMCTL_GSI_ACTION_MASK".
> Delete unnecessary goto statements and change to direct break.
> Add description in commit message to explain how gsi to irq isconverted.
> 
> v11->v12 changes:
> Change nr_irqs_gsi to highest_gsi() to check gsi boundary, then need to 
> remove "__init" of highest_gsi function.
> Change the check of irq boundary from <0 to <=0, and remove unnecessary space.
> Add #define XEN_DOMCTL_GSI_PERMISSION_MASK 1 to get lowest bit.
> 
> v10->v11 changes:
> Extracted from patch#5 of v10 into a separate patch.
> Add non-zero judgment for other bits of allow_access.
> Delete unnecessary judgment "if ( is_pv_domain(currd) || has_pirq(currd) )".
> Change the error exit path identifier "out" to "gsi_permission_out".
> Use ARRAY_SIZE() instead of open coed.
> 
> v9->v10 changes:
> Modified the commit message to further describe the purpose of adding 
> XEN_DOMCTL_gsi_permission.
> Added a check for all zeros in the padding field in 
> XEN_DOMCTL_gsi_permission, and used currd instead of current->domain.
> In the function gsi_2_irq, apic_pin_2_gsi_irq was used instead of the 
> original new code, and error handling for irq0 was added.
> Deleted the extra spaces in the upper and lower lines of the struct 
> xen_domctl_gsi_permission definition.
> 
> v8->v9 changes:
> Change the commit message to describe more why we need this new hypercall.
> Add comment above "if ( is_pv_domain(current->domain) || 
> has_pirq(current->domain) )" to explain why we need this check.
> Add gsi_2_irq to transform gsi to irq, instead of considering gsi == irq.
> Add explicit padding to struct xen_domctl_gsi_permission.
> 
> v5->v8 changes:
> Nothing.
> 
> v4->v5 changes:
> New implementation to add new hypercall XEN_DOMCTL_gsi_permission to grant 
> gsi.
> ---
>  xen/arch/x86/domctl.c              | 29 +++++++++++++++++++++++++++++
>  xen/arch/x86/include/asm/io_apic.h |  2 ++
>  xen/arch/x86/io_apic.c             | 19 +++++++++++++++++++
>  xen/arch/x86/mpparse.c             |  7 +++----
>  xen/include/public/domctl.h        | 10 ++++++++++
>  xen/xsm/flask/hooks.c              |  1 +
>  6 files changed, 64 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 68b5b46d1a83..939b1de0ee59 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,34 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        int irq;
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        uint32_t flags = domctl->u.gsi_permission.flags;
> +
> +        /* Check only valid bits are set */
> +        ret = -EINVAL;
> +        if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK )
> +            break;
> +
> +        ret = irq = gsi_2_irq(gsi);
> +        if ( ret <= 0 )
> +            break;
> +
> +        ret = -EPERM;
> +        if ( !irq_access_permitted(currd, irq) ||
> +             xsm_irq_permission(XSM_HOOK, d, irq, flags) )
> +            break;
> +
> +        if ( flags )
> +            ret = irq_permit_access(d, irq);
> +        else
> +            ret = irq_deny_access(d, irq);
> +
> +        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..62456806c7af 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(unsigned int gsi);
> +int gsi_2_irq(unsigned int gsi);
>  
>  #endif
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index 772700584639..e40d2f7dbd75 100644
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -955,6 +955,25 @@ static int pin_2_irq(int idx, int apic, int pin)
>      return irq;
>  }
>  
> +int gsi_2_irq(unsigned int gsi)
> +{
> +    int ioapic, irq;
> +    unsigned int pin;
> +
> +    if ( gsi > highest_gsi() )
> +        return -ERANGE;
> +
> +    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);
> +
> +    return irq ?: -EINVAL;
> +}
> +
>  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 306d8ed97a83..e13b83bbe9dd 100644
> --- a/xen/arch/x86/mpparse.c
> +++ b/xen/arch/x86/mpparse.c
> @@ -842,8 +842,7 @@ static struct mp_ioapic_routing {
>  } mp_ioapic_routing[MAX_IO_APICS];
>  
>  
> -static int mp_find_ioapic (
> -     int                     gsi)
> +int mp_find_ioapic(unsigned int gsi)
>  {
>       unsigned int            i;
>  
> @@ -854,7 +853,7 @@ static int mp_find_ioapic (
>                       return i;
>       }
>  
> -     printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %d\n", gsi);
> +     printk(KERN_ERR "ERROR: Unable to locate IOAPIC for GSI %u\n", gsi);
>  
>       return -1;
>  }
> @@ -915,7 +914,7 @@ void __init mp_register_ioapic (
>       return;
>  }
>  
> -unsigned __init highest_gsi(void)
> +unsigned highest_gsi(void)
>  {
>       unsigned x, res = 0;
>       for (x = 0; x < nr_ioapics; x++)
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 2a49fe46ce25..e1028fc524cf 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -464,6 +464,14 @@ struct xen_domctl_irq_permission {
>      uint8_t pad[3];
>  };
>  
> +/* XEN_DOMCTL_gsi_permission */
> +struct xen_domctl_gsi_permission {
> +    uint32_t gsi;
> +#define XEN_DOMCTL_GSI_REVOKE      0
> +#define XEN_DOMCTL_GSI_GRANT       1
> +#define XEN_DOMCTL_GSI_ACTION_MASK 1
> +    uint32_t flags;
> +};
>  
>  /* XEN_DOMCTL_iomem_permission */
>  struct xen_domctl_iomem_permission {
> @@ -1306,6 +1314,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 +1337,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 278ad38c2af3..dfa23738cd8a 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -695,6 +695,7 @@ static int cf_check flask_domctl(struct domain *d, 
> unsigned 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.