Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5da779b4.1c69fb81.2d904.a23f@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAODwPW-x1fwGSrvLNWCU4GAfGbD0zqo2HLm+33D8eUtxbnFLCg@mail.gmail.com>
References: <20191008115342.28483-1-patrick.rudolph@9elements.com> <20191008115342.28483-2-patrick.rudolph@9elements.com> <5d9d120b.1c69fb81.b6201.1477@mx.google.com> <CAODwPW-mfySMQUejCwT+G45BtOysq_JCRQa8GwoYTkjY_yRwgA@mail.gmail.com> <6cfca8c34ccd51f12b4418e9a74d8961e32077ed.camel@9elements.com> <5d9f3b9f.1c69fb81.62109.325d@mx.google.com> <CAODwPW-x1fwGSrvLNWCU4GAfGbD0zqo2HLm+33D8eUtxbnFLCg@mail.gmail.com>
From:   Stephen Boyd <swboyd@chromium.org>
To:     Julius Werner <jwerner@chromium.org>
Cc:     Julius Werner <jwerner@chromium.org>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ben Zhang <benzh@chromium.org>,
        Duncan Laurie <dlaurie@chromium.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Samuel Holland <samuel@sholland.org>
Subject: Re: [PATCH 2/2] firmware: coreboot: Export active CBFS partition
User-Agent: alot/0.8.1
Date:   Wed, 16 Oct 2019 13:12:35 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Julius Werner (2019-10-10 11:37:58)
> > > I'll expose the coreboot tables using a sysfs driver, which then can =
be
> > > used by coreboot tools instead of accessing /dev/mem. As it holds the
> > > FMAP and "boot media params" that's all I need for now.
> > >
> > > The downside is that the userspace tools need to be keep in sync with
> > > the binary interface the coreboot tables are providing.
>=20
> Well, in the other version the kernel needs to be kept in sync
> instead. No matter where you do the parsing, something needs to be
> kept in sync. I think userspace would be easier, especially if we
> would host a small userspace library in the coreboot repository that
> other tools could just link.
>=20
> > I'd rather we export this information in sysfs via some coreboot_fmap
> > class and then make the "boot media params" another property of one of
> > the fmap devices. Then userspace can search through all the fmap devices
> > and find the "boot media params" one. Is anything else needed?
>=20
> Okay, this is the fundamental question we need to answer first... do
> you really think it's better to add a separate interface for each of
> these, Stephen? The coreboot table[1] currently contains 49 entries
> with all sorts of random firmware information, and most of these will
> never be interesting to the kernel. Do we want to establish a pattern
> where we add a new sysfs interface for each of them whenever someone
> has a use case for reading it from userspace? I think this might be a
> good point to implement a generic interface to read any coreboot table
> entry from userspace instead, and say that if the kernel doesn't need
> the information in a specific entry itself, it shouldn't need to know
> how to parse it.

I don't know why we need to draw a line in the sand and say that if the
kernel doesn't need to know about it then it shouldn't parse it. I want
there to be a consistent userspace ABI that doesn't just move things
straight from memory to userspace in some binary format. I'd rather we
have an ABI that decodes and exposes information about the coreboot
tables through existing frameworks/subsystems where possible and makes
up new ones otherwise.

One concern I have is endianness of the binary data. Is it big endian or
little endian or CPU native endian? The kernel can boot into big or
little endian on ARM platforms and userspace can be different vs. the
bootloader too. Userspace shouldn't need to know this detail, the kernel
should know and do the conversions and expose it somehow. That's why I'm
suggesting in this case we describe fmap as a sysfs class. I don't see
how we could export that information otherwise, besides in a binary blob
that falls into traps like this.

Right now we make devices for all the coreboot table entries, which is
pretty weird considering that some table entries are things like
LB_TAG_LINKER. That isn't a device. It's some information about how
coreboot was linked. We should probably blacklist tags so we don't make
devices and capture these ones in the bus code and expose them in
/sys/firware/coreboot/ somehow. We should also add device randomness
from the serial numbers, etc. that coreboot has stashed away in the
tables.

>=20
> [1] https://review.coreboot.org/cgit/coreboot.git/tree/src/commonlib/incl=
ude/commonlib/coreboot_tables.h