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

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

On 2024/6/20 22:38, Anthony PERARD wrote:
> On Mon, Jun 17, 2024 at 05:00:34PM +0800, Jiqian Chen wrote:
>> diff --git a/tools/include/xencall.h b/tools/include/xencall.h
>> index fc95ed0fe58e..750aab070323 100644
>> --- 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);
> 
> I don't think that an appropriate library for this new feature.
> libxencall is a generic lib to make hypercall.
Do you have a suggested place to put this new function?
This new function is to get gsi of a pci device, and only depend on the dom0 
kernel, doesn't need to interact with hypervisor.

> 
>>  /* Variant(s) of the above, as needed, returning "long" instead of "int". */
>>  long xencall2L(xencall_handle *xcall, unsigned int op,
>>                 uint64_t arg1, uint64_t arg2);
>> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
>> index 9ceca0cffc2f..a0381f74d24b 100644
>> --- a/tools/include/xenctrl.h
>> +++ b/tools/include/xenctrl.h
>> @@ -1641,6 +1641,8 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
>>                            uint32_t domid,
>>                            int pirq);
>>  
>> +int xc_physdev_gsi_from_dev(xc_interface *xch, uint32_t sbdf);
>> +
>>  /*
>>   *  LOGGING AND ERROR REPORTING
>>   */
>> diff --git a/tools/libs/call/core.c b/tools/libs/call/core.c
>> index 02c4f8e1aefa..6dae50c9a6ba 100644
>> --- a/tools/libs/call/core.c
>> +++ b/tools/libs/call/core.c
>> @@ -173,6 +173,11 @@ int xencall5(xencall_handle *xcall, unsigned int op,
>>      return osdep_hypercall(xcall, &call);
>>  }
>>  
>> +int xen_oscall_gsi_from_dev(xencall_handle *xcall, unsigned int sbdf)
>> +{
>> +    return osdep_oscall(xcall, sbdf);
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/tools/libs/call/libxencall.map b/tools/libs/call/libxencall.map
>> index d18a3174e9dc..b92a0b5dc12c 100644
>> --- 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;
> 
> FYI, never change already released version of a library, this would add
> a new function to libxencall.1.0. Instead, when adding a new function
> to a library that is supposed to be stable (they have a *.map file in
> xen case), add it to a new section, that woud be VERS_1.4 in this case.
> But libxencall isn't approriate for this new function, so just for
> future reference.
> 
>>              xencall_alloc_buffer;
>>              xencall_free_buffer;
>>              xencall_alloc_buffer_pages;
>> diff --git a/tools/libs/call/linux.c b/tools/libs/call/linux.c
>> index 6d588e6bea8f..92c740e176f2 100644
>> --- a/tools/libs/call/linux.c
>> +++ b/tools/libs/call/linux.c
>> @@ -85,6 +85,21 @@ long osdep_hypercall(xencall_handle *xcall, 
>> privcmd_hypercall_t *hypercall)
>>      return ioctl(xcall->fd, IOCTL_PRIVCMD_HYPERCALL, hypercall);
>>  }
>>  
>> +int osdep_oscall(xencall_handle *xcall, unsigned int sbdf)
>> +{
>> +    privcmd_gsi_from_dev_t dev_gsi = {
>> +        .sbdf = sbdf,
>> +        .gsi = -1,
>> +    };
>> +
>> +    if (ioctl(xcall->fd, IOCTL_PRIVCMD_GSI_FROM_DEV, &dev_gsi)) {
> 
> Looks like libxencall is only for hypercall, and so I don't think
> it's the right place to introducing another ioctl() call.
It seems IOCTL_PRIVCMD_HYPERCALL is for hypercall.
What I do here is to introduce new call into privcmd fd.
Maybe I can open "/dev/xen/privcmd" directly, so that I don't have to add the 
*_oscal function.

> 
>> +        PERROR("failed to get gsi from dev");
>> +        return -1;
>> +    }
>> +
>> +    return dev_gsi.gsi;
>> +}
>> +
>>  static void *alloc_pages_bufdev(xencall_handle *xcall, size_t npages)
>>  {
>>      void *p;
>> diff --git a/tools/libs/call/private.h b/tools/libs/call/private.h
>> index 9c3aa432efe2..cd6eb5a3e66f 100644
>> --- a/tools/libs/call/private.h
>> +++ b/tools/libs/call/private.h
>> @@ -57,6 +57,15 @@ int osdep_xencall_close(xencall_handle *xcall);
>>  
>>  long osdep_hypercall(xencall_handle *xcall, privcmd_hypercall_t *hypercall);
>>  
>> +#if defined(__linux__)
>> +int osdep_oscall(xencall_handle *xcall, unsigned int sbdf);
>> +#else
>> +static inline int osdep_oscall(xencall_handle *xcall, unsigned int sbdf)
>> +{
>> +    return -1;
>> +}
>> +#endif
>> +
>>  void *osdep_alloc_pages(xencall_handle *xcall, size_t nr_pages);
>>  void osdep_free_pages(xencall_handle *xcall, void *p, size_t nr_pages);
>>  
>> diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
>> index 460a8e779ce8..c1458f3a38b5 100644
>> --- a/tools/libs/ctrl/xc_physdev.c
>> +++ b/tools/libs/ctrl/xc_physdev.c
>> @@ -111,3 +111,7 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
>>      return rc;
>>  }
>>  
>> +int xc_physdev_gsi_from_dev(xc_interface *xch, uint32_t sbdf)
>> +{
> 
> I'm not sure if this is the best place for this new function, but I
> can't find another one, so that will do.
Thanks.

> 
>> +    return xen_oscall_gsi_from_dev(xch->xcall, sbdf);
>> +}
>> diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile
>> index 37e4d1670986..6b616d5ee9b6 100644
>> --- a/tools/libs/light/Makefile
>> +++ b/tools/libs/light/Makefile
>> @@ -40,7 +40,7 @@ OBJS-$(CONFIG_X86) += $(ACPI_OBJS)
>>  
>>  CFLAGS += -Wno-format-zero-length -Wmissing-declarations -Wformat-nonliteral
>>  
>> -CFLAGS-$(CONFIG_X86) += -DCONFIG_PCI_SUPP_LEGACY_IRQ
>> +CFLAGS-$(CONFIG_X86) += -DCONFIG_PCI_SUPP_LEGACY_IRQ -DCONFIG_X86
>>  
>>  OBJS-$(CONFIG_X86) += libxl_cpuid.o
>>  OBJS-$(CONFIG_X86) += libxl_x86.o
>> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
>> index 96cb4da0794e..376f91759ac6 100644
>> --- 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)))
>> +
>>  static void pci_add_dm_done(libxl__egc *egc,
>>                              pci_add_state *pas,
>>                              int rc)
>> @@ -1418,6 +1424,10 @@ static void pci_add_dm_done(libxl__egc *egc,
>>      unsigned long long start, end, flags, size;
>>      int irq, i;
>>      int r;
>> +#ifdef CONFIG_X86
>> +    int gsi;
>> +    uint32_t sbdf;
>> +#endif
>>      uint32_t flag = XEN_DOMCTL_DEV_RDM_RELAXED;
>>      uint32_t domainid = domid;
>>      bool isstubdom = libxl_is_stubdom(ctx, domid, &domainid);
>> @@ -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
> 
> Could you avoid these #ifdef, and move the new arch specific code (and
> maybe existing code) into libxl_x86.c ? There's already examples of arch
> specific code.
OK, will do in next version.

> 
>> +        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,
>> +         * so if fail, keep using irq; if success, use gsi
>> +         */
>> +        if (gsi > 0) {
>> +            irq = gsi;
>> +        }
>> +#endif
>>          r = xc_physdev_map_pirq(ctx->xch, domid, irq, &irq);
>>          if (r < 0) {
>>              LOGED(ERROR, domainid, "xc_physdev_map_pirq irq=%d (error=%d)",
>> @@ -2172,6 +2194,10 @@ static void pci_remove_detached(libxl__egc *egc,
>>      int  irq = 0, i, stubdomid = 0;
>>      const char *sysfs_path;
>>      FILE *f;
>> +#ifdef CONFIG_X86
>> +    int gsi;
>> +    uint32_t sbdf;
>> +#endif
>>      uint32_t domainid = prs->domid;
>>      bool isstubdom;
>>  
>> @@ -2239,6 +2265,18 @@ skip_bar:
>>      }
>>  
>>      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,
>> +         * so if fail, keep using irq; if success, use gsi
>> +         */
>> +        if (gsi > 0) {
>> +            irq = gsi;
>> +        }
>> +#endif
>>          rc = xc_physdev_unmap_pirq(ctx->xch, domid, irq);
>>          if (rc < 0) {
>>              /*
> 
> Thanks,
> 

-- 
Best regards,
Jiqian Chen.