Re: [PATCH 4.19 070/118] iommu/arm-smmu-v3: Dont disable SMMU in kdump kernel

From: Will Deacon
Date: Sun Jun 16 2019 - 16:11:52 EST


[FYI: This was in my spam folder. I'll reserve judgement on whether that's
the right decision.]

On Sun, Jun 16, 2019 at 09:42:36PM +0200, Pavel Machek wrote:
> > [ Upstream commit 3f54c447df34ff9efac7809a4a80fd3208efc619 ]
> >
> > Disabling the SMMU when probing from within a kdump kernel so that all
> > incoming transactions are terminated can prevent the core of the crashed
> > kernel from being transferred off the machine if all I/O devices are
> > behind the SMMU.
> >
> > Instead, continue to probe the SMMU after it is disabled so that we can
> > reinitialise it entirely and re-attach the DMA masters as they are reset.
> > Since the kdump kernel may not have drivers for all of the active DMA
> > masters, we suppress fault reporting to avoid spamming the console and
> > swamping the IRQ threads.
>
> > +++ b/drivers/iommu/arm-smmu-v3.c
> > @@ -2414,13 +2414,9 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> > /* Clear CR0 and sync (disables SMMU and queue processing) */
> > reg = readl_relaxed(smmu->base + ARM_SMMU_CR0);
> > if (reg & CR0_SMMUEN) {
> > - if (is_kdump_kernel()) {
> > - arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > - arm_smmu_device_disable(smmu);
> > - return -EBUSY;
> > - }
> > -
> > dev_warn(smmu->dev, "SMMU currently enabled! Resetting...\n");
> > + WARN_ON(is_kdump_kernel() && !disable_bypass);
> > + arm_smmu_update_gbpa(smmu, GBPA_ABORT, 0);
> > }
> >
>
> This changes behaviour in !is_kdump_kernel() case. Is that
> ok/intended?

Yes, that's intentional. If we find the SMMU in an enabled state, it's
probably a good idea to configure it to abort all transactions before
disabling it, otherwise virtual addresses suddenly become physical addresses
and we could corrupt random memory. However, I don't think I've ever seen
this happen outside of kdump so it's admittedly a bit of a theoretical
scenario.

Regardless, patches to -stable should probably match their upstream
counterparts so even if this was an issue, I don't think this is the right
place to discuss it.

Will