>-----Original Message-----
>From: Cédric Le Goater <c...@redhat.com>
>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 <c...@redhat.com>
>>> 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 <c...@redhat.com>
>>>>> 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 <yi.l....@intel.com>
>>>>>>
>>>>>> 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 <yi.l....@intel.com>
>>>>>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>>>>> ---
>>>>>> 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()
|
.------------------------------.
| |
HIODLegacy HIODIOMMUFD
| | .iommufd
.--------------. | .devid
| | | [.ioas_id]
HIODLegacyVFIO [HIODLegacyVDPA] | [.attach_hwpt()]
| .vdev | [.vdev] | [.detach_hwpt()]
|
.----------------------.
| |
HIODIOMMUFDVFIO [HIODIOMMUFDVDPA]
| .vdev | [.vdev]
HostIOMMUDevice only declare .check_hdev(), but
HIODLegacy and HIODIOMMUFD will implement .check_hdev().
E.g., hiod_legacy_check_hdev() and hiod_iommufd_check_hdev().
int hiod_legacy_check_hdev(HostIOMMUDevice *hiod, IOMMUCheckHDev *viommu, Error
**errp)
{
IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_GET_CLASS(viommu);
return chdc->check_legacy_hdev(viommu, hiod, errp);
}
int hiod_iommufd_check_hdev(HostIOMMUDevice *hiod, IOMMUCheckHDev *viommu,
Error **errp)
{
IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_GET_CLASS(viommu);
return chdc->check_iommufd_hdev(viommu, hiod, errp);
}
And we implement interface TYPE_IOMMU_CHECK_HDEV in intel-iommu module.
Certainly, we can also implement the same in other vIOMMUs we want.
See below pseudo change:
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 68380d50ca..173c702b9f 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -5521,12 +5521,9 @@ 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);
- if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
- return vtd_check_iommufd_hdev(s, vtd_hdev, errp);
- }
-
- return vtd_check_legacy_hdev(s, hiod, errp);
+ return hiodc->check_hdev(IOMMU_CHECK_HDEV(s), hiod, errp);
}
static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
@@ -6076,6 +6073,7 @@ static void vtd_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass);
+ IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_CLASS(klass);
dc->reset = vtd_reset;
dc->vmsd = &vtd_vmstate;
@@ -6087,6 +6085,8 @@ static void vtd_class_init(ObjectClass *klass, void *data)
dc->user_creatable = true;
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
dc->desc = "Intel IOMMU (VT-d) DMA Remapping device";
+ chdc->check_legacy_hdev = vtd_check_legacy_hdev;
+ chdc->check_iommufd_hdev = vtd_check_iommufd_hdev;
}
static const TypeInfo vtd_info = {
@@ -6094,6 +6094,10 @@ static const TypeInfo vtd_info = {
.parent = TYPE_X86_IOMMU_DEVICE,
.instance_size = sizeof(IntelIOMMUState),
.class_init = vtd_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_IOMMU_CHECK_HDEV },
+ { }
+ }
};
Thanks
Zhenzhong
