Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

From: Julius Werner <jwerner@chromium.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Patrick Rudolph <patrick.rudolph@9elements.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Ben Zhang <benzh@chromium.org>,
	Filipe Brandenburger <filbranden@chromium.org>,
	Duncan Laurie <dlaurie@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Samuel Holland <samuel@sholland.org>,
	Julius Werner <jwerner@chromium.org>
Subject: Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition
Date: Wed, 9 Oct 2019 14:19:15 -0700	[thread overview]
Message-ID: <CAODwPW-mfySMQUejCwT+G45BtOysq_JCRQa8GwoYTkjY_yRwgA@mail.gmail.com> (raw)
In-Reply-To: <5d9d120b.1c69fb81.b6201.1477@mx.google.com>

> Somehow we've gotten /sys/firmware/log to be the coreboot log, and quite
> frankly that blows my mind that this path was accepted upstream.
> Userspace has to know it's running on coreboot firmware to know that
> /sys/firmware/log is actually the coreboot log.

Not really sure I understand your concern here? That's the generic
node for the log from the mainboard firmware, whatever it is. It was
originally added for non-coreboot firmware and that use is still
supported. If some other non-coreboot firmware wants to join in, it's
welcome to do so -- the interface is separated out enough to make it
easy to add more backends.

I do agree that if we want to add other, more coreboot-specific nodes,
they should be explicitly namespaced.

> But I also wonder why this is being exposed by the kernel at all?

I share Stephen's concern that I'm not sure this belongs in the kernel
at all. There are existing ways for userspace to access this
information like the cbmem utility does... if you want it accessible
from fwupd, it could chain-call into cbmem or we could factor that
functionality out into a library. If you want to get away from using
/dev/mem for this, we could maybe add a driver that exports CBMEM or
coreboot table areas via sysfs... but then I think that should be a
generic driver which makes them all accessible in one go, rather than
having to add yet another driver whenever someone needs to parse
another coreboot table blob for some reason. We could design an
interface like /sys/firmware/coreboot/table/<tag> where every entry in
the table gets exported as a binary file.

I think a specific sysfs driver only makes sense for things that are
human readable and that you'd actually expect a human to want to go
read directly, like the log. Maybe exporting FMAP entries one by one
like Stephen suggests could be such a case, but I doubt that there's a
common enough need for that since there are plenty of existing ways to
show FMAP entries from userspace (and if there was a generic interface
like /sys/firmware/coreboot/table/37 to access it, we could just add a
new flag to the dump_fmap utility to read it from there).