1. Patchew
  2. QEMU
  3. [PULL 0/3] VFIO updates 2023-05-09
  4. View patch

[PULL 1/3] vfio/pci: add support for VF token

Alex Williamson posted 3 patches 2 years, 9 months ago
Maintainers: Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>
[PULL 1/3] vfio/pci: add support for VF token
Posted by Alex Williamson 2 years, 9 months ago 
From: Minwoo Im <minwoo.im@samsung.com>

VF token was introduced [1] to kernel vfio-pci along with SR-IOV
support [2].  This patch adds support VF token among PF and VF(s). To
passthu PCIe VF to a VM, kernel >= v5.7 needs this.

It can be configured with UUID like:

  -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,...

[1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/
[2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/

Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci.c | 13 ++++++++++++-
 hw/vfio/pci.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index ec9a854361ac..cf27f28936cb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     int groupid;
     int i, ret;
     bool is_mdev;
+    char uuid[UUID_FMT_LEN];
+    char *name;
 
     if (!vbasedev->sysfsdev) {
         if (!(~vdev->host.domain || ~vdev->host.bus ||
@@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto error;
     }
 
-    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
+    if (!qemu_uuid_is_null(&vdev->vf_token)) {
+        qemu_uuid_unparse(&vdev->vf_token, uuid);
+        name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
+    } else {
+        name = vbasedev->name;
+    }
+
+    ret = vfio_get_device(group, name, vbasedev, errp);
+    g_free(name);
     if (ret) {
         vfio_put_group(group);
         goto error;
@@ -3268,6 +3278,7 @@ static void vfio_instance_init(Object *obj)
 
 static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_PCI_HOST_DEVADDR("host", VFIOPCIDevice, host),
+    DEFINE_PROP_UUID_NODEFAULT("vf-token", VFIOPCIDevice, vf_token),
     DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
     DEFINE_PROP_ON_OFF_AUTO("x-pre-copy-dirty-page-tracking", VFIOPCIDevice,
                             vbasedev.pre_copy_dirty_page_tracking,
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 177abcc8fb67..2674476d6c77 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -137,6 +137,7 @@ struct VFIOPCIDevice {
     VFIOVGA *vga; /* 0xa0000, 0x3b0, 0x3c0 */
     void *igd_opregion;
     PCIHostDeviceAddress host;
+    QemuUUID vf_token;
     EventNotifier err_notifier;
     EventNotifier req_notifier;
     int (*resetfn)(struct VFIOPCIDevice *);
-- 
2.39.2
Re: [PULL 1/3] vfio/pci: add support for VF token
Posted by Peter Maydell 2 years, 3 months ago 
On Tue, 9 May 2023 at 23:01, Alex Williamson <alex.williamson@redhat.com> wrote:
>
> From: Minwoo Im <minwoo.im@samsung.com>
>
> VF token was introduced [1] to kernel vfio-pci along with SR-IOV
> support [2].  This patch adds support VF token among PF and VF(s). To
> passthu PCIe VF to a VM, kernel >= v5.7 needs this.
>
> It can be configured with UUID like:
>
>   -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,...
>
> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/
> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Hi; Coverity points out that this change introduces a buffer
overrun (CID 1522913). I dunno why it's taken it so long
to notice...

> ---
>  hw/vfio/pci.c | 13 ++++++++++++-
>  hw/vfio/pci.h |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ec9a854361ac..cf27f28936cb 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      int groupid;
>      int i, ret;
>      bool is_mdev;
> +    char uuid[UUID_FMT_LEN];

We define the array uuid[] as UUID_FMT_LEN bytes long...

> +    char *name;
>
>      if (!vbasedev->sysfsdev) {
>          if (!(~vdev->host.domain || ~vdev->host.bus ||
> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>
> -    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
> +    if (!qemu_uuid_is_null(&vdev->vf_token)) {
> +        qemu_uuid_unparse(&vdev->vf_token, uuid);

...but qemu_uuid_unparse() writes UUID_FMT_LEN + 1 bytes,
including a trailing NUL.

Every other use of UUID_FMT_LEN to declare an array
uses "UUID_FMT_LEN + 1" to avoid this.

(In fact, every use of UUID_FMT_LEN at all uses "+ 1",
which suggests that perhaps defining it differently (and
perhaps with a different name) would reduce the risk of
this particular bug...)

thanks
-- PMM
Re: [PULL 1/3] vfio/pci: add support for VF token
Posted by Cédric Le Goater 2 years, 3 months ago 
On 10/20/23 15:32, Peter Maydell wrote:
> On Tue, 9 May 2023 at 23:01, Alex Williamson <alex.williamson@redhat.com> wrote:
>>
>> From: Minwoo Im <minwoo.im@samsung.com>
>>
>> VF token was introduced [1] to kernel vfio-pci along with SR-IOV
>> support [2].  This patch adds support VF token among PF and VF(s). To
>> passthu PCIe VF to a VM, kernel >= v5.7 needs this.
>>
>> It can be configured with UUID like:
>>
>>    -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,...
>>
>> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/
>> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/
>>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Hi; Coverity points out that this change introduces a buffer
> overrun (CID 1522913). I dunno why it's taken it so long
> to notice...
> 
>> ---
>>   hw/vfio/pci.c | 13 ++++++++++++-
>>   hw/vfio/pci.h |  1 +
>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ec9a854361ac..cf27f28936cb 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>       int groupid;
>>       int i, ret;
>>       bool is_mdev;
>> +    char uuid[UUID_FMT_LEN];
> 
> We define the array uuid[] as UUID_FMT_LEN bytes long...
> 
>> +    char *name;
>>
>>       if (!vbasedev->sysfsdev) {
>>           if (!(~vdev->host.domain || ~vdev->host.bus ||
>> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>           goto error;
>>       }
>>
>> -    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
>> +    if (!qemu_uuid_is_null(&vdev->vf_token)) {
>> +        qemu_uuid_unparse(&vdev->vf_token, uuid);
> 
> ...but qemu_uuid_unparse() writes UUID_FMT_LEN + 1 bytes,
> including a trailing NUL.
> 
> Every other use of UUID_FMT_LEN to declare an array
> uses "UUID_FMT_LEN + 1" to avoid this.

We also have :

     char uuidstr[37];

in vdi_header_print() and other places like test-uuid.


> (In fact, every use of UUID_FMT_LEN at all uses "+ 1",
> which suggests that perhaps defining it differently (and
> perhaps with a different name) would reduce the risk of
> this particular bug...)

libuuid defines :

   #define UUID_STR_LEN	37

QEMU could do the same ?

Thanks,

C.

> 
> thanks
> -- PMM
>
Re: [PULL 1/3] vfio/pci: add support for VF token
Posted by Matthew Rosato 2 years, 8 months ago 
On 5/9/23 5:59 PM, Alex Williamson wrote:
> From: Minwoo Im <minwoo.im@samsung.com>
> 
> VF token was introduced [1] to kernel vfio-pci along with SR-IOV
> support [2].  This patch adds support VF token among PF and VF(s). To
> passthu PCIe VF to a VM, kernel >= v5.7 needs this.
> 
> It can be configured with UUID like:
> 
>   -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,...
> 
> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/
> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Hi Minwoo, Alex,

I'm seeing a regression in vfio-pci on s390 and bisect points to this commit.  I don't believe it's specific to s390 though, but rather when not using this new vf-token, see below...

> ---
>  hw/vfio/pci.c | 13 ++++++++++++-
>  hw/vfio/pci.h |  1 +
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ec9a854361ac..cf27f28936cb 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>      int groupid;
>      int i, ret;
>      bool is_mdev;
> +    char uuid[UUID_FMT_LEN];
> +    char *name;
>  
>      if (!vbasedev->sysfsdev) {
>          if (!(~vdev->host.domain || ~vdev->host.bus ||
> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>  
> -    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
> +    if (!qemu_uuid_is_null(&vdev->vf_token)) {
> +        qemu_uuid_unparse(&vdev->vf_token, uuid);
> +        name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
> +    } else {
> +        name = vbasedev->name;

^ here we copy the pointer when a vf-token was not specified.

> +    }
> +
> +    ret = vfio_get_device(group, name, vbasedev, errp);
> +    g_free(name);

^ and then free it regardless.  But I don't think we meant to free what vbasedev->name points to, this was meant to free a duplicate string.  I'm subsequently seeing qemu crashes later on e.g. during device unplug.

I think doing a strdup in either case would fix the issue OR skipping the g_free when qemu_uuid_is_null(&vdev->vf_token).

FWIW, I tried the following and it resolved the issue for me:

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bf27a39905..73874a94de 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2994,7 +2994,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         qemu_uuid_unparse(&vdev->vf_token, uuid);
         name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
     } else {
-        name = vbasedev->name;
+        name = g_strdup(vbasedev->name);
     }
 
     ret = vfio_get_device(group, name, vbasedev, errp);
Re: [PULL 1/3] vfio/pci: add support for VF token
Posted by Cédric Le Goater 2 years, 8 months ago 
Hello Matthew,

On 5/23/23 18:51, Matthew Rosato wrote:
> On 5/9/23 5:59 PM, Alex Williamson wrote:
>> From: Minwoo Im <minwoo.im@samsung.com>
>>
>> VF token was introduced [1] to kernel vfio-pci along with SR-IOV
>> support [2].  This patch adds support VF token among PF and VF(s). To
>> passthu PCIe VF to a VM, kernel >= v5.7 needs this.
>>
>> It can be configured with UUID like:
>>
>>    -device vfio-pci,host=DDDD:BB:DD:F,vf-token=<uuid>,...
>>
>> [1] https://lore.kernel.org/linux-pci/158396393244.5601.10297430724964025753.stgit@gimli.home/
>> [2] https://lore.kernel.org/linux-pci/158396044753.5601.14804870681174789709.stgit@gimli.home/
>>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Signed-off-by: Minwoo Im <minwoo.im@samsung.com>
>> Reviewed-by: Klaus Jensen <k.jensen@samsung.com>
>> Link: https://lore.kernel.org/r/20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4
>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> 
> Hi Minwoo, Alex,
> 
> I'm seeing a regression in vfio-pci on s390 and bisect points to this commit.  I don't believe it's specific to s390 though, but rather when not using this new vf-token, see below...
> 
>> ---
>>   hw/vfio/pci.c | 13 ++++++++++++-
>>   hw/vfio/pci.h |  1 +
>>   2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index ec9a854361ac..cf27f28936cb 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2856,6 +2856,8 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>       int groupid;
>>       int i, ret;
>>       bool is_mdev;
>> +    char uuid[UUID_FMT_LEN];
>> +    char *name;
>>   
>>       if (!vbasedev->sysfsdev) {
>>           if (!(~vdev->host.domain || ~vdev->host.bus ||
>> @@ -2936,7 +2938,15 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>           goto error;
>>       }
>>   
>> -    ret = vfio_get_device(group, vbasedev->name, vbasedev, errp);
>> +    if (!qemu_uuid_is_null(&vdev->vf_token)) {
>> +        qemu_uuid_unparse(&vdev->vf_token, uuid);
>> +        name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
>> +    } else {
>> +        name = vbasedev->name;
> 
> ^ here we copy the pointer when a vf-token was not specified.
> 
>> +    }
>> +
>> +    ret = vfio_get_device(group, name, vbasedev, errp);
>> +    g_free(name);
> 
> ^ and then free it regardless.  But I don't think we meant to free what vbasedev->name points to, this was meant to free a duplicate string.  I'm subsequently seeing qemu crashes later on e.g. during device unplug.
> 
> I think doing a strdup in either case would fix the issue OR skipping the g_free when qemu_uuid_is_null(&vdev->vf_token).
> 
> FWIW, I tried the following and it resolved the issue for me:

Zhenzhong provided the exact same fix :

   https://lore.kernel.org/qemu-devel/20230517024651.82248-1-zhenzhong.duan@intel.com/

Thanks,

C.

> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index bf27a39905..73874a94de 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2994,7 +2994,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           qemu_uuid_unparse(&vdev->vf_token, uuid);
>           name = g_strdup_printf("%s vf_token=%s", vbasedev->name, uuid);
>       } else {
> -        name = vbasedev->name;
> +        name = g_strdup(vbasedev->name);
>       }
>   
>       ret = vfio_get_device(group, name, vbasedev, errp);
> 
> 
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.