Re: [RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev

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

On 2024/8/1 21:01, Roger Pau Monné wrote:
> On Mon, Jul 08, 2024 at 07:41:23PM +0800, Jiqian Chen wrote:
>> When passthrough a device to domU, QEMU and xl tools use its gsi
>> number to do pirq mapping, see QEMU code
>> xen_pt_realize->xc_physdev_map_pirq, and xl code
>> pci_add_dm_done->xc_physdev_map_pirq, but the gsi number is got
>> from file /sys/bus/pci/devices/<sbdf>/irq, that is wrong, because
>> irq is not equal with gsi, they are in different spaces, so pirq
>> mapping fails.
>>
>> And in current codes, there is no method to get gsi for userspace.
>> For above purpose, add new function to get gsi, and the
>> corresponding ioctl is implemented on linux kernel side.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
>> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
>> Signed-off-by: Chen Jiqian <Jiqian.Chen@xxxxxxx>
>> ---
>> RFC: it needs to wait for the corresponding third patch on linux kernel side 
>> to be merged.
>> https://lore.kernel.org/xen-devel/20240607075109.126277-4-Jiqian.Chen@xxxxxxx/
>> This patch must be merged after the patch on linux kernel side
>>
>> CC: Anthony PERARD <anthony@xxxxxxxxxxxxxx>
>> Remaining comment @Anthony PERARD:
>> Do I need to make " opening of /dev/xen/privcmd " as a single function, then 
>> use it in this
>> patch and other libraries?
>> ---
>>  tools/include/xen-sys/Linux/privcmd.h |  7 ++++++
>>  tools/include/xenctrl.h               |  2 ++
>>  tools/libs/ctrl/xc_physdev.c          | 35 +++++++++++++++++++++++++++
>>  3 files changed, 44 insertions(+)
>>
>> diff --git a/tools/include/xen-sys/Linux/privcmd.h 
>> b/tools/include/xen-sys/Linux/privcmd.h
>> index bc60e8fd55eb..4cf719102116 100644
>> --- 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_pcidev {
>> +    __u32 sbdf;
>> +    __u32 gsi;
>> +} privcmd_gsi_from_pcidev_t;
>> +
>>  /*
>>   * @cmd: IOCTL_PRIVCMD_HYPERCALL
>>   * @arg: &privcmd_hypercall_t
>> @@ -114,6 +119,8 @@ typedef struct privcmd_mmap_resource {
>>      _IOC(_IOC_NONE, 'P', 6, sizeof(domid_t))
>>  #define IOCTL_PRIVCMD_MMAP_RESOURCE                         \
>>      _IOC(_IOC_NONE, 'P', 7, sizeof(privcmd_mmap_resource_t))
>> +#define IOCTL_PRIVCMD_GSI_FROM_PCIDEV                               \
>> +    _IOC(_IOC_NONE, 'P', 10, sizeof(privcmd_gsi_from_pcidev_t))
>>  #define IOCTL_PRIVCMD_UNIMPLEMENTED                         \
>>      _IOC(_IOC_NONE, 'P', 0xFF, 0)
>>  
>> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
>> index 9ceca0cffc2f..3720e22b399a 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_pcidev(xc_interface *xch, uint32_t sbdf);
>> +
>>  /*
>>   *  LOGGING AND ERROR REPORTING
>>   */
>> diff --git a/tools/libs/ctrl/xc_physdev.c b/tools/libs/ctrl/xc_physdev.c
>> index e9fcd755fa62..54edb0f3c0dc 100644
>> --- a/tools/libs/ctrl/xc_physdev.c
>> +++ b/tools/libs/ctrl/xc_physdev.c
>> @@ -111,3 +111,38 @@ int xc_physdev_unmap_pirq(xc_interface *xch,
>>      return rc;
>>  }
>>  
>> +int xc_physdev_gsi_from_pcidev(xc_interface *xch, uint32_t sbdf)
> 
> FWIW, I'm not sure it's fine to use the xc_physdev prefix here, as
> this is not a PHYSDEVOP hypercall.
> 
> As Anthony suggested, it would be better placed in xc_linux.c, and
> possibly named xc_pcidev_get_gsi() or similar, to avoid polluting the
> xc_physdev namespace.
Thanks, will change in next version.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.