Message-ID: <CAE-0n504q_NeB9e2BYF8P9Scp7TPqsp9KEWu2DO=ZzbYdJfyew@mail.gmail.com>
Date:   Wed, 10 Aug 2022 11:40:34 -0500
From:   Stephen Boyd <swboyd@...omium.org>
To:     Julius Werner <jwerner@...omium.org>
Cc:     Jack Rosenthal <jrosenth@...omium.org>,
        chrome-platform@...ts.linux.dev,
        LKML <linux-kernel@...r.kernel.org>,
        Tzung-Bi Shih <tzungbi@...nel.org>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH v7] firmware: google: Implement cbmem in sysfs driver

Quoting Julius Werner (2022-08-04 16:14:43)
> > Quoting Jack Rosenthal (2022-08-04 07:28:56)
> > > cbmem entries can be read from coreboot table
> > > 0x31 (LB_TAG_CBMEM_ENTRY).  This commit exports access to cbmem
> > > entries in sysfs under /sys/firmware/coreboot/cbmem-*.
> > >
> > > Link: https://issuetracker.google.com/239604743
> >
> > Is what you intend to use from here essentially an nvram? If so, it may
> > make more sense to expose just that entry in drivers/nvmem/ as a
> > read-only sort of nvmem.
>
> No, it is not NV. It's just a normal memory buffer allocated and
> filled by firmware on boot and placed in a region of normal DRAM
> that's marked as reserved.

Alright cool. The bug says 'vbnv' so I was guessing 'nv' meant
non-volatile.

>
> > It isn't clear to me what the structure of this path is. I'd expect to
> > see an entry for each 'address', 'size', 'id', 'mem' with a different
> > What/Date/Contact/Description. If those attributes are all within a
> > directory in sysfs then there could be a top-level description for that
> > as well.
>
> CBMEM buffers are used by coreboot for all sorts of things and we
> regularly define new ones. We can't maintain a full list of all IDs in
> kernel sources because it would be a ton of churn for no reason --
> best we could do is to add a link to the ID list in the coreboot repo
> (e.g. https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h).

Agreed, that's a good approach.

> We really only care about one of these right now and many of them will
> never be relevant to the kernel, but I still thought that while we're
> doing this we might as well provide a generic interface to all of them
> because others may become useful in the future (and then you don't
> have to update the kernel every time you get a new use case for some
> userspace tool wanting to interact with some resident data structure
> from coreboot).

Exposing more than is necessary in the kernel ABI could get into
problems with backwards compatibility. It also means we have to review
the ABI with the consideration that something may change in the future
and cbmem would be exposing something we don't want exposed, or maybe it
is writeable when it shouldn't be?

Furthermore, if the ABI that the kernel can expose already exists then
we're better off because there may already be userspace programs written
for that ABI with lessons learned and bugs ironed out. In this case, I
was hoping that it was an nvmem, in which case we could tie it into the
nvmem framework and piggyback on the nvmem APIs. It also helps to expose
a more generic ABI to userspace instead of developing a bespoke solution
requiring boutique software. Of course sometimes things can't be so
generic so code has to be written.

>
> > Do you need to be able to write cbmem entries?
>
> Yes, see the use case in the bug (using the vboot workbuffer from
> coreboot as a write-through cache for the vboot nvdata on flash).

Is it a cached non-volatile memory?

Powered by blists - more mailing lists