Re: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap

[Cédric Le Goater]

On 4/17/24 06:21, Duan, Zhenzhong wrote:
> > -----Original Message-----
> > From: Cédric Le Goater <clg@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 <clg@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.liu@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.liu@intel.com>
> > > > > Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> > > > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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]


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