Re: [PATCH v3 13/19] vfio: Create host IOMMU device instance

2024-04-30 Thread Cédric Le Goater 

On 4/30/24 12:16, Duan, Zhenzhong wrote:




-Original Message-
From: Cédric Le Goater 
Subject: Re: [PATCH v3 13/19] vfio: Create host IOMMU device instance

On 4/29/24 08:50, Zhenzhong Duan wrote:

Create host IOMMU device instance in vfio_attach_device() and call
.realize() to initialize it further.

Suggested-by: Cédric Le Goater 
Signed-off-by: Zhenzhong Duan 
---
   include/hw/vfio/vfio-common.h |  1 +
   hw/vfio/common.c  | 18 +-
   2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-

common.h

index 0943add3bc..b204b93a55 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -126,6 +126,7 @@ typedef struct VFIODevice {
   OnOffAuto pre_copy_dirty_page_tracking;
   bool dirty_pages_supported;
   bool dirty_tracking;
+HostIOMMUDevice *hiod;
   int devid;
   IOMMUFDBackend *iommufd;
   } VFIODevice;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8f9cbdc026..0be8b70ebd 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice

*vbasedev,

   {
   const VFIOIOMMUClass *ops =


VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));

+HostIOMMUDevice *hiod;
+int ret;

   if (vbasedev->iommufd) {
   ops =

VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
D));

@@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name,

VFIODevice *vbasedev,


   assert(ops);

-return ops->attach_device(name, vbasedev, as, errp);
+ret = ops->attach_device(name, vbasedev, as, errp);
+if (ret < 0) {
+return ret;



hmm, I wonder if we should change the return value of vfio_attach_device()
to be a bool.


I see, also VFIOIOMMUClass:: setup and VFIOIOMMUClass::add_window.
I can add cleanup patches to fix them if you have no other plan.


Yes please. I don't have plans for changes in that area. The only pending
patches are from the series "[v4] migration: Improve error reporting" [*].

Thanks,

C.


[*] https://lore.kernel.org/qemu-devel/20240306133441.2351700-1-...@redhat.com/

RE: [PATCH v3 13/19] vfio: Create host IOMMU device instance

2024-04-30 Thread Duan, Zhenzhong 


>-Original Message-
>From: Cédric Le Goater 
>Subject: Re: [PATCH v3 13/19] vfio: Create host IOMMU device instance
>
>On 4/29/24 08:50, Zhenzhong Duan wrote:
>> Create host IOMMU device instance in vfio_attach_device() and call
>> .realize() to initialize it further.
>>
>> Suggested-by: Cédric Le Goater 
>> Signed-off-by: Zhenzhong Duan 
>> ---
>>   include/hw/vfio/vfio-common.h |  1 +
>>   hw/vfio/common.c  | 18 +-
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index 0943add3bc..b204b93a55 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -126,6 +126,7 @@ typedef struct VFIODevice {
>>   OnOffAuto pre_copy_dirty_page_tracking;
>>   bool dirty_pages_supported;
>>   bool dirty_tracking;
>> +HostIOMMUDevice *hiod;
>>   int devid;
>>   IOMMUFDBackend *iommufd;
>>   } VFIODevice;
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 8f9cbdc026..0be8b70ebd 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice
>*vbasedev,
>>   {
>>   const VFIOIOMMUClass *ops =
>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>> +HostIOMMUDevice *hiod;
>> +int ret;
>>
>>   if (vbasedev->iommufd) {
>>   ops =
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUF
>D));
>> @@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name,
>VFIODevice *vbasedev,
>>
>>   assert(ops);
>>
>> -return ops->attach_device(name, vbasedev, as, errp);
>> +ret = ops->attach_device(name, vbasedev, as, errp);
>> +if (ret < 0) {
>> +return ret;
>
>
>hmm, I wonder if we should change the return value of vfio_attach_device()
>to be a bool.

I see, also VFIOIOMMUClass:: setup and VFIOIOMMUClass::add_window.
I can add cleanup patches to fix them if you have no other plan.

Thanks
Zhenzhong

>
>
>Thanks,
>
>C.
>
>
>
>> +}
>> +
>> +hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> +if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>errp)) {
>> +object_unref(hiod);
>> +ops->detach_device(vbasedev);
>> +return -EINVAL;
>> +}
>> +vbasedev->hiod = hiod;
>> +
>> +return 0;
>>   }
>>
>>   void vfio_detach_device(VFIODevice *vbasedev)
>> @@ -1512,5 +1527,6 @@ void vfio_detach_device(VFIODevice *vbasedev)
>>   if (!vbasedev->bcontainer) {
>>   return;
>>   }
>> +object_unref(vbasedev->hiod);
>>   vbasedev->bcontainer->ops->detach_device(vbasedev);
>>   }

Re: [PATCH v3 13/19] vfio: Create host IOMMU device instance

2024-04-30 Thread Cédric Le Goater 

On 4/29/24 08:50, Zhenzhong Duan wrote:

Create host IOMMU device instance in vfio_attach_device() and call
.realize() to initialize it further.

Suggested-by: Cédric Le Goater 
Signed-off-by: Zhenzhong Duan 
---
  include/hw/vfio/vfio-common.h |  1 +
  hw/vfio/common.c  | 18 +-
  2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 0943add3bc..b204b93a55 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -126,6 +126,7 @@ typedef struct VFIODevice {
  OnOffAuto pre_copy_dirty_page_tracking;
  bool dirty_pages_supported;
  bool dirty_tracking;
+HostIOMMUDevice *hiod;
  int devid;
  IOMMUFDBackend *iommufd;
  } VFIODevice;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 8f9cbdc026..0be8b70ebd 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1497,6 +1497,8 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev,
  {
  const VFIOIOMMUClass *ops =
  VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
+HostIOMMUDevice *hiod;
+int ret;
  
  if (vbasedev->iommufd) {

  ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
@@ -1504,7 +1506,20 @@ int vfio_attach_device(char *name, VFIODevice *vbasedev,
  
  assert(ops);
  
-return ops->attach_device(name, vbasedev, as, errp);

+ret = ops->attach_device(name, vbasedev, as, errp);
+if (ret < 0) {
+return ret;



hmm, I wonder if we should change the return value of vfio_attach_device()
to be a bool.


Thanks,

C.




+}
+
+hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
+if (!HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp)) {
+object_unref(hiod);
+ops->detach_device(vbasedev);
+return -EINVAL;
+}
+vbasedev->hiod = hiod;
+
+return 0;
  }
  
  void vfio_detach_device(VFIODevice *vbasedev)

@@ -1512,5 +1527,6 @@ void vfio_detach_device(VFIODevice *vbasedev)
  if (!vbasedev->bcontainer) {
  return;
  }
+object_unref(vbasedev->hiod);
  vbasedev->bcontainer->ops->detach_device(vbasedev);
  }