[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC XEN PATCH v5 4/5] libxl: Use gsi instead of irq for mapping pirq
Hi Jiqian, On Fri, Jan 12, 2024 at 02:13:16PM +0800, Jiqian Chen wrote: > In PVH dom0, it uses the linux local interrupt mechanism, > when it allocs irq for a gsi, it is dynamic, and follow > the principle of applying first, distributing first. And > the irq number is alloced from small to large, but the > applying gsi number is not, may gsi 38 comes before gsi > 28, that causes the irq number is not equal with the gsi > number. And when passthrough a device, xl wants to use > gsi to map pirq, see pci_add_dm_done->xc_physdev_map_pirq, > but the gsi number is got from file > /sys/bus/pci/devices/<sbdf>/irq in current code, so it > will fail when mapping. > > So, use real gsi number read from gsi sysfs. > > Co-developed-by: Huang Rui <ray.huang@xxxxxxx> The "Co-developed-by" tag isn't really defined in Xen, it's fine to keep I guess, but it mean that another tag is missing, I'm pretty sure you need to add "Signed-off-by: Huang Rui <ray.huang@xxxxxxx>" Beyond that, the patch looks good, I've only coding style issues. > Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > --- > tools/libs/light/libxl_pci.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c > index 96cb4da0794e..a1c6e82631e9 100644 > --- a/tools/libs/light/libxl_pci.c > +++ b/tools/libs/light/libxl_pci.c > @@ -1478,8 +1478,19 @@ static void pci_add_dm_done(libxl__egc *egc, > fclose(f); > if (!pci_supp_legacy_irq()) > goto out_no_irq; > - sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain, > + sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/gsi", pci->domain, > pci->bus, pci->dev, pci->func); > + > + if ( access(sysfs_path, F_OK) != 0 ) { So, the coding style in libxl is a bit different, first could you store the return value of access() in 'r', then check that value? Then "if (r)" will be enough, no need for "!= 0". Second, there's no around space the condition in the if statement, so just "if (r)". > + if ( errno == ENOENT ) > + sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", > pci->domain, > + pci->bus, pci->dev, pci->func); Has the else part of this if..else is in a {}-block, could you add { } here, for the if part? To quote the CODING_STYLE libxl document: "To avoid confusion, either all the blocks in an if...else chain have braces, or none of them do. > + else { > + LOGED(ERROR, domainid, "Can't access %s", sysfs_path); This error message is kind of redundant, we could could simply let the code try fopen() on this "/gsi" path, even if we know that it's not going to work, as the error check on fopen() will produce the same error message. ;-) And without that else part, the code could be simplified to: /* If /gsi path doesn't exist, fallback to /irq */ if (r && errno == ENOENT) sysfs_path = "..../irq"; And those comments also apply to the changes in pci_remove_detached(). Thanks, -- Anthony PERARD
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |