WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

RE: [Xen-devel] [PATCH 5 of 7] IOMMU VTD BUG: disable Extended Interrupt

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

<Prev in Thread] Current Thread [Next in Thread>