RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>On 4/25/24 10:46, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello Zhenzhong,
>>>
>>> On 4/18/24 10:42, Duan, Zhenzhong wrote:
Hi Cédric,
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> Hello Zhenzhong
>
> On 4/17/24 11:24, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> On 4/17/24 06:21, Duan, Zhenzhong wrote:
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> Hello,
>
> On 4/16/24 09:09, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to
>do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device)
>>> should
>>> not
be passed to guest.
Implementation details for different backends will be in
>>> following
> patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
>>> +++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
> VTDHostIOMMUDevice
>>> *vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod),
>>> TYPE_HIOD_IOMMUFD))
> {
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
>>>
>>>
>>> I think we should be using the .get_host_iommu_info() class
>>> handler
>>> instead. Can we refactor the code slightly to avoid this check on
>>> the type ?
>>
>> There is some difficulty ini avoiding this check, the behavior of
> vtd_check_legacy_hdev
>> and vtd_check_iommufd_hdev are different especially after
>>> nesting
> support introduced.
>> vtd_check_iommufd_hdev() has much wider check over
>cap/ecap
>>> bits
> besides aw_bits.
>
> I think it is important to fully separate the vIOMMU model from
>the
> host IOMMU backing device.
>
> This comment is true for the structures also.
>
> Could we introduce a new HostIOMMUDeviceClass
> handler .check_hdev() handler, which would
> call .get_host_iommu_info() ?
>
> This means that HIOD_LEGACY_INFO and
Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
On 4/25/24 10:46, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello Zhenzhong,
On 4/18/24 10:42, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello Zhenzhong
On 4/17/24 11:24, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/17/24 06:21, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello,
On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device)
should
not
be passed to guest.
Implementation details for different backends will be in
following
patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
+++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
VTDHostIOMMUDevice
*vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod),
TYPE_HIOD_IOMMUFD))
{
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class
handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after
nesting
support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap
bits
besides aw_bits.
I think it is important to fully separate the vIOMMU model from the
host IOMMU backing device.
This comment is true for the structures also.
Could we introduce a new HostIOMMUDeviceClass
handler .check_hdev() handler, which would
call .get_host_iommu_info() ?
This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should
be
a common structure 'HostIOMMUDeviceInfo' holding all attributes
for the different backends. Each .get_host_iommu_info() implementation
would translate the specific host iommu device data presentation
into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
I see, it's just not easy to define the unified elements in
HostIOMMUDeviceInfo
so that they maps to bits or fields in host return IOMMU info.
The proposal is adding a vIOMMU <-> HostIOMMUDevice interface and a
new
API needs to be completely defined for it. The IOMMU backend
implementation
could be anything, legacy, iommufd, iommufd v2, some other framework
and
the vIOMMU shouldn't be aware of its implementation.
Exposing the kernel structures as done below should be avoided because
they are part of the QEMU <-> kernel IOMMUFD interface.
Different platform returned host IOMMU info is platform specific.
For vtd and siommu:
struct iommu_hw_info_vtd {
__u32 flags;
__u32 __reserved;
__aligned_u64 cap_reg;
__aligned_u64 ecap_reg;
};
struct iommu_hw_info_arm_smmuv3 {
__u32 flags;
__u32 __reserved;
__u32 idr[6];
__u32 iidr;
__u32 aidr;
};
I can think of two kinds of declaration of HostIOMMUDeviceInfo:
struct HostIOMMUDeviceInfo {
uint8_t aw_bits;
enum iommu_hw_info_type type;
union {
struct iommu_hw_info_vtd vtd;
struct iommu_hw_info_arm_smmuv3;
RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
Hi Cédric,
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>Hello Zhenzhong,
>
>On 4/18/24 10:42, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello Zhenzhong
>>>
>>> On 4/17/24 11:24, Duan, Zhenzhong wrote:
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> On 4/17/24 06:21, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello,
>>>
>>> On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> On 4/8/24 10:44, Zhenzhong Duan wrote:
>> From: Yi Liu
>>
>> If check fails, the host side device(either vfio or vdpa device)
>should
> not
>> be passed to guest.
>>
>> Implementation details for different backends will be in
>following
>>> patches.
>>
>> Signed-off-by: Yi Liu
>> Signed-off-by: Yi Sun
>> Signed-off-by: Zhenzhong Duan
>> ---
>> hw/i386/intel_iommu.c | 35
> +++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4f84e2e801..a49b587c73 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -35,6 +35,7 @@
>> #include "sysemu/kvm.h"
>> #include "sysemu/dma.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/iommufd.h"
>> #include "hw/i386/apic_internal.h"
>> #include "kvm/kvm_i386.h"
>> #include "migration/vmstate.h"
>> @@ -3819,6 +3820,32 @@ VTDAddressSpace
> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>> return vtd_dev_as;
>> }
>>
>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_hdev(IntelIOMMUState *s,
>>> VTDHostIOMMUDevice
> *vtd_hdev,
>> + Error **errp)
>> +{
>> +HostIOMMUDevice *hiod = vtd_hdev->dev;
>> +
>> +if (object_dynamic_cast(OBJECT(hiod),
>TYPE_HIOD_IOMMUFD))
>>> {
>> +return vtd_check_iommufd_hdev(s, hiod, errp);
>> +}
>> +
>> +return vtd_check_legacy_hdev(s, hiod, errp);
>> +}
>
>
> I think we should be using the .get_host_iommu_info() class
>handler
> instead. Can we refactor the code slightly to avoid this check on
> the type ?
There is some difficulty ini avoiding this check, the behavior of
>>> vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after
>nesting
>>> support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap
>bits
>>> besides aw_bits.
>>>
>>> I think it is important to fully separate the vIOMMU model from the
>>> host IOMMU backing device.
>>>
>>> This comment is true for the structures also.
>>>
>>> Could we introduce a new HostIOMMUDeviceClass
>>> handler .check_hdev() handler, which would
>>> call .get_host_iommu_info() ?
>>>
>>> This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should
>be
>>> a common structure 'HostIOMMUDeviceInfo' holding all attributes
>>> for the different backends. Each .get_host_iommu_info() implementation
>>> would translate the specific host iommu device data presentation
>>> into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
>>
>> I see, it's just not easy to define the unified elements in
>HostIOMMUDeviceInfo
>> so that they maps to bits or fields in host return IOMMU info.
>
>The proposal is adding a vIOMMU <->
RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>Hello Zhenzhong,
>
>On 4/18/24 10:42, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello Zhenzhong
>>>
>>> On 4/17/24 11:24, Duan, Zhenzhong wrote:
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> On 4/17/24 06:21, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello,
>>>
>>> On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> On 4/8/24 10:44, Zhenzhong Duan wrote:
>> From: Yi Liu
>>
>> If check fails, the host side device(either vfio or vdpa device)
>should
> not
>> be passed to guest.
>>
>> Implementation details for different backends will be in
>following
>>> patches.
>>
>> Signed-off-by: Yi Liu
>> Signed-off-by: Yi Sun
>> Signed-off-by: Zhenzhong Duan
>> ---
>> hw/i386/intel_iommu.c | 35
> +++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4f84e2e801..a49b587c73 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -35,6 +35,7 @@
>> #include "sysemu/kvm.h"
>> #include "sysemu/dma.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/iommufd.h"
>> #include "hw/i386/apic_internal.h"
>> #include "kvm/kvm_i386.h"
>> #include "migration/vmstate.h"
>> @@ -3819,6 +3820,32 @@ VTDAddressSpace
> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>> return vtd_dev_as;
>> }
>>
>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_hdev(IntelIOMMUState *s,
>>> VTDHostIOMMUDevice
> *vtd_hdev,
>> + Error **errp)
>> +{
>> +HostIOMMUDevice *hiod = vtd_hdev->dev;
>> +
>> +if (object_dynamic_cast(OBJECT(hiod),
>TYPE_HIOD_IOMMUFD))
>>> {
>> +return vtd_check_iommufd_hdev(s, hiod, errp);
>> +}
>> +
>> +return vtd_check_legacy_hdev(s, hiod, errp);
>> +}
>
>
> I think we should be using the .get_host_iommu_info() class
>handler
> instead. Can we refactor the code slightly to avoid this check on
> the type ?
There is some difficulty ini avoiding this check, the behavior of
>>> vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after
>nesting
>>> support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap
>bits
>>> besides aw_bits.
>>>
>>> I think it is important to fully separate the vIOMMU model from the
>>> host IOMMU backing device.
>>>
>>> This comment is true for the structures also.
>>>
>>> Could we introduce a new HostIOMMUDeviceClass
>>> handler .check_hdev() handler, which would
>>> call .get_host_iommu_info() ?
>>>
>>> This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should
>be
>>> a common structure 'HostIOMMUDeviceInfo' holding all attributes
>>> for the different backends. Each .get_host_iommu_info() implementation
>>> would translate the specific host iommu device data presentation
>>> into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
>>
>> I see, it's just not easy to define the unified elements in
>HostIOMMUDeviceInfo
>> so that they maps to bits or fields in host return IOMMU info.
>
>The proposal is adding a vIOMMU <-> HostIOMMUDevice
Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
Hello Zhenzhong,
On 4/18/24 10:42, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello Zhenzhong
On 4/17/24 11:24, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/17/24 06:21, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello,
On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should
not
be passed to guest.
Implementation details for different backends will be in following
patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
+++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
VTDHostIOMMUDevice
*vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD))
{
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting
support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
besides aw_bits.
I think it is important to fully separate the vIOMMU model from the
host IOMMU backing device.
This comment is true for the structures also.
Could we introduce a new HostIOMMUDeviceClass
handler .check_hdev() handler, which would
call .get_host_iommu_info() ?
This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should be
a common structure 'HostIOMMUDeviceInfo' holding all attributes
for the different backends. Each .get_host_iommu_info() implementation
would translate the specific host iommu device data presentation
into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
I see, it's just not easy to define the unified elements in HostIOMMUDeviceInfo
so that they maps to bits or fields in host return IOMMU info.
The proposal is adding a vIOMMU <-> HostIOMMUDevice interface and a new
API needs to be completely defined for it. The IOMMU backend implementation
could be anything, legacy, iommufd, iommufd v2, some other framework and
the vIOMMU shouldn't be aware of its implementation.
Exposing the kernel structures as done below should be avoided because
they are part of the QEMU <-> kernel IOMMUFD interface.
Different platform returned host IOMMU info is platform specific.
For vtd and siommu:
struct iommu_hw_info_vtd {
__u32 flags;
__u32 __reserved;
__aligned_u64 cap_reg;
__aligned_u64 ecap_reg;
};
struct iommu_hw_info_arm_smmuv3 {
__u32 flags;
__u32 __reserved;
__u32 idr[6];
__u32 iidr;
__u32 aidr;
};
I can think of two kinds of declaration of HostIOMMUDeviceInfo:
struct HostIOMMUDeviceInfo {
uint8_t aw_bits;
enum iommu_hw_info_type type;
union {
struct iommu_hw_info_vtd vtd;
struct iommu_hw_info_arm_smmuv3;
..
} data;
}
or
struct HostIOMMUDeviceInfo {
uint8_t aw_bits;
enum iommu_hw_info_type type;
__u32 flags;
__aligned_u64 cap_reg;
__aligned_u64 ecap_reg;
__u32 idr[6];
__u32 iidr;
__u32 aidr;
RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
Hi Cédric,
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>Hello Zhenzhong
>
>On 4/17/24 11:24, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> On 4/17/24 06:21, Duan, Zhenzhong wrote:
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> Hello,
>
> On 4/16/24 09:09, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should
>>> not
be passed to guest.
Implementation details for different backends will be in following
> patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
>>> +++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
> VTDHostIOMMUDevice
>>> *vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD))
>{
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
>>>
>>>
>>> I think we should be using the .get_host_iommu_info() class handler
>>> instead. Can we refactor the code slightly to avoid this check on
>>> the type ?
>>
>> There is some difficulty ini avoiding this check, the behavior of
> vtd_check_legacy_hdev
>> and vtd_check_iommufd_hdev are different especially after nesting
> support introduced.
>> vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
> besides aw_bits.
>
> I think it is important to fully separate the vIOMMU model from the
> host IOMMU backing device.
>
>This comment is true for the structures also.
>
> Could we introduce a new HostIOMMUDeviceClass
> handler .check_hdev() handler, which would
>call .get_host_iommu_info() ?
>
>This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should be
>a common structure 'HostIOMMUDeviceInfo' holding all attributes
>for the different backends. Each .get_host_iommu_info() implementation
>would translate the specific host iommu device data presentation
>into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
I see, it's just not easy to define the unified elements in HostIOMMUDeviceInfo
so that they maps to bits or fields in host return IOMMU info.
Different platform returned host IOMMU info is platform specific.
For vtd and siommu:
struct iommu_hw_info_vtd {
__u32 flags;
__u32 __reserved;
__aligned_u64 cap_reg;
__aligned_u64 ecap_reg;
};
struct iommu_hw_info_arm_smmuv3 {
__u32 flags;
__u32 __reserved;
__u32 idr[6];
__u32 iidr;
__u32 aidr;
};
I can think of two kinds of declaration of HostIOMMUDeviceInfo:
struct HostIOMMUDeviceInfo {
uint8_t aw_bits;
enum iommu_hw_info_type type;
union {
struct iommu_hw_info_vtd vtd;
Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
Hello Zhenzhong
On 4/17/24 11:24, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/17/24 06:21, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello,
On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should
not
be passed to guest.
Implementation details for different backends will be in following
patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
+++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
VTDHostIOMMUDevice
*vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting
support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
besides aw_bits.
I think it is important to fully separate the vIOMMU model from the
host IOMMU backing device.
This comment is true for the structures also.
Could we introduce a new HostIOMMUDeviceClass
handler .check_hdev() handler, which would call .get_host_iommu_info() ?
This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO should be
a common structure 'HostIOMMUDeviceInfo' holding all attributes
for the different backends. Each .get_host_iommu_info() implementation
would translate the specific host iommu device data presentation
into the common 'HostIOMMUDeviceInfo', this is true for host_aw_bits.
'type' could be handled the same way, with a 'HostIOMMUDeviceInfo'
type attribute and host iommu device type definitions, or as you
suggested with a QOM interface. This is more complex however. In
this case, I would suggest to implement a .compatible() handler to
compare the host iommu device type with the vIOMMU type.
The resulting check_hdev routine would look something like :
static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev,
Error **errp)
{
HostIOMMUDevice *hiod = vtd_hdev->dev;
HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
HostIOMMUDevice info;
int host_aw_bits, ret;
ret = hiodc->get_host_iommu_info(hiod, , sizeof(info), errp);
if (ret) {
return ret;
}
ret = hiodc->is_compatible(hiod, VIOMMU_INTERFACE(s));
if (ret) {
return ret;
}
if (s->aw_bits > info.aw_bits) {
error_setg(errp, "aw-bits %d > host aw-bits %d",
s->aw_bits, info.aw_bits);
return -EINVAL;
}
}
and the HostIOMMUDeviceClass::is_compatible() handler would call a
vIOMMUInterface::compatible() handler simply returning
IOMMU_HW_INFO_TYPE_INTEL_VTD. How does that sound ?
Including the type in HostIOMMUDeviceInfo is much simpler to start with.
Thanks,
C.
Understood, besides the new .check_hdev() handler, I think we also need a
new interface
class TYPE_IOMMU_CHECK_HDEV which has two handlers
check_[legacy|iommufd]_hdev(),
and different vIOMMUs have different implementation.
I am not sure to understand. Which class hierarchy would implement this
new "TYPE_IOMMU_CHECK_HDEV" interface ?
RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>On 4/17/24 06:21, Duan, Zhenzhong wrote:
>>
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello,
>>>
>>> On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
> -Original Message-
> From: Cédric Le Goater
> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
> compatibility check with host IOMMU cap/ecap
>
> On 4/8/24 10:44, Zhenzhong Duan wrote:
>> From: Yi Liu
>>
>> If check fails, the host side device(either vfio or vdpa device) should
>not
>> be passed to guest.
>>
>> Implementation details for different backends will be in following
>>> patches.
>>
>> Signed-off-by: Yi Liu
>> Signed-off-by: Yi Sun
>> Signed-off-by: Zhenzhong Duan
>> ---
>> hw/i386/intel_iommu.c | 35
> +++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4f84e2e801..a49b587c73 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -35,6 +35,7 @@
>> #include "sysemu/kvm.h"
>> #include "sysemu/dma.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/iommufd.h"
>> #include "hw/i386/apic_internal.h"
>> #include "kvm/kvm_i386.h"
>> #include "migration/vmstate.h"
>> @@ -3819,6 +3820,32 @@ VTDAddressSpace
> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>> return vtd_dev_as;
>> }
>>
>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_hdev(IntelIOMMUState *s,
>>> VTDHostIOMMUDevice
> *vtd_hdev,
>> + Error **errp)
>> +{
>> +HostIOMMUDevice *hiod = vtd_hdev->dev;
>> +
>> +if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
>> +return vtd_check_iommufd_hdev(s, hiod, errp);
>> +}
>> +
>> +return vtd_check_legacy_hdev(s, hiod, errp);
>> +}
>
>
> I think we should be using the .get_host_iommu_info() class handler
> instead. Can we refactor the code slightly to avoid this check on
> the type ?
There is some difficulty ini avoiding this check, the behavior of
>>> vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting
>>> support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
>>> besides aw_bits.
>>>
>>> I think it is important to fully separate the vIOMMU model from the
>>> host IOMMU backing device. Could we introduce a new
>>> HostIOMMUDeviceClass
>>> handler .check_hdev() handler, which would call .get_host_iommu_info() ?
>>
>> Understood, besides the new .check_hdev() handler, I think we also need a
>new interface
>> class TYPE_IOMMU_CHECK_HDEV which has two handlers
>check_[legacy|iommufd]_hdev(),
>> and different vIOMMUs have different implementation.
>
>I am not sure to understand. Which class hierarchy would implement this
>new "TYPE_IOMMU_CHECK_HDEV" interface ? vIOMMU or host iommu ?
>
>Could you please explain with an update of your diagram :
>
> HostIOMMUDevice
>| .get_host_iommu_info()
>|
>|
> ..
> | | |
> HIODLegacyVFIO[HIODLegacyVDPA]HIODIOMMUFD
> | .vdev| [.vdev] | .iommufd
> | .devid
> | [.ioas_id]
> | [.attach_hwpt()]
> | [.detach_hwpt()]
> |
> .--.
> | |
>HIODIOMMUFDVFIO [HIODIOMMUFDVDPA]
> | .vdev| [.vdev]
>
Sure.
HostIOMMUDevice
| .get_host_iommu_info()
| .check_hdev()
Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
On 4/17/24 06:21, Duan, Zhenzhong wrote:
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
Hello,
On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should not
be passed to guest.
Implementation details for different backends will be in following
patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
+++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
VTDHostIOMMUDevice
*vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting
support introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
besides aw_bits.
I think it is important to fully separate the vIOMMU model from the
host IOMMU backing device. Could we introduce a new
HostIOMMUDeviceClass
handler .check_hdev() handler, which would call .get_host_iommu_info() ?
Understood, besides the new .check_hdev() handler, I think we also need a new
interface
class TYPE_IOMMU_CHECK_HDEV which has two handlers
check_[legacy|iommufd]_hdev(),
and different vIOMMUs have different implementation.
I am not sure to understand. Which class hierarchy would implement this
new "TYPE_IOMMU_CHECK_HDEV" interface ? vIOMMU or host iommu ?
Could you please explain with an update of your diagram :
HostIOMMUDevice
| .get_host_iommu_info()
|
|
..
| | |
HIODLegacyVFIO[HIODLegacyVDPA]HIODIOMMUFD
| .vdev| [.vdev] | .iommufd
| .devid
| [.ioas_id]
| [.attach_hwpt()]
| [.detach_hwpt()]
|
.--.
| |
HIODIOMMUFDVFIO [HIODIOMMUFDVDPA]
| .vdev| [.vdev]
Thanks,
C.
Then legacy and iommufd host device have different implementation of
.check_hdev()
and calls into one of the two interface handlers.
Let me know if I misunderstand any of your point.
Thanks
Zhenzhong
Thanks,
C.
That the reason I have two functions to do different thing.
See:
https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfc
v2/hw/i386/intel_iommu.c#L5472
Meanwhile in vtd_check_legacy_hdev(), when legacy VFIO device attaches
to modern vIOMMU,
this is unsupported and error out early, it will not
call .get_host_iommu_info().
I mean we don't need to unconditionally call .get_host_iommu_info() in
some cases.
Thanks
Zhenzhong
RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>Hello,
>
>On 4/16/24 09:09, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -Original Message-
>>> From: Cédric Le Goater
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should not
be passed to guest.
Implementation details for different backends will be in following
>patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
>>> +++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s,
>VTDHostIOMMUDevice
>>> *vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
>>>
>>>
>>> I think we should be using the .get_host_iommu_info() class handler
>>> instead. Can we refactor the code slightly to avoid this check on
>>> the type ?
>>
>> There is some difficulty ini avoiding this check, the behavior of
>vtd_check_legacy_hdev
>> and vtd_check_iommufd_hdev are different especially after nesting
>support introduced.
>> vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
>besides aw_bits.
>
>I think it is important to fully separate the vIOMMU model from the
>host IOMMU backing device. Could we introduce a new
>HostIOMMUDeviceClass
>handler .check_hdev() handler, which would call .get_host_iommu_info() ?
Understood, besides the new .check_hdev() handler, I think we also need a new
interface
class TYPE_IOMMU_CHECK_HDEV which has two handlers
check_[legacy|iommufd]_hdev(),
and different vIOMMUs have different implementation.
Then legacy and iommufd host device have different implementation of
.check_hdev()
and calls into one of the two interface handlers.
Let me know if I misunderstand any of your point.
Thanks
Zhenzhong
>
>
>Thanks,
>
>C.
>
>
>> That the reason I have two functions to do different thing.
>> See:
>>
>https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfc
>v2/hw/i386/intel_iommu.c#L5472
>>
>> Meanwhile in vtd_check_legacy_hdev(), when legacy VFIO device attaches
>to modern vIOMMU,
>> this is unsupported and error out early, it will not
>call .get_host_iommu_info().
>> I mean we don't need to unconditionally call .get_host_iommu_info() in
>some cases.
>>
>> Thanks
>> Zhenzhong
Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
Hello,
On 4/16/24 09:09, Duan, Zhenzhong wrote:
Hi Cédric,
-Original Message-
From: Cédric Le Goater
Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should not
be passed to guest.
Implementation details for different backends will be in following patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35
+++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace
*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice
*vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting support
introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits besides
aw_bits.
I think it is important to fully separate the vIOMMU model from the
host IOMMU backing device. Could we introduce a new HostIOMMUDeviceClass
handler .check_hdev() handler, which would call .get_host_iommu_info() ?
Thanks,
C.
That the reason I have two functions to do different thing.
See:
https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfcv2/hw/i386/intel_iommu.c#L5472
Meanwhile in vtd_check_legacy_hdev(), when legacy VFIO device attaches to
modern vIOMMU,
this is unsupported and error out early, it will not call
.get_host_iommu_info().
I mean we don't need to unconditionally call .get_host_iommu_info() in some
cases.
Thanks
Zhenzhong
RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
Hi Cédric,
>-Original Message-
>From: Cédric Le Goater
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>On 4/8/24 10:44, Zhenzhong Duan wrote:
>> From: Yi Liu
>>
>> If check fails, the host side device(either vfio or vdpa device) should not
>> be passed to guest.
>>
>> Implementation details for different backends will be in following patches.
>>
>> Signed-off-by: Yi Liu
>> Signed-off-by: Yi Sun
>> Signed-off-by: Zhenzhong Duan
>> ---
>> hw/i386/intel_iommu.c | 35
>+++
>> 1 file changed, 35 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 4f84e2e801..a49b587c73 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -35,6 +35,7 @@
>> #include "sysemu/kvm.h"
>> #include "sysemu/dma.h"
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/iommufd.h"
>> #include "hw/i386/apic_internal.h"
>> #include "kvm/kvm_i386.h"
>> #include "migration/vmstate.h"
>> @@ -3819,6 +3820,32 @@ VTDAddressSpace
>*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>> return vtd_dev_as;
>> }
>>
>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>> + HostIOMMUDevice *hiod,
>> + Error **errp)
>> +{
>> +return 0;
>> +}
>> +
>> +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice
>*vtd_hdev,
>> + Error **errp)
>> +{
>> +HostIOMMUDevice *hiod = vtd_hdev->dev;
>> +
>> +if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
>> +return vtd_check_iommufd_hdev(s, hiod, errp);
>> +}
>> +
>> +return vtd_check_legacy_hdev(s, hiod, errp);
>> +}
>
>
>I think we should be using the .get_host_iommu_info() class handler
>instead. Can we refactor the code slightly to avoid this check on
>the type ?
There is some difficulty ini avoiding this check, the behavior of
vtd_check_legacy_hdev
and vtd_check_iommufd_hdev are different especially after nesting support
introduced.
vtd_check_iommufd_hdev() has much wider check over cap/ecap bits besides
aw_bits.
That the reason I have two functions to do different thing.
See:
https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfcv2/hw/i386/intel_iommu.c#L5472
Meanwhile in vtd_check_legacy_hdev(), when legacy VFIO device attaches to
modern vIOMMU,
this is unsupported and error out early, it will not call
.get_host_iommu_info().
I mean we don't need to unconditionally call .get_host_iommu_info() in some
cases.
Thanks
Zhenzhong
Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap
On 4/8/24 10:44, Zhenzhong Duan wrote:
From: Yi Liu
If check fails, the host side device(either vfio or vdpa device) should not
be passed to guest.
Implementation details for different backends will be in following patches.
Signed-off-by: Yi Liu
Signed-off-by: Yi Sun
Signed-off-by: Zhenzhong Duan
---
hw/i386/intel_iommu.c | 35 +++
1 file changed, 35 insertions(+)
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 4f84e2e801..a49b587c73 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -35,6 +35,7 @@
#include "sysemu/kvm.h"
#include "sysemu/dma.h"
#include "sysemu/sysemu.h"
+#include "sysemu/iommufd.h"
#include "hw/i386/apic_internal.h"
#include "kvm/kvm_i386.h"
#include "migration/vmstate.h"
@@ -3819,6 +3820,32 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
PCIBus *bus,
return vtd_dev_as;
}
+static int vtd_check_legacy_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
+ HostIOMMUDevice *hiod,
+ Error **errp)
+{
+return 0;
+}
+
+static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev,
+ Error **errp)
+{
+HostIOMMUDevice *hiod = vtd_hdev->dev;
+
+if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
+return vtd_check_iommufd_hdev(s, hiod, errp);
+}
+
+return vtd_check_legacy_hdev(s, hiod, errp);
+}
I think we should be using the .get_host_iommu_info() class handler
instead. Can we refactor the code slightly to avoid this check on
the type ?
Thanks,
C.
+
static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
HostIOMMUDevice *hiod, Error **errp)
{
@@ -3829,6 +3856,7 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void
*opaque, int devfn,
.devfn = devfn,
};
struct vtd_as_key *new_key;
+int ret;
assert(hiod);
@@ -3848,6 +3876,13 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
vtd_hdev->iommu_state = s;
vtd_hdev->dev = hiod;
+ret = vtd_check_hdev(s, vtd_hdev, errp);
+if (ret) {
+g_free(vtd_hdev);
+vtd_iommu_unlock(s);
+return ret;
+}
+
new_key = g_malloc(sizeof(*new_key));
new_key->bus = bus;
new_key->devfn = devfn;
