[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v12 4/7] x86/domctl: Add hypercall to set the access of x86 gsi
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.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |