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 >*/
+ HostIOMMUDevice parent;
+ VFIODevice *vdev;
+};
+
typedef struct VFIODMABuf {
QemuDmaBuf buf;
uint32_t pos_x, pos_y, pos_updates;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276e..44018ef085 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1143,12 +1143,21 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
};
+static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
+{
+};
+
static const TypeInfo types[] = {
{
.name = TYPE_VFIO_IOMMU_LEGACY,
.parent = TYPE_VFIO_IOMMU,
.class_init = vfio_iommu_legacy_class_init,
- },
+ }, {
+ .name = TYPE_HIOD_LEGACY_VFIO,
+ .parent = TYPE_HOST_IOMMU_DEVICE,
+ .instance_size = sizeof(HIODLegacyVFIO),
+ .class_init = hiod_legacy_vfio_class_init,
+ }
};
DEFINE_TYPES(types)
--
2.34.1
On 4/8/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"
I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
> +
> +/* Abstraction of VFIO legacy host IOMMU device */
> +struct HIODLegacyVFIO {
same here
> + /*< private >*/
> + HostIOMMUDevice parent;
> + VFIODevice *vdev;
It seems to me that the back pointer should be on the container instead.
Looks more correct conceptually.
> +};
> +
> typedef struct VFIODMABuf {
> QemuDmaBuf buf;
> uint32_t pos_x, pos_y, pos_updates;
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276e..44018ef085 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1143,12 +1143,21 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
> };
>
> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
> +{
> +};
Is it preferable to introduce routines when they are actually useful.
Please drop the .class_init definition.
Thanks,
C.
> +
> static const TypeInfo types[] = {
> {
> .name = TYPE_VFIO_IOMMU_LEGACY,
> .parent = TYPE_VFIO_IOMMU,
> .class_init = vfio_iommu_legacy_class_init,
> - },
> + }, {
> + .name = TYPE_HIOD_LEGACY_VFIO,
> + .parent = TYPE_HOST_IOMMU_DEVICE,
> + .instance_size = sizeof(HIODLegacyVFIO),
> + .class_init = hiod_legacy_vfio_class_init,
> + }
> };
>
> DEFINE_TYPES(types)
Hi Cédric,
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>On 4/8/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"
>
>I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
Will do.
>
>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>> +
>> +/* Abstraction of VFIO legacy host IOMMU device */
>> +struct HIODLegacyVFIO {
>
>same here
Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass sub-structures?
The reason I used 'HIOD' abbreviation is some function names become extremely long
and exceed 80 characters. E.g.:
@@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
};
-static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
- void *data, uint32_t len,
- Error **errp)
+static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
+ void *data, uint32_t len,
+ Error **errp)
{
VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
/* iova_ranges is a sorted list */
@@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
{
HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
- hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
+ hioc->get_host_iommu_info = host_iommu_device_legacy_vfio_get_host_iommu_info;
};
I didn't find other way to make it meet the 80 chars limitation. Any suggestions on this?
>
>> + /*< private >*/
>> + HostIOMMUDevice parent;
>> + VFIODevice *vdev;
>
>It seems to me that the back pointer should be on the container instead.
>Looks more correct conceptually.
Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer.
>
>
>> +};
>> +
>> typedef struct VFIODMABuf {
>> QemuDmaBuf buf;
>> uint32_t pos_x, pos_y, pos_updates;
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 77bdec276e..44018ef085 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1143,12 +1143,21 @@ static void
>vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>> };
>>
>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>> +{
>> +};
>
>Is it preferable to introduce routines when they are actually useful.
>Please drop the .class_init definition.
Sure.
Thanks
Zhenzhong
>
>Thanks,
>
>C.
>
>
>> +
>> static const TypeInfo types[] = {
>> {
>> .name = TYPE_VFIO_IOMMU_LEGACY,
>> .parent = TYPE_VFIO_IOMMU,
>> .class_init = vfio_iommu_legacy_class_init,
>> - },
>> + }, {
>> + .name = TYPE_HIOD_LEGACY_VFIO,
>> + .parent = TYPE_HOST_IOMMU_DEVICE,
>> + .instance_size = sizeof(HIODLegacyVFIO),
>> + .class_init = hiod_legacy_vfio_class_init,
>> + }
>> };
>>
>> DEFINE_TYPES(types)
Hello,
On 4/16/24 05:41, Duan, Zhenzhong wrote:
> Hi Cédric,
>
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>
>> On 4/8/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"
>>
>> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
>
> Will do.
>
>>
>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>> +
>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>> +struct HIODLegacyVFIO {
>>
>> same here
>
> Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass sub-structures?
I would for type names. The main reason is for naming consistency, which is
useful for grep and code analysis.
>
> The reason I used 'HIOD' abbreviation is some function names become extremely long
> and exceed 80 characters. E.g.:
>
> @@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
> };
>
> -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
> - void *data, uint32_t len,
> - Error **errp)
> +static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
> + void *data, uint32_t len,
> + Error **errp)
> {
> VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
> /* iova_ranges is a sorted list */
> @@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
> {
> HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>
> - hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
> + hioc->get_host_iommu_info = host_iommu_device_legacy_vfio_get_host_iommu_info;
> };
>
> I didn't find other way to make it meet the 80 chars limitation. Any suggestions on this?
Try :
@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init(
{
HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
- hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
+ hioc->get_host_iommu_info =
+ host_iommu_device_legacy_vfio_get_host_iommu_info;
};
static const TypeInfo types[] = {
That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix
could be shortened to 'hiod_legacy_vfio'.
Thanks,
C.
>
>>
>>> + /*< private >*/
>>> + HostIOMMUDevice parent;
>>> + VFIODevice *vdev;
>>
>> It seems to me that the back pointer should be on the container instead.
>> Looks more correct conceptually.
>
> Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer.
>
>>
>>
>>> +};
>>> +
>>> typedef struct VFIODMABuf {
>>> QemuDmaBuf buf;
>>> uint32_t pos_x, pos_y, pos_updates;
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index 77bdec276e..44018ef085 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -1143,12 +1143,21 @@ static void
>> vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>> };
>>>
>>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +};
>>
>> Is it preferable to introduce routines when they are actually useful.
>> Please drop the .class_init definition.
>
> Sure.
>
> Thanks
> Zhenzhong
>
>>
>> Thanks,
>>
>> C.
>>
>>
>>> +
>>> static const TypeInfo types[] = {
>>> {
>>> .name = TYPE_VFIO_IOMMU_LEGACY,
>>> .parent = TYPE_VFIO_IOMMU,
>>> .class_init = vfio_iommu_legacy_class_init,
>>> - },
>>> + }, {
>>> + .name = TYPE_HIOD_LEGACY_VFIO,
>>> + .parent = TYPE_HOST_IOMMU_DEVICE,
>>> + .instance_size = sizeof(HIODLegacyVFIO),
>>> + .class_init = hiod_legacy_vfio_class_init,
>>> + }
>>> };
>>>
>>> DEFINE_TYPES(types)
>
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>Hello,
>
>On 4/16/24 05:41, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>>
>>> On 4/8/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"
>>>
>>> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
>>
>> Will do.
>>
>>>
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>>> +
>>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>>> +struct HIODLegacyVFIO {
>>>
>>> same here
>>
>> Should I do the same for all the HostIOMMUDevice and
>HostIOMMUDeviceClass sub-structures?
>
>I would for type names. The main reason is for naming consistency, which is
>useful for grep and code analysis.
Got it.
>
>>
>> The reason I used 'HIOD' abbreviation is some function names become
>extremely long
>> and exceed 80 characters. E.g.:
>>
>> @@ -1148,9 +1148,9 @@ static void
>vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>> vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>> };
>>
>> -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> - void *data, uint32_t len,
>> - Error **errp)
>> +static int
>host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> + void *data, uint32_t len,
>> + Error **errp)
>> {
>> VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
>> /* iova_ranges is a sorted list */
>> @@ -1173,7 +1173,7 @@ static void
>hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>> {
>> HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>>
>> - hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>> + hioc->get_host_iommu_info =
>host_iommu_device_legacy_vfio_get_host_iommu_info;
>> };
>>
>> I didn't find other way to make it meet the 80 chars limitation. Any
>suggestions on this?
>
>Try :
>
>@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init(
> {
> HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>
>- hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>+ hioc->get_host_iommu_info =
>+ host_iommu_device_legacy_vfio_get_host_iommu_info;
> };
>
> static const TypeInfo types[] = {
>
>That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix
>could be shortened to 'hiod_legacy_vfio'.
Got it.
Thanks
Zhenzhong
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.
> + HostIOMMUDevice parent;
Please name 'parent_obj'.
> + VFIODevice *vdev;
> +};
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?
I see some QOM use that comment to mark private vs. public, for example:
struct AccelState {
/*< private >*/
Object parent_obj;
};
typedef struct AccelClass {
/*< private >*/
ObjectClass parent_class;
/*< public >*/
>
>> + HostIOMMUDevice parent;
>
>Please name 'parent_obj'.
Will do.
Thanks
Zhenzhong
>
>> + VFIODevice *vdev;
>> +};
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;
>>> +};
>
>-----Original Message-----
>From: Philippe Mathieu-Daudé <philmd@linaro.org>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>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
Learned, thanks Philippe.
BRs.
Zhenzhong
© 2016 - 2026 Red Hat, Inc.