RE: [Xen-devel] [PATCH 5 of 7] IOMMU VTD BUG: disable Extended Interrupt Mode when disabling Interupt Remapping

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2011-06-15 at 07:48 +0100, Jan Beulich wrote:
> >>> On 14.06.11 at 23:20, "Kay, Allen M" <allen.m.kay@xxxxxxxxx> wrote:
> >>  Let's see what our friends at Intel think...
> > 
> > IOMMU_WAIT_OP() has a timeout value of 1 second.  When it times out, it 
> > will 
> > cause a system panic.
> 
> Sort of bad when we're already bringing down the system possibly
> because of a panic.

Right. I think we could do with an __IOMMU_WAIT_OP (if one doesn't
already exist) which just waits for the timeout and returns. There's no
harm in trying to continue with the kexec if it fails IMHO.

Ian.

> 
> Jan
> 
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx] 
> > Sent: Tuesday, June 14, 2011 2:02 AM
> > To: Andrew Cooper
> > Cc: Kay, Allen M; xen-devel@xxxxxxxxxxxxxxxxxxx 
> > Subject: Re: [Xen-devel] [PATCH 5 of 7] IOMMU VTD BUG: disable Extended 
> > Interrupt Mode when disabling Interupt Remapping
> > 
> >>>> On 13.06.11 at 19:02, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> >> Experimental evidence shows that Extended Interrupt Mode remains in
> >> effect even after Interrupt Remapping is disabled in each DMAR Global
> >> Command Register.  A consiquence of this is that when we switch from
> >> x2apic mode back to xapic mode, and disable interrupt remapping for
> >> the kdump kernel, interrupts passing through the IO APICs are in
> >> x2apic format as opposed xapic.  This causes a triple fault in the
> >> kexec kernel.
> >> 
> >> As EIM is explicitly set up each time Interrup Remapping is enabled,
> >> it is safe for us to clobber this when taring down.
> >> 
> >> Also, change the header definition of IRTA_REG_EIME_SHIFT.  It caused
> >> verbose and error-prone code, and was only used in 1 place before.  We
> >> now have IRTA_EIME which is the specific bit in the register.
> >> 
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> 
> >> diff -r 615db56671fa -r 2635774346c4 xen/drivers/passthrough/vtd/intremap.c
> >> --- a/xen/drivers/passthrough/vtd/intremap.c       Mon Jun 13 17:45:43 
> >> 2011 +0100
> >> +++ b/xen/drivers/passthrough/vtd/intremap.c       Mon Jun 13 17:45:43 
> >> 2011 +0100
> >> @@ -776,8 +776,7 @@ int enable_intremap(struct iommu *iommu,
> >>  
> >>  #ifdef CONFIG_X86
> >>      /* set extended interrupt mode bit */
> >> -    ir_ctrl->iremap_maddr |=
> >> -            eim ? (1 << IRTA_REG_EIME_SHIFT) : 0;
> >> +    ir_ctrl->iremap_maddr |= eim ? IRTA_EIME : 0;
> >>  #endif
> >>      spin_lock_irqsave(&iommu->register_lock, flags);
> >>  
> >> @@ -817,6 +816,22 @@ void disable_intremap(struct iommu *iomm
> >>      if ( !ecap_intr_remap(iommu->ecap) )
> >>          return;
> >>  
> >> +    /* If we are disabling Interrupt Remapping, make sure we dont stay in
> >> +     * Extended Interrupt Mode, as this is unaffected by the Interrupt 
> >> +     * Remapping flag in each DMAR Global Control Register.
> >> +     * Specifically, local apics in xapic mode do not like interrupts 
> >> delivered
> >> +     * in x2apic mode.  Any code turning interrupt remapping back on will 
> >> set
> >> +     * EIME back correctly.
> >> +     */
> >> +    if ( iommu_supports_eim() )
> >> +    {
> >> +        u64 irta;
> >> +        irta = dmar_readl(iommu->reg, DMAR_IRTA_REG);
> >> +        dmar_writel(iommu->reg, DMAR_IRTA_REG, irta & ~IRTA_EIME);
> >> +        IOMMU_WAIT_OP(iommu, DMAR_IRTA_REG, dmar_readl,
> >> +                      !(irta & IRTA_EIME), irta);
> > 
> > Hmm, this of course can be problematic in the context of a crash:
> > I realize that you want it cleared, but does system state guarantee
> > that this WAIT_OP will always be able to complete? You'd get a
> > hung system instead if it won't. Admittedly it's not clear which one
> > is better, but a best effort approach would probably skip the wait
> > loop. Let's see what our friends at Intel think...
> > 
> > Jan
> > 
> >> +    }
> >> +
> >>      spin_lock_irqsave(&iommu->register_lock, flags);
> >>      sts = dmar_readl(iommu->reg, DMAR_GSTS_REG);
> >>      if ( !(sts & DMA_GSTS_IRES) )
> >> diff -r 615db56671fa -r 2635774346c4 xen/drivers/passthrough/vtd/iommu.h
> >> --- a/xen/drivers/passthrough/vtd/iommu.h  Mon Jun 13 17:45:43 2011 +0100
> >> +++ b/xen/drivers/passthrough/vtd/iommu.h  Mon Jun 13 17:45:43 2011 +0100
> >> @@ -471,7 +471,7 @@ struct qinval_entry {
> >>  
> >>  #define IEC_GLOBAL_INVL         0
> >>  #define IEC_INDEX_INVL          1
> >> -#define IRTA_REG_EIME_SHIFT     11
> >> +#define IRTA_EIME               (1 << 11)
> >>  
> >>  /* 2^(IRTA_REG_TABLE_SIZE + 1) = IREMAP_ENTRY_NR */
> >>  #define IRTA_REG_TABLE_SIZE     ( IREMAP_PAGE_ORDER + 7 )
> >> 
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@xxxxxxxxxxxxxxxxxxx 
> >> http://lists.xensource.com/xen-devel 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel