Re: [PATCH v1 06/24] vfio-user: Define type vfio_user_pci_dev_info

["Cédric Le Goater" <clg@redhat.com>]

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