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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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.

> +     int gsi;

Is "int" legitimate to use here? Doesn't this want to similarly be __u32?

> --- 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.

> --- 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.

> @@ -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?

> +         * 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.)

Jan