RE: [PATCH] vfio/pci: add support for VF token

[Minwoo Im]

> -----Original Message-----
> From: qemu-devel-bounces+minwoo.im=samsung.com@nongnu.org <qemu-devel-
> bounces+minwoo.im=samsung.com@nongnu.org> On Behalf Of Alex Williamson
> Sent: Friday, March 24, 2023 3:46 AM
> To: Minwoo Im <minwoo.im@samsung.com>
> Cc: Cédric Le Goater <clg@kaod.org>; qemu-devel@nongnu.org; SSDR Gost Dev
> <gost.dev@samsung.com>; Klaus Birkelund Jensen <k.jensen@samsung.com>
> Subject: Re: [PATCH] vfio/pci: add support for VF token
> 
> On Thu, 23 Mar 2023 06:19:45 +0900
> Minwoo Im <minwoo.im@samsung.com> wrote:
> 
> > > On Mon, 20 Mar 2023 11:03:40 +0100
> > > Cédric Le Goater <clg@kaod.org> wrote:
> > >
> > > > On 3/20/23 08:35, Minwoo Im wrote:
> > > > > 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>
> > > > > ---
> > > > >   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 ec9a854361..cf27f28936 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;
> > > >
> > > > Shouldn't we set the VF token in the kernel also ? See this QEMU
> implementation
> > > >
> > > >    https://lore.kernel.org/lkml/20200204161737.34696b91@w520.home/
> > > >
> > > > May be I misunderstood.
> > > >
> > >
> > > I think you're referring to the part there that calls
> > > VFIO_DEVICE_FEATURE in order to set a VF token.  I don't think that's
> > > necessarily applicable here.  I believe this patch is only trying to
> > > make it so that QEMU can consume a VF associated with a PF owned by a
> > > userspace vfio driver, ie. not QEMU.
> >
> > Yes, that's what this patch exactly does.
> >
> > >
> > > Setting the VF token is only relevant to PFs, which would require
> > > significantly more SR-IOV infrastructure in QEMU than sketched out in
> > > that proof-of-concept patch.  Even if we did have a QEMU owned PF where
> > > we wanted to generate VFs, the token we use in that case would likely
> > > need to be kept private to QEMU, not passed on the command line.
> > > Thanks,
> >
> > Can we also take a command line property for the PF for that case that
> > QEMU owns a PF?  I think the one who wants to make QEMU owns PF or VF
> > should know the VF token.  If I've missed anything, please let me know.
> 
> IIRC, the only case where a VF token is required for a PF is if there
> are existing VFs in use.  Opening the PF would then require a token
> matching the VFs.  In general though, if the PF is owned by QEMU, the
> VF token should likely be an internal secret to QEMU.  Configuring the
> PF device with a token suggests that VFs could be created and bound to
> other userspace drivers outside of the control of the QEMU instance
> that owns the PF.  Therefore I would not suggest adding the ability to
> set the VF token for a PF without a strong use case in-hand, an
> certainly not when QEMU doesn't expose SR-IOV support to be able to
> manage VFs itself.  Thanks,
> 
> Alex
> 

Thanks for the explanation!