| Message ID | 20240429065046.3688701-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add a host IOMMU device abstraction to check with vIOMMU | expand |
On 4/29/24 08:50, Zhenzhong Duan wrote: > Hi, > > The most important change in this version is instroducing a common > HostIOMMUDeviceCaps structure in HostIOMMUDevice and a new interface > between vIOMMU and HostIOMMUDevice. > > HostIOMMUDeviceClass::realize() is introduced to initialize > HostIOMMUDeviceCaps and other fields of HostIOMMUDevice variants. > > HostIOMMUDeviceClass::check_cap() is introduced to query host IOMMU > device capabilities. > > After the change, part2 is only 3 patches, so merge it with part1 to be > a single prerequisite series, same for changelog. If anyone doesn't like > that, I can split again. > > The class tree is as below: > > HostIOMMUDevice > | .caps > | .realize() > | .check_cap() > | > .-----------------------------------------------. > | | | > HostIOMMUDeviceLegacyVFIO {HostIOMMUDeviceLegacyVDPA} HostIOMMUDeviceIOMMUFD > | .vdev | {.vdev} | .iommufd > | .devid > | [.ioas_id] > | [.attach_hwpt()] > | [.detach_hwpt()] > | > .----------------------. > | | > HostIOMMUDeviceIOMMUFDVFIO {HostIOMMUDeviceIOMMUFDVDPA} > | .vdev | {.vdev} > > * The attributes in [] will be implemented in nesting series. > * The classes in {} will be implemented in future. > * .vdev in different class points to different agent device, > * i.e., for VFIO it points to VFIODevice. > > PATCH1-4: Introduce HostIOMMUDevice and its sub classes > PATCH5-11: Introduce HostIOMMUDeviceCaps, implement .realize() and .check_cap() handler > PATCH12-16: Create HostIOMMUDevice instance and pass to vIOMMU > PATCH17-19: Implement compatibility check between host IOMMU and vIOMMU(intel_iommu) > > Qemu code can be found at: > https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_preq_v3 > > Besides the compatibility check in this series, in nesting series, this > host IOMMU device is extended for much wider usage. For anyone interested > on the nesting series, here is the link: > https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_rfcv2 v4 should be a good candidate, we will need feedback from the vIOMMU maintainers though. However, have you considered another/complementary approach which would be to create an host IOMMU (iommufd) backend object and a vIOMMU device object together for each vfio-pci device being plugged in the machine ? Something like, -device pcie-root-port,port=23,chassis=8,id=pci.8,bus=pcie.0 \ -object iommufd,id=iommufd1 \ -device intel-iommu,intremap=on,device-iotlb=on,caching-mode=on,iommufd=iommufd1 \ -device vfio-pci,host=0000:08:10.0,bus=pci.1,iommufd=iommufd0 The vIOMMU device would be linked to the host IOMMU (iommufd) backend object at realize time and it would simplify the discovery of the host IOMMU properties. The implementation would be more straight forward. That said, I didn't study deeply what needs to be done. The vIOMMU implementation is not ready yet to support multiple instances and some massaging is needed to change that first. Thanks, C.
On Fri, May 03, 2024 at 04:04:25PM +0200, Cédric Le Goater wrote: > However, have you considered another/complementary approach which > would be to create an host IOMMU (iommufd) backend object and a vIOMMU > device object together for each vfio-pci device being plugged in the > machine ? > > Something like, > -device pcie-root-port,port=23,chassis=8,id=pci.8,bus=pcie.0 \ > -object iommufd,id=iommufd1 \ > -device intel-iommu,intremap=on,device-iotlb=on,caching-mode=on,iommufd=iommufd1 \ > -device vfio-pci,host=0000:08:10.0,bus=pci.1,iommufd=iommufd0 ? The main point of this is to have a single iommufd FD open in qemu. Not multiple. Would you achieve this with a iommufd0 and iommufd1 ? Jason
On 5/3/24 16:10, Jason Gunthorpe wrote: > On Fri, May 03, 2024 at 04:04:25PM +0200, Cédric Le Goater wrote: >> However, have you considered another/complementary approach which >> would be to create an host IOMMU (iommufd) backend object and a vIOMMU >> device object together for each vfio-pci device being plugged in the >> machine ? >> >> Something like, >> -device pcie-root-port,port=23,chassis=8,id=pci.8,bus=pcie.0 \ >> -object iommufd,id=iommufd1 \ >> -device intel-iommu,intremap=on,device-iotlb=on,caching-mode=on,iommufd=iommufd1 \ >> -device vfio-pci,host=0000:08:10.0,bus=pci.1,iommufd=iommufd0 > > ? The main point of this is to have a single iommufd FD open in > qemu. Not multiple. oups. The above example should have the same IOMMUFD object device instance: iommufd0. This is bogus copy-paste of a command line with multiple vfio-pci devices, each using its own IOMMUFD object device instance. That's where the idea comes from. Sorry for the noise. C.
Hi Cédric, >-----Original Message----- >From: Cédric Le Goater <clg@redhat.com> >Sent: Friday, May 3, 2024 10:04 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu- >devel@nongnu.org >Cc: alex.williamson@redhat.com; eric.auger@redhat.com; mst@redhat.com; >peterx@redhat.com; jasowang@redhat.com; jgg@nvidia.com; >nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian, Kevin ><kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P ><chao.p.peng@intel.com> >Subject: Re: [PATCH v3 00/19] Add a host IOMMU device abstraction to >check with vIOMMU > >On 4/29/24 08:50, Zhenzhong Duan wrote: >> Hi, >> >> The most important change in this version is instroducing a common >> HostIOMMUDeviceCaps structure in HostIOMMUDevice and a new >interface >> between vIOMMU and HostIOMMUDevice. >> >> HostIOMMUDeviceClass::realize() is introduced to initialize >> HostIOMMUDeviceCaps and other fields of HostIOMMUDevice variants. >> >> HostIOMMUDeviceClass::check_cap() is introduced to query host IOMMU >> device capabilities. >> >> After the change, part2 is only 3 patches, so merge it with part1 to be >> a single prerequisite series, same for changelog. If anyone doesn't like >> that, I can split again. >> >> The class tree is as below: >> >> HostIOMMUDevice >> | .caps >> | .realize() >> | .check_cap() >> | >> .-----------------------------------------------. >> | | | >> HostIOMMUDeviceLegacyVFIO {HostIOMMUDeviceLegacyVDPA} >HostIOMMUDeviceIOMMUFD >> | .vdev | {.vdev} | .iommufd >> | .devid >> | [.ioas_id] >> | [.attach_hwpt()] >> | [.detach_hwpt()] >> | >> .----------------------. >> | | >> HostIOMMUDeviceIOMMUFDVFIO >{HostIOMMUDeviceIOMMUFDVDPA} >> | .vdev | {.vdev} >> >> * The attributes in [] will be implemented in nesting series. >> * The classes in {} will be implemented in future. >> * .vdev in different class points to different agent device, >> * i.e., for VFIO it points to VFIODevice. >> >> PATCH1-4: Introduce HostIOMMUDevice and its sub classes >> PATCH5-11: Introduce HostIOMMUDeviceCaps, implement .realize() >and .check_cap() handler >> PATCH12-16: Create HostIOMMUDevice instance and pass to vIOMMU >> PATCH17-19: Implement compatibility check between host IOMMU and >vIOMMU(intel_iommu) >> >> Qemu code can be found at: >> >https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_pre >q_v3 >> >> Besides the compatibility check in this series, in nesting series, this >> host IOMMU device is extended for much wider usage. For anyone >interested >> on the nesting series, here is the link: >> >https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_rfc >v2 > > >v4 should be a good candidate, we will need feedback from the vIOMMU >maintainers though. > >However, have you considered another/complementary approach which >would be to create an host IOMMU (iommufd) backend object and a >vIOMMU >device object together for each vfio-pci device being plugged in the >machine ? I did consider about a single iommufd instance for qemu and finally chose to support multiple iommufd instances, reason below: I was taking iommufd as a backend of VFIO device not a backend of vIOMMU. So there is an iommufd property linked to iommufd instances. We do support multiple iommufd instances in nesting series just as we do in cdev series, such as: -device intel-iommu,caching-mode=on,dma-drain=on,device-iotlb=on,x-scalable-mode=modern -object iommufd,id=iommufd0 -device vfio-pci,host=6f:01.0,id=vfio0,bus=root0,iommufd=iommufd0 -object iommufd,id=iommufd1 -device vfio-pci,host=6f:01.1,id=vfio1,bus=root1,iommufd=iommufd1 -device vfio-pci,host=6f:01.2,id=vfio2,bus=root2 Adding iommufd property to vIOMMU will limit the whole qemu to use only one iommufd instance, it's also confusing if there is also vfio device with legacy backend. I'm not clear how useful multiple iommufd instances support are. One possible benefit is for security? It may bring a slightly fine-grained isolation in kernel. We can discuss this further, if it's unnecessary to support multiple iommufd instances in nesting series, I can change to single iommufd instance support and add an iommufd property for vIOMMU just as you suggested. Thanks Zhenzhong > >Something like, > > -device pcie-root-port,port=23,chassis=8,id=pci.8,bus=pcie.0 \ > -object iommufd,id=iommufd1 \ > -device intel-iommu,intremap=on,device-iotlb=on,caching- >mode=on,iommufd=iommufd1 \ > -device vfio-pci,host=0000:08:10.0,bus=pci.1,iommufd=iommufd0 > >The vIOMMU device would be linked to the host IOMMU (iommufd) backend >object at realize time and it would simplify the discovery of the host >IOMMU properties. The implementation would be more straight forward. > >That said, I didn't study deeply what needs to be done. The vIOMMU >implementation is not ready yet to support multiple instances and some >massaging is needed to change that first.
On Mon, May 06, 2024 at 02:30:47AM +0000, Duan, Zhenzhong wrote: > I'm not clear how useful multiple iommufd instances support are. > One possible benefit is for security? It may bring a slightly fine-grained > isolation in kernel. No. I don't think there is any usecase, it is only harmful. Jason
>-----Original Message----- >From: Jason Gunthorpe <jgg@nvidia.com> >Subject: Re: [PATCH v3 00/19] Add a host IOMMU device abstraction to >check with vIOMMU > >On Mon, May 06, 2024 at 02:30:47AM +0000, Duan, Zhenzhong wrote: > >> I'm not clear how useful multiple iommufd instances support are. >> One possible benefit is for security? It may bring a slightly fine-grained >> isolation in kernel. > >No. I don't think there is any usecase, it is only harmful. OK, so we need to limit QEMU to only one iommufd instance. In cdev series, we support mix of legacy and iommufd backend and multiple iommufd backend instances for flexibility. We need to make a choice to have this limitation only for nesting series or globally(including cdev). May I ask what harmfulness we may have? Thanks Zhenzhong
On Tue, May 07, 2024 at 02:24:30AM +0000, Duan, Zhenzhong wrote: > >On Mon, May 06, 2024 at 02:30:47AM +0000, Duan, Zhenzhong wrote: > > > >> I'm not clear how useful multiple iommufd instances support are. > >> One possible benefit is for security? It may bring a slightly fine-grained > >> isolation in kernel. > > > >No. I don't think there is any usecase, it is only harmful. > > OK, so we need to limit QEMU to only one iommufd instance. I don't know about limit, but you don't need to do extra stuff to make it work. The main issue will be to get all the viommu instances to share the same iommufd IOAS for the guest physical mapping. Otherwise each viommu should be largely unware of the others sharing (or not) a iommufd. If you can structure things properly it probably doesn't need a hard limit, it will just work worse. Jason
>-----Original Message----- >From: Jason Gunthorpe <jgg@nvidia.com> >Subject: Re: [PATCH v3 00/19] Add a host IOMMU device abstraction to >check with vIOMMU > >On Tue, May 07, 2024 at 02:24:30AM +0000, Duan, Zhenzhong wrote: >> >On Mon, May 06, 2024 at 02:30:47AM +0000, Duan, Zhenzhong wrote: >> > >> >> I'm not clear how useful multiple iommufd instances support are. >> >> One possible benefit is for security? It may bring a slightly fine-grained >> >> isolation in kernel. >> > >> >No. I don't think there is any usecase, it is only harmful. >> >> OK, so we need to limit QEMU to only one iommufd instance. > >I don't know about limit, but you don't need to do extra stuff to make >it work. > >The main issue will be to get all the viommu instances to share the >same iommufd IOAS for the guest physical mapping. Otherwise each >viommu should be largely unware of the others sharing (or not) a >iommufd. I see. > >If you can structure things properly it probably doesn't need a hard >limit, it will just work worse. OK, thanks for clarify. The extra code to support multiple instances in intel_iommu is trivial. So I'd like to keep this flexibility to user just like cdev. User can configure QEMU cmdline to use one IOMMUFD instance easily whenever they want. Thanks Zhenzhong
Hi, The most important change in this version is instroducing a common HostIOMMUDeviceCaps structure in HostIOMMUDevice and a new interface between vIOMMU and HostIOMMUDevice. HostIOMMUDeviceClass::realize() is introduced to initialize HostIOMMUDeviceCaps and other fields of HostIOMMUDevice variants. HostIOMMUDeviceClass::check_cap() is introduced to query host IOMMU device capabilities. After the change, part2 is only 3 patches, so merge it with part1 to be a single prerequisite series, same for changelog. If anyone doesn't like that, I can split again. The class tree is as below: HostIOMMUDevice | .caps | .realize() | .check_cap() | .-----------------------------------------------. | | | HostIOMMUDeviceLegacyVFIO {HostIOMMUDeviceLegacyVDPA} HostIOMMUDeviceIOMMUFD | .vdev | {.vdev} | .iommufd | .devid | [.ioas_id] | [.attach_hwpt()] | [.detach_hwpt()] | .----------------------. | | HostIOMMUDeviceIOMMUFDVFIO {HostIOMMUDeviceIOMMUFDVDPA} | .vdev | {.vdev} * The attributes in [] will be implemented in nesting series. * The classes in {} will be implemented in future. * .vdev in different class points to different agent device, * i.e., for VFIO it points to VFIODevice. PATCH1-4: Introduce HostIOMMUDevice and its sub classes PATCH5-11: Introduce HostIOMMUDeviceCaps, implement .realize() and .check_cap() handler PATCH12-16: Create HostIOMMUDevice instance and pass to vIOMMU PATCH17-19: Implement compatibility check between host IOMMU and vIOMMU(intel_iommu) Qemu code can be found at: https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_preq_v3 Besides the compatibility check in this series, in nesting series, this host IOMMU device is extended for much wider usage. For anyone interested on the nesting series, here is the link: https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_rfcv2 Thanks Zhenzhong Changelog: v3: - refine declaration and doc for HostIOMMUDevice (Cédric, Philippe) - introduce HostIOMMUDeviceCaps, .realize() and .check_cap() (Cédric) - introduce helper range_get_last_bit() for range operation (Cédric) - separate pci_device_get_iommu_bus_devfn() in a prereq patch (Cédric) - replace HIOD_ abbreviation with HOST_IOMMU_DEVICE_ (Cédric) - add header in include/sysemu/iommufd.h (Cédric) v2: - use QOM to abstract host IOMMU device and its sub-classes (Cédric) - move host IOMMU device creation in attach_device() (Cédric) - refine pci_device_set/unset_iommu_device doc further (Eric) - define host IOMMU info format of different backend - implement get_host_iommu_info() for different backend (Cédric) - drop cap/ecap update logic (MST) - check aw-bits from get_host_iommu_info() in legacy mode v1: - use HostIOMMUDevice handle instead of union in VFIODevice (Eric) - change host_iommu_device_init to host_iommu_device_create - allocate HostIOMMUDevice in host_iommu_device_create callback and set the VFIODevice base_hdev handle (Eric) - refine pci_device_set/unset_iommu_device doc (Eric) - use HostIOMMUDevice handle instead of union in VTDHostIOMMUDevice (Eric) - convert HostIOMMUDevice to sub object pointer in vtd_check_hdev rfcv2: - introduce common abstract HostIOMMUDevice and sub struct for different BEs (Eric, Cédric) - remove iommufd_device.[ch] (Cédric) - remove duplicate iommufd/devid define from VFIODevice (Eric) - drop the p in aliased_pbus and aliased_pdevfn (Eric) - assert devfn and iommu_bus in pci_device_get_iommu_bus_devfn (Cédric, Eric) - use errp in iommufd_device_get_info (Eric) - split and simplify cap/ecap check/sync code in intel_iommu.c (Cédric) - move VTDHostIOMMUDevice declaration to intel_iommu_internal.h (Cédric) - make '(vtd->cap_reg >> 16) & 0x3fULL' a MACRO and add missed '+1' (Cédric) - block migration if vIOMMU cap/ecap updated based on host IOMMU cap/ecap - add R-B Yi Liu (2): hw/pci: Introduce pci_device_[set|unset]_iommu_device() intel_iommu: Implement [set|unset]_iommu_device() callbacks Zhenzhong Duan (17): backends: Introduce HostIOMMUDevice abstract vfio/container: Introduce HostIOMMUDeviceLegacyVFIO device backends/iommufd: Introduce abstract HostIOMMUDeviceIOMMUFD device vfio/iommufd: Introduce HostIOMMUDeviceIOMMUFDVFIO device backends/host_iommu_device: Introduce HostIOMMUDeviceCaps range: Introduce range_get_last_bit() vfio/container: Implement HostIOMMUDeviceClass::realize() handler backends/iommufd: Introduce helper function iommufd_backend_get_device_info() vfio/iommufd: Implement HostIOMMUDeviceClass::realize() handler vfio/container: Implement HostIOMMUDeviceClass::check_cap() handler backends/iommufd: Implement HostIOMMUDeviceClass::check_cap() handler vfio: Introduce VFIOIOMMUClass::hiod_typename attribute vfio: Create host IOMMU device instance hw/pci: Introduce helper function pci_device_get_iommu_bus_devfn() vfio/pci: Pass HostIOMMUDevice to vIOMMU intel_iommu: Extract out vtd_cap_init() to initialize cap/ecap intel_iommu: Check compatibility with host IOMMU capabilities MAINTAINERS | 2 + hw/i386/intel_iommu_internal.h | 8 ++ include/hw/i386/intel_iommu.h | 3 + include/hw/pci/pci.h | 38 ++++- include/hw/vfio/vfio-common.h | 26 ++++ include/hw/vfio/vfio-container-base.h | 3 + include/qemu/range.h | 11 ++ include/sysemu/host_iommu_device.h | 95 +++++++++++++ include/sysemu/iommufd.h | 34 +++++ backends/host_iommu_device.c | 59 ++++++++ backends/iommufd.c | 75 +++++++--- hw/i386/intel_iommu.c | 197 ++++++++++++++++++++------ hw/pci/pci.c | 75 +++++++++- hw/vfio/common.c | 18 ++- hw/vfio/container.c | 49 ++++++- hw/vfio/iommufd.c | 52 ++++++- hw/vfio/pci.c | 20 ++- backends/Kconfig | 5 + backends/meson.build | 1 + 19 files changed, 701 insertions(+), 70 deletions(-) create mode 100644 include/sysemu/host_iommu_device.h create mode 100644 backends/host_iommu_device.c