Re: [Xen-devel] [PATCH] arm, smmu: backport "Disable stalling faults for all endpoints"

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/15/18 12:03 AM, Stefano Stabellini wrote:
> > Backport commit 3714ce1d6655098ee69ede632883e5874d67e4ab
> > "iommu/arm-smmu: Disable stalling faults for all endpoints" from the
> > Linux kernel.
> > 
> > Original commit message:
> > 
> >    Enabling stalling faults can result in hardware deadlock on poorly
> >    designed systems, particularly those with a PCI root complex upstream of
> >    the SMMU.
> > 
> >    Although it's not really Linux's job to save hardware integrators from
> >    their own misfortune, it *is* our job to stop userspace (e.g. VFIO
> >    clients) from hosing the system for everybody else, even if they might
> >    already be required to have elevated privileges.
> > 
> >    Given that the fault handling code currently executes entirely in IRQ
> >    context, there is nothing that can sensibly be done to recover from
> >    things like page faults anyway, so let's rip this code out for now and
> >    avoid the potential for deadlock.
> > 
> >    Cc: <stable@xxxxxxxxxxxxxxx>
> >    Fixes: 48ec83bcbcf5 ("iommu/arm-smmu: Add initial driver support for ARM
> > SMMUv3 devices")
> >    Reported-by: Matt Evans <matt.evans@xxxxxxx>
> >    Signed-off-by: Will Deacon <will.deacon@xxxxxxx>
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> 
> This is actually an errata on some SMMUv2 implementation. Can you update
> docs/misc/arm/silicon-errata.txt accordingly?

OK


> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > index b510399..8de5d16 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -898,8 +898,7 @@ static void arm_smmu_tlb_inv_context(struct
> > arm_smmu_domain *smmu_domain)
> >     static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> >   {
> > -   int flags, ret;
> > -   u32 fsr, far, fsynr, resume;
> > +   u32 fsr, far, fsynr;
> >     unsigned long iova;
> >     struct iommu_domain *domain = dev;
> >     struct arm_smmu_domain *smmu_domain = domain->priv;
> > @@ -913,13 +912,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void
> > *dev)
> >     if (!(fsr & FSR_FAULT))
> >             return IRQ_NONE;
> >   - if (fsr & FSR_IGN)
> > -           dev_err_ratelimited(smmu->dev,
> > -                               "Unexpected context fault (fsr 0x%x)\n",
> > -                               fsr);
> > -
> >     fsynr = readl_relaxed(cb_base + ARM_SMMU_CB_FSYNR0);
> > -   flags = fsynr & FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ;
> >             far = readl_relaxed(cb_base + ARM_SMMU_CB_FAR_LO);
> >     iova = far;
> > @@ -928,25 +921,12 @@ static irqreturn_t arm_smmu_context_fault(int irq,
> > void *dev)
> >     iova |= ((unsigned long)far << 32);
> >   #endif
> >   - if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
> > -           ret = IRQ_HANDLED;
> > -           resume = RESUME_RETRY;
> > -   } else {
> > -           dev_err_ratelimited(smmu->dev,
> > -               "Unhandled context fault: iova=0x%08lx, fsynr=0x%x,
> > cb=%d\n",
> > -               iova, fsynr, cfg->cbndx);
> > -           ret = IRQ_NONE;
> > -           resume = RESUME_TERMINATE;
> > -   }
> > -
> > -   /* Clear the faulting FSR */
> > +   dev_err_ratelimited(smmu->dev,
> > +   "Unhandled context fault: fsr=0x%x, iova=0x%08lx, fsynr=0x%x,
> > cb=%d\n",
> > +                       fsr, iova, fsynr, cfg->cbndx);
> > +
> >     writel(fsr, cb_base + ARM_SMMU_CB_FSR);
> > -
> > -   /* Retry or terminate any stalled transactions */
> > -   if (fsr & FSR_SS)
> > -           writel_relaxed(resume, cb_base + ARM_SMMU_CB_RESUME);
> > -
> > -   return ret;
> > +   return IRQ_HANDLED;
> >   }
> >     static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> > @@ -1181,7 +1161,7 @@ static void arm_smmu_init_context_bank(struct
> > arm_smmu_domain *smmu_domain)
> >     }
> >             /* SCTLR */
> > -   reg = SCTLR_CFCFG | SCTLR_CFIE | SCTLR_CFRE | SCTLR_M |
> > SCTLR_EAE_SBOP;
> > +   reg = SCTLR_CFIE | SCTLR_CFRE | SCTLR_M | SCTLR_EAE_SBOP;
> 
> It would also document here SCTLR_CFIE is not set because of erratum #... This
> would avoid us to turn it on again by mistake.

OK


> >     if (stage1)
> >             reg |= SCTLR_S1_ASIDPNE;
> >   #ifdef __BIG_ENDIAN
> > 
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel