On Mon, 8 Jul 2024, 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
> has no PIRQs.
>
> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
> Signed-off-by: Huang Rui <ray.hu...@amd.com>
> Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
> ---
> CC: Daniel P . Smith <dpsm...@apertussolutions.com>
> 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?
> ---
> 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 ) )
> + 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 )
gsi is unsigned int but it is passed to gsi_2_irq which takes an int as
parameter. If gsi >= INT32_MAX we have a problem. I think we should
explicitly check for the possible overflow and return error in that
case.
> + 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;
> + }
> +
> 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)
> +{
> + 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..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)
> {
> 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
> + uint8_t access_flag; /* flag to specify enable/disable of x86 gsi
> access */
> + uint8_t pad[3];
> +};
>
> /* XEN_DOMCTL_iomem_permission */
> struct xen_domctl_iomem_permission {
> @@ -1306,6 +1313,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 +1336,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
> /*
> --
> 2.34.1
>
