[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC XEN PATCH v4 4/5] domctl: Use gsi to grant/revoke irq permission
On Fri, 5 Jan 2024, Stefano Stabellini wrote: > On Fri, 5 Jan 2024, Jiqian Chen wrote: > > Some type of domain don't have PIRQ, like PVH, current > > implementation is not suitable for those domain. > > > > When passthrough a device to guest on PVH dom0, this > > pci_add_dm_done->XEN_DOMCTL_irq_permission will failed > > at domain_pirq_to_irq. > > > > So, change it to use gsi to grant/revoke irq permission. > > And change the caller of XEN_DOMCTL_irq_permission to > > pass gsi to it instead of pirq. > > > > Co-developed-by: Huang Rui <ray.huang@xxxxxxx> > > Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> > > I realize there is no explanation or comment right now, but I think this > change would benefit from a in-code comment in xen/include/public/domctl.h > E.g.: > > /* XEN_DOMCTL_irq_permission */ > struct xen_domctl_irq_permission { > uint32_t pirq; /* pirq is the GSI on x86 */ > uint8_t allow_access; /* flag to specify enable/disable of IRQ access > */ > uint8_t pad[3]; > }; > > > > --- > > tools/libs/light/libxl_pci.c | 6 ++++-- > > tools/libs/light/libxl_x86.c | 5 ++++- > > xen/common/domctl.c | 12 ++++++++++-- > > 3 files changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > > index 96cb4da0794e..e1341d1e9850 100644 > > --- a/tools/libs/light/libxl_pci.c > > +++ b/tools/libs/light/libxl_pci.c > > @@ -1418,6 +1418,7 @@ static void pci_add_dm_done(libxl__egc *egc, > > unsigned long long start, end, flags, size; > > int irq, i; > > int r; > > + int gsi; After reading patch #5 I think you could just rename the int irq local variable to int gsi. > > uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED; > > uint32_t domainid = domid; > > bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid); > > @@ -1486,6 +1487,7 @@ static void pci_add_dm_done(libxl__egc *egc, > > goto out_no_irq; > > } > > if ((fscanf(f, "%u", &irq) == 1) && irq) { > > + gsi = irq; > > A question for Roger and Jan: are we always guaranteed that gsi == irq > (also in the PV case)? > > Also I don't think we necessarily need the gsi local variable, I would > just add an in-code comment below... > > > > r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq); > > if (r < 0) { > > LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)", > > @@ -1494,10 +1496,10 @@ static void pci_add_dm_done(libxl__egc *egc, > > rc = ERROR_FAIL; > > goto out; > > } > > ... like this: > > /* xc_domain_irq_permission takes a gsi, here we assume irq == gsi */ > r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > > > > - r = xc_domain_irq_permission(ctx->xch, domid, irq, 1); > > + r = xc_domain_irq_permission(ctx->xch, domid, gsi, 1); > > if (r < 0) { > > LOGED(ERROR, domainid, > > - "xc_domain_irq_permission irq=%d (error=%d)", irq, r); > > + "xc_domain_irq_permission irq=%d (error=%d)", gsi, r); > > fclose(f); > > rc = ERROR_FAIL; > > goto out; > > diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c > > index d16573e72cd4..88ad50cf6360 100644 > > --- a/tools/libs/light/libxl_x86.c > > +++ b/tools/libs/light/libxl_x86.c > > @@ -654,12 +654,15 @@ out: > > int libxl__arch_domain_map_irq(libxl__gc *gc, uint32_t domid, int irq) > > you could just rename the int irq parameter to int gsi ? > > > > { > > int ret; > > + int gsi; > > + > > + gsi = irq; > > > > ret = xc_physdev_map_pirq(CTX->xch, domid, irq, &irq); > > if (ret) > > return ret; > > > > - ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1); > > + ret = xc_domain_irq_permission(CTX->xch, domid, gsi, 1); > > return ret; > > } > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > > index f5a71ee5f78d..eeb975bd0194 100644 > > --- a/xen/common/domctl.c > > +++ b/xen/common/domctl.c > > @@ -653,12 +653,20 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) > > u_domctl) > > unsigned int pirq = op->u.irq_permission.pirq, irq; > > int allow = op->u.irq_permission.allow_access; > > here we could benefit from renaming pirq to gsi, at least it becomes > clear: > > unsigned int gsi = op->u.irq_permission.pirq, irq; > > > > - if ( pirq >= current->domain->nr_pirqs ) > > + if ( pirq >= nr_irqs_gsi ) > > { > > ret = -EINVAL; > > break; > > } > > - irq = pirq_access_permitted(current->domain, pirq); > > + > > + if ( irq_access_permitted(current->domain, pirq) ) > > + irq = pirq; > > + else > > + { > > + ret = -EPERM; > > + break; > > + } > > + > > if ( !irq || xsm_irq_permission(XSM_HOOK, d, irq, allow) ) > > ret = -EPERM; > > else if ( allow ) > > -- > > 2.34.1 > > >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |