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.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?
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.
