| Message ID | 20240112061317.418658-5-Jiqian.Chen@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support device passthrough when dom0 is PVH on Xen | expand |
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@amd.com> 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@amd.com>" Beyond that, the patch looks good, I've only coding style issues. > Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > 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,
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 <ray.huang@amd.com> > > 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@amd.com>" 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 <Jiqian.Chen@amd.com> >> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >> --- >> 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, >
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 ) { + if ( errno == ENOENT ) + sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain, + pci->bus, pci->dev, pci->func); + else { + LOGED(ERROR, domainid, "Can't access %s", sysfs_path); + goto out_no_irq; + } + } + f = fopen(sysfs_path, "r"); if (f == NULL) { LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path); @@ -2229,9 +2240,19 @@ skip_bar: if (!pci_supp_legacy_irq()) goto skip_legacy_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 ) { + if ( errno == ENOENT ) + sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain, + pci->bus, pci->dev, pci->func); + else { + LOGED(ERROR, domid, "Can't access %s", sysfs_path); + goto skip_legacy_irq; + } + } + f = fopen(sysfs_path, "r"); if (f == NULL) { LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);