Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
Quoting Julius Werner (2018-08-09 11:24:29) > One thing to note is that we still want this space to be mappable by > userspace applications via /dev/mem, so we need to make sure that > there's no weird memory type mismatch that causes problems with that. > Adding Aaron to see if he has any concerns here, since I think he's > seen something like that in the past (not sure if it was related to > what this kernel driver does). > > Can you please test this on an x86 Chromebook and run the 'cbmem' > userspace utility, make sure it doesn't fail after this? Sure! > > Also, stupid question after taking a step back and looking at this > again: why do we keep a mapping alive for the lifetime of the driver > at all? It used to be necessary when this driver was > find-entry-on-demand, but nowadays it just goes through all entries > once at probe time and immediately memcpy_fromio()s out all the > relevant information into (struct coreboot_device)s. After that we're > done accessing the "real" coreboot table, forever. Why not just unmap > it again at the end of coreboot_table_init()? Yes, we just copy everything over so it makes it simpler to just unmap at the end of coreboot_table_init(). Then userspace is free to make a questionable mapping into system RAM to find the table itself. I'll make this change.
Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
Quoting Julius Werner (2018-08-09 11:24:29) > One thing to note is that we still want this space to be mappable by > userspace applications via /dev/mem, so we need to make sure that > there's no weird memory type mismatch that causes problems with that. > Adding Aaron to see if he has any concerns here, since I think he's > seen something like that in the past (not sure if it was related to > what this kernel driver does). > > Can you please test this on an x86 Chromebook and run the 'cbmem' > userspace utility, make sure it doesn't fail after this? Sure! > > Also, stupid question after taking a step back and looking at this > again: why do we keep a mapping alive for the lifetime of the driver > at all? It used to be necessary when this driver was > find-entry-on-demand, but nowadays it just goes through all entries > once at probe time and immediately memcpy_fromio()s out all the > relevant information into (struct coreboot_device)s. After that we're > done accessing the "real" coreboot table, forever. Why not just unmap > it again at the end of coreboot_table_init()? Yes, we just copy everything over so it makes it simpler to just unmap at the end of coreboot_table_init(). Then userspace is free to make a questionable mapping into system RAM to find the table itself. I'll make this change.
Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
One thing to note is that we still want this space to be mappable by
userspace applications via /dev/mem, so we need to make sure that
there's no weird memory type mismatch that causes problems with that.
Adding Aaron to see if he has any concerns here, since I think he's
seen something like that in the past (not sure if it was related to
what this kernel driver does).
Can you please test this on an x86 Chromebook and run the 'cbmem'
userspace utility, make sure it doesn't fail after this?
Also, stupid question after taking a step back and looking at this
again: why do we keep a mapping alive for the lifetime of the driver
at all? It used to be necessary when this driver was
find-entry-on-demand, but nowadays it just goes through all entries
once at probe time and immediately memcpy_fromio()s out all the
relevant information into (struct coreboot_device)s. After that we're
done accessing the "real" coreboot table, forever. Why not just unmap
it again at the end of coreboot_table_init()?
On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd wrote:
>
> This is all system memory, so we shouldn't be mapping this all with
> ioremap() as these aren't I/O regions. Instead, they're memory regions
> so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
> RAM directly if that's possible, otherwise it falls back to
> ioremap_cache() like is being done here already. This also nicely
> silences the sparse warnings in this code and reduces the need to copy
> anything around anymore.
>
> Cc: Wei-Ning Huang
> Cc: Julius Werner
> Cc: Brian Norris
> Cc: Samuel Holland
> Signed-off-by: Stephen Boyd
> ---
> drivers/firmware/google/coreboot_table.c | 42 +++-
> 1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c
> b/drivers/firmware/google/coreboot_table.c
> index feb31502f64b..f343dbe86448 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,7 +32,7 @@
> #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
> #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header __iomem *ptr_header;
> +static struct coreboot_table_header *ptr_header;
>
> static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
> {
> @@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver
> *driver)
> }
> EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void *ptr)
> {
> int i, ret;
> void *ptr_entry;
> struct coreboot_device *device;
> - struct coreboot_table_entry entry;
> - struct coreboot_table_header header;
> + struct coreboot_table_entry *entry;
> + struct coreboot_table_header *header;
>
> ptr_header = ptr;
> - memcpy_fromio(, ptr_header, sizeof(header));
> + header = ptr;
>
> - if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
> + if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> pr_warn("coreboot_table: coreboot table missing or
> corrupt!\n");
> return -ENODEV;
> }
> @@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void
> __iomem *ptr)
> if (ret)
> return ret;
>
> - ptr_entry = (void *)ptr_header + header.header_bytes;
> - for (i = 0; i < header.table_entries; i++) {
> - memcpy_fromio(, ptr_entry, sizeof(entry));
> + ptr_entry = ptr_header + header->header_bytes;
> + for (i = 0; i < header->table_entries; i++) {
> + entry = ptr_entry;
>
> - device = kzalloc(sizeof(struct device) + entry.size,
> GFP_KERNEL);
> + device = kzalloc(sizeof(struct device) + entry->size,
> GFP_KERNEL);
> if (!device) {
> ret = -ENOMEM;
> break;
> @@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void
> __iomem *ptr)
> device->dev.parent = dev;
> device->dev.bus = _bus_type;
> device->dev.release = coreboot_device_release;
> - memcpy_fromio(>entry, ptr_entry, entry.size);
> + memcpy(>entry, ptr_entry, entry->size);
>
> ret = device_register(>dev);
> if (ret) {
> @@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void
> __iomem *ptr)
> break;
> }
>
> - ptr_entry += entry.size;
> + ptr_entry += entry->size;
> }
>
> if (ret) {
> bus_unregister(_bus_type);
> - iounmap(ptr);
> + memunmap(ptr);
> }
>
> return ret;
> @@ -149,11 +149,10 @@ static int
Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
One thing to note is that we still want this space to be mappable by
userspace applications via /dev/mem, so we need to make sure that
there's no weird memory type mismatch that causes problems with that.
Adding Aaron to see if he has any concerns here, since I think he's
seen something like that in the past (not sure if it was related to
what this kernel driver does).
Can you please test this on an x86 Chromebook and run the 'cbmem'
userspace utility, make sure it doesn't fail after this?
Also, stupid question after taking a step back and looking at this
again: why do we keep a mapping alive for the lifetime of the driver
at all? It used to be necessary when this driver was
find-entry-on-demand, but nowadays it just goes through all entries
once at probe time and immediately memcpy_fromio()s out all the
relevant information into (struct coreboot_device)s. After that we're
done accessing the "real" coreboot table, forever. Why not just unmap
it again at the end of coreboot_table_init()?
On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd wrote:
>
> This is all system memory, so we shouldn't be mapping this all with
> ioremap() as these aren't I/O regions. Instead, they're memory regions
> so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
> RAM directly if that's possible, otherwise it falls back to
> ioremap_cache() like is being done here already. This also nicely
> silences the sparse warnings in this code and reduces the need to copy
> anything around anymore.
>
> Cc: Wei-Ning Huang
> Cc: Julius Werner
> Cc: Brian Norris
> Cc: Samuel Holland
> Signed-off-by: Stephen Boyd
> ---
> drivers/firmware/google/coreboot_table.c | 42 +++-
> 1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c
> b/drivers/firmware/google/coreboot_table.c
> index feb31502f64b..f343dbe86448 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,7 +32,7 @@
> #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
> #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header __iomem *ptr_header;
> +static struct coreboot_table_header *ptr_header;
>
> static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
> {
> @@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver
> *driver)
> }
> EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void *ptr)
> {
> int i, ret;
> void *ptr_entry;
> struct coreboot_device *device;
> - struct coreboot_table_entry entry;
> - struct coreboot_table_header header;
> + struct coreboot_table_entry *entry;
> + struct coreboot_table_header *header;
>
> ptr_header = ptr;
> - memcpy_fromio(, ptr_header, sizeof(header));
> + header = ptr;
>
> - if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
> + if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> pr_warn("coreboot_table: coreboot table missing or
> corrupt!\n");
> return -ENODEV;
> }
> @@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void
> __iomem *ptr)
> if (ret)
> return ret;
>
> - ptr_entry = (void *)ptr_header + header.header_bytes;
> - for (i = 0; i < header.table_entries; i++) {
> - memcpy_fromio(, ptr_entry, sizeof(entry));
> + ptr_entry = ptr_header + header->header_bytes;
> + for (i = 0; i < header->table_entries; i++) {
> + entry = ptr_entry;
>
> - device = kzalloc(sizeof(struct device) + entry.size,
> GFP_KERNEL);
> + device = kzalloc(sizeof(struct device) + entry->size,
> GFP_KERNEL);
> if (!device) {
> ret = -ENOMEM;
> break;
> @@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void
> __iomem *ptr)
> device->dev.parent = dev;
> device->dev.bus = _bus_type;
> device->dev.release = coreboot_device_release;
> - memcpy_fromio(>entry, ptr_entry, entry.size);
> + memcpy(>entry, ptr_entry, entry->size);
>
> ret = device_register(>dev);
> if (ret) {
> @@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void
> __iomem *ptr)
> break;
> }
>
> - ptr_entry += entry.size;
> + ptr_entry += entry->size;
> }
>
> if (ret) {
> bus_unregister(_bus_type);
> - iounmap(ptr);
> + memunmap(ptr);
> }
>
> return ret;
> @@ -149,11 +149,10 @@ static int
[PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
This is all system memory, so we shouldn't be mapping this all with
ioremap() as these aren't I/O regions. Instead, they're memory regions
so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
RAM directly if that's possible, otherwise it falls back to
ioremap_cache() like is being done here already. This also nicely
silences the sparse warnings in this code and reduces the need to copy
anything around anymore.
Cc: Wei-Ning Huang
Cc: Julius Werner
Cc: Brian Norris
Cc: Samuel Holland
Signed-off-by: Stephen Boyd
---
drivers/firmware/google/coreboot_table.c | 42 +++-
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/drivers/firmware/google/coreboot_table.c
b/drivers/firmware/google/coreboot_table.c
index feb31502f64b..f343dbe86448 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -32,7 +32,7 @@
#define CB_DEV(d) container_of(d, struct coreboot_device, dev)
#define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
-static struct coreboot_table_header __iomem *ptr_header;
+static struct coreboot_table_header *ptr_header;
static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
{
@@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver
*driver)
}
EXPORT_SYMBOL(coreboot_driver_unregister);
-static int coreboot_table_init(struct device *dev, void __iomem *ptr)
+static int coreboot_table_init(struct device *dev, void *ptr)
{
int i, ret;
void *ptr_entry;
struct coreboot_device *device;
- struct coreboot_table_entry entry;
- struct coreboot_table_header header;
+ struct coreboot_table_entry *entry;
+ struct coreboot_table_header *header;
ptr_header = ptr;
- memcpy_fromio(, ptr_header, sizeof(header));
+ header = ptr;
- if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
+ if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
return -ENODEV;
}
@@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
if (ret)
return ret;
- ptr_entry = (void *)ptr_header + header.header_bytes;
- for (i = 0; i < header.table_entries; i++) {
- memcpy_fromio(, ptr_entry, sizeof(entry));
+ ptr_entry = ptr_header + header->header_bytes;
+ for (i = 0; i < header->table_entries; i++) {
+ entry = ptr_entry;
- device = kzalloc(sizeof(struct device) + entry.size,
GFP_KERNEL);
+ device = kzalloc(sizeof(struct device) + entry->size,
GFP_KERNEL);
if (!device) {
ret = -ENOMEM;
break;
@@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
device->dev.parent = dev;
device->dev.bus = _bus_type;
device->dev.release = coreboot_device_release;
- memcpy_fromio(>entry, ptr_entry, entry.size);
+ memcpy(>entry, ptr_entry, entry->size);
ret = device_register(>dev);
if (ret) {
@@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
break;
}
- ptr_entry += entry.size;
+ ptr_entry += entry->size;
}
if (ret) {
bus_unregister(_bus_type);
- iounmap(ptr);
+ memunmap(ptr);
}
return ret;
@@ -149,11 +149,10 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
static int coreboot_table_probe(struct platform_device *pdev)
{
- phys_addr_t phyaddr;
resource_size_t len;
- struct coreboot_table_header __iomem *header = NULL;
+ struct coreboot_table_header *header;
struct resource *res;
- void __iomem *ptr = NULL;
+ void *ptr;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
@@ -163,14 +162,13 @@ static int coreboot_table_probe(struct platform_device
*pdev)
if (!res->start || !len)
return -EINVAL;
- phyaddr = res->start;
- header = ioremap_cache(phyaddr, sizeof(*header));
+ header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
if (header == NULL)
return -ENOMEM;
- ptr = ioremap_cache(phyaddr,
- header->header_bytes + header->table_bytes);
- iounmap(header);
+ ptr = memremap(res->start, header->header_bytes + header->table_bytes,
+ MEMREMAP_WB);
+ memunmap(header);
if (!ptr)
return -ENOMEM;
@@ -181,7 +179,7 @@ static int coreboot_table_remove(struct platform_device
*pdev)
{
if
[PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
This is all system memory, so we shouldn't be mapping this all with
ioremap() as these aren't I/O regions. Instead, they're memory regions
so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
RAM directly if that's possible, otherwise it falls back to
ioremap_cache() like is being done here already. This also nicely
silences the sparse warnings in this code and reduces the need to copy
anything around anymore.
Cc: Wei-Ning Huang
Cc: Julius Werner
Cc: Brian Norris
Cc: Samuel Holland
Signed-off-by: Stephen Boyd
---
drivers/firmware/google/coreboot_table.c | 42 +++-
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/drivers/firmware/google/coreboot_table.c
b/drivers/firmware/google/coreboot_table.c
index feb31502f64b..f343dbe86448 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -32,7 +32,7 @@
#define CB_DEV(d) container_of(d, struct coreboot_device, dev)
#define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
-static struct coreboot_table_header __iomem *ptr_header;
+static struct coreboot_table_header *ptr_header;
static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
{
@@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver
*driver)
}
EXPORT_SYMBOL(coreboot_driver_unregister);
-static int coreboot_table_init(struct device *dev, void __iomem *ptr)
+static int coreboot_table_init(struct device *dev, void *ptr)
{
int i, ret;
void *ptr_entry;
struct coreboot_device *device;
- struct coreboot_table_entry entry;
- struct coreboot_table_header header;
+ struct coreboot_table_entry *entry;
+ struct coreboot_table_header *header;
ptr_header = ptr;
- memcpy_fromio(, ptr_header, sizeof(header));
+ header = ptr;
- if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
+ if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
return -ENODEV;
}
@@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
if (ret)
return ret;
- ptr_entry = (void *)ptr_header + header.header_bytes;
- for (i = 0; i < header.table_entries; i++) {
- memcpy_fromio(, ptr_entry, sizeof(entry));
+ ptr_entry = ptr_header + header->header_bytes;
+ for (i = 0; i < header->table_entries; i++) {
+ entry = ptr_entry;
- device = kzalloc(sizeof(struct device) + entry.size,
GFP_KERNEL);
+ device = kzalloc(sizeof(struct device) + entry->size,
GFP_KERNEL);
if (!device) {
ret = -ENOMEM;
break;
@@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
device->dev.parent = dev;
device->dev.bus = _bus_type;
device->dev.release = coreboot_device_release;
- memcpy_fromio(>entry, ptr_entry, entry.size);
+ memcpy(>entry, ptr_entry, entry->size);
ret = device_register(>dev);
if (ret) {
@@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
break;
}
- ptr_entry += entry.size;
+ ptr_entry += entry->size;
}
if (ret) {
bus_unregister(_bus_type);
- iounmap(ptr);
+ memunmap(ptr);
}
return ret;
@@ -149,11 +149,10 @@ static int coreboot_table_init(struct device *dev, void
__iomem *ptr)
static int coreboot_table_probe(struct platform_device *pdev)
{
- phys_addr_t phyaddr;
resource_size_t len;
- struct coreboot_table_header __iomem *header = NULL;
+ struct coreboot_table_header *header;
struct resource *res;
- void __iomem *ptr = NULL;
+ void *ptr;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
@@ -163,14 +162,13 @@ static int coreboot_table_probe(struct platform_device
*pdev)
if (!res->start || !len)
return -EINVAL;
- phyaddr = res->start;
- header = ioremap_cache(phyaddr, sizeof(*header));
+ header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
if (header == NULL)
return -ENOMEM;
- ptr = ioremap_cache(phyaddr,
- header->header_bytes + header->table_bytes);
- iounmap(header);
+ ptr = memremap(res->start, header->header_bytes + header->table_bytes,
+ MEMREMAP_WB);
+ memunmap(header);
if (!ptr)
return -ENOMEM;
@@ -181,7 +179,7 @@ static int coreboot_table_remove(struct platform_device
*pdev)
{
if
