[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC XEN PATCH v9 5/5] domctl: Add XEN_DOMCTL_gsi_permission to grant gsi
On 2024/6/12 18:34, Jan Beulich wrote: > On 12.06.2024 12:12, Chen, Jiqian wrote: >> On 2024/6/11 22:39, Jan Beulich wrote: >>> On 07.06.2024 10:11, Jiqian Chen wrote: >>>> Some type of domain don't have PIRQ, like PVH, it do not do >>>> PHYSDEVOP_map_pirq for each gsi. When passthrough a device >>>> to guest on PVH dom0, callstack >>>> pci_add_dm_done->XEN_DOMCTL_irq_permission will failed at >>>> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq >>>> and irq on Xen side. >>> >>> All of this is, to me at least, in pretty sharp contradiction to what >>> patch 2 says and does. IOW: Do we want the concept of pIRQ in PVH, or >>> do we want to keep that to PV? >> It's not contradictory. >> What I did is not to add the concept of PIRQs for PVH. > > After your further explanations on patch 2 - yes, I see now. But in particular > there it needs making more clear what case it is that is being enabled by the > changes. OK, I will add some descriptions in next version. > >>>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx> >>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> >>> >>> A problem throughout the series as it seems: Who's the author of these >>> patches? There's no From: saying it's not you, but your S-o-b also >>> isn't first. >> So I need to change to: >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> means I am the author. >> Signed-off-by: Huang Rui <ray.huang@xxxxxxx> means Rui sent them to upstream >> firstly. >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> means I take continue to >> upstream. > > I guess so, yes. Thanks. > >>>> --- a/tools/libs/light/libxl_pci.c >>>> +++ b/tools/libs/light/libxl_pci.c >>>> @@ -1412,6 +1412,37 @@ static bool pci_supp_legacy_irq(void) >>>> #define PCI_SBDF(seg, bus, devfn) \ >>>> ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn))) >>>> >>>> +static int pci_device_set_gsi(libxl_ctx *ctx, >>>> + libxl_domid domid, >>>> + libxl_device_pci *pci, >>>> + bool map, >>>> + int *gsi_back) >>>> +{ >>>> + int r, gsi, pirq; >>>> + uint32_t sbdf; >>>> + >>>> + sbdf = PCI_SBDF(pci->domain, pci->bus, (PCI_DEVFN(pci->dev, >>>> pci->func))); >>>> + r = xc_physdev_gsi_from_dev(ctx->xch, sbdf); >>>> + *gsi_back = r; >>>> + if (r < 0) >>>> + return r; >>>> + >>>> + gsi = r; >>>> + pirq = r; >>> >>> r is a GSI as per above; why would you store such in a variable named pirq? >>> And how can ... >>> >>>> + if (map) >>>> + r = xc_physdev_map_pirq(ctx->xch, domid, gsi, &pirq); >>>> + else >>>> + r = xc_physdev_unmap_pirq(ctx->xch, domid, pirq); >>> >>> ... that value be the correct one to pass into here? In fact, the pIRQ >>> number >>> you obtain above in the "map" case isn't handed to the caller, i.e. it is >>> effectively lost. Yet that's what would need passing into such an unmap >>> call. >> Yes r is GSI and I know pirq will be replaced by xc_physdev_map_pirq. >> What I do "pirq = r" is for xc_physdev_unmap_pirq, unmap need passing in >> pirq, >> and the number of pirq is always equal to gsi. > > Why would that be? pIRQ is purely a software construct (of Xen's), I > don't think there's any guarantee whatsoever on the numbering. And even > if there was (for e.g. non-MSI ones), it would be pIRQ == IRQ. And recall > that elsewhere I think I meanwhile succeeded in explaining to you that > IRQ != GSI (in the common case, even if in most cases they match). OK, will change in next version. > >>>> + if (r) >>>> + return r; >>>> + >>>> + r = xc_domain_gsi_permission(ctx->xch, domid, gsi, map); >>> >>> Looking at the hypervisor side, this will fail for PV Dom0. In which case >>> imo >>> you better would avoid making the call in the first place. >> Yes, for PV dom0, the errno is EOPNOTSUPP, then it will do below >> xc_domain_irq_permission. > > Hence why call xc_domain_gsi_permission() at all on a PV Dom0? Is there a function to distinguish that current dom0 is PV or PVH dom0 in tools/libs? > >>>> + if (r && errno == EOPNOTSUPP) >>> >>> Before here you don't really need the pIRQ number; if all it really is >>> needed >>> for is ... >>> >>>> + r = xc_domain_irq_permission(ctx->xch, domid, pirq, map); >>> >>> ... this, then it probably also should only be obtained when it's needed. >>> Yet >>> overall the intentions here aren't quite clear to me. >> Adding the function pci_device_set_gsi is for PVH dom0, while also ensuring >> compatibility with PV dom0. >> When PVH dom0, it does xc_physdev_map_pirq and xc_domain_gsi_permission(new >> hypercall for PVH dom0) >> When PV dom0, it keeps the same actions as before codes, it does >> xc_physdev_map_pirq and xc_domain_irq_permission. > > And why does PVH Dom0 need to call xc_physdev_map_pirq(), when in that case > the pIRQ isn't used? I didn't expect that introducing pci_device_set_gsi causes so many confusions. I will remove it in the next version and make modifications directly in pci_add_dm_done. > > Jan -- Best regards, Jiqian Chen.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |