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

[patrick . rudolph]

On Mon, 2019-12-09 at 22:54 -0800, Julius Werner 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 wasn't aware that CBMEM is used for DMA as there's no such concept in
coreboot yet. For me it looks like the UCSI is regular DRAM mapped as
WB accessed by the ACPI interpreter.
I'll prepare a new patch-set using ioremap in all drivers that access
CBMEM.