Re: [XEN PATCH v10 4/5] tools: Add new function to get gsi from dev

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

On 2024/6/17 23:10, Jan Beulich wrote:
> On 17.06.2024 11:00, 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
>> 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.
> 
> Hmm, see my earlier explanations on patch 5: GSI and IRQ generally aren't
> the same anyway. Therefore this part of the description, while not wrong,
> is at least at risk of misleading people.
OK, I wll change to say "irq is not the same as gsi".
> 
>> --- a/tools/include/xen-sys/Linux/privcmd.h
>> +++ b/tools/include/xen-sys/Linux/privcmd.h
>> @@ -95,6 +95,11 @@ typedef struct privcmd_mmap_resource {
>>      __u64 addr;
>>  } privcmd_mmap_resource_t;
>>  
>> +typedef struct privcmd_gsi_from_dev {
>> +    __u32 sbdf;
> 
> That's PCI-centric, without struct and IOCTL names reflecting this fact.
So, change to privcmd_gsi_from_pcidev ?

> 
>> +    int gsi;
> 
> Is "int" legitimate to use here? Doesn't this want to similarly be __u32?
I want to set gsi to negative if there is no record of this translation.

> 
>> --- a/tools/include/xencall.h
>> +++ b/tools/include/xencall.h
>> @@ -113,6 +113,8 @@ int xencall5(xencall_handle *xcall, unsigned int op,
>>               uint64_t arg1, uint64_t arg2, uint64_t arg3,
>>               uint64_t arg4, uint64_t arg5);
>>  
>> +int xen_oscall_gsi_from_dev(xencall_handle *xcall, unsigned int sbdf);
> 
> Hmm, something (by name at least) OS-specific being in the public header
> and ...
> 
>> --- a/tools/libs/call/libxencall.map
>> +++ b/tools/libs/call/libxencall.map
>> @@ -10,6 +10,8 @@ VERS_1.0 {
>>              xencall4;
>>              xencall5;
>>  
>> +            xen_oscall_gsi_from_dev;
> 
> ... map file. I'm not sure things are intended to be this way.
Let's see other maintainer's opinion.

> 
>> --- a/tools/libs/light/libxl_pci.c
>> +++ b/tools/libs/light/libxl_pci.c
>> @@ -1406,6 +1406,12 @@ static bool pci_supp_legacy_irq(void)
>>  #endif
>>  }
>>  
>> +#define PCI_DEVID(bus, devfn)\
>> +            ((((uint16_t)(bus)) << 8) | ((devfn) & 0xff))
>> +
>> +#define PCI_SBDF(seg, bus, devfn) \
>> +            ((((uint32_t)(seg)) << 16) | (PCI_DEVID(bus, devfn)))
> 
> I'm not a maintainer of this file; if I were, I'd ask that for readability's
> sake all excess parentheses be dropped from these.
Isn't it a coding requirement to enclose each element in parentheses in the 
macro definition?
It seems other files also do this. See tools/libs/light/libxl_internal.h

> 
>> @@ -1486,6 +1496,18 @@ static void pci_add_dm_done(libxl__egc *egc,
>>          goto out_no_irq;
>>      }
>>      if ((fscanf(f, "%u", &irq) == 1) && irq) {
>> +#ifdef CONFIG_X86
>> +        sbdf = PCI_SBDF(pci->domain, pci->bus,
>> +                        (PCI_DEVFN(pci->dev, pci->func)));
>> +        gsi = xc_physdev_gsi_from_dev(ctx->xch, sbdf);
>> +        /*
>> +         * Old kernel version may not support this function,
> 
> Just kernel?
Yes, xc_physdev_gsi_from_dev depends on the function implemented on linux 
kernel side.
> 
>> +         * so if fail, keep using irq; if success, use gsi
>> +         */
>> +        if (gsi > 0) {
>> +            irq = gsi;
> 
> I'm still puzzled by this, when by now I think we've sufficiently clarified
> that IRQs and GSIs use two distinct numbering spaces.
> 
> Also, as previously indicated, you call this for PV Dom0 as well. Aiui on
> the assumption that it'll fail. What if we decide to make the functionality
> available there, too (if only for informational purposes, or for
> consistency)? Suddenly you're fallback logic wouldn't work anymore, and
> you'd call ...
> 
>> +        }
>> +#endif
>>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
> 
> ... the function with a GSI when a pIRQ is meant. Imo, as suggested before,
> you strictly want to avoid the call on PV Dom0.
> 
> Also for PVH Dom0: I don't think I've seen changes to the hypercall
> handling, yet. How can that be when GSI and IRQ aren't the same, and hence
> incoming GSI would need translating to IRQ somewhere? I can once again only
> assume all your testing was done with IRQs whose numbers happened to match
> their GSI numbers. (The difference, imo, would also need calling out in the
> public header, where the respective interface struct(s) is/are defined.)
I feel like you missed out on many of the previous discussions.
Without my changes, the original codes use irq (read from file 
/sys/bus/pci/devices/<sbdf>/irq) to do xc_physdev_map_pirq,
but xc_physdev_map_pirq require passing into gsi instead of irq, so we need to 
use gsi whether dom0 is PV or PVH, so for the original codes, they are wrong.
Just because by chance, the irq value in the Linux kernel of pv dom0 is equal 
to the gsi value, so there was no problem with the original pv passthrough.
But not when using PVH, so passthrough failed.
With my changes, I got gsi through function xc_physdev_gsi_from_dev firstly, 
and to be compatible with old kernel versions(if the ioctl
IOCTL_PRIVCMD_GSI_FROM_DEV is not implemented), I still need to use the irq 
number, so I need to check the result
of gsi, if gsi > 0 means IOCTL_PRIVCMD_GSI_FROM_DEV is implemented I can use 
the right one gsi, otherwise keep using wrong one irq. 

And regarding to the implementation of ioctl IOCTL_PRIVCMD_GSI_FROM_DEV, it 
doesn't have any xen heypercall handling changes, all of its processing logic 
is on the kernel side.
I know, so you might want to say, "Then you shouldn't put this in xen's code." 
But this concern was discussed in previous versions, and since the pci 
maintainer disallowed to add
gsi sysfs on linux kernel side, I had to do so.
Roger, Stefano and Juergen may know more about this part.

> 
> Jan

-- 
Best regards,
Jiqian Chen.