Re: [Qemu-devel] [PATCH v10 16/26] intel_iommu: add support for split irqchip

[Jan Kiszka]

On 2016-06-21 09:47, Peter Xu wrote:
> In split irqchip mode, IOAPIC is working in user space, only update
> kernel irq routes when entry changed. When IR is enabled, we directly
> update the kernel with translated messages. It works just like a kernel
> cache for the remapping entries.
> 
> Since KVM irqfd is using kernel gsi routes to deliver interrupts, as
> long as we can support split irqchip, we will support irqfd as
> well. Also, since kernel gsi routes will cache translated interrupts,
> irqfd delivery will not suffer from any performance impact due to IR.
> 
> And, since we supported irqfd, vhost devices will be able to work
> seamlessly with IR now. Logically this should contain both vhost-net and
> vhost-user case.
> 
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  hw/i386/intel_iommu.c         |  7 +++++++
>  include/hw/i386/intel_iommu.h |  1 +
>  include/hw/i386/x86-iommu.h   |  4 ++++
>  target-i386/kvm.c             | 27 +++++++++++++++++++++++++++
>  trace-events                  |  3 +++
>  5 files changed, 42 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index d874596..0eaffc6 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2149,6 +2149,12 @@ do_not_translate:
>      return 0;
>  }
>  
> +static int vtd_int_remap(X86IOMMUState *iommu, MSIMessage *src,
> +                         MSIMessage *dst, uint16_t sid)
> +{
> +    return vtd_interrupt_remap_msi(INTEL_IOMMU_DEVICE(iommu), src, dst);
> +}
> +
>  static MemTxResult vtd_mem_ir_read(void *opaque, hwaddr addr,
>                                     uint64_t *data, unsigned size,
>                                     MemTxAttrs attrs)
> @@ -2393,6 +2399,7 @@ static void vtd_class_init(ObjectClass *klass, void 
> *data)
>      dc->props = vtd_properties;
>      x86_class->realize = vtd_realize;
>      x86_class->find_add_as = vtd_find_add_as;
> +    x86_class->int_remap = vtd_int_remap;
>  }
>  
>  static const TypeInfo vtd_info = {
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index b3f17d7..3bca390 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -26,6 +26,7 @@
>  #include "hw/i386/x86-iommu.h"
>  #include "hw/i386/ioapic.h"
>  #include "hw/pci/msi.h"
> +#include "hw/sysbus.h"
>  
>  #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
>  #define INTEL_IOMMU_DEVICE(obj) \
> diff --git a/include/hw/i386/x86-iommu.h b/include/hw/i386/x86-iommu.h
> index 07199be..b419ae5 100644
> --- a/include/hw/i386/x86-iommu.h
> +++ b/include/hw/i386/x86-iommu.h
> @@ -22,6 +22,7 @@
>  
>  #include "hw/sysbus.h"
>  #include "exec/memory.h"
> +#include "hw/pci/pci.h"
>  
>  #define  TYPE_X86_IOMMU_DEVICE  ("x86-iommu")
>  #define  X86_IOMMU_DEVICE(obj) \
> @@ -43,6 +44,9 @@ struct X86IOMMUClass {
>      DeviceRealize realize;
>      /* Find/Add IOMMU address space for specific PCI device */
>      AddressSpace *(*find_add_as)(X86IOMMUState *s, PCIBus *bus, int devfn);
> +    /* MSI-based interrupt remapping */
> +    int (*int_remap)(X86IOMMUState *iommu, MSIMessage *src,
> +                     MSIMessage *dst, uint16_t sid);
>  };
>  
>  struct X86IOMMUState {
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index f3698f1..bfa40b2 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -35,6 +35,7 @@
>  #include "hw/i386/apic.h"
>  #include "hw/i386/apic_internal.h"
>  #include "hw/i386/apic-msidef.h"
> +#include "hw/i386/intel_iommu.h"
>  
>  #include "exec/ioport.h"
>  #include "standard-headers/asm-x86/hyperv.h"
> @@ -42,6 +43,7 @@
>  #include "hw/pci/msi.h"
>  #include "migration/migration.h"
>  #include "exec/memattrs.h"
> +#include "trace.h"
>  
>  //#define DEBUG_KVM
>  
> @@ -3323,6 +3325,31 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t 
> dev_id)
>  int kvm_arch_fixup_msi_route(struct kvm_irq_routing_entry *route,
>                               uint64_t address, uint32_t data, PCIDevice *dev)
>  {
> +    X86IOMMUState *iommu = x86_iommu_get_default();
> +
> +    if (iommu) {
> +        int ret;
> +        MSIMessage src, dst;
> +        X86IOMMUClass *class = X86_IOMMU_GET_CLASS(iommu);
> +
> +        src.address = route->u.msi.address_hi;
> +        src.address <<= VTD_MSI_ADDR_HI_SHIFT;
> +        src.address |= route->u.msi.address_lo;
> +        src.data = route->u.msi.data;
> +
> +        ret = class->int_remap(iommu, &src, &dst, dev ? \
> +                               pci_requester_id(dev) : \
> +                               X86_IOMMU_SID_INVALID);
> +        if (ret) {
> +            trace_kvm_x86_fixup_msi_error(route->gsi);
> +            return 1;
> +        }
> +
> +        route->u.msi.address_hi = dst.address >> VTD_MSI_ADDR_HI_SHIFT;
> +        route->u.msi.address_lo = dst.address & VTD_MSI_ADDR_LO_MASK;
> +        route->u.msi.data = dst.data;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/trace-events b/trace-events
> index da0d060..2982f64 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -2206,3 +2206,6 @@ gicv3_redist_write(uint32_t cpu, uint64_t offset, 
> uint64_t data, unsigned size,
>  gicv3_redist_badwrite(uint32_t cpu, uint64_t offset, uint64_t data, unsigned 
> size, bool secure) "GICv3 redistributor %x write: offset 0x%" PRIx64 " data 
> 0x%" PRIx64 " size %u secure %d: error"
>  gicv3_redist_set_irq(uint32_t cpu, int irq, int level) "GICv3 redistributor 
> %x interrupt %d level changed to %d"
>  gicv3_redist_send_sgi(uint32_t cpu, int irq) "GICv3 redistributor %x pending 
> SGI %d"
> +
> +# target-i386/kvm.c
> +kvm_x86_fixup_msi_error(uint32_t gsi) "VT-d failed to remap interrupt for 
> GSI %" PRIu32
> 

For successful remappings, this is fine - it just caches the result in
an interrupt route. But what will happen with invalid interrupts?

My current understanding is that, because the translation happens on
activation of that interrupt source, not on actual signalling, the IOMMU
will report an error too early and none when the interrupt is actually
sent. That will lead to unwanted results, in the worst case
false-positiv IR error reports to the guest, no?

I think we need to do this:
- silently remap broken sources to an error sink
- hook up the error sink with the actual IOMMU model (Intel or AMD)
- when that source actually fires, let the sink report an IR
  translation error to the guest

Am I right?

Jan

signature.asc
Description: OpenPGP digital signature