Hi Anthony, On 2024/2/21 21:38, Anthony PERARD wrote: > 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 <[email protected]> > > 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 <[email protected]>" Ok, will add this in next version.
> > Beyond that, the patch looks good, I've only coding style issues. There are two encoding styles in the current file, and I was also struggling with which one to follow when I write my code, it seems that I made the wrong choice. Thank you very much for your suggestions. I will fix all coding style issues that you pointed out in next version. > >> Signed-off-by: Jiqian Chen <[email protected]> >> Reviewed-by: Stefano Stabellini <[email protected]> >> --- >> 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, > -- Best regards, Jiqian Chen.
