Re: [PATCH] iommu/arm-smmu: Report USF more clearly

[Will Deacon <will@kernel.org>]

On Fri, Sep 13, 2019 at 03:44:12PM -0700, Doug Anderson wrote:
> On Fri, Sep 13, 2019 at 4:48 AM Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > Although CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT is a welcome tool
> > for smoking out inadequate firmware, the failure mode is non-obvious
> > and can be confusing for end users. Add some special-case reporting of
> > Unidentified Stream Faults to help clarify this particular symptom.
> >
> > CC: Douglas Anderson <dianders@chromium.org>
> 
> nit that I believe that "Cc" (lowercase 2nd c) is correct.
> 
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >  drivers/iommu/arm-smmu.c | 5 +++++
> >  drivers/iommu/arm-smmu.h | 2 ++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index b7cf24402a94..76ac8c180695 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -499,6 +499,11 @@ static irqreturn_t arm_smmu_global_fault(int irq, void *dev)
> >         dev_err_ratelimited(smmu->dev,
> >                 "\tGFSR 0x%08x, GFSYNR0 0x%08x, GFSYNR1 0x%08x, GFSYNR2 0x%08x\n",
> >                 gfsr, gfsynr0, gfsynr1, gfsynr2);
> > +       if (IS_ENABLED(CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT) &&
> > +           (gfsr & sGFSR_USF))
> > +               dev_err_ratelimited(smmu->dev,
> > +                       "Stream ID %hu may not be described by firmware, try booting with \"arm-smmu.disable_bypass=0\"\n",
> > +                       (u16)gfsynr1);
> 
> In general it seems like a sane idea to surface an error like this.  I
> guess a few nits:
> 
> 1. "By firmware" might be a bit misleading.  In most cases I'm aware
> of the problem is in the device tree that was bundled together with
> the kernel.  If there are actually cases where firmware has baked in a
> device tree and it got this wrong then we might want to spend time
> figuring out what to do about it.

I thought that was usually the way UEFI systems worked, where the kernel
is updated independently of the device-tree? Either way, that should be
what we're aiming for, even if many platforms require the two to be tied
together.

> 2. Presumably booting with "arm-smmu.disable_bypass=0" is in most
> cases the least desirable option available.  I always consider kernel
> command line parameters as something of a last resort for
> configuration and would only be something that and end user might do
> if they were given a kernel compiled by someone else (like if someone
> where taking a prebuilt Linux distro and trying to install it onto a
> generic PC).  Are you seeing cases where this is happening?  If people
> are compiling their own kernel I'd argue that telling them to set
> "CONFIG_ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT" to "no" is much better
> than trying to jam a command line option on.  Command line options
> don't scale well.

Hmm. Recompiling seems like even more of a last resort to me!

> 3. Any chance you could make it more obvious that this change is
> undesirable and a last resort?  AKA:
> 
> "Stream ID x blocked for security reasons; allow anyway by booting
> with arm-smmu.disable_bypass=0"

How about:

  "Blocked transaction from unknown Stream ID x; boot with
   \"arm-smmu.disable_bypass=0\" to allow these transactions, although this
   may have security implications."

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel