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

[Mat King]

On Mon, Dec 9, 2019 at 11:57 PM Julius Werner <jwerner@xxxxxxxxxxxx> 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.

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.