| Message ID | 20240105070920.350113-5-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
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@amd.com> > Signed-off-by: Jiqian Chen <Jiqian.Chen@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; > 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 >
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@amd.com> > > Signed-off-by: Jiqian Chen <Jiqian.Chen@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 > > >
On 2024/1/6 09:08, 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@amd.com> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@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]; > }; Will add this comment in next version, thanks. > > >> --- >> 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; >> 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... Here pass the pointer of irq to xc_physdev_map_pirq, and that function will assign the value of pirq to irq on Xen side, so after calling xc_physdev_map_pirq, the value of irq isn't the original value, we need a local variable to record the original value. > > >> 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); Will add this comment in next version, thanks. > > >> - 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 ? No, the same reason as above. > > >> { >> 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; Thank you, I will rename it in next version. > > >> - 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 >>
On 06.01.2024 02:08, Stefano Stabellini wrote: > On Fri, 5 Jan 2024, Jiqian Chen wrote: >> --- 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; >> 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)? Iirc for IO-APIC based IRQs that's always the case; for MSI ones there simply is no GSI, and Xen's IRQ number then normally isn't the same as the pIRQ a domain would see. Jan
On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote: > On 06.01.2024 02:08, Stefano Stabellini wrote: > > On Fri, 5 Jan 2024, Jiqian Chen wrote: > >> --- 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; > >> 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)? > > Iirc for IO-APIC based IRQs that's always the case; I think that's always the case on Linux, because it calls PHYSDEVOP_map_pirq with index == pirq (see Linux pci_xen_initial_domain()). But other OSes could possibly make the call with pirq == -1 and get a randomly allocated pirq for GSIs. IOW: I don't think the pirq field in xen_domctl_irq_permission can be altered like proposed here to switch from passing a pirq to a GSI. A new hypercall should be introduced that either is GSI specific, or contains a type field in order to specify the namespace the field targets. Thanks, Roger.
On 2024/1/8 23:05, Roger Pau Monné wrote: > On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote: >> On 06.01.2024 02:08, Stefano Stabellini wrote: >>> On Fri, 5 Jan 2024, Jiqian Chen wrote: >>>> --- 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; >>>> 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)? >> >> Iirc for IO-APIC based IRQs that's always the case; > > I think that's always the case on Linux, because it calls > PHYSDEVOP_map_pirq with index == pirq (see Linux > pci_xen_initial_domain()). But other OSes could possibly make the > call with pirq == -1 and get a randomly allocated pirq for GSIs. I don't think it's important whether pirq is randomly generated. What's important is whether irq always equals gsi in xen. If so, we can directly pass in and grant gsi. However, according to Jan's previous email reply, in the case of Msi, irq may not be equal to gsi, so my patch cannot meet this situation. I am confusing if the current domain doesn't have PIRQ flag, then regarding to XEN_DOMCTL_irq_permission, which kind of irq we should grant to caller domain? The gsi or other irq? Or can we add a check in XEN_DOMCTL_irq_permission, if the current domain has PRIQ, we can get irq from pirq(like the original implementation), if not we can assign gsi to irq, and then grant irq. Of course, that needs to require the caller to pass in both the pirq and gsi. > > IOW: I don't think the pirq field in xen_domctl_irq_permission can be > altered like proposed here to switch from passing a pirq to a GSI. A > new hypercall should be introduced that either is GSI specific, or > contains a type field in order to specify the namespace the field > targets. A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall? I mean how the caller know if the current domain has PIRQ or not and when to pass in pirq number, when to pass in gsi number. > > Thanks, Roger.
On 09.01.2024 09:18, Chen, Jiqian wrote: > On 2024/1/8 23:05, Roger Pau Monné wrote: >> On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote: >>> On 06.01.2024 02:08, Stefano Stabellini wrote: >>>> On Fri, 5 Jan 2024, Jiqian Chen wrote: >>>>> --- 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; >>>>> 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)? >>> >>> Iirc for IO-APIC based IRQs that's always the case; >> >> I think that's always the case on Linux, because it calls >> PHYSDEVOP_map_pirq with index == pirq (see Linux >> pci_xen_initial_domain()). But other OSes could possibly make the >> call with pirq == -1 and get a randomly allocated pirq for GSIs. > I don't think it's important whether pirq is randomly generated. What's important is whether irq always equals gsi in xen. > If so, we can directly pass in and grant gsi. However, according to Jan's previous email reply, in the case of Msi, irq may not be equal to gsi, so my patch cannot meet this situation. > I am confusing if the current domain doesn't have PIRQ flag, then regarding to XEN_DOMCTL_irq_permission, which kind of irq we should grant to caller domain? The gsi or other irq? > Or can we add a check in XEN_DOMCTL_irq_permission, if the current domain has PRIQ, we can get irq from pirq(like the original implementation), if not we can assign gsi to irq, and then grant irq. Of course, that needs to require the caller to pass in both the pirq and gsi. I expect MSI will need handling differently from GSIs. When MSI is set up for a device (and hence for a domain in possession of that device), access ought to be granted right away. >> IOW: I don't think the pirq field in xen_domctl_irq_permission can be >> altered like proposed here to switch from passing a pirq to a GSI. A >> new hypercall should be introduced that either is GSI specific, or >> contains a type field in order to specify the namespace the field >> targets. > A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall? Either we add a feature indicator, or the caller simply tries the new GSI interface first. > I mean how the caller know if the current domain has PIRQ or not and when to pass in pirq number, when to pass in gsi number. An interface that's GSI-centric would only ever be passed a GSI (of course, I'm inclined to add). Jan
On 2024/1/9 17:38, Jan Beulich wrote: > On 09.01.2024 09:18, Chen, Jiqian wrote: >> On 2024/1/8 23:05, Roger Pau Monné wrote: >>> On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote: >>>> On 06.01.2024 02:08, Stefano Stabellini wrote: >>>>> On Fri, 5 Jan 2024, Jiqian Chen wrote: >>>>>> --- 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; >>>>>> 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)? >>>> >>>> Iirc for IO-APIC based IRQs that's always the case; >>> >>> I think that's always the case on Linux, because it calls >>> PHYSDEVOP_map_pirq with index == pirq (see Linux >>> pci_xen_initial_domain()). But other OSes could possibly make the >>> call with pirq == -1 and get a randomly allocated pirq for GSIs. >> I don't think it's important whether pirq is randomly generated. What's important is whether irq always equals gsi in xen. >> If so, we can directly pass in and grant gsi. However, according to Jan's previous email reply, in the case of Msi, irq may not be equal to gsi, so my patch cannot meet this situation. >> I am confusing if the current domain doesn't have PIRQ flag, then regarding to XEN_DOMCTL_irq_permission, which kind of irq we should grant to caller domain? The gsi or other irq? >> Or can we add a check in XEN_DOMCTL_irq_permission, if the current domain has PRIQ, we can get irq from pirq(like the original implementation), if not we can assign gsi to irq, and then grant irq. Of course, that needs to require the caller to pass in both the pirq and gsi. > > I expect MSI will need handling differently from GSIs. When MSI is > set up for a device (and hence for a domain in possession of that > device), access ought to be granted right away. > >>> IOW: I don't think the pirq field in xen_domctl_irq_permission can be >>> altered like proposed here to switch from passing a pirq to a GSI. A >>> new hypercall should be introduced that either is GSI specific, or >>> contains a type field in order to specify the namespace the field >>> targets. >> A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall? > > Either we add a feature indicator, or the caller simply tries the > new GSI interface first. I am still not sure how to use and implement it. Taking pci_add_dm_done as an example, for now its implementation is: pci_add_dm_done xc_physdev_map_pirq xc_domain_irq_permission(,,pirq,) XEN_DOMCTL_irq_permission And assume the new hypercall is XEN_DOMCTL_gsi_permission, do you mean: pci_add_dm_done xc_physdev_map_pirq ret = xc_domain_gsi_permission(,,gsi,) XEN_DOMCTL_gsi_permission if ( ret != 0 ) xc_domain_irq_permission(,,pirq,) XEN_DOMCTL_irq_permission But if so, I have a question that in XEN_DOMCTL_gsi_permission, when to fail and when to success? Or do you mean: pci_add_dm_done xc_physdev_map_pirq ret = xc_domain_irq_permission(,,pirq,) XEN_DOMCTL_irq_permission if ( ret != 0 ) xc_domain_gsi_permission(,,gsi,) XEN_DOMCTL_gsi_permission And in XEN_DOMCTL_gsi_permission, as long as the current domain has the access of gsi, then granting gsi to caller should be successful. Right? > >> I mean how the caller know if the current domain has PIRQ or not and when to pass in pirq number, when to pass in gsi number. > > An interface that's GSI-centric would only ever be passed a GSI > (of course, I'm inclined to add). > > Jan
On Tue, Jan 09, 2024 at 10:16:26AM +0000, Chen, Jiqian wrote: > On 2024/1/9 17:38, Jan Beulich wrote: > > On 09.01.2024 09:18, Chen, Jiqian wrote: > >> On 2024/1/8 23:05, Roger Pau Monné wrote: > >>> On Mon, Jan 08, 2024 at 09:55:26AM +0100, Jan Beulich wrote: > >>>> On 06.01.2024 02:08, Stefano Stabellini wrote: > >>>>> On Fri, 5 Jan 2024, Jiqian Chen wrote: > >>>>>> --- 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; > >>>>>> 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)? > >>>> > >>>> Iirc for IO-APIC based IRQs that's always the case; > >>> > >>> I think that's always the case on Linux, because it calls > >>> PHYSDEVOP_map_pirq with index == pirq (see Linux > >>> pci_xen_initial_domain()). But other OSes could possibly make the > >>> call with pirq == -1 and get a randomly allocated pirq for GSIs. > >> I don't think it's important whether pirq is randomly generated. What's important is whether irq always equals gsi in xen. My 'randomly generated' comment was in that direction, not so much as the pirq being truly random, but no longer matching the gsi. > >> If so, we can directly pass in and grant gsi. However, according to Jan's previous email reply, in the case of Msi, irq may not be equal to gsi, so my patch cannot meet this situation. > >> I am confusing if the current domain doesn't have PIRQ flag, then regarding to XEN_DOMCTL_irq_permission, which kind of irq we should grant to caller domain? The gsi or other irq? > >> Or can we add a check in XEN_DOMCTL_irq_permission, if the current domain has PRIQ, we can get irq from pirq(like the original implementation), if not we can assign gsi to irq, and then grant irq. Of course, that needs to require the caller to pass in both the pirq and gsi. > > > > I expect MSI will need handling differently from GSIs. When MSI is > > set up for a device (and hence for a domain in possession of that > > device), access ought to be granted right away. > > > >>> IOW: I don't think the pirq field in xen_domctl_irq_permission can be > >>> altered like proposed here to switch from passing a pirq to a GSI. A > >>> new hypercall should be introduced that either is GSI specific, or > >>> contains a type field in order to specify the namespace the field > >>> targets. > >> A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall? > > > > Either we add a feature indicator, or the caller simply tries the > > new GSI interface first. > I am still not sure how to use and implement it. > Taking pci_add_dm_done as an example, for now its implementation is: > pci_add_dm_done > xc_physdev_map_pirq > xc_domain_irq_permission(,,pirq,) > XEN_DOMCTL_irq_permission > > And assume the new hypercall is XEN_DOMCTL_gsi_permission, do you mean: > pci_add_dm_done > xc_physdev_map_pirq > ret = xc_domain_gsi_permission(,,gsi,) > XEN_DOMCTL_gsi_permission Making this first call would also depend on whether the 'gsi' can be fetched from sysfs, otherwise the call should be avoided. > if ( ret != 0 ) > xc_domain_irq_permission(,,pirq,) > XEN_DOMCTL_irq_permission You can only fallback when you have the pirq (ie: when in a PV dom0), on a PVH dom0 there's no pirq, so no fallback. The code in pci_add_dm_done() must be aware of this, and only fallback when in PV dom0. > But if so, I have a question that in XEN_DOMCTL_gsi_permission, when to fail and when to success? > > Or do you mean: > pci_add_dm_done > xc_physdev_map_pirq > ret = xc_domain_irq_permission(,,pirq,) > XEN_DOMCTL_irq_permission > if ( ret != 0 ) > xc_domain_gsi_permission(,,gsi,) > XEN_DOMCTL_gsi_permission > And in XEN_DOMCTL_gsi_permission, as long as the current domain has the access of gsi, then granting gsi to caller should be successful. Right? No, I think that would be backwards, the logic above looks correct regarding the ordering of the hypercalls. Thanks, Roger.
On 09.01.2024 11:16, Chen, Jiqian wrote: > On 2024/1/9 17:38, Jan Beulich wrote: >> On 09.01.2024 09:18, Chen, Jiqian wrote: >>> A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall? >> >> Either we add a feature indicator, or the caller simply tries the >> new GSI interface first. > I am still not sure how to use and implement it. > Taking pci_add_dm_done as an example, for now its implementation is: > pci_add_dm_done > xc_physdev_map_pirq > xc_domain_irq_permission(,,pirq,) > XEN_DOMCTL_irq_permission > > And assume the new hypercall is XEN_DOMCTL_gsi_permission, do you mean: > pci_add_dm_done > xc_physdev_map_pirq > ret = xc_domain_gsi_permission(,,gsi,) > XEN_DOMCTL_gsi_permission > if ( ret != 0 ) > xc_domain_irq_permission(,,pirq,) > XEN_DOMCTL_irq_permission No, falling back shouldn't be "blind". Fallback should only happen when the new sub-op isn't implemented (hence why a feature indicator may be necessary), and only if calling the existing sub-op promises to be useful (which iirc would limit that to the PV Dom0 case). > But if so, I have a question that in XEN_DOMCTL_gsi_permission, when to fail and when to success? I'm afraid I don't understand the question. Behavior there isn't to be fundamentally different from that for XEN_DOMCTL_irq_permission. It's just that the incoming value is in another value space. > Or do you mean: > pci_add_dm_done > xc_physdev_map_pirq > ret = xc_domain_irq_permission(,,pirq,) > XEN_DOMCTL_irq_permission > if ( ret != 0 ) > xc_domain_gsi_permission(,,gsi,) > XEN_DOMCTL_gsi_permission No, this looks the wrong way round. > And in XEN_DOMCTL_gsi_permission, as long as the current domain has the access of gsi, then granting gsi to caller should be successful. Right? I think so; see above. Jan
Thank Jan and Roger, I may know how to add a new hypercall XEN_DOMCTL_gsi_permission, I will implement it in next version. On 2024/1/9 18:46, Jan Beulich wrote: > On 09.01.2024 11:16, Chen, Jiqian wrote: >> On 2024/1/9 17:38, Jan Beulich wrote: >>> On 09.01.2024 09:18, Chen, Jiqian wrote: >>>> A new hypercall using for granting gsi? If so, how does the caller know to call which hypercall to grant permission, XEN_DOMCTL_irq_permission or that new hypercall? >>> >>> Either we add a feature indicator, or the caller simply tries the >>> new GSI interface first. >> I am still not sure how to use and implement it. >> Taking pci_add_dm_done as an example, for now its implementation is: >> pci_add_dm_done >> xc_physdev_map_pirq >> xc_domain_irq_permission(,,pirq,) >> XEN_DOMCTL_irq_permission >> >> And assume the new hypercall is XEN_DOMCTL_gsi_permission, do you mean: >> pci_add_dm_done >> xc_physdev_map_pirq >> ret = xc_domain_gsi_permission(,,gsi,) >> XEN_DOMCTL_gsi_permission >> if ( ret != 0 ) >> xc_domain_irq_permission(,,pirq,) >> XEN_DOMCTL_irq_permission > > No, falling back shouldn't be "blind". Fallback should only happen > when the new sub-op isn't implemented (hence why a feature indicator > may be necessary), and only if calling the existing sub-op promises > to be useful (which iirc would limit that to the PV Dom0 case). > >> But if so, I have a question that in XEN_DOMCTL_gsi_permission, when to fail and when to success? > > I'm afraid I don't understand the question. Behavior there isn't to > be fundamentally different from that for XEN_DOMCTL_irq_permission. > It's just that the incoming value is in another value space. > >> Or do you mean: >> pci_add_dm_done >> xc_physdev_map_pirq >> ret = xc_domain_irq_permission(,,pirq,) >> XEN_DOMCTL_irq_permission >> if ( ret != 0 ) >> xc_domain_gsi_permission(,,gsi,) >> XEN_DOMCTL_gsi_permission > > No, this looks the wrong way round. > >> And in XEN_DOMCTL_gsi_permission, as long as the current domain has the access of gsi, then granting gsi to caller should be successful. Right? > > I think so; see above. > > Jan
On 1/5/24 02:09, Jiqian Chen wrote: > 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; > > - if ( pirq >= current->domain->nr_pirqs ) > + if ( pirq >= nr_irqs_gsi ) This doesn't build on ARM, as nr_irqs_gsi is x86 only. This is a wild guess: we may want keep the existing current->domain->nr_pirqs check, then add the new nr_irqs_gsi check wrapped in #ifdef CONFIG_X86. > { > 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 )
On 2024/1/11 04:09, Stewart Hildebrand wrote: > On 1/5/24 02:09, Jiqian Chen wrote: >> 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; >> >> - if ( pirq >= current->domain->nr_pirqs ) >> + if ( pirq >= nr_irqs_gsi ) > > This doesn't build on ARM, as nr_irqs_gsi is x86 only. This is a wild guess: we may want keep the existing current->domain->nr_pirqs check, then add the new nr_irqs_gsi check wrapped in #ifdef CONFIG_X86. As I will add a new hypercall in next version, then this change will not exist, thank you. > >> { >> 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 )
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; 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; 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; } - 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) { 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; - 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 )
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@amd.com> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> --- 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(-)