Re: [PATCH v3 06/23] vfio-user: add vfio-user class and container

[John Levon <john.levon@nutanix.com>]

On Tue, Jun 10, 2025 at 12:42:35PM +0100, Mark Cave-Ayland wrote:

> Question: how do you see the division between hw/vfio and hw/vfio-user? My
> initial feeling is that there is substantial sharing between the two, in
> which case I'd expect the files to be in hw/vfio as e.g.
> hw/vfio/container-user.c etc. instead of its own directory.

That was also in the earlier patchsets! Cédric asked for hw/vfio-user - and I
think I actually prefer it myself. The amount we export from hw/vfio is actually
fairly minimal (now).

> > +#include "hw/vfio/vfio-container-base.h"
> > +
> > +/* MMU container sub-class for vfio-user. */
> > +typedef struct VFIOUserContainer {
> > +    VFIOContainerBase bcontainer;
> > +} VFIOUserContainer;
> > +
> > +OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserContainer, VFIO_IOMMU_USER);
> 
> As per the documentation at https://qemu-project.gitlab.io/qemu/devel/style.html#qemu-object-model-declarations
> the parent object should always be named parent_obj and struct shouldn't
> have a typedef i.e.
> 
> /* MMU container sub-class for vfio-user. */
> struct VFIOUserContainer {
>     VFIOContainerBase parent_obj;
> };
> 
> OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserContainer, VFIO_IOMMU_USER);

I don't think I want to diverge from VFIOContainer here, though, right?

> > +static VFIOUserContainer *
> > +vfio_user_container_connect(AddressSpace *as, Error **errp)
> > +{
> > +    VFIOContainerBase *bcontainer;
> > +    VFIOUserContainer *container;
> > +    VFIOAddressSpace *space;
> > +    VFIOIOMMUClass *vioc;
> > +
> > +    space = vfio_address_space_get(as);
> > +
> > +    container = vfio_user_create_container(errp);
> > +    if (!container) {
> > +        goto put_space_exit;
> > +    }
> > +
> > +    bcontainer = &container->bcontainer;
> 
> References to the object hierarchy should always be done with the
> automatically generated QOM cast macros since they are easier to read, and
> also ensure type safety e.g.:
> 
>        bcontainer = VFIO_IOMMU(container);

Ditto. Sounds like a fine future cleanup that should be applied to all the code,
but for now, and for review reasons, I'd prefer to be "the same" as hw/vfio/

> > +struct VFIOUserPCIDevice {
> > +    VFIOPCIDevice device;
> > +    char *sock_name;
> > +};
> 
> Again as per the documentation link above, device should be called
> parent_obj plus there should be a empty line between parent_obj and the
> other members i.e.
> 
> struct VFIOUserPCIDevice {
>     VFIOPCIDevice parent_obj;
> 
>     char *sock_name;
> }
> 
> Note that by using QOM casts the name of the parent object member is not
> exposed to the remainder of the code.

I can make this change, though, as there's no vfio equivalent, if Cédric thinks
I should too.

> > +static void register_vfio_user_dev_type(void)
> > +{
> > +    type_register_static(&vfio_user_pci_dev_info);
> > +}
> > +
> > + type_init(register_vfio_user_dev_type)
> 
> Use DEFINE_TYPES as you've already done above instead of type_init() here.

Again, same as hw/vfio/pci.c

regards
john