| Message ID | 957361f0e497e0b95f10a3caf617fe04d9c6fb72.1667542066.git.john.g.johnson@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-user client | expand |
On Tue, Nov 08, 2022 at 03:13:28PM -0800, John Johnson wrote: > New class for vfio-user with its class and instance > constructors and destructors, and its pci ops. LGTM Reviewed-by: John Levon <john.levon@nutanix.com> regards john
On 11/9/22 00:13, John Johnson wrote: > New class for vfio-user with its class and instance > constructors and destructors, and its pci ops. > > Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com> > Signed-off-by: John G Johnson <john.g.johnson@oracle.com> > Signed-off-by: Jagannathan Raman <jag.raman@oracle.com> > --- > hw/vfio/Kconfig | 10 +++++++ > hw/vfio/common.c | 5 ++++ > hw/vfio/pci.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/vfio/pci.h | 8 +++++ > 4 files changed, 112 insertions(+) > > diff --git a/hw/vfio/Kconfig b/hw/vfio/Kconfig > index 7cdba05..301894e 100644 > --- a/hw/vfio/Kconfig > +++ b/hw/vfio/Kconfig > @@ -2,6 +2,10 @@ config VFIO > bool > depends on LINUX > > +config VFIO_USER > + bool > + depends on VFIO > + > config VFIO_PCI > bool > default y > @@ -9,6 +13,12 @@ config VFIO_PCI > select EDID > depends on LINUX && PCI > > +config VFIO_USER_PCI > + bool > + default y > + select VFIO_USER > + depends on VFIO_PCI > + > config VFIO_CCW > bool > default y > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index c7bf0aa..c589bd9 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -1774,6 +1774,11 @@ void vfio_reset_handler(void *opaque) > QLIST_FOREACH(group, &vfio_group_list, next) { > QLIST_FOREACH(vbasedev, &group->device_list, next) { > if (vbasedev->dev->realized && vbasedev->needs_reset) { > + if (vbasedev->ops->vfio_hot_reset_multi == NULL) { > + error_printf("%s: No hot reset handler specified\n", > + vbasedev->name); > + continue; > + } Since needs_reset is false, which is the case for VFIO User, vfio_hot_reset_multi won't be called. I don't think we care much about adding this error message. In another patch, may be. > vbasedev->ops->vfio_hot_reset_multi(vbasedev); > } > } > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 80b03a2..dc19869 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -19,6 +19,7 @@ > */ > > #include "qemu/osdep.h" > +#include CONFIG_DEVICES > #include <linux/vfio.h> > #include <sys/ioctl.h> > > @@ -3421,3 +3422,91 @@ static void register_vfio_pci_dev_type(void) > } > > type_init(register_vfio_pci_dev_type) > + > + > +#ifdef CONFIG_VFIO_USER_PCI Why not introduce a new file hw/vfio/user.c file ? It would be cleaner. > + > +/* > + * vfio-user routines. > + */ > + > +/* > + * Emulated devices don't use host hot reset > + */ > +static void vfio_user_compute_needs_reset(VFIODevice *vbasedev) > +{ > + vbasedev->needs_reset = false; > +} > + > +static VFIODeviceOps vfio_user_pci_ops = { > + .vfio_compute_needs_reset = vfio_user_compute_needs_reset, > + .vfio_eoi = vfio_intx_eoi, > + .vfio_get_object = vfio_pci_get_object, > + .vfio_save_config = vfio_pci_save_config, > + .vfio_load_config = vfio_pci_load_config, > +}; > + > +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp) > +{ > + ERRP_GUARD(); > + VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); > + VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev); > + VFIODevice *vbasedev = &vdev->vbasedev; > + > + /* > + * TODO: make option parser understand SocketAddress > + * and use that instead of having scalar options > + * for each socket type. > + */ > + if (!udev->sock_name) { > + error_setg(errp, "No socket specified"); > + error_append_hint(errp, "Use -device vfio-user-pci,socket=<name>\n"); > + return; > + } > + > + vbasedev->name = g_strdup_printf("VFIO user <%s>", udev->sock_name); > + vbasedev->ops = &vfio_user_pci_ops; > + vbasedev->type = VFIO_DEVICE_TYPE_PCI; > + vbasedev->dev = DEVICE(vdev); > + > +} > + > +static void vfio_user_instance_finalize(Object *obj) > +{ > + VFIOPCIDevice *vdev = VFIO_PCI_BASE(obj); > + > + vfio_put_device(vdev); > +} > + > +static Property vfio_user_pci_dev_properties[] = { > + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), This looks like a good candidate for using a chardev. It could only support AF_UNIX to start with if fd passing is the required feature. But at least, the model would be using a well known backend. I think vhost has the same kind of constraints. Thanks, C. > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void vfio_user_pci_dev_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass); > + > + device_class_set_props(dc, vfio_user_pci_dev_properties); > + dc->desc = "VFIO over socket PCI device assignment"; > + pdc->realize = vfio_user_pci_realize; > +} > + > +static const TypeInfo vfio_user_pci_dev_info = { > + .name = TYPE_VFIO_USER_PCI, > + .parent = TYPE_VFIO_PCI_BASE, > + .instance_size = sizeof(VFIOUserPCIDevice), > + .class_init = vfio_user_pci_dev_class_init, > + .instance_init = vfio_instance_init, > + .instance_finalize = vfio_user_instance_finalize, > +}; > + > +static void register_vfio_user_dev_type(void) > +{ > + type_register_static(&vfio_user_pci_dev_info); > +} > + > +type_init(register_vfio_user_dev_type) > + > +#endif /* VFIO_USER_PCI */ > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h > index 7c5c8ec..27db931 100644 > --- a/hw/vfio/pci.h > +++ b/hw/vfio/pci.h > @@ -189,6 +189,14 @@ struct VFIOKernPCIDevice { > VFIOPCIDevice device; > }; > > +#define TYPE_VFIO_USER_PCI "vfio-user-pci" > +OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserPCIDevice, VFIO_USER_PCI) > + > +struct VFIOUserPCIDevice { > + VFIOPCIDevice device; > + char *sock_name; > +}; > + > /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw */ > static inline bool vfio_pci_is(VFIOPCIDevice *vdev, uint32_t vendor, uint32_t device) > {
On Mon, Dec 12, 2022 at 10:01:33AM +0100, Cédric Le Goater wrote: > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index 80b03a2..dc19869 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -19,6 +19,7 @@ > > */ > > #include "qemu/osdep.h" > > +#include CONFIG_DEVICES > > #include <linux/vfio.h> > > #include <sys/ioctl.h> > > @@ -3421,3 +3422,91 @@ static void register_vfio_pci_dev_type(void) > > } > > type_init(register_vfio_pci_dev_type) > > + > > + > > +#ifdef CONFIG_VFIO_USER_PCI > > Why not introduce a new file hw/vfio/user.c file ? It would be > cleaner. user.c is in this series, and holds the vfio-user implementation - it's not a PCI specific thing. So it would have to be hw/vfio/user_pci.c or something regards john
On 12/12/22 12:03, John Levon wrote: > On Mon, Dec 12, 2022 at 10:01:33AM +0100, Cédric Le Goater wrote: > >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index 80b03a2..dc19869 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -19,6 +19,7 @@ >>> */ >>> #include "qemu/osdep.h" >>> +#include CONFIG_DEVICES >>> #include <linux/vfio.h> >>> #include <sys/ioctl.h> >>> @@ -3421,3 +3422,91 @@ static void register_vfio_pci_dev_type(void) >>> } >>> type_init(register_vfio_pci_dev_type) >>> + >>> + >>> +#ifdef CONFIG_VFIO_USER_PCI >> >> Why not introduce a new file hw/vfio/user.c file ? It would be >> cleaner. > > user.c is in this series, and holds the vfio-user implementation - it's not a > PCI specific thing. So it would have to be hw/vfio/user_pci.c or something Or hw/vfio/pci-user.c
> On Dec 12, 2022, at 3:46 AM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 12/12/22 12:03, John Levon wrote: >> On Mon, Dec 12, 2022 at 10:01:33AM +0100, Cédric Le Goater wrote: >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index 80b03a2..dc19869 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -19,6 +19,7 @@ >>>> */ >>>> #include "qemu/osdep.h" >>>> +#include CONFIG_DEVICES >>>> #include <linux/vfio.h> >>>> #include <sys/ioctl.h> >>>> @@ -3421,3 +3422,91 @@ static void register_vfio_pci_dev_type(void) >>>> } >>>> type_init(register_vfio_pci_dev_type) >>>> + >>>> + >>>> +#ifdef CONFIG_VFIO_USER_PCI >>> >>> Why not introduce a new file hw/vfio/user.c file ? It would be >>> cleaner. >> user.c is in this series, and holds the vfio-user implementation - it's not a >> PCI specific thing. So it would have to be hw/vfio/user_pci.c or something > > Or hw/vfio/pci-user.c The vfio_user_* routines use a lot of hw/vfio/pci.c routines, so we’d need to make a lot of those routines non-static if we made a new file. JJ
> On Dec 12, 2022, at 1:01 AM, Cédric Le Goater <clg@redhat.com> wrote: > > On 11/9/22 00:13, John Johnson wrote: >> >> + >> +static Property vfio_user_pci_dev_properties[] = { >> + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), > > This looks like a good candidate for using a chardev. It could only > support AF_UNIX to start with if fd passing is the required feature. > But at least, the model would be using a well known backend. I think > vhost has the same kind of constraints. > It should be a SocketAddress, but there is no command line parser for that. The next best option was to name the struct members individually, so a string for addr.u.q_unix.path JJ
diff --git a/hw/vfio/Kconfig b/hw/vfio/Kconfig index 7cdba05..301894e 100644 --- a/hw/vfio/Kconfig +++ b/hw/vfio/Kconfig @@ -2,6 +2,10 @@ config VFIO bool depends on LINUX +config VFIO_USER + bool + depends on VFIO + config VFIO_PCI bool default y @@ -9,6 +13,12 @@ config VFIO_PCI select EDID depends on LINUX && PCI +config VFIO_USER_PCI + bool + default y + select VFIO_USER + depends on VFIO_PCI + config VFIO_CCW bool default y diff --git a/hw/vfio/common.c b/hw/vfio/common.c index c7bf0aa..c589bd9 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -1774,6 +1774,11 @@ void vfio_reset_handler(void *opaque) QLIST_FOREACH(group, &vfio_group_list, next) { QLIST_FOREACH(vbasedev, &group->device_list, next) { if (vbasedev->dev->realized && vbasedev->needs_reset) { + if (vbasedev->ops->vfio_hot_reset_multi == NULL) { + error_printf("%s: No hot reset handler specified\n", + vbasedev->name); + continue; + } vbasedev->ops->vfio_hot_reset_multi(vbasedev); } } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 80b03a2..dc19869 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -19,6 +19,7 @@ */ #include "qemu/osdep.h" +#include CONFIG_DEVICES #include <linux/vfio.h> #include <sys/ioctl.h> @@ -3421,3 +3422,91 @@ static void register_vfio_pci_dev_type(void) } type_init(register_vfio_pci_dev_type) + + +#ifdef CONFIG_VFIO_USER_PCI + +/* + * vfio-user routines. + */ + +/* + * Emulated devices don't use host hot reset + */ +static void vfio_user_compute_needs_reset(VFIODevice *vbasedev) +{ + vbasedev->needs_reset = false; +} + +static VFIODeviceOps vfio_user_pci_ops = { + .vfio_compute_needs_reset = vfio_user_compute_needs_reset, + .vfio_eoi = vfio_intx_eoi, + .vfio_get_object = vfio_pci_get_object, + .vfio_save_config = vfio_pci_save_config, + .vfio_load_config = vfio_pci_load_config, +}; + +static void vfio_user_pci_realize(PCIDevice *pdev, Error **errp) +{ + ERRP_GUARD(); + VFIOUserPCIDevice *udev = VFIO_USER_PCI(pdev); + VFIOPCIDevice *vdev = VFIO_PCI_BASE(pdev); + VFIODevice *vbasedev = &vdev->vbasedev; + + /* + * TODO: make option parser understand SocketAddress + * and use that instead of having scalar options + * for each socket type. + */ + if (!udev->sock_name) { + error_setg(errp, "No socket specified"); + error_append_hint(errp, "Use -device vfio-user-pci,socket=<name>\n"); + return; + } + + vbasedev->name = g_strdup_printf("VFIO user <%s>", udev->sock_name); + vbasedev->ops = &vfio_user_pci_ops; + vbasedev->type = VFIO_DEVICE_TYPE_PCI; + vbasedev->dev = DEVICE(vdev); + +} + +static void vfio_user_instance_finalize(Object *obj) +{ + VFIOPCIDevice *vdev = VFIO_PCI_BASE(obj); + + vfio_put_device(vdev); +} + +static Property vfio_user_pci_dev_properties[] = { + DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name), + DEFINE_PROP_END_OF_LIST(), +}; + +static void vfio_user_pci_dev_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + PCIDeviceClass *pdc = PCI_DEVICE_CLASS(klass); + + device_class_set_props(dc, vfio_user_pci_dev_properties); + dc->desc = "VFIO over socket PCI device assignment"; + pdc->realize = vfio_user_pci_realize; +} + +static const TypeInfo vfio_user_pci_dev_info = { + .name = TYPE_VFIO_USER_PCI, + .parent = TYPE_VFIO_PCI_BASE, + .instance_size = sizeof(VFIOUserPCIDevice), + .class_init = vfio_user_pci_dev_class_init, + .instance_init = vfio_instance_init, + .instance_finalize = vfio_user_instance_finalize, +}; + +static void register_vfio_user_dev_type(void) +{ + type_register_static(&vfio_user_pci_dev_info); +} + +type_init(register_vfio_user_dev_type) + +#endif /* VFIO_USER_PCI */ diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index 7c5c8ec..27db931 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -189,6 +189,14 @@ struct VFIOKernPCIDevice { VFIOPCIDevice device; }; +#define TYPE_VFIO_USER_PCI "vfio-user-pci" +OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserPCIDevice, VFIO_USER_PCI) + +struct VFIOUserPCIDevice { + VFIOPCIDevice device; + char *sock_name; +}; + /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw */ static inline bool vfio_pci_is(VFIOPCIDevice *vdev, uint32_t vendor, uint32_t device) {