Re: [Qemu-devel] [PATCH v3 0/5] intel-iommu: introduce Intel IOMMU (VT-d) emulation to q35 chipset

[Michael S. Tsirkin]

On Mon, Aug 11, 2014 at 03:04:57PM +0800, Le Tan wrote:
> Hi,
> 
> These patches are intended to introduce Intel IOMMU (VT-d) emulation to q35
> chipset. The major job in these patches is to add support for emulating Intel
> IOMMU according to the VT-d specification, including basic responses to CSRs
> accesses, the logics of DMAR (DMA remapping) and DMA memory address
> translations.

Thanks!
Looks very good overall, I noted some coding style issues - I didn't
bother reporting each issue in every place where it appears - reported
each issue once only, so please find and fix all instances of each
issue.

> Features implemented for now are:
> 1. Response to important CSRs accesses;
> 2. DMAR (DMA remapping) without PASID support;
> 3. Primary fault logging;
> 4. Support both register-based and queued invalidation for IOTLB and context
>    cache invalidation;
> 5. Add DMAR table to ACPI tables to expose VT-d to BIOS;
> 6. Add "-machine iommu=on|off" option to enable/disable VT-d;
> 7. Only one DMAR unit for all the devices of PCI Segment 0.
> 
> Testing:
> 1. L1 guest with Linux with intel_iommu=on can interact with VT-d and boot
> smoothly, and there exists information about VT-d in the log of kernel;
> 2. Run L1 with VT-d, L2 guest with Linux can boot smoothly withou PCI device
> passthrough;
> 3. Run L1 with VT-d and "-soundhw ac97 (QEMU_AUDIO_DRV=alsa)", then assign the
> sound card to L2; L2 can boot smoothly with legacy PCI assignment and I can
> hear the music played in L2 from the host speakers;
> 4. Jailhouse hypervisor can run smoothly(tested by Jan).
> 5. Run L1 with VT-d and e1000 network card, then assign e1000 to L2; L2 will 
> be
> STUCK when booting. This still remains unsolved now. As far as I know, I 
> suppose
> that the L2 crashes when doing e1000_probe(). The QEMU of L1 will dump
> something with "KVM: entry failed, hardware error 0x0", and the KVM of host
> will print "nested_vmx_exit_handled failed vm entry 7". Unlike assigning the
> sound card, after being assigned to L2, there is no translation entry of e1000
> through VT-d, which I think means that e1000 doesn't issue any DMA access 
> during
> the boot of L2. Sometimes the kernel of L2 will print "divide error" during
> booting. Maybe it results from the lack of reset mechanism.
> 6. VFIO is tested and is similar to legacy pci assignment.
> 
> Discussion:
> 1. There is one functionality called Zero-Length-Read (ZLR) which supports 
> zero
> length DMA read requests to write-only pages. If the VT-d emulation supports
> ZLR, we need to know the exact length of one access. For now can QEMU express
> zero-length requests?
> 
> TODO:
> 1. Context cache and IOTLB cache;
> 2. Fix the bug of legacy PCI assignment;
> 
> Changes since v2:
> *address reviewing suggestions given by Jan
> -add support for primary fault logging
> -add support for queued invalidation
> 
> Changes since v1:
> *address reviewing suggestions given by Michael, Paolo, Stefan and Jan
> -split intel_iommu.h to include/hw/i386/intel_iommu.h and
>  hw/i386/intel_iommu_internal.h
> -change the copyright information
> -change D() to VTD_DPRINTF()
> -remove dead code
> -rename constant definitions with consistent prefix VTD_
> -rename some struct definitions according to QEMU standard
> -rename some CSRs access functions
> -use endian-save functions to access CSRs
> -change machine option to "iommu=on|off"
> 
> Thanks very much!
> 
> Git trees:
> https://github.com/tamlok/qemu
> 
> Le Tan (5):
>   iommu: add is_write as a parameter to the translate function of
>     MemoryRegionIOMMUOps
>   intel-iommu: introduce Intel IOMMU (VT-d) emulation
>   intel-iommu: add DMAR table to ACPI tables
>   intel-iommu: add Intel IOMMU emulation to q35 and add a machine option
>     "iommu" as a switch
>   intel-iommu: add supports for queued invalidation interface
> 
>  exec.c                         |    2 +-
>  hw/alpha/typhoon.c             |    3 +-
>  hw/core/machine.c              |   27 +-
>  hw/i386/Makefile.objs          |    1 +
>  hw/i386/acpi-build.c           |   41 +
>  hw/i386/acpi-defs.h            |   70 ++
>  hw/i386/intel_iommu.c          | 1722 
> ++++++++++++++++++++++++++++++++++++++++
>  hw/i386/intel_iommu_internal.h |  358 +++++++++
>  hw/pci-host/apb.c              |    3 +-
>  hw/pci-host/q35.c              |   64 +-
>  hw/ppc/spapr_iommu.c           |    3 +-
>  include/exec/memory.h          |    2 +-
>  include/hw/boards.h            |    1 +
>  include/hw/i386/intel_iommu.h  |   90 +++
>  include/hw/pci-host/q35.h      |    2 +
>  qemu-options.hx                |    5 +-
>  vl.c                           |    4 +
>  17 files changed, 2384 insertions(+), 14 deletions(-)
>  create mode 100644 hw/i386/intel_iommu.c
>  create mode 100644 hw/i386/intel_iommu_internal.h
>  create mode 100644 include/hw/i386/intel_iommu.h
> 
> -- 
> 1.9.1