[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH v10 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi
On 17.06.2024 11:00, Jiqian Chen wrote: > Some type of domain 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, it is not suitable for dom0 that doesn't have > PIRQs. > > So, add a new hypercall XEN_DOMCTL_gsi_permission to grant the > permission of irq(translate from gsi) to dumU when dom0 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> > --- > RFC: it needs review and needs to wait for the corresponding third patch on > linux kernel side to be merged. > --- > tools/include/xenctrl.h | 5 +++ > tools/libs/ctrl/xc_domain.c | 15 +++++++ > tools/libs/light/libxl_pci.c | 67 +++++++++++++++++++++++++++--- > xen/arch/x86/domctl.c | 43 +++++++++++++++++++ > xen/arch/x86/include/asm/io_apic.h | 2 + > xen/arch/x86/io_apic.c | 17 ++++++++ > xen/arch/x86/mpparse.c | 3 +- > xen/include/public/domctl.h | 8 ++++ > xen/xsm/flask/hooks.c | 1 + > 9 files changed, 153 insertions(+), 8 deletions(-) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index a0381f74d24b..f3feb6848e25 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -1382,6 +1382,11 @@ int xc_domain_irq_permission(xc_interface *xch, > uint32_t pirq, > bool allow_access); > > +int xc_domain_gsi_permission(xc_interface *xch, > + uint32_t domid, > + uint32_t gsi, > + bool allow_access); > + > int xc_domain_iomem_permission(xc_interface *xch, > uint32_t domid, > unsigned long first_mfn, > diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c > index f2d9d14b4d9f..8540e84fda93 100644 > --- a/tools/libs/ctrl/xc_domain.c > +++ b/tools/libs/ctrl/xc_domain.c > @@ -1394,6 +1394,21 @@ int xc_domain_irq_permission(xc_interface *xch, > return do_domctl(xch, &domctl); > } > > +int xc_domain_gsi_permission(xc_interface *xch, > + uint32_t domid, > + uint32_t gsi, > + bool allow_access) > +{ > + struct xen_domctl domctl = { > + .cmd = XEN_DOMCTL_gsi_permission, > + .domain = domid, > + .u.gsi_permission.gsi = gsi, > + .u.gsi_permission.allow_access = allow_access, > + }; > + > + return do_domctl(xch, &domctl); > +} > + > int xc_domain_iomem_permission(xc_interface *xch, > uint32_t domid, > unsigned long first_mfn, > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > index 376f91759ac6..f027f22c0028 100644 > --- a/tools/libs/light/libxl_pci.c > +++ b/tools/libs/light/libxl_pci.c > @@ -1431,6 +1431,9 @@ static void pci_add_dm_done(libxl__egc *egc, > uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED; > uint32_t domainid = domid; > bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid); > +#ifdef CONFIG_X86 > + xc_domaininfo_t info; > +#endif > > /* Convenience aliases */ > bool starting = pas->starting; > @@ -1516,14 +1519,39 @@ static void pci_add_dm_done(libxl__egc *egc, > rc = ERROR_FAIL; > goto out; > } > - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > +#ifdef CONFIG_X86 > + /* If dom0 doesn't have PIRQs, need to use xc_domain_gsi_permission > */ > + r = xc_domain_getinfo_single(ctx->xch, 0, &info); Hard-coded 0 is imposing limitations. Ideally you would use DOMID_SELF, but I didn't check if that can be used with the underlying hypercall(s). Otherwise you want to pass the actual domid of the local domain here. > if (r < 0) { > - LOGED(ERROR, domainid, > - "xc_domain_irq_permission irq=%d (error=%d)", irq, r); > + LOGED(ERROR, domainid, "getdomaininfo failed (error=%d)", errno); > fclose(f); > rc = ERROR_FAIL; > goto out; > } > + if (info.flags & XEN_DOMINF_hvm_guest && You want to parenthesize the & here. > + !(info.arch_config.emulation_flags & XEN_X86_EMU_USE_PIRQ) && > + gsi > 0) { So if gsi < 0 failure of xc_domain_getinfo_single() would needlessly result in failure of this function? > --- 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,48 @@ long arch_do_domctl( > break; > } > > + case XEN_DOMCTL_gsi_permission: > + { > + unsigned int gsi = domctl->u.gsi_permission.gsi; > + int irq; > + bool allow = domctl->u.gsi_permission.allow_access; See my earlier comments on this conversion of 8 bits into just one. > + /* Check all pads are zero */ > + ret = -EINVAL; > + for ( i = 0; > + i < sizeof(domctl->u.gsi_permission.pad) / > + sizeof(domctl->u.gsi_permission.pad[0]); Please don't open-code ARRAY_SIZE(). > + ++i ) > + if ( domctl->u.gsi_permission.pad[i] ) > + goto out; > + > + /* > + * If current domain is PV or it has PIRQ flag, it has a mapping > + * of gsi, pirq and irq, so it should use XEN_DOMCTL_irq_permission > + * to grant irq permission. > + */ > + ret = -EOPNOTSUPP; > + if ( is_pv_domain(currd) || has_pirq(currd) ) > + goto out; I'm curious what other x86 maintainers think: I for one would not impose such an artificial restriction. > + ret = -EINVAL; > + if ( gsi >= nr_irqs_gsi || (irq = gsi_2_irq(gsi)) < 0 ) > + goto out; > + > + ret = -EPERM; > + if ( !irq_access_permitted(currd, irq) || > + xsm_irq_permission(XSM_HOOK, d, irq, allow) ) > + goto out; > + > + if ( allow ) > + ret = irq_permit_access(d, irq); > + else > + ret = irq_deny_access(d, irq); > + > + out: Please use a less generic name for such a label local to just one case block. However, with .. > + break; .. this being all that's done here: Why have a label in the first place? Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |