diff mbox series

[RFC,XEN,v11,7/8] tools: Add new function to get gsi from dev

Message ID 20240630123344.20623-8-Jiqian.Chen@amd.com (mailing list archive)
State Superseded
Headers show
Series Support device passthrough when dom0 is PVH on Xen | expand

Commit Message

Chen, Jiqian June 30, 2024, 12:33 p.m. UTC 
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@amd.com>
Signed-off-by: Huang Rui <ray.huang@amd.com>
Signed-off-by: Chen Jiqian <Jiqian.Chen@amd.com>
---
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@amd.com/
This patch must be merged after the patch on linux kernel side
---
 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(+)

Comments

Jan Beulich July 1, 2024, 7:32 a.m. UTC | #1 
On 30.06.2024 14:33, Jiqian Chen wrote:
> --- 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, &dev_gsi);
> +    close(fd);
> +
> +    if (rc) {
> +        PERROR("Failed to get gsi from dev");
> +    } else {
> +        rc = dev_gsi.gsi;
> +    }
> +#endif
> +
> +    return rc;
> +}

I realize Anthony had asked to move this out of libxencall, yet doing it like
this (without really abstracting away the OS specifics) doesn't look quite
right either. In particular the opening of /dev/xen/privcmd looks questionable
to now have yet another instance in yet another library. Couldn't we split
osdep_xencall_open(), making available its former half for use here and in the
other two libraries? Of course that'll still leave the ioctl() invocation,
which necessarily is OS-specific, too.

Jan
Chen, Jiqian July 2, 2024, 3:47 a.m. UTC | #2 
On 2024/7/1 15:32, Jan Beulich wrote:
> On 30.06.2024 14:33, Jiqian Chen wrote:
>> --- 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, &dev_gsi);
>> +    close(fd);
>> +
>> +    if (rc) {
>> +        PERROR("Failed to get gsi from dev");
>> +    } else {
>> +        rc = dev_gsi.gsi;
>> +    }
>> +#endif
>> +
>> +    return rc;
>> +}
> 
> I realize Anthony had asked to move this out of libxencall, yet doing it like
> this (without really abstracting away the OS specifics) doesn't look quite
> right either. In particular the opening of /dev/xen/privcmd looks questionable
> to now have yet another instance in yet another library. Couldn't we split
> osdep_xencall_open(), making available its former half for use here and in the
> other two libraries? 
Hi Anthony, what about your opinion?

> Of course that'll still leave the ioctl() invocation, which necessarily is OS-specific, too.
> 
> Jan
Chen, Jiqian July 4, 2024, 3:03 a.m. UTC | #3 
Hi Anthony,

On 2024/7/2 11:47, Chen, Jiqian wrote:
> On 2024/7/1 15:32, Jan Beulich wrote:
>> On 30.06.2024 14:33, Jiqian Chen wrote:
>>> --- 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, &dev_gsi);
>>> +    close(fd);
>>> +
>>> +    if (rc) {
>>> +        PERROR("Failed to get gsi from dev");
>>> +    } else {
>>> +        rc = dev_gsi.gsi;
>>> +    }
>>> +#endif
>>> +
>>> +    return rc;
>>> +}
>>
>> I realize Anthony had asked to move this out of libxencall, yet doing it like
>> this (without really abstracting away the OS specifics) doesn't look quite
>> right either. In particular the opening of /dev/xen/privcmd looks questionable
>> to now have yet another instance in yet another library. Couldn't we split
>> osdep_xencall_open(), making available its former half for use here and in the
>> other two libraries? 
> Hi Anthony, what about your opinion?
What's your opinion about taking " opening of /dev/xen/privcmd " as a single function, then use it in this patch and other libraries?

> 
>> Of course that'll still leave the ioctl() invocation, which necessarily is OS-specific, too.
>>
>> Jan
>
diff mbox series

Patch

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)
+{
+    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, &dev_gsi);
+    close(fd);
+
+    if (rc) {
+        PERROR("Failed to get gsi from dev");
+    } else {
+        rc = dev_gsi.gsi;
+    }
+#endif
+
+    return rc;
+}