RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-31 Thread Duan, Zhenzhong


>-Original Message-
>From: Cédric Le Goater 
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hello Zhenzhong,
>
>On 3/28/24 04:06, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-----
>>> From: Cédric Le Goater 
>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>> HostIOMMUDevice
>>>
>>> Hello Zhenzhong,
>>>
>>> On 3/19/24 12:58, Duan, Zhenzhong wrote:
>>>> Hi Cédric,
>>>>
>>>>> -Original Message-
>>>>> From: Cédric Le Goater 
>>>>> Sent: Tuesday, March 19, 2024 4:17 PM
>>>>> To: Duan, Zhenzhong ; qemu-
>>>>> de...@nongnu.org
>>>>> Cc: alex.william...@redhat.com; eric.au...@redhat.com;
>>>>> pet...@redhat.com; jasow...@redhat.com; m...@redhat.com;
>>>>> j...@nvidia.com; nicol...@nvidia.com; joao.m.mart...@oracle.com;
>Tian,
>>>>> Kevin ; Liu, Yi L ; Sun, Yi Y
>>>>> ; Peng, Chao P 
>>>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>>>> HostIOMMUDevice
>>>>>
>>>>> Hello Zhenzhong,
>>>>>
>>>>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>>>>> HostIOMMUDevice will be inherited by two sub classes,
>>>>>> legacy and iommufd currently.
>>>>>>
>>>>>> Introduce a helper function host_iommu_base_device_init to
>initialize it.
>>>>>>
>>>>>> Suggested-by: Eric Auger 
>>>>>> Signed-off-by: Zhenzhong Duan 
>>>>>> ---
>>>>>> include/sysemu/host_iommu_device.h | 22
>>> ++
>>>>>> 1 file changed, 22 insertions(+)
>>>>>> create mode 100644 include/sysemu/host_iommu_device.h
>>>>>>
>>>>>> diff --git a/include/sysemu/host_iommu_device.h
>>>>> b/include/sysemu/host_iommu_device.h
>>>>>> new file mode 100644
>>>>>> index 00..fe80ab25fb
>>>>>> --- /dev/null
>>>>>> +++ b/include/sysemu/host_iommu_device.h
>>>>>> @@ -0,0 +1,22 @@
>>>>>> +#ifndef HOST_IOMMU_DEVICE_H
>>>>>> +#define HOST_IOMMU_DEVICE_H
>>>>>> +
>>>>>> +typedef enum HostIOMMUDevice_Type {
>>>>>> +HID_LEGACY,
>>>>>> +HID_IOMMUFD,
>>>>>> +HID_MAX,
>>>>>> +} HostIOMMUDevice_Type;
>>>>>> +
>>>>>> +typedef struct HostIOMMUDevice {
>>>>>> +HostIOMMUDevice_Type type;
>>>>>
>>>>> A type field is not a good sign and that's where QOM is useful.
>>>>
>>>> Yes, agree.
>>>> I didn't choose QOM because in iommufd-cdev series, VFIOContainer
>>> chooses not using QOM model.
>>>> See the discussion:
>>> https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
>>>> I thought HostIOMMUDevice need to follow same rule.
>>>>
>>>> But after further digging into this, I think it may be ok to use QOM
>model
>>> as long as we don't expose
>>>> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE
>>> interface. Your thoughts?
>>>
>>> yes. Can we change a bit this series to use QOM ? something like :
>>>
>>>  typedef struct HostIOMMUDevice {
>>>  Object parent;
>>>  } HostIOMMUDevice;
>>>
>>>  #define TYPE_HOST_IOMMU "host.iommu"
>>>  OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass,
>>> HOST_IOMMU)
>>>
>>>  struct HostIOMMUClass {
>>>  ObjectClass parent_class;
>>>
>>>  int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error
>**errp);
>>>  int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error
>**errp);
>>>  };
>>>
>>> Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
>>> TYPE_HOST_IOMMU_LEGACY.
>>> Each class implementing the handlers or not (legacy mode).
>>
>> Understood, thanks for your guide.
>>
>>>
>>> The class handlers are introduced for the intel-iommu helper
>>> vtd_check_hdev()
>>> in order to avoid using iommufd routines directly. HostIOMMUDevice is
>>> supposed
>>> to abstract the Host IOMMU device, so we need to abstract also all the
>>> interfaces to this object.
>>
>> I'd like to have a minimal adjustment to class handers. Just let me know if
>you have strong
>> preference.
>>
>> Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for
>arm smmu usage,
>> and merge get_type and get_cap into one function as they both calls
>ioctl(IOMMU_GET_HW_INFO),
>> something like:
>> get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type,
>void **data, void **len,  Error **errp);
>
>OK. Let's see how it goes. Having more users of this new object Host
>IOMMU device is important to get a better feeling of the interface.
>As of today, it doesn't have not much value. The iommufd object could
>be QOM linked to the vIOMMU when available and we could get the bind
>devid in some other ways I suppose. Anyhow, please keep it simple and
>let's explore.

Got it, thanks Cédric!

BRs.
Zhenzhong


Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-29 Thread Cédric Le Goater

Hello Zhenzhong,

On 3/28/24 04:06, Duan, Zhenzhong wrote:

Hi Cédric,


-Original Message-
From: Cédric Le Goater 
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
HostIOMMUDevice

Hello Zhenzhong,

On 3/19/24 12:58, Duan, Zhenzhong wrote:

Hi Cédric,


-Original Message-
From: Cédric Le Goater 
Sent: Tuesday, March 19, 2024 4:17 PM
To: Duan, Zhenzhong ; qemu-
de...@nongnu.org
Cc: alex.william...@redhat.com; eric.au...@redhat.com;
pet...@redhat.com; jasow...@redhat.com; m...@redhat.com;
j...@nvidia.com; nicol...@nvidia.com; joao.m.mart...@oracle.com; Tian,
Kevin ; Liu, Yi L ; Sun, Yi Y
; Peng, Chao P 
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
HostIOMMUDevice

Hello Zhenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:

HostIOMMUDevice will be inherited by two sub classes,
legacy and iommufd currently.

Introduce a helper function host_iommu_base_device_init to initialize it.

Suggested-by: Eric Auger 
Signed-off-by: Zhenzhong Duan 
---
include/sysemu/host_iommu_device.h | 22

++

1 file changed, 22 insertions(+)
create mode 100644 include/sysemu/host_iommu_device.h

diff --git a/include/sysemu/host_iommu_device.h

b/include/sysemu/host_iommu_device.h

new file mode 100644
index 00..fe80ab25fb
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,22 @@
+#ifndef HOST_IOMMU_DEVICE_H
+#define HOST_IOMMU_DEVICE_H
+
+typedef enum HostIOMMUDevice_Type {
+HID_LEGACY,
+HID_IOMMUFD,
+HID_MAX,
+} HostIOMMUDevice_Type;
+
+typedef struct HostIOMMUDevice {
+HostIOMMUDevice_Type type;


A type field is not a good sign and that's where QOM is useful.


Yes, agree.
I didn't choose QOM because in iommufd-cdev series, VFIOContainer

chooses not using QOM model.

See the discussion:

https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/

I thought HostIOMMUDevice need to follow same rule.

But after further digging into this, I think it may be ok to use QOM model

as long as we don't expose

HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE

interface. Your thoughts?

yes. Can we change a bit this series to use QOM ? something like :

 typedef struct HostIOMMUDevice {
 Object parent;
 } HostIOMMUDevice;

 #define TYPE_HOST_IOMMU "host.iommu"
 OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass,
HOST_IOMMU)

 struct HostIOMMUClass {
 ObjectClass parent_class;

 int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
 int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
 };

Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
TYPE_HOST_IOMMU_LEGACY.
Each class implementing the handlers or not (legacy mode).


Understood, thanks for your guide.



The class handlers are introduced for the intel-iommu helper
vtd_check_hdev()
in order to avoid using iommufd routines directly. HostIOMMUDevice is
supposed
to abstract the Host IOMMU device, so we need to abstract also all the
interfaces to this object.


I'd like to have a minimal adjustment to class handers. Just let me know if you 
have strong
preference.

Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for 
arm smmu usage,
and merge get_type and get_cap into one function as they both calls 
ioctl(IOMMU_GET_HW_INFO),
something like:
get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type, void **data, 
void **len,  Error **errp);


OK. Let's see how it goes. Having more users of this new object Host
IOMMU device is important to get a better feeling of the interface.
As of today, it doesn't have not much value. The iommufd object could
be QOM linked to the vIOMMU when available and we could get the bind
devid in some other ways I suppose. Anyhow, please keep it simple and
let's explore.

Thanks,

C.





and let iommu emulater to extract content of *data. For intel_iommu, it's:

struct iommu_hw_info_vtd {
 __u32 flags;
 __u32 __reserved;
 __aligned_u64 cap_reg;
 __aligned_u64 ecap_reg;
};



The .host_iommu_device_create() handler could be merged
in .attach_device()
possibly. Anyhow, please use now object_new() and object_unref() instead.
host_iommu_base_device_init() is useless IMHO.


Good idea, will do.







Is vtd_check_hdev() the only use of this field ?


Currently yes. virtio-iommu may have similar usage.


If so, can we simplify with a QOM interface in any way ?


QOM interface is a set of callbacks, guess you mean QOM class,
saying HostIOMMUDevice class, IOMMULegacyDevice class and

IOMMUFDDevice class?

See above proposal. it should work fine.

Also, I think it is better to use a IOMMUFDBackend* parameter for
iommufd_device_get_info() to be consistent with the other routines.


Sure, then I'd like to also rename it to iommufd_backend_get_device_info().

Thanks
Zhenzhong



Then It would interesting to see how this applies to Eric's series.

Thanks,

C.









RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-27 Thread Duan, Zhenzhong
Hi Cédric,

>-Original Message-
>From: Cédric Le Goater 
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hello Zhenzhong,
>
>On 3/19/24 12:58, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater 
>>> Sent: Tuesday, March 19, 2024 4:17 PM
>>> To: Duan, Zhenzhong ; qemu-
>>> de...@nongnu.org
>>> Cc: alex.william...@redhat.com; eric.au...@redhat.com;
>>> pet...@redhat.com; jasow...@redhat.com; m...@redhat.com;
>>> j...@nvidia.com; nicol...@nvidia.com; joao.m.mart...@oracle.com; Tian,
>>> Kevin ; Liu, Yi L ; Sun, Yi Y
>>> ; Peng, Chao P 
>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>> HostIOMMUDevice
>>>
>>> Hello Zhenzhong,
>>>
>>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>>> HostIOMMUDevice will be inherited by two sub classes,
>>>> legacy and iommufd currently.
>>>>
>>>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>>>
>>>> Suggested-by: Eric Auger 
>>>> Signed-off-by: Zhenzhong Duan 
>>>> ---
>>>>include/sysemu/host_iommu_device.h | 22
>++
>>>>1 file changed, 22 insertions(+)
>>>>create mode 100644 include/sysemu/host_iommu_device.h
>>>>
>>>> diff --git a/include/sysemu/host_iommu_device.h
>>> b/include/sysemu/host_iommu_device.h
>>>> new file mode 100644
>>>> index 00..fe80ab25fb
>>>> --- /dev/null
>>>> +++ b/include/sysemu/host_iommu_device.h
>>>> @@ -0,0 +1,22 @@
>>>> +#ifndef HOST_IOMMU_DEVICE_H
>>>> +#define HOST_IOMMU_DEVICE_H
>>>> +
>>>> +typedef enum HostIOMMUDevice_Type {
>>>> +HID_LEGACY,
>>>> +HID_IOMMUFD,
>>>> +HID_MAX,
>>>> +} HostIOMMUDevice_Type;
>>>> +
>>>> +typedef struct HostIOMMUDevice {
>>>> +HostIOMMUDevice_Type type;
>>>
>>> A type field is not a good sign and that's where QOM is useful.
>>
>> Yes, agree.
>> I didn't choose QOM because in iommufd-cdev series, VFIOContainer
>chooses not using QOM model.
>> See the discussion:
>https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
>> I thought HostIOMMUDevice need to follow same rule.
>>
>> But after further digging into this, I think it may be ok to use QOM model
>as long as we don't expose
>> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE
>interface. Your thoughts?
>
>yes. Can we change a bit this series to use QOM ? something like :
>
> typedef struct HostIOMMUDevice {
> Object parent;
> } HostIOMMUDevice;
>
> #define TYPE_HOST_IOMMU "host.iommu"
> OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass,
>HOST_IOMMU)
>
> struct HostIOMMUClass {
> ObjectClass parent_class;
>
> int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
> int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
> };
>
>Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
>TYPE_HOST_IOMMU_LEGACY.
>Each class implementing the handlers or not (legacy mode).

Understood, thanks for your guide.

>
>The class handlers are introduced for the intel-iommu helper
>vtd_check_hdev()
>in order to avoid using iommufd routines directly. HostIOMMUDevice is
>supposed
>to abstract the Host IOMMU device, so we need to abstract also all the
>interfaces to this object.

I'd like to have a minimal adjustment to class handers. Just let me know if you 
have strong
preference.

Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for 
arm smmu usage,
and merge get_type and get_cap into one function as they both calls 
ioctl(IOMMU_GET_HW_INFO),
something like:
get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type, void **data, 
void **len,  Error **errp);

and let iommu emulater to extract content of *data. For intel_iommu, it's:

struct iommu_hw_info_vtd {
__u32 flags;
__u32 __reserved;
__aligned_u64 cap_reg;
__aligned_u64 ecap_reg;
};

>
>The .host_iommu_device_create() handler could be merged
>in .attach_device()
>possibly. Anyhow, please use now object_new() and object_unref() instead.
>host_iommu_base_device_init() is useless IMHO.

Good idea, will do.

>
>>
>>>
>>> Is vtd_check_hdev() the only use of this field ?
>>
>> Currently yes. virtio-iommu may have similar usage.
>>
>>> If so, can we simplify with a QOM interface in any way ?
>>
>> QOM interface is a set of callbacks, guess you mean QOM class,
>> saying HostIOMMUDevice class, IOMMULegacyDevice class and
>IOMMUFDDevice class?
>
>See above proposal. it should work fine.
>
>Also, I think it is better to use a IOMMUFDBackend* parameter for
>iommufd_device_get_info() to be consistent with the other routines.

Sure, then I'd like to also rename it to iommufd_backend_get_device_info().

Thanks
Zhenzhong

>
>Then It would interesting to see how this applies to Eric's series.
>
>Thanks,
>
>C.
>
>



Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-27 Thread Cédric Le Goater

Hello Zhenzhong,

On 3/19/24 12:58, Duan, Zhenzhong wrote:

Hi Cédric,


-Original Message-
From: Cédric Le Goater 
Sent: Tuesday, March 19, 2024 4:17 PM
To: Duan, Zhenzhong ; qemu-
de...@nongnu.org
Cc: alex.william...@redhat.com; eric.au...@redhat.com;
pet...@redhat.com; jasow...@redhat.com; m...@redhat.com;
j...@nvidia.com; nicol...@nvidia.com; joao.m.mart...@oracle.com; Tian,
Kevin ; Liu, Yi L ; Sun, Yi Y
; Peng, Chao P 
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
HostIOMMUDevice

Hello Zhenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:

HostIOMMUDevice will be inherited by two sub classes,
legacy and iommufd currently.

Introduce a helper function host_iommu_base_device_init to initialize it.

Suggested-by: Eric Auger 
Signed-off-by: Zhenzhong Duan 
---
   include/sysemu/host_iommu_device.h | 22 ++
   1 file changed, 22 insertions(+)
   create mode 100644 include/sysemu/host_iommu_device.h

diff --git a/include/sysemu/host_iommu_device.h

b/include/sysemu/host_iommu_device.h

new file mode 100644
index 00..fe80ab25fb
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,22 @@
+#ifndef HOST_IOMMU_DEVICE_H
+#define HOST_IOMMU_DEVICE_H
+
+typedef enum HostIOMMUDevice_Type {
+HID_LEGACY,
+HID_IOMMUFD,
+HID_MAX,
+} HostIOMMUDevice_Type;
+
+typedef struct HostIOMMUDevice {
+HostIOMMUDevice_Type type;


A type field is not a good sign and that's where QOM is useful.


Yes, agree.
I didn't choose QOM because in iommufd-cdev series, VFIOContainer chooses not 
using QOM model.
See the discussion: https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
I thought HostIOMMUDevice need to follow same rule.

But after further digging into this, I think it may be ok to use QOM model as 
long as we don't expose
HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE interface. Your 
thoughts?


yes. Can we change a bit this series to use QOM ? something like :

typedef struct HostIOMMUDevice {
Object parent;
} HostIOMMUDevice;

#define TYPE_HOST_IOMMU "host.iommu"

OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass, HOST_IOMMU)

struct HostIOMMUClass {

ObjectClass parent_class;

int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);

int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
};

Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and TYPE_HOST_IOMMU_LEGACY.
Each class implementing the handlers or not (legacy mode).

The class handlers are introduced for the intel-iommu helper vtd_check_hdev()
in order to avoid using iommufd routines directly. HostIOMMUDevice is supposed
to abstract the Host IOMMU device, so we need to abstract also all the
interfaces to this object.

The .host_iommu_device_create() handler could be merged in .attach_device()
possibly. Anyhow, please use now object_new() and object_unref() instead.
host_iommu_base_device_init() is useless IMHO.





Is vtd_check_hdev() the only use of this field ?


Currently yes. virtio-iommu may have similar usage.


If so, can we simplify with a QOM interface in any way ?


QOM interface is a set of callbacks, guess you mean QOM class,
saying HostIOMMUDevice class, IOMMULegacyDevice class and IOMMUFDDevice class?


See above proposal. it should work fine.

Also, I think it is better to use a IOMMUFDBackend* parameter for
iommufd_device_get_info() to be consistent with the other routines.

Then It would interesting to see how this applies to Eric's series.

Thanks,

C.






RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-19 Thread Duan, Zhenzhong
Hi Cédric,

>-Original Message-
>From: Cédric Le Goater 
>Sent: Tuesday, March 19, 2024 4:17 PM
>To: Duan, Zhenzhong ; qemu-
>de...@nongnu.org
>Cc: alex.william...@redhat.com; eric.au...@redhat.com;
>pet...@redhat.com; jasow...@redhat.com; m...@redhat.com;
>j...@nvidia.com; nicol...@nvidia.com; joao.m.mart...@oracle.com; Tian,
>Kevin ; Liu, Yi L ; Sun, Yi Y
>; Peng, Chao P 
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hello Zhenzhong,
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> HostIOMMUDevice will be inherited by two sub classes,
>> legacy and iommufd currently.
>>
>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>
>> Suggested-by: Eric Auger 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>   include/sysemu/host_iommu_device.h | 22 ++
>>   1 file changed, 22 insertions(+)
>>   create mode 100644 include/sysemu/host_iommu_device.h
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> new file mode 100644
>> index 00..fe80ab25fb
>> --- /dev/null
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -0,0 +1,22 @@
>> +#ifndef HOST_IOMMU_DEVICE_H
>> +#define HOST_IOMMU_DEVICE_H
>> +
>> +typedef enum HostIOMMUDevice_Type {
>> +HID_LEGACY,
>> +HID_IOMMUFD,
>> +HID_MAX,
>> +} HostIOMMUDevice_Type;
>> +
>> +typedef struct HostIOMMUDevice {
>> +HostIOMMUDevice_Type type;
>
>A type field is not a good sign and that's where QOM is useful.

Yes, agree.
I didn't choose QOM because in iommufd-cdev series, VFIOContainer chooses not 
using QOM model.
See the discussion: https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
I thought HostIOMMUDevice need to follow same rule.

But after further digging into this, I think it may be ok to use QOM model as 
long as we don't expose
HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE interface. Your 
thoughts?

>
>Is vtd_check_hdev() the only use of this field ?

Currently yes. virtio-iommu may have similar usage.

> If so, can we simplify with a QOM interface in any way ?

QOM interface is a set of callbacks, guess you mean QOM class,
saying HostIOMMUDevice class, IOMMULegacyDevice class and IOMMUFDDevice class?

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>
>
>
>> +size_t size;
>> +} HostIOMMUDevice;
>> +
>> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
>> +   HostIOMMUDevice_Type type,
>> +   size_t size)
>> +{
>> +dev->type = type;
>> +dev->size = size;
>> +}
>> +#endif



Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-19 Thread Cédric Le Goater

Hello Zhenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:

HostIOMMUDevice will be inherited by two sub classes,
legacy and iommufd currently.

Introduce a helper function host_iommu_base_device_init to initialize it.

Suggested-by: Eric Auger 
Signed-off-by: Zhenzhong Duan 
---
  include/sysemu/host_iommu_device.h | 22 ++
  1 file changed, 22 insertions(+)
  create mode 100644 include/sysemu/host_iommu_device.h

diff --git a/include/sysemu/host_iommu_device.h 
b/include/sysemu/host_iommu_device.h
new file mode 100644
index 00..fe80ab25fb
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,22 @@
+#ifndef HOST_IOMMU_DEVICE_H
+#define HOST_IOMMU_DEVICE_H
+
+typedef enum HostIOMMUDevice_Type {
+HID_LEGACY,
+HID_IOMMUFD,
+HID_MAX,
+} HostIOMMUDevice_Type;
+
+typedef struct HostIOMMUDevice {
+HostIOMMUDevice_Type type;


A type field is not a good sign and that's where QOM is useful.

Is vtd_check_hdev() the only use of this field ? If so, can we simplify
with a QOM interface in any way ?

Thanks,

C.

 




+size_t size;
+} HostIOMMUDevice;
+
+static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
+   HostIOMMUDevice_Type type,
+   size_t size)
+{
+dev->type = type;
+dev->size = size;
+}
+#endif





RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-18 Thread Duan, Zhenzhong


>-Original Message-
>From: Eric Auger 
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hi Zhenzhong,
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> HostIOMMUDevice will be inherited by two sub classes,
>> legacy and iommufd currently.
>As this patch introduces the object, you describe what the object is
>meant for and used for. Maybe reuse text from the cover letter

Sure, will do.

Thanks
Zhenzhong

>
>Thanks
>
>Eric
>>
>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>
>> Suggested-by: Eric Auger 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>  include/sysemu/host_iommu_device.h | 22 ++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 include/sysemu/host_iommu_device.h
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> new file mode 100644
>> index 00..fe80ab25fb
>> --- /dev/null
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -0,0 +1,22 @@
>> +#ifndef HOST_IOMMU_DEVICE_H
>> +#define HOST_IOMMU_DEVICE_H
>> +
>> +typedef enum HostIOMMUDevice_Type {
>> +HID_LEGACY,
>> +HID_IOMMUFD,
>> +HID_MAX,
>> +} HostIOMMUDevice_Type;
>> +
>> +typedef struct HostIOMMUDevice {
>> +HostIOMMUDevice_Type type;
>> +size_t size;
>> +} HostIOMMUDevice;
>> +
>> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
>> +   HostIOMMUDevice_Type type,
>> +   size_t size)
>> +{
>> +dev->type = type;
>> +dev->size = size;
>> +}
>> +#endif



Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

2024-03-18 Thread Eric Auger
Hi Zhenzhong,
On 2/28/24 04:58, Zhenzhong Duan wrote:
> HostIOMMUDevice will be inherited by two sub classes,
> legacy and iommufd currently.
As this patch introduces the object, you describe what the object is
meant for and used for. Maybe reuse text from the cover letter

Thanks

Eric
>
> Introduce a helper function host_iommu_base_device_init to initialize it.
>
> Suggested-by: Eric Auger 
> Signed-off-by: Zhenzhong Duan 
> ---
>  include/sysemu/host_iommu_device.h | 22 ++
>  1 file changed, 22 insertions(+)
>  create mode 100644 include/sysemu/host_iommu_device.h
>
> diff --git a/include/sysemu/host_iommu_device.h 
> b/include/sysemu/host_iommu_device.h
> new file mode 100644
> index 00..fe80ab25fb
> --- /dev/null
> +++ b/include/sysemu/host_iommu_device.h
> @@ -0,0 +1,22 @@
> +#ifndef HOST_IOMMU_DEVICE_H
> +#define HOST_IOMMU_DEVICE_H
> +
> +typedef enum HostIOMMUDevice_Type {
> +HID_LEGACY,
> +HID_IOMMUFD,
> +HID_MAX,
> +} HostIOMMUDevice_Type;
> +
> +typedef struct HostIOMMUDevice {
> +HostIOMMUDevice_Type type;
> +size_t size;
> +} HostIOMMUDevice;
> +
> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
> +   HostIOMMUDevice_Type type,
> +   size_t size)
> +{
> +dev->type = type;
> +dev->size = size;
> +}
> +#endif