Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs

[Mat King <mathewk@google.com>]

On Tue, Dec 17, 2019 at 12:02 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Mat King (2019-12-13 13:31:46)
> > On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@chromium.org> wrote:
> > > > +static int cbmem_probe(struct coreboot_device *cdev)
> > > > +{
> > > > +       struct device *dev = &cdev->dev;
> > > > +       struct cb_priv *priv;
> > > > +       int err;
> > > > +
> > > > +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > +       if (!priv)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       memcpy(&priv->entry, &cdev->cbmem_entry, sizeof(priv->entry));
> > > > +
> > > > +       priv->remap = memremap(priv->entry.address,
> > > > +                              priv->entry.entry_size, MEMREMAP_WB);
> > >
> > > We've just been discussing some problems with CBMEM areas and memory
> > > mapping types in Chrome OS. CBMEM is not guaranteed to be page-aligned
> > > (at least not the "small" entries), but the kernel can only assign
> > > memory attributes for a page at a time (and refuses to map the same
> > > area twice with two different memory types, for good reason). So if
> > > CBMEM entries sharing a page are mapped as writeback by one driver but
> > > uncached by the other, things break.
> > >
> > > There are some CBMEM entries that need to be mapped uncached (e.g. the
> > > ACPI UCSI table, which isn't even handled by anything using this CBMEM
> > > code) and others for which it would make more sense (e.g. the memory
> > > console, where firmware may add more lines at runtime), but I don't
> > > think there are any regions that really *need* to be writeback. None
> > > of the stuff accessing these areas should access them often enough
> > > that caching matters, and I think it's generally more common to map
> > > firmware memory areas as uncached anyway. So how about we standardize
> > > on mapping it all uncached to avoid any attribute clashes? (That would
> > > mean changing the existing VPD and memconsole drivers to use
> > > ioremap(), too.)
> >
> > I don't think that uncached would work here either because the acpi
> > driver will have already mapped some of these regions as write-back
> > before this driver is loaded so the mapping will fail.
>
> Presumably the ucsi driver is drivers/usb/typec/ucsi/ucsi_acpi.c? Is
> that right? And on ACPI based systems is this I/O memory or just some
> carved out memory region that is used to communicate something to the
> ACPI firmware? From looking at the ucsi driver it seems like it should
> be mapped with memremap() instead of ioremap() given that it's not
> actual I/O memory that has any sort of memory barrier or access width
> constraints. It looks more like some sort of memory region that is being
> copied into and out of while triggering some DSM. Can it at least be
> memremap()ed with MEMREMAP_WT?

Yes this is the ucsi_acpi.c driver that has caused this issue to come
up. It does just use a region of memory carved in the BIOS out for the
purpose of this device. The kernel can write to this memory and call a
_DSM to push data to an EC or call the _DSM to pull from the EC into
this memory region. See
https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/bios-implementation-of-ucsi.pdf
. The driver is very explicit about using uncached memory and I
suspect that is why memremap() was not used, but I am not sure why
uncahed memory is needed. The only consumers of this memory are the
driver itself and the ACPI asl code in the _DSM which as far as I know
is being exectued by the kernel directly. Are there any other reasons
to use uncached memory when dealing with ACPI asl code?

>
> It also looks like this sort of problem has come up before, see commit
> 1f9f9d168ce6 ("usb: typec: ucsi: acpi: Workaround for cache mode
> issue"). Ugh.
>
> >
> > Perhaps the best way to make this work is to not call memremap at all
> > in the probe function but instead call memremap and memunmap every
> > time that data_read is called. memremap can take multiple caching mode
> > flags and will try each until one succeeds. So if you call it with
> > MEMREMAP_WB | MEMREMAP_WT in data_read it should succeed no matter how
> > it has been mapped by other drivers which should already be loaded
> > when it is called.
>
> I've been wanting to change memremap() to be a "just give me memory"
> type of API but haven't finished that task. It got hung up on mapping
> memory specially for UEFI framebuffers to match whatever the UEFI mmap
> table tells us.
>