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
