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

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

On 2024/3/29 01:32, Anthony PERARD wrote:
> On Thu, Mar 28, 2024 at 02:34:01PM +0800, Jiqian Chen wrote:
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da0794e..2cec83e0b734 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1478,8 +1478,14 @@ 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);
>> +    r = access(sysfs_path, F_OK);
>> +    if (r && errno == ENOENT) {
>> +        /* To compitable with old version of kernel, still need to use irq 
>> */
> 
> There's a typo, this would be "To be compatible ...". Also maybe
> something like "Fallback to "/irq" for compatibility with old version of
> the kernel." might sound better.
> 
>> +        sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> +                               pci->bus, pci->dev, pci->func);
>> +    }
>>      f = fopen(sysfs_path, "r");
>>      if (f == NULL) {
>>          LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>> @@ -2229,9 +2235,15 @@ 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);
>>  
>> +    rc = access(sysfs_path, F_OK);
> 
> Please, don't use the variable `rc` here, this one is reserved for libxl
> error/return code in libxl. Introduce `int r` instead.
> 
>> +    if (rc && errno == ENOENT) {
>> +        /* To compitable with old version of kernel, still need to use irq 
>> */
>> +        sysfs_path = GCSPRINTF(SYSFS_PCI_DEV"/"PCI_BDF"/irq", pci->domain,
>> +                               pci->bus, pci->dev, pci->func);
>> +    }
>>      f = fopen(sysfs_path, "r");
>>      if (f == NULL) {
>>          LOGED(ERROR, domid, "Couldn't open %s", sysfs_path);
> 
> With those two things fixed: Reviewed-by: Anthony PERARD 
> <anthony.perard@xxxxxxxxxx>
Thank you very much!
I will fix those two in next version.

> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.