Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <YuuBrUmiRYoaSmPw@google.com> <20220804142856.306032-1-jrosenth@chromium.org>
 <CAE-0n51UcauP1ojoskV9_2+CFPoJrs08jGHVPCZdSA_oXX1zUw@mail.gmail.com>
 <CAODwPW_mYQ1gcd2Xw5s+7dL2kLcYn-VTxKHUH1iM1V4mn7pT_w@mail.gmail.com>
 <CAE-0n504q_NeB9e2BYF8P9Scp7TPqsp9KEWu2DO=ZzbYdJfyew@mail.gmail.com>
 <CAODwPW-LaaD+ptch=R-S4GWoBDjTgWzfvwjyQeBRETZnnosR7g@mail.gmail.com> <CAE-0n50cVxFhOU7ULfs8KWkhji8-P57ffzC8sAUGFoYzdhnV5w@mail.gmail.com>
In-Reply-To: <CAE-0n50cVxFhOU7ULfs8KWkhji8-P57ffzC8sAUGFoYzdhnV5w@mail.gmail.com>
From:   Julius Werner <jwerner@chromium.org>
Date:   Wed, 10 Aug 2022 18:17:15 -0700
Message-ID: <CAODwPW9PBAdgnL3vuXXFG1hAG6nCFDgA7w8EYbx7UWU6A4X-=g@mail.gmail.com>
Subject: Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Julius Werner <jwerner@chromium.org>,
        Jack Rosenthal <jrosenth@chromium.org>,
        chrome-platform@lists.linux.dev,
        LKML <linux-kernel@vger.kernel.org>,
        Tzung-Bi Shih <tzungbi@kernel.org>,
        Guenter Roeck <groeck@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> Got it. Thanks for the background. Is it possible to create new entries
> in the table? Or to resize existing entries? Or to delete entries
> entirely?

Not easily (would have to see if there's still space at the end and
rewrite the table header), and more importantly there should be no
reason to ever do that at OS runtime. This table is only meant for
coreboot to publish information about itself or store data that needs
to stay resident for whatever reason. Userspace should be able to
access the things that are already there but it isn't meant as a
free-for-all for other environments to add on to.

> The /dev/mem interface has been restricted over the years. At this point
> we're trying to steer users away from /dev/mem to anything else. I
> suspect it happens to work right now because coreboot tells the kernel
> that there isn't actually memory in this address range so that devmem
> can map it. I don't know but I wonder if the memory is being mapped
> uncached on ARM systems, leading to slower access times? Usually when
> memory addresses aren't marked as normal memory that's reserved it
> doesn't get mapped until the memremap() time, and that would be mapped
> with whatever attributes are used in the call. /dev/mem doesn't optimize
> this from what I recall.

Yes, we mark those areas as reserved in the e820 / device tree. The
kernel drivers (this one and the older ones) use MEMREMAP_WB which
should do the right thing. `cbmem` uses mmap(MAP_SHARED) on /dev/mem
which I thought does the right thing but I'm not quite sure. That's
another good reason to implement a dedicated sysfs interface where we
have finer control about these things, once we have that we can make
the older tools use it as well on supporting kernels. (`cbmem` is
currently not called in any critical boot path on Chrome OS as far as
I know, so its performance is not that critical. The new use case Jack
wants to build _is_ going to be in a critical path, though, so we
should make sure it is as performant as it can be.)

> Fair enough. How similar is this to efivars? I don't know, and you may
> not either, but at a high level it looks similar. The sysfs interface to
> efivars was deprecated and I saw recently that there's an effort to
> remove it entirely. The new way of interacting with those firmware
> variables is through a filesystem that's mounted at
> /sys/firmware/efi/efivars. The documentation[1] states that the sysfs
> interface didn't work when the variables got large. Hopefully that won't
> be a similar scenario here?

I don't even know what efivars is tbh. But whatever other cases with
other firmwares may exist that may or may not be more standardized and
make more sense to implement high-touch drivers for in kernel space,
here we really have something that's really opaque and really not
meant to be tampered with by external code, so I think this agnostic
byte buffer access is the only thing that makes sense. I don't see any
reason why there would be size restrictions with the implementation
Jack proposed? FWIW, the total size of the CBMEM buffer we want to
access is 80K on current builds of coreboot (but our use case will
likely only need to read the first couple hundred bytes).