Messages in this thread | | | From | Mat King <> | Date | Tue, 17 Dec 2019 13:16:33 -0700 | Subject | Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs |
| |
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. >
| |