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.hu...@amd.com>
> > Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
>
> 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
> >
>
