| Message ID | 20230320073522epcms2p48f682ecdb73e0ae1a4850ad0712fd780@epcms2p4 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: add support for VF token | expand |
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. Thanks, C. > @@ -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 177abcc8fb..2674476d6c 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 *);
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. 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, Alex > > @@ -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 177abcc8fb..2674476d6c 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 *); > >
> 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. Thanks!
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
> -----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!
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; @@ -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 177abcc8fb..2674476d6c 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 *);