[PATCH] ramoops appears geared to not support ARM

[Bryan Freed]

I had some difficulty in getting ramoops to work on our ARM systems.
The driver maps memory with ioremap() which is supposed to map IO memory,
not physical RAM. This happens to work on x86 and apparently some other
architectures, but it does not work on ARM.
Specifically, I see this comment in __arm_ioremap_pfn_caller():
Don't allow RAM to be mapped - this causes problems with ARMv6+

So here is a patch that hacks around the issue using page_is_ram() to
differentiate between the two.

Am I missing something here?
Is ramoops working on any ARM systems yet, and I am just doing something wrong?

My ARM platform reserves a section of RAM for use by ramoops, but it is still
mapped along with the rest of main memory. This is so /dev/mem can find it
with xlate_dev_mem_ptr().
On x86, I see our BIOS reserves the memory so that it is not counted as main
memory, and it is not mapped until ramoops ioremaps it.

Bryan Freed (1):
ramoops: Add support for ARM systems.

drivers/char/ramoops.c | 67 +++++++++++++++++++++++++++++++++++++----------
1 files changed, 52 insertions(+), 15 deletions(-)

--
1.7.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Bryan Freed]

The ramoops driver erroneously (I belive) uses ioremap() to map physical RAM
to a virtual address. This happens to work on x86 which has this additional
support, but it does not work on ARM.

Add support for ARM by using xlate_dev_mem_ptr() for systems (like ARM) where
page_is_ram() returns true. This is what the /dev/mem driver uses, and we use
that driver to access the data.

Signed-off-by: Bryan Freed <bfr...@chromium.org>
---

drivers/char/ramoops.c | 67 +++++++++++++++++++++++++++++++++++++----------
1 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/drivers/char/ramoops.c b/drivers/char/ramoops.c
index 810aff9..7c50309 100644
--- a/drivers/char/ramoops.c
+++ b/drivers/char/ramoops.c
@@ -31,6 +31,7 @@
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/ramoops.h>
+#include <linux/mm.h>

#define RAMOOPS_KERNMSG_HDR "===="
#define MIN_MEM_SIZE 4096UL
@@ -113,6 +114,53 @@ static void ramoops_do_dump(struct kmsg_dumper *dumper,
cxt->count = (cxt->count + 1) % cxt->max_count;
}

+void __weak unxlate_dev_mem_ptr(unsigned long phys, void *addr)
+{
+}
+
+/*
+ * For systems that recognize the region as RAM (eg, ARM), the
+ * region is already reserved and mapped. Just xlate it here
+ * as /dev/mem does it in drivers/char/mem.c.
+ *
+ * For systems that do not recognize the region as RAM (eg, x86),
+ * reserve and map the region here so /dev/mem can xlate it.
+ */
+static int map_context(struct ramoops_context *cxt)
+{
+ if (page_is_ram(cxt->phys_addr << PAGE_SHIFT)) {
+ cxt->virt_addr = xlate_dev_mem_ptr(cxt->phys_addr);
+ if (!cxt->virt_addr) {
+ pr_err("xlate_dev_mem_ptr failed\n");
+ return -EINVAL;
+ }
+ } else {
+ if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
+ pr_err("request mem region failed\n");
+ return -EINVAL;
+ }
+
+ cxt->virt_addr = ioremap(cxt->phys_addr, cxt->size);
+ if (!cxt->virt_addr) {
+ pr_err("ioremap failed\n");
+ release_mem_region(cxt->phys_addr, cxt->size);
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
+static void unmap_context(struct ramoops_context *cxt)
+{
+ if (page_is_ram(cxt->phys_addr << PAGE_SHIFT)) {
+ unxlate_dev_mem_ptr(cxt->phys_addr, cxt->virt_addr);
+ } else {
+ iounmap(cxt->virt_addr);
+ release_mem_region(cxt->phys_addr, cxt->size);
+ }
+}
+
static int __init ramoops_probe(struct platform_device *pdev)
{
struct ramoops_platform_data *pdata = pdev->dev.platform_data;
@@ -156,17 +204,9 @@ static int __init ramoops_probe(struct platform_device *pdev)
record_size = pdata->record_size;
dump_oops = pdata->dump_oops;

- if (!request_mem_region(cxt->phys_addr, cxt->size, "ramoops")) {
- pr_err("request mem region failed\n");
- err = -EINVAL;
+ err = map_context(cxt);
+ if (err)
goto fail3;
- }
-
- cxt->virt_addr = ioremap(cxt->phys_addr, cxt->size);
- if (!cxt->virt_addr) {
- pr_err("ioremap failed\n");
- goto fail2;
- }

cxt->dump.dump = ramoops_do_dump;
err = kmsg_dump_register(&cxt->dump);
@@ -178,9 +218,7 @@ static int __init ramoops_probe(struct platform_device *pdev)
return 0;

fail1:
- iounmap(cxt->virt_addr);
-fail2:
- release_mem_region(cxt->phys_addr, cxt->size);
+ unmap_context(cxt);
fail3:
return err;
}
@@ -192,8 +230,7 @@ static int __exit ramoops_remove(struct platform_device *pdev)
if (kmsg_dump_unregister(&cxt->dump) < 0)
pr_warn("could not unregister kmsg_dumper\n");

- iounmap(cxt->virt_addr);
- release_mem_region(cxt->phys_addr, cxt->size);
+ unmap_context(cxt);
return 0;

[Marco Stornelli]

(Added linux-arm)

Il 29/10/2011 01:21, Bryan Freed ha scritto:
> I had some difficulty in getting ramoops to work on our ARM systems.
> The driver maps memory with ioremap() which is supposed to map IO memory,
> not physical RAM. This happens to work on x86 and apparently some other
> architectures, but it does not work on ARM.
> Specifically, I see this comment in __arm_ioremap_pfn_caller():
> Don't allow RAM to be mapped - this causes problems with ARMv6+
>
> So here is a patch that hacks around the issue using page_is_ram() to
> differentiate between the two.
>
> Am I missing something here?
> Is ramoops working on any ARM systems yet, and I am just doing something wrong?
>
> My ARM platform reserves a section of RAM for use by ramoops, but it is still
> mapped along with the rest of main memory. This is so /dev/mem can find it
> with xlate_dev_mem_ptr().
> On x86, I see our BIOS reserves the memory so that it is not counted as main
> memory, and it is not mapped until ramoops ioremaps it.
>
> Bryan Freed (1):
> ramoops: Add support for ARM systems.
>
> drivers/char/ramoops.c | 67 +++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 52 insertions(+), 15 deletions(-)
>

Can some ARM guys give an opinion about that?

Thanks.

Marco

[Russell King - ARM Linux]

On Sat, Oct 29, 2011 at 10:39:47AM +0200, Marco Stornelli wrote:
> (Added linux-arm)
>
> Il 29/10/2011 01:21, Bryan Freed ha scritto:
>> I had some difficulty in getting ramoops to work on our ARM systems.
>> The driver maps memory with ioremap() which is supposed to map IO memory,
>> not physical RAM. This happens to work on x86 and apparently some other
>> architectures, but it does not work on ARM.
>> Specifically, I see this comment in __arm_ioremap_pfn_caller():
>> Don't allow RAM to be mapped - this causes problems with ARMv6+
>>
>> So here is a patch that hacks around the issue using page_is_ram() to
>> differentiate between the two.
>>
>> Am I missing something here?
>> Is ramoops working on any ARM systems yet, and I am just doing something wrong?
>>
>> My ARM platform reserves a section of RAM for use by ramoops, but it is still
>> mapped along with the rest of main memory. This is so /dev/mem can find it
>> with xlate_dev_mem_ptr().
>> On x86, I see our BIOS reserves the memory so that it is not counted as main
>> memory, and it is not mapped until ramoops ioremaps it.
>>
>> Bryan Freed (1):
>> ramoops: Add support for ARM systems.
>>
>> drivers/char/ramoops.c | 67 +++++++++++++++++++++++++++++++++++++----------
>> 1 files changed, 52 insertions(+), 15 deletions(-)
>>
>
> Can some ARM guys give an opinion about that?

Opinion about the patch which isn't present in this email (so we can't
see it) or about the commentry above?

[Marco Stornelli]

About the ioremap problem. It seems there is a problem on some ARM arch
to use ioremap (ramoops driver) to remap a piece of RAM even if it's not
used by kernel (reserved at boot with mem option, Bryan can you
confirm?). It has been suggested by Bryan to use xlate_dev_mem_ptr(),
but I'm not sure if the problem is how to reserve the memory on these
ARM archs. I believe ioremap is a general way to do it, of course not
using kernel already used by kernel, am I right? Or to be compliant with
all archs (in this particular case with ARMv6+) we should use
xlate_dev_mem_ptr()?

Marco

[Russell King - ARM Linux]

On Sat, Oct 29, 2011 at 01:04:30PM +0200, Marco Stornelli wrote:
> About the ioremap problem. It seems there is a problem on some ARM arch
> to use ioremap (ramoops driver) to remap a piece of RAM even if it's not
> used by kernel (reserved at boot with mem option, Bryan can you
> confirm?).

It's all very simple.

We have three major 'memory types' - 'normal memory' which must be used
for things like RAM that we execute code from and use atomic operations
within. This can be prefetched and reordered at will.

'device memory' is for devices, which tighter restrictions on reordering
and guarantees concerning reads and writes.

'strongly ordered memory' is much like device memory.

It is absolutely not permitted to map the same physical addresses with
different types - this is a stronger requirement than getting the cache
attributes the same.

System memory is mapped using 'normal memory' - obviously because we need
to be able to execute code and have working atomic operations throughout
kernel memory.

Now, ioremap creates device memory - because its main function is to
dynamically map memory regions in devices.

Now think: if we have system memory mapped as 'normal memory', and then
we try to use ioremap() to remap some of that memory, that will create
a new 'device memory' mapping with the existing 'normal memory' mapping
still present. Now look at the paragraph 'It is absolutely not permitted'
and realise that the requirements for the architecture are being violated
if we permitted this to occur.

Also realise that if that condition is violated, 'unpredictable behaviour'
will occur - not to the extent that the CPU will hang, but it could cause
data errors which could influence overall system stability.

So, the whole idea of using plain ioremap() with system memory is one
that is just completely unsupportable on ARM without _first_ removing
memory from the system mapping, which in turn means updating the page
tables for every task in the system.

[Marco Stornelli]

Ok, I understand, but other question: isn't there any way to reserve
normal memory? Or at least, hasn't the mem kernel option any effect from
that point of view?

[Russell King - ARM Linux]

On Sat, Oct 29, 2011 at 02:42:42PM +0200, Marco Stornelli wrote:
> Ok, I understand, but other question: isn't there any way to reserve
> normal memory? Or at least, hasn't the mem kernel option any effect from
> that point of view?

mem= can be used to redefine the available memory to the kernel, but
then you have to have some way to define a region of the memory you've
excluded to the kernel to use for ramoops.

A platform can also use the memblock stuff to extract some memory
dynamically from the information passed from the firmware - by using
memblock_alloc + memblock_remove + memblock_free. The memblock_remove
call will take the memory out of the available memory to the kernel,
and thus will prevent it being mapped.

This must be done via the ->reserve callback in the ARM board/SoC
record (it can't be done later because the memory will have already
been mapped.)

[Marco Stornelli]

Il 29/10/2011 01:21, Bryan Freed ha scritto:

> I had some difficulty in getting ramoops to work on our ARM systems.
> The driver maps memory with ioremap() which is supposed to map IO memory,
> not physical RAM. This happens to work on x86 and apparently some other
> architectures, but it does not work on ARM.
> Specifically, I see this comment in __arm_ioremap_pfn_caller():
> Don't allow RAM to be mapped - this causes problems with ARMv6+
>
> So here is a patch that hacks around the issue using page_is_ram() to
> differentiate between the two.
>
> Am I missing something here?
> Is ramoops working on any ARM systems yet, and I am just doing something wrong?
>
> My ARM platform reserves a section of RAM for use by ramoops, but it is still
> mapped along with the rest of main memory. This is so /dev/mem can find it
> with xlate_dev_mem_ptr().
> On x86, I see our BIOS reserves the memory so that it is not counted as main
> memory, and it is not mapped until ramoops ioremaps it.
>
> Bryan Freed (1):
> ramoops: Add support for ARM systems.
>
> drivers/char/ramoops.c | 67 +++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 52 insertions(+), 15 deletions(-)
>

Bryan,

I think the right thing to do here, it's to reserve in right way the
piece of RAM you use according to your arch (see the Russell response).

Marco

[Marco Stornelli]

Il 30/10/2011 03:07, Bryan Freed ha scritto:
> Right, and that is what I do to get ARM working. The reserve() function
> calls memblock_reserve() to reserve the memory for RAMOOPS. Keeping it
> part of main memory (by not using memblock_remove()) gets the memory
> properly mapped.
>

According to Russell, it needs to use memblock_remove to exclude that
piece of memory.

> The problem I think we need to resolve is that this makes the ramoops
> driver messy.

I agree. Indeed I think we don't need to do anything in the driver. The
problem is only how to exclude a piece of memory from kernel main memory
view. For x86 it's trivial, for ARM it doesn't, but it's still possible.

[Bryan Freed]

On Sun, Oct 30, 2011 at 4:33 AM, Marco Stornelli
<marco.s...@gmail.com> wrote:
>
> Il 30/10/2011 03:07, Bryan Freed ha scritto:
>>
>> Right, and that is what I do to get ARM working.  The reserve() function
>> calls memblock_reserve() to reserve the memory for RAMOOPS.  Keeping it
>> part of main memory (by not using memblock_remove()) gets the memory
>> properly mapped.
>>
>
> According to Russell, it needs to use memblock_remove to exclude that piece of memory.
>
>> The problem I think we need to resolve is that this makes the ramoops
>> driver messy.
>
> I agree. Indeed I think we don't need to do anything in the driver. The problem is only how to exclude a piece of memory from kernel main memory view. For x86 it's trivial, for ARM it doesn't, but it's still possible.
>
> Marco

I will give that (using mem_remove()) a shot tomorrow.
My recollection on using it in last week's investigation showed the
ramoops driver ioremap() giving a mapping that did not correspond with
what the /dev/mem expected to see.  I vaguely recall /dev/mem was
effectively calling __va(0x02000000) which added a base address of
0xc0000000 giving 0xc2000000 as the resulting virtual address.  The
ramoops ioremap(), however, gave some arbitrary virtual address for
this memory.
The result was that using /dev/mem to read 0x02000000 caused a panic.

Another problem was that removing just 512KiB of memory for ramoops
screwed up the system memory initialization.  It appeared to me that
the ARM memory code expected sections to be 1MiB aligned. I could
memblock_reserve anything, but I could only memblock_remove on a 1MiB
boundary.

And I cannot shake the feeling that we have a fairly simple disconnect
here.  Ramoops expects to use _device_ memory because it uses
ioremap().  But the buffer itself is accessed through /dev/mem which
(as we use it with no mmap() calls) expects to give access to _system_
memory.
It seems this disconnect could be removed if we provided a
/sys/class/ramoops/buffer file that an application could read instead
of having to rely on /dev/mem.

Marco, what is your opinion on that?

bryan.

[Marco Stornelli]

Il 31/10/2011 07:03, Bryan Freed ha scritto:
> On Sun, Oct 30, 2011 at 4:33 AM, Marco Stornelli
> <marco.s...@gmail.com> wrote:
>>
>> Il 30/10/2011 03:07, Bryan Freed ha scritto:
>>>
>>> Right, and that is what I do to get ARM working. The reserve() function
>>> calls memblock_reserve() to reserve the memory for RAMOOPS. Keeping it
>>> part of main memory (by not using memblock_remove()) gets the memory
>>> properly mapped.
>>>
>>
>> According to Russell, it needs to use memblock_remove to exclude that piece of memory.
>>
>>> The problem I think we need to resolve is that this makes the ramoops
>>> driver messy.
>>
>> I agree. Indeed I think we don't need to do anything in the driver. The problem is only how to exclude a piece of memory from kernel main memory view. For x86 it's trivial, for ARM it doesn't, but it's still possible.
>>
>> Marco
>
> I will give that (using mem_remove()) a shot tomorrow.
> My recollection on using it in last week's investigation showed the
> ramoops driver ioremap() giving a mapping that did not correspond with
> what the /dev/mem expected to see. I vaguely recall /dev/mem was
> effectively calling __va(0x02000000) which added a base address of
> 0xc0000000 giving 0xc2000000 as the resulting virtual address. The
> ramoops ioremap(), however, gave some arbitrary virtual address for
> this memory.
> The result was that using /dev/mem to read 0x02000000 caused a panic.

I don't understand this point. We have different virtual addresses and
then? The linear transformation with a fixed offset is the normal way
for the kernel to have a virtual address from a physical one when you
are using a memory address directly mapped. The virtual address returned
by ioremap it can be an address not directly mapped, I mean it isn't a
simple linear transformation of the physical address used in input.
It isn't normal to have a kernel panic, even if there is something
wrong, you should receive EINVAL or something similar.

>
> Another problem was that removing just 512KiB of memory for ramoops
> screwed up the system memory initialization. It appeared to me that
> the ARM memory code expected sections to be 1MiB aligned. I could
> memblock_reserve anything, but I could only memblock_remove on a 1MiB
> boundary.
>

It's an arch problem not related to the driver.

> And I cannot shake the feeling that we have a fairly simple disconnect
> here. Ramoops expects to use _device_ memory because it uses
> ioremap(). But the buffer itself is accessed through /dev/mem which
> (as we use it with no mmap() calls) expects to give access to _system_

no mmap calls?! I don't understand how you are using /dev/mem.

Marco

[Bryan Freed]

On Mon, Oct 31, 2011 at 1:57 AM, Marco Stornelli

Yes, we get different virtual addresses.
The /dev/mem xlate_dev_mem_ptr() is defined (for ARM) as __va(), so it
gives 0xc2000000.
The ramoops ioremap() gives 0xef90000 because
__arm_ioremap_pfn_caller() makes a fresh get_vm_area_caller() call.

On x86, the /dev/mem xlate_dev_mem_ptr() gives an ioremap_cache()
address rather than a __va() address because it (properly?) detects
the memory is not RAM (because it was reserved by BIOS, so it was not
configured with system memory).

> The linear transformation with a fixed offset is the normal way for the
> kernel to have a virtual address from a physical one when you are using a
> memory address directly mapped. The virtual address returned by ioremap it
> can be an address not directly mapped, I mean it isn't a simple linear
> transformation of the physical address used in input.

Right, that is what I see on the ARM side as well.
__arm_ioremap_pfn_caller() gets a virtual address with
get_vm_area_caller().

> It isn't normal to have a kernel panic, even if there is something wrong,
> you should receive EINVAL or something similar.

Yeah, /dev/mem will give an EFAULT on ARM if (addr + count >
__pa(high_memory)). But we are not beyond the end of our physical
memory. We are somewhere in the middle. So read_mem() gets the
virtual address with xlate_dev_mem_ptr() which on ARM is just __va().
If this memory is memblock_removed, then that virtual address is not mapped.

Now that I look closer, we use copy_to_user(). So we should be
catching this fault.
I will take a closer look at this.
Either way, /dev/mem will not give us proper access to the ARM ramoops buffer.

>
>>
>> Another problem was that removing just 512KiB of memory for ramoops
>> screwed up the system memory initialization.  It appeared to me that
>> the ARM memory code expected sections to be 1MiB aligned.  I could
>> memblock_reserve anything, but I could only memblock_remove on a 1MiB
>> boundary.
>>
>
> It's an arch problem not related to the driver.

Yes, that is an arch problem. But it is another reason to move away
from memblock_remove() and toward memblock_reserve().

>> And I cannot shake the feeling that we have a fairly simple disconnect
>> here.  Ramoops expects to use _device_ memory because it uses
>> ioremap().  But the buffer itself is accessed through /dev/mem which
>> (as we use it with no mmap() calls) expects to give access to _system_
>
> no mmap calls?! I don't understand how you are using /dev/mem.

open(), lseek(), read(). No mmap is required for RAM, right?
dd if=/dev/mem bs=1 count=1000 skip=32M

My point above about the disconnect between system memory and device
memory would be resolved if ramoops provided its own access to the
buffer. What do you think of that idea?

bryan.

[Marco Stornelli]

Il 01/11/2011 00:03, Bryan Freed ha scritto:
> On Mon, Oct 31, 2011 at 1:57 AM, Marco Stornelli

>>> And I cannot shake the feeling that we have a fairly simple disconnect
>>> here. Ramoops expects to use _device_ memory because it uses
>>> ioremap(). But the buffer itself is accessed through /dev/mem which
>>> (as we use it with no mmap() calls) expects to give access to _system_
>>
>> no mmap calls?! I don't understand how you are using /dev/mem.
>
> open(), lseek(), read(). No mmap is required for RAM, right?
> dd if=/dev/mem bs=1 count=1000 skip=32M
>

Mmmm, the operations done are different. Try: reserve the memory with
memblock_reserve and read some data with this useful program
http://free-electrons.com/pub/mirror/devmem2.c from the right location
(the address used for ramoops).

Let me know.