Re: [RFC XEN PATCH v3 3/3] libxl: Use gsi instead of irq for mapping pirq

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

On 2023/12/11 23:48, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:40:09AM +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>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> ---
>>  tools/libs/light/libxl_pci.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da079..9e75f0c263 100644
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1416,7 +1416,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      char *sysfs_path;
>>      FILE *f;
>>      unsigned long long start, end, flags, size;
>> -    int irq, i;
>> +    int gsi, i;
>>      int r;
>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>      uint32_t domainid = domid;
>> @@ -1439,7 +1439,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>>                             pci->bus, pci->dev, pci->func);
>>      f = fopen(sysfs_path, "r");
>>      start = end = flags = size = 0;
>> -    irq = 0;
>> +    gsi = 0;
> 
> unsigned int (so it matches the fscanf format), and initialized at
> definition.
As what you said below, I need to use irq if there is no gsi sysfs. So, I think 
it is not necessary to change the name of this local variable.

> 
>>  
>>      if (f == NULL) {
>>          LOGED(ERROR, domainid, "Couldn't open %s", sysfs_path);
>> @@ -1478,26 +1478,26 @@ 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);
> 
> You need to keep the fallback mechanism of reading the irq node, or
> else xl would stop working on any kernel that doesn't expose this
> sysfs node, you would break passthrough on all current Linux versions.
Yes, you are right. If there is no gsi sysfs, will still use irq, in next 
version. Thanks.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.