Message-ID: <CA+V-a8s3MjvpD8gAE7-mOUc6PEytbPOR6x_PHuw0J0hOLkaz-w@mail.gmail.com>
Date:   Thu, 7 Dec 2023 13:02:00 +0000
From:   "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To:     Conor Dooley <conor@...nel.org>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Conor Dooley <conor.dooley@...rochip.com>,
        geert+renesas@...der.be, Atish Patra <atishp@...osinc.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        apatel@...tanamicro.com, alexghiti@...osinc.com,
        Bjorn Topel <bjorn@...osinc.com>, suagrfillet@...il.com,
        jeeheng.sia@...rfivetech.com, petrtesarik@...weicloud.com,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, prabhakar.mahadev-lad.rj@...renesas.com
Subject: Re: [RFT 1/2] RISC-V: handle missing "no-map" properties for
 OpenSBI's PMP protected regions

On Wed, Dec 6, 2023 at 2:26 PM Conor Dooley <conor@...nel.org> wrote:
>
> On Wed, Dec 06, 2023 at 04:52:11AM -0800, Palmer Dabbelt wrote:
> > On Thu, 10 Aug 2023 02:07:10 PDT (-0700), Conor Dooley wrote:
> > > On Wed, Aug 09, 2023 at 02:01:07AM -0700, Atish Kumar Patra wrote:
> > > > On Tue, Aug 8, 2023 at 6:39 AM Conor Dooley <conor@...nel.org> wrote:
> > > > >
> > > > > On Tue, Aug 08, 2023 at 12:54:11AM -0700, Atish Kumar Patra wrote:
> > > > > > On Wed, Aug 2, 2023 at 4:14 AM Conor Dooley <conor.dooley@...rochip.com> wrote:
> > > > > > >
> > > > > > > Add an erratum for versions [v0.8 to v1.3) of OpenSBI which fail to add
> > > > > > > the "no-map" property to the reserved memory nodes for the regions it
> > > > > > > has protected using PMPs.
> > > > > > >
> > > > > > > Our existing fix sweeping hibernation under the carpet by marking it
> > > > > > > NONPORTABLE is insufficient as there are other ways to generate
> > > > > > > accesses to these reserved memory regions, as Petr discovered [1]
> > > > > > > while testing crash kernels & kdump.
> > > > > > >
> > > > > > > Intercede during the boot process when the afflicted versions of OpenSBI
> > > > > > > are present & set the "no-map" property in all "mmode_resv" nodes before
> > > > > > > the kernel does its reserved memory region initialisation.
> > > > > > >
> > > > > >
> > > > > > We have different mechanisms of DT being passed to the kernel.
> > > > > >
> > > > > > 1. A prior stage(e.g U-Boot SPL) to M-mode runtime firmware (e.g.
> > > > > > OpenSBI, rustSBI) passes the DT to M-mode runtime firmware and it
> > > > > > passes to the next stage.
> > > > > > In this case, M-mode runtime firmware gets a chance to update the
> > > > > > no-map property in DT that the kernel can parse.
> > > > > >
> > > > > > 2. User loads the DT from the boot loader (e.g EDK2, U-Boot proper).
> > > > > > Any DT patching done by the M-mode firmware is useless. If these DTBs
> > > > > > don't have the no-map
> > > > > > property, hibernation or EFI booting will have issues as well.
> > > > > >
> > > > >
> > > > > > We are trying to solve only one part of problem #1 in this patch.
> > > > >
> > > > > Correct.
> > > > >
> > > > > If someone's second stage is also providing an incorrect devicetree
> > > > > then, yeah, this approach would fall apart - but it's the firmware
> > > > > provided devicetree being incorrect that I am trying to account for
> > > > > here. If a person incorrectly constructed one, I am not really sure what
> > > > > we can do for them, they incorrect described their hardware /shrug
> > > > > My patch should of course help in some of the scenarios you mention above
> > > > > if the name of the reserved memory region from OpenSBI is propagated by
> > > > > the second-stage bootloader, but that is just an extension of case 1,
> > > > > not case 2.
> > > > >
> > > > > > I
> > > > > > don't think any other M-mode runtime firmware patches DT with no-map
> > > > > > property as well.
> > > > > > Please let me know if I am wrong about that. The problem is not
> > > > > > restricted to [v0.8 to v1.3) of OpenSBI.
> > > > >
> > > > > It comes down to Alex's question - do we want to fix this kind of
> > > > > firmware issue in the kernel? Ultimately this is a policy decision that
> > > > > "somebody" has to make. Maybe the list of firmwares that need this
> > > >
> > > > IMO, we shouldn't as this is a slippery slope. Kernel can't fix every
> > > > firmware bug by having erratas.
> > > > I agree with your point below about firmware in shipping products. I
> > > > am not aware of any official products shipping anything other than
> > > > OpenSBI either.
> > >
> > > > However, I have seen users using other firmwares in their dev
> > > > environment.
> > >
> > > If someone's already changed their boards firmware, I have less sympathy
> > > for them, as they should be able to make further changes. Punters buying
> > > SBCs to install Fedora or Debian w/o having to consider their firmware
> > > are who I am more interested in helping.
> > >
> > > > IMHO, this approach sets a bad precedent for the future especially
> > > > when it only solves one part of the problem.
> > >
> > > Yeah, I'm certainly wary of setting an unwise precedent here.
> > > Inevitably we will need to have firmware-related errata and it'd be good
> > > to have a policy for what is (or more importantly what isn't
> > > acceptable). Certainly we have said that known-broken version of OpenSBI
> > > that T-Head puts in their SDK is not supported by the mainline kernel.
> > > On the latter part, I'm perfectly happy to expand the erratum to cover
> > > all affected firmwares, but I wasn't even sure if my fix worked
> > > properly, hence the request for testing from those who encountered the
> > > problem.
> > >
> > > > We shouldn't hide firmware bugs in the kernel when an upgraded
> > > > firmware is already available.
> > >
> > > Just to note, availability of an updated firmware upstream does not
> > > necessarily mean that corresponding update is possible for affected
> > > hardware.
> >
> > Yep.  I think we're been in a very hobbist-centric world in RISC-V land, but
> > in general trying to get people to update firmware is hard.  Part of the
> > whole "kernel updates don't break users" thing is what's underneath the
> > kernel, it's not just a uABI thing.
>
> Yeah, there's certainly an attitude that I think needs to go away, that
> updating firmware etc is something we can expect to be carried out on a
> universal basis. Or that fixing things in the upstream version of
> OpenSBI means it'll actually propagate down to system integrators.
>
> >
> > > > This bug is well documented in various threads and fixed in the latest
> > > > version of OpenSBI.
> > > > I am assuming other firmwares will follow it as well.
> > > >
> > > > Anybody facing hibernation or efi related booting issues should just
> > > > upgrade to the latest version of firmware (e.g OpenSBI v1.3)
> > > > Latest version of Qemu will support(if not happened already) the
> > > > latest version of OpenSBI.
> > > >
> > > > This issue will only manifest in kernels 6.4 or higher. Any user
> > > > facing these with the latest kernel can also upgrade the firmware.
> > > > Do you see any issue with that ?
> > >
> > > I don't think it is fair to compare the ease of upgrading the kernel
> > > to that required to upgrade a boards firmware, with the latter being
> > > far, far more inconvenient on pretty much all of the boards that I have.
> >
> > IMO we're in the same spot as every other port here, and generally they work
> > around firmware bugs when they've rolled out into production somewhere that
> > firmware updates aren't likely to happen quickly.  I'm not sure if there's
> > any sort of exact rules written down anywhere, but IMO if the bug is going
> > to impact users then we should deal with it.
> >
> > That applies for hardware bugs, but also firmware bugs (at a certain point
> > we won't be able to tell the difference).  We're sort of doing this with the
> > misaligned access handling, for example.
> >
> > > I'm perfectly happy to drop this series though, if people generally are
> > > of the opinion that this sort of firmware workaround is ill-advised.
> > > We are unaffected by it, so I certainly have no pressure to have
> > > something working here. It's my desire not to be user-hostile that
> > > motivated this patch.
> >
> > IIUC you guys and Reneas are the only ones who have hardware that might be
> > in a spot where users aren't able to update the firmware (ie, it's out in
> > production somewhere).
>
> I dunno if we can really keep thinking like that though. In terms of
> people who have devicetrees in the kernel and stuff available in western
> catalog distribution, sure.
> I don't think we can assume that that covers all users though, certainly
> the syntacore folks pop up every now and then, and I sure hope that
> Andes etc have larger customer bases than the in-kernel users would
> suggest.
>
> > So I'm adding Geert, though he probably saw this
> > months ago...
>
> Prabhakar might be a good call on that front. I'm not sure if the
> Renesas stuff works on affected versions of OpenSBI though, guess it
> depends on the sequencing of the support for the non-coherent stuff and
> when this bug was fixed.
>
ATM, I dont think there are any users who are using the upstream
kernel + OpenSBI (apart from me and Geert!). Currently the customers
are using the BSP releases.

Cheers,
Prabhakar

> > On that note: It's been ~4 months and it look like nobody's tested anything
> > (and the comments aren't really things that would preculde testing).
>
> Yeah, nobody seems to really have given a crap. I was hoping the
> StarFive guys that actually added the support for this would be
> interested in it, but alas they were not.
> I don't really care all that much - the platform I support is not
> affected by the problem and I just don't enable the option elsewhere.
>
> > So
> > maybe we just pick that second patch up into for-next and see what happens?
> > IIUC that will result in broken systems for users who haven't updated their
> > firmware.
> >
> > I agree that's a user-hostile way to do things, which is generally a bad way
> > to go, but if it's really true that there's no users then we're safe.
> > Probably also worth calling it out on sw-dev just to be safe.
>
> And if there are users, the fix is actually relatively straight-forward,
> just apply patch #1.
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists