Re: [Qemu-devel] [PATCH v4 00/16] IOMMU: Enable interrupt remapping for Intel IOMMU

[Peter Xu]

On Tue, Apr 26, 2016 at 12:51:44PM +0200, Jan Kiszka wrote:
> On 2016-04-26 12:38, Peter Xu wrote:
> >>>> Hi, Jan,
> >>>>
> >>>> The above issue should be caused by EOI missing of level-triggered
> >>>> interrupts. Before that, I was always using edge-triggered
> >>>> interrupts for test, so didn't encounter this one. Would you please
> >>>> help try below patch? It can be applied directly onto the series,
> >>>> and should solve the issue (it works on my test vm, and I'll take it
> >>>> in v5 as well if it also works for you):
> >>>>
> >>>
> >>> Works here as well. I even made EIM working with some hack, though
> >>> Jailhouse spits out strange warnings, despite it works fine (x2apic
> >>> mode, split irqchip).
> >>
> >> Corrections: the warnings are issued by qemu, not Jailhouse, e.g.
> >>
> >> qemu-system-x86_64: VT-d Failed to remap interrupt for gsi 22.
> >>
> >> I suspect that comes from the hand-over phase of Jailhouse, when it
> >> mutes all interrupts in the system while reconfiguring IR and IOAPIC.
> >>
> >> Please convert this error (in kvm_arch_fixup_msi_route) into a trace
> >> point. It shall not annoy the host. Also check if you have more of such
> >> guest-triggerable error messages.
> > 
> > Okay. This should be the only one. I can use trace instead.
> > 
> > Meanwhile, I still suppose we should not seen it even with
> > error_report().. Would this happen when boot e.g. generic kernels?
> 
> No, this is - most probably, still need to validate in details - an
> effect due to the special case with Jailhouse that interrupt and VT-d
> management is handed over from a running OS to a hypervisor.
> 
> Jan
> 
> PS: Current quick hack for EIM support (lacks compat mode blocking at
> least):
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 1082ab5..709a92c 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -907,6 +907,7 @@ static void 
> vtd_interrupt_remap_table_setup(IntelIOMMUState *s)
>      value = vtd_get_quad_raw(s, DMAR_IRTA_REG);
>      s->intr_size = 1UL << ((value & VTD_IRTA_SIZE_MASK) + 1);
>      s->intr_root = value & VTD_IRTA_ADDR_MASK;
> +    s->intr_eime = value & VTD_IRTA_EIME;
>  
>      QLIST_FOREACH(notifier, &s->iec_notifiers, list) {
>          if (notifier->iec_notify) {
> @@ -2052,11 +2053,13 @@ static int vtd_remap_irq_get(IntelIOMMUState *iommu, 
> uint16_t index, VTDIrq *irq
>      irq->trigger_mode = irte.trigger_mode;
>      irq->vector = irte.vector;
>      irq->delivery_mode = irte.delivery_mode;
> -    /* Not support EIM yet: please refer to vt-d 9.10 DST bits */
> +    irq->dest = irte.dest_id;
> +    if (!iommu->intr_eime) {
>  #define  VTD_IR_APIC_DEST_MASK         (0xff00ULL)
>  #define  VTD_IR_APIC_DEST_SHIFT        (8)
> -    irq->dest = (irte.dest_id & VTD_IR_APIC_DEST_MASK) >> \
> -        VTD_IR_APIC_DEST_SHIFT;
> +        irq->dest = (irq->dest & VTD_IR_APIC_DEST_MASK) >>
> +            VTD_IR_APIC_DEST_SHIFT;
> +    }
>      irq->dest_mode = irte.dest_mode;
>      irq->redir_hint = irte.redir_hint;
>  
> @@ -2316,7 +2319,7 @@ static void vtd_init(IntelIOMMUState *s)
>      s->ecap = VTD_ECAP_QI | VTD_ECAP_IRO;
>  
>      if (ms->iommu_intr) {
> -        s->ecap |= VTD_ECAP_IR;
> +        s->ecap |= VTD_ECAP_IR | VTD_ECAP_EIM;
>      }
>  
>      vtd_reset_context_cache(s);
> @@ -2370,10 +2373,9 @@ static void vtd_init(IntelIOMMUState *s)
>      vtd_define_quad(s, DMAR_FRCD_REG_0_2, 0, 0, 0x8000000000000000ULL);
>  
>      /*
> -     * Interrupt remapping registers, not support extended interrupt
> -     * mode for now.
> +     * Interrupt remapping registers.
>       */
> -    vtd_define_quad(s, DMAR_IRTA_REG, 0, 0xfffffffffffff00fULL, 0);
> +    vtd_define_quad(s, DMAR_IRTA_REG, 0, 0xfffffffffffff80fULL, 0);
>  }
>  
>  /* Should not reset address_spaces when reset because devices will still use
> diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> index 10c20fe..72b0114 100644
> --- a/hw/i386/intel_iommu_internal.h
> +++ b/hw/i386/intel_iommu_internal.h
> @@ -176,6 +176,7 @@
>  
>  /* IRTA_REG */
>  #define VTD_IRTA_ADDR_MASK          (VTD_HAW_MASK ^ 0xfffULL)
> +#define VTD_IRTA_EIME               (1ULL << 11)
>  #define VTD_IRTA_SIZE_MASK          (0xfULL)
>  
>  /* ECAP_REG */
> @@ -184,6 +185,7 @@
>  #define VTD_ECAP_QI                 (1ULL << 1)
>  /* Interrupt Remapping support */
>  #define VTD_ECAP_IR                 (1ULL << 3)
> +#define VTD_ECAP_EIM                (1ULL << 4)
>  
>  /* CAP_REG */
>  /* (offset >> 4) << 24 */
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index d7613af..c6c53c6 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -261,6 +261,7 @@ struct IntelIOMMUState {
>      bool intr_enabled;              /* Whether guest enabled IR */
>      dma_addr_t intr_root;           /* Interrupt remapping table pointer */
>      uint32_t intr_size;             /* Number of IR table entries */
> +    bool intr_eime;                 /* Extended interrupt mode enabled */
>      QLIST_HEAD(, VTD_IEC_Notifier) iec_notifiers; /* IEC notify list */
>  };
>  

Currently, all the interrupts will be translated into one MSI in
vtd_generate_msi_message(), in which only 8 bits of dest_id is used
(msg.dest = irq->dest). We may possibly need to use the high 32 bits
of MSI address to store the rest dest[31:8]? Don't know whether this
would be enough though.

Thanks.

-- peterx