Re: [RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev
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//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
>> Signed-off-by: Huang Rui
>> Signed-off-by: Chen Jiqian
>> ---
>> 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.c...@amd.com/
>> This patch must be merged after the patch on linux kernel side
>>
>> CC: Anthony PERARD
>> 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: _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.
Re: [RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev
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//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
> Signed-off-by: Huang Rui
> Signed-off-by: Chen Jiqian
> ---
> 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.c...@amd.com/
> This patch must be merged after the patch on linux kernel side
>
> CC: Anthony PERARD
> 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: _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, Roger.
Re: [RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev
On Tue, Jul 09, 2024 at 03:35:57AM +, Chen, Jiqian wrote: > On 2024/7/8 21:27, Anthony PERARD wrote: > > You could reuse the already opened fd from libxencall: > > xencall_fd(xch->xcall) > Do I need to check it this fd<0? No, it should be good to use. Cheers, -- Anthony Perard | Vates XCP-ng Developer XCP-ng & Xen Orchestra - Vates solutions web: https://vates.tech
Re: [RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev
On 2024/7/8 21:27, Anthony PERARD wrote:
> On Mon, Jul 08, 2024 at 07:41:23PM +0800, Jiqian Chen wrote:
>> 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_interface *xch, uint32_t sbdf)
>> +{
>> +int rc = -1;
>> +
>> +#if defined(__linux__)
>> +int fd;
>> +privcmd_gsi_from_pcidev_t dev_gsi = {
>> +.sbdf = sbdf,
>> +.gsi = 0,
>> +};
>> +
>> +fd = open("/dev/xen/privcmd", O_RDWR);
>
>
> You could reuse the already opened fd from libxencall:
> xencall_fd(xch->xcall)
Do I need to check it this fd<0?
>
>> +
>> +if (fd < 0 && (errno == ENOENT || errno == ENXIO || errno == ENODEV)) {
>> +/* Fallback to /proc/xen/privcmd */
>> +fd = open("/proc/xen/privcmd", O_RDWR);
>> +}
>> +
>> +if (fd < 0) {
>> +PERROR("Could not obtain handle on privileged command interface");
>> +return rc;
>> +}
>> +
>> +rc = ioctl(fd, IOCTL_PRIVCMD_GSI_FROM_PCIDEV, _gsi);
>
> I think this would be better implemented in Linux only C file instead of
> using #define. There's already "xc_linux.c" which is probably good
> enough to be used here.
>
> Implementation for other OS would just set errno to ENOSYS and
> return -1.
Thanks, will change in next version.
>
>
--
Best regards,
Jiqian Chen.
Re: [RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev
On Mon, Jul 08, 2024 at 07:41:23PM +0800, Jiqian Chen wrote:
> 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)
> +{
> +int rc = -1;
> +
> +#if defined(__linux__)
> +int fd;
> +privcmd_gsi_from_pcidev_t dev_gsi = {
> +.sbdf = sbdf,
> +.gsi = 0,
> +};
> +
> +fd = open("/dev/xen/privcmd", O_RDWR);
You could reuse the already opened fd from libxencall:
xencall_fd(xch->xcall)
> +
> +if (fd < 0 && (errno == ENOENT || errno == ENXIO || errno == ENODEV)) {
> +/* Fallback to /proc/xen/privcmd */
> +fd = open("/proc/xen/privcmd", O_RDWR);
> +}
> +
> +if (fd < 0) {
> +PERROR("Could not obtain handle on privileged command interface");
> +return rc;
> +}
> +
> +rc = ioctl(fd, IOCTL_PRIVCMD_GSI_FROM_PCIDEV, _gsi);
I think this would be better implemented in Linux only C file instead of
using #define. There's already "xc_linux.c" which is probably good
enough to be used here.
Implementation for other OS would just set errno to ENOSYS and
return -1.
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
[RFC XEN PATCH v12 6/7] tools: Add new function to get gsi from dev
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//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
Signed-off-by: Huang Rui
Signed-off-by: Chen Jiqian
---
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.c...@amd.com/
This patch must be merged after the patch on linux kernel side
CC: Anthony PERARD
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: _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)
+{
+int rc = -1;
+
+#if defined(__linux__)
+int fd;
+privcmd_gsi_from_pcidev_t dev_gsi = {
+.sbdf = sbdf,
+.gsi = 0,
+};
+
+fd = open("/dev/xen/privcmd", O_RDWR);
+
+if (fd < 0 && (errno == ENOENT || errno == ENXIO || errno == ENODEV)) {
+/* Fallback to /proc/xen/privcmd */
+fd = open("/proc/xen/privcmd", O_RDWR);
+}
+
+if (fd < 0) {
+PERROR("Could not obtain handle on privileged command interface");
+return rc;
+}
+
+rc = ioctl(fd, IOCTL_PRIVCMD_GSI_FROM_PCIDEV, _gsi);
+close(fd);
+
+if (rc) {
+PERROR("Failed to get gsi from dev");
+} else {
+rc = dev_gsi.gsi;
+}
+#endif
+
+return rc;
+}
--
2.34.1
