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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

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@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>"
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@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,
> 

-- 
Best regards,
Jiqian Chen.