Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 15/4/24 12:10, Duan, Zhenzhong wrote:
> Hi Philippe,
> 
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Sent: Monday, April 15, 2024 5:20 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>> devel@nongnu.org
>> Cc: alex.williamson@redhat.com; clg@redhat.com; eric.auger@redhat.com;
>> peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
>> jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian,
>> Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P
>> <chao.p.peng@intel.com>
>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>
>> On 8/4/24 10:12, Zhenzhong Duan wrote:
>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>>> container backend.
>>>
>>> It includes a link to VFIODevice.
>>>
>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    include/hw/vfio/vfio-common.h | 11 +++++++++++
>>>    hw/vfio/container.c           | 11 ++++++++++-
>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>>> index b9da6c08ef..f30772f534 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -31,6 +31,7 @@
>>>    #endif
>>>    #include "sysemu/sysemu.h"
>>>    #include "hw/vfio/vfio-container-base.h"
>>> +#include "sysemu/host_iommu_device.h"
>>>
>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>
>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>>        bool ram_block_discard_allowed;
>>>    } VFIOGroup;
>>>
>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-
>> vfio"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>> +
>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>> +struct HIODLegacyVFIO {
>>> +    /*< private >*/
>>
>> Please drop this comment.
> 
> Will do. But may I ask the rules when to use that comment and when not?

Sure, see 
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations

> I see some QOM use that comment to mark private vs. public, for example:
> 
> struct AccelState {
>      /*< private >*/
>      Object parent_obj;

This is old style which might be cleaned some day...

> };
> 
> typedef struct AccelClass {
>      /*< private >*/
>      ObjectClass parent_class;
>      /*< public >*/
> 
>>
>>> +    HostIOMMUDevice parent;
>>
>> Please name 'parent_obj'.
> 
> Will do.

Thanks,

Phil.

> 
> Thanks
> Zhenzhong
> 
>>
>>> +    VFIODevice *vdev;
>>> +};
>