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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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);