RE: [PATCH rfcv2 04/18] vfio: Add host iommu device instance into VFIODevice

[Duan, Zhenzhong]

Hi Eric,

>-Original Message-
>From: Eric Auger 
>Subject: Re: [PATCH rfcv2 04/18] vfio: Add host iommu device instance into
>VFIODevice
>
>
>
>On 2/1/24 08:28, Zhenzhong Duan wrote:
>> Either IOMMULegacyDevice or IOMMUFDDevice into VFIODevice, neither
>> both.
>>
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>  include/hw/vfio/vfio-common.h | 11 +++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index 8bfb9cbe94..1bbad003ee 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -32,6 +32,7 @@
>>  #include "sysemu/sysemu.h"
>>  #include "hw/vfio/vfio-container-base.h"
>>  #include "sysemu/host_iommu_device.h"
>> +#include "sysemu/iommufd.h"
>>
>>  #define VFIO_MSG_PREFIX "vfio %s: "
>>
>> @@ -132,8 +133,18 @@ typedef struct VFIODevice {
>>  bool dirty_tracking;
>>  int devid;
>>  IOMMUFDBackend *iommufd;
>> +union {
>> +HostIOMMUDevice base_hdev;
>> +IOMMULegacyDevice legacy_dev;
>> +IOMMUFDDevice iommufd_dev;
>I think you should rather have a HostIOMMUDevice handle.
>
>host_iommu_device_init cb would allocate the right type of the derived
>object and you would store the base object pointer here.

Sorry for late response, just back from vacation.

I didn't use a HostIOMMUDevice handle but a statically allocated one
Because in following patch:

'[PATCH rfcv2 05/18] vfio: Remove redundant iommufd and devid elements in 
VFIODevice'

iommufd and devid are removed from VFIODevice. I need a place to store
them early in attachment. The handle is dynamically allocated after
attachment and can't be used for that purpose.

I'll change to use HostIOMMUDevice handle and drop patch5 in rfcv3,
Let me know if you have other comments.

Thanks
Zhenzhong

Re: [PATCH rfcv2 04/18] vfio: Add host iommu device instance into VFIODevice

[Eric Auger]

On 2/1/24 08:28, Zhenzhong Duan wrote:
> Either IOMMULegacyDevice or IOMMUFDDevice into VFIODevice, neither
> both.
>
> Signed-off-by: Zhenzhong Duan 
> ---
>  include/hw/vfio/vfio-common.h | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 8bfb9cbe94..1bbad003ee 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -32,6 +32,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-container-base.h"
>  #include "sysemu/host_iommu_device.h"
> +#include "sysemu/iommufd.h"
>  
>  #define VFIO_MSG_PREFIX "vfio %s: "
>  
> @@ -132,8 +133,18 @@ typedef struct VFIODevice {
>  bool dirty_tracking;
>  int devid;
>  IOMMUFDBackend *iommufd;
> +union {
> +HostIOMMUDevice base_hdev;
> +IOMMULegacyDevice legacy_dev;
> +IOMMUFDDevice iommufd_dev;
I think you should rather have a HostIOMMUDevice handle.

host_iommu_device_init cb would allocate the right type of the derived object 
and you would store the base object pointer here.

Eric
> +};
>  } VFIODevice;
>  
> +QEMU_BUILD_BUG_ON(offsetof(VFIODevice, legacy_dev.base) !=
> +  offsetof(VFIODevice, base_hdev));
> +QEMU_BUILD_BUG_ON(offsetof(VFIODevice, iommufd_dev.base) !=
> +  offsetof(VFIODevice, base_hdev));
> +
>  struct VFIODeviceOps {
>  void (*vfio_compute_needs_reset)(VFIODevice *vdev);
>  int (*vfio_hot_reset_multi)(VFIODevice *vdev);

Re: [PATCH rfcv2 04/18] vfio: Add host iommu device instance into VFIODevice

[Eric Auger]

Hi Zhenzhong,

On 2/1/24 08:28, Zhenzhong Duan wrote:
> Either IOMMULegacyDevice or IOMMUFDDevice into VFIODevice, neither
> both.
>
> Signed-off-by: Zhenzhong Duan 
> ---
>  include/hw/vfio/vfio-common.h | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 8bfb9cbe94..1bbad003ee 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -32,6 +32,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/vfio/vfio-container-base.h"
>  #include "sysemu/host_iommu_device.h"
> +#include "sysemu/iommufd.h"
>  
>  #define VFIO_MSG_PREFIX "vfio %s: "
>  
> @@ -132,8 +133,18 @@ typedef struct VFIODevice {
>  bool dirty_tracking;
>  int devid;
>  IOMMUFDBackend *iommufd;
> +union {
> +HostIOMMUDevice base_hdev;
I don't think we want the base object above to be usable here

Thanks

Eric
> +IOMMULegacyDevice legacy_dev;
> +IOMMUFDDevice iommufd_dev;
> +};
>  } VFIODevice;
>  
> +QEMU_BUILD_BUG_ON(offsetof(VFIODevice, legacy_dev.base) !=
> +  offsetof(VFIODevice, base_hdev));
> +QEMU_BUILD_BUG_ON(offsetof(VFIODevice, iommufd_dev.base) !=
> +  offsetof(VFIODevice, base_hdev));
> +
>  struct VFIODeviceOps {
>  void (*vfio_compute_needs_reset)(VFIODevice *vdev);
>  int (*vfio_hot_reset_multi)(VFIODevice *vdev);