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

[Julien Grall <julien.grall@xxxxxxx>]

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?
> 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.
Cheers,

>         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