Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191128125100.14291-1-patrick.rudolph@9elements.com> <20191128125100.14291-2-patrick.rudolph@9elements.com>
In-Reply-To: <20191128125100.14291-2-patrick.rudolph@9elements.com>
From:   Julius Werner <jwerner@chromium.org>
Date:   Mon, 9 Dec 2019 22:54:23 -0800
Message-ID: <CAODwPW8Koy1BvKGJU6PKexYx+PNE+WY7+m69gcxT689vBy+AoQ@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] firmware: google: Expose CBMEM over sysfs
To:     Patrick Rudolph <patrick.rudolph@9elements.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Allison Randal <allison@lohutok.net>,
        Alexios Zavras <alexios.zavras@intel.com>,
        Stephen Boyd <swboyd@chromium.org>,
        Samuel Holland <samuel@sholland.org>,
        Julius Werner <jwerner@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> +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.)