Date:   Thu, 10 Oct 2019 19:03:38 -0500
From:   Samuel Holland <samuel@...lland.org>
To:     Julius Werner <jwerner@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Patrick Rudolph <patrick.rudolph@...ements.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ben Zhang <benzh@...omium.org>,
        Filipe Brandenburger <filbranden@...omium.org>,
        Duncan Laurie <dlaurie@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition

On 10/9/19 4:19 PM, Julius Werner wrote:
>> 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)
There's already a node in sysfs for every coreboot table entry:

  /sys/bus/coreboot/devices/corebootN

where N is the index of the entry in the coreboot table. I suggest

1) Rename the devices based on their tag instead of their position in the table,
   so the names are stable and meaningful. I don't know why I didn't do that in
   the first place. Doing that gives you a device kobject you can hang
   additional sysfs attributes off of.
2) If we want userspace to have access to the raw binary table entry (which
   sounds like a good idea to me), add that sysfs attribute in
   coreboot_table_populate() after creating each device. That way it would be
   present regardless of whether or not there's a specific driver loaded for
   that table entry.

This solves the immediate problem of needing /dev/mem to access coreboot tables,
while leaving open the possibility of writing kernel drivers in the future if
that would be more beneficial than parsing the data in userspace (i.e. if the
kernel can do something more with the data than just exporting it).

Samuel

Powered by blists - more mailing lists