Re: [RFC XEN PATCH v5 4/5] libxl: Use gsi instead of irq for mapping pirq

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

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