Re: [XEN PATCH v12 4/7] x86/domctl: Add hypercall to set the access of x86 gsi

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 08, 2024 at 07:41:21PM +0800, Jiqian Chen wrote:
> Some type of domains 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 to set the access of irq, it is not suitable for
> dom0 that doesn't have PIRQs.
> 
> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny
> the permission of irq(translate from x86 gsi) to dumU when dom0
                       ^ missing space, and s/translate/translated/

> 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>
> ---
> 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, access_flag) )
> +            goto gsi_permission_out;
> Is it okay to issue the XSM check using the translated value, 
> not the one that was originally passed into the hypercall?

FWIW, I don't see the GSI -> IRQ translation much different from the
pIRQ -> IRQ translation done by pirq_access_permitted(), which is also
ahead of the xsm check.

> ---
>  xen/arch/x86/domctl.c              | 32 ++++++++++++++++++++++++++++++
>  xen/arch/x86/include/asm/io_apic.h |  2 ++
>  xen/arch/x86/io_apic.c             | 17 ++++++++++++++++
>  xen/arch/x86/mpparse.c             |  5 ++---
>  xen/include/public/domctl.h        |  9 +++++++++
>  xen/xsm/flask/hooks.c              |  1 +
>  6 files changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 9190e11faaa3..4e9e4c4cfed3 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,37 @@ long arch_do_domctl(
>          break;
>      }
>  
> +    case XEN_DOMCTL_gsi_permission:
> +    {
> +        int irq;
> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> +        uint8_t access_flag = domctl->u.gsi_permission.access_flag;
> +
> +        /* Check all bits and pads are zero except lowest bit */
> +        ret = -EINVAL;
> +        if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_MASK ) )
                              ^ unneeded parentheses and spaces.
> +            goto gsi_permission_out;
> +        for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
> +            if ( domctl->u.gsi_permission.pad[i] )
> +                goto gsi_permission_out;
> +
> +        if ( gsi > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 )

FWIW, I would place the gsi > highest_gsi() check inside gsi_2_irq().
There's no reason to open-code it here, and it could help other
users of gsi_2_irq().  The error code could also be ERANGE here
instead of EINVAL IMO.

> +            goto gsi_permission_out;
> +
> +        ret = -EPERM;
> +        if ( !irq_access_permitted(currd, irq) ||
> +             xsm_irq_permission(XSM_HOOK, d, irq, access_flag) )
> +            goto gsi_permission_out;
> +
> +        if ( access_flag )
> +            ret = irq_permit_access(d, irq);
> +        else
> +            ret = irq_deny_access(d, irq);
> +
> +    gsi_permission_out:
> +        break;

Why do you need a label when it just contains a break?  Instead of the
goto gsi_permission_out just use break directly.

> +    }
> +
>      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 d2a313c4ac72..5968c8055671 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)

unsigned int for gsi.

> +{
> +    int ioapic, pin, irq;

pin would better be unsigned int also.

> +
> +    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..7786a3337760 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)

If you are changing this, you might as well make the gsi parameter
unsigned int.

>  {
>       unsigned int            i;
>  
> @@ -914,7 +913,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..877e35ab1376 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -464,6 +464,13 @@ 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_PERMISSION_MASK 1

IMO this would be better named GRANT or similar, maybe something like:

/* Low bit used to signal grant/revoke action. */
#define XEN_DOMCTL_GSI_REVOKE 0
#define XEN_DOMCTL_GSI_GRANT  1

> +    uint8_t access_flag;    /* flag to specify enable/disable of x86 gsi 
> access */
> +    uint8_t pad[3];

We might as well declare the flags field as uint32_t and avoid the
padding field.

Thanks, Roger.