linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
       [not found] <49D17E5D.30306@gmx.net>
@ 2009-03-31 15:58 ` Grant Erickson
  0 siblings, 0 replies; 11+ messages in thread
From: Grant Erickson @ 2009-03-31 15:58 UTC (permalink / raw)
  To: Carl-Daniel Hailfinger
  Cc: Andrew Morton, David Miller, Timo Juhani Lindfors, Wolfgang Denk,
	linux-kernel

On 3/30/09 7:22 PM, Carl-Daniel Hailfinger wrote:
> On 21.01.2009 18:39, Grant Erickson wrote:
>> This merges support for the previously DENX-only kernel feature of
>> specifying an alternative, "external" buffer for kernel printk
>> messages and their associated metadata. In addition, this ports
>> architecture support for this feature from arch/ppc to arch/powerpc.
>> 
>> When this option is enabled, an architecture- or machine-specific log
>> buffer is used for all printk messages. This allows entities such as
>> boot loaders (e.g. U-Boot) to place printk-compatible messages into
>> this buffer and for the kernel to coalesce them with its normal
>> messages.
>>   
> 
> What is your current status for this patch? I'd like to make sure the
> implementation will not be incompatible with the coreboot log buffer.

Carl-Daniel:

Unfortunately, the project with which the patch was associated has since
wrapped up and I have not had the cycles in the intervening period to
follow-up "mainlining" the patch.

Philosophically, my perspective, based on the ensuing RFC dialog, is that my
preferred tack would be something akin to a log buffer driver model. For
99.99% of the cases, the standard would be the generic log buffer driver we
all know and use today.

However, under the driver model, also available would be the u-boot
read/write log buffer driver, the read-only
slurp-up-the-firmware-log-and-append driver proposed by Andrew, whatever
David proposes for Sparc, perhaps something slightly different for Coreboot,
etc. Some of these may/may not support ALL the options the generic driver
supports (e.g. resizing through a kernel parameter).

Compatibility on the back end among all these is a laudable goal; however,
given the varying requirements of the embedded space in which these variant
drivers are inevitably targeted, it seems unreasonable to expect they'll all
converge into a "one true log buffer driver".

So long as the front-end driver API is compatible with the current generic
driver, printk, klogd, etc., the kernel configurator is free to select the
driver that makes the most sense for his/her board/application/etc.

So, that's as far as I got with the philosophy. My next step would have been
creating drivers/log, moving the generic driver pieces there from
kernel/printk, establishing a u-boot driver as a representative variant,
roll in Andrew's feedback, etc.

Regards,

Grant



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
@ 2009-02-18 21:27 Timo Juhani Lindfors
  0 siblings, 0 replies; 11+ messages in thread
From: Timo Juhani Lindfors @ 2009-02-18 21:27 UTC (permalink / raw)
  To: linux-kernel; +Cc: Grant Erickson

Hi,

we have been looking for a way to preserve panic messages over reboot
on the openmoko neo freerunner phone so you might find useful ideas
from

http://docs.openmoko.org/trac/ticket/2135

I wrote a 55-line patch (before knowing about your patch) that does
what I need but I am not a kernel developer so it is not of very high
quality. I hope your more general patch will get to the mainline.

best regards,
Timo Lindfors

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-31  9:59     ` Wolfgang Denk
@ 2009-02-03  1:08       ` Carl-Daniel Hailfinger
  0 siblings, 0 replies; 11+ messages in thread
From: Carl-Daniel Hailfinger @ 2009-02-03  1:08 UTC (permalink / raw)
  To: Wolfgang Denk; +Cc: Grant Erickson, Andrew Morton, linux-kernel

On 31.01.2009 10:59, Wolfgang Denk wrote:
> Dear Grant & Andrew,
>
> In message <C5A8B8CD.143B5%gerickson@nuovations.com> you wrote:
>   
>>> We have an external log buffer, into which stuff was written by (say)
>>> the boot prom code.
>>>       
>
> Correct. Typically we use this in U-Boot to pass Power-On Self Test
> results to applications running under Linux, using the standard syslog
> tools.
>
> Another are of use is to save the log buffer over a reset (crash)  of
> the  system,  so  that  it can be read either rom the boot loader or,
> after a reboot, from Linux.
>   

That part is not yet implemented in coreboot, but it certainly is useful.


>>> We want that to come out on the console during bootup.
>>>       
>
> Not necessarily. The main function is to feed it into the syslog
> processing.
>   

Indeed.


>>> So what we do is to divert the normal printk log buffer so that it
>>> starts writing at the tail of that existing external buffer.  We emit
>>> the entire buffer some time during bootup.  We henceforth continue to
>>> use that external buffer in the normal manner. If so, why do it this way?
>>>       
>
> Do you have a better approach how to share  the  log  buffer  between
> Linux  and  a boot loader in such a way that the content will survive
> crahses and reboots?
>
>
>   
>> I believe this to be a useful feature for the embedded system space;
>> however, I am not wedded in any way to the current implementation and am
>> happy to evolve it as needed to meet the needs of key stakeholders.
>>     
>
> Indeed we use this heavily in embedded systems, mainly to process POST
> results under Linux (for example, to disable certain functions when
> the POST results indicate hardware issues, etc.).
>   

For coreboot, the feature is used to avoid long delays for serial output
and to make the coreboot POST logs available on machines where you don't
want to or can't store the POST logs. Coreboot POST logs at maximum
verbosity are roughly 61kB large on my machine and may be bigger on
others. Default verbosity still has a few dozen lines in the log.


>>> Why not just copy the external buffer's contents into the normal buffer during
>>> bootup?  ie: printk("%s", external_buffer).
>>>       
>
> For two reasons:
>
> 1) Additional copy operation (ok, that's cheap).
>   

Indeed. It probably won't take more than a few microseconds if the
external buffer is already in DRAM.


> 2) We would lose the capability that the buffer content even survives 
>    a reset / crash / reboot.
>
>    This is a feature can be very useful in embedded systems where
>    there is (at least normally) nobody looking at any console
>    messages; being able to see the kernel's crash messages after a
>    reboot in the normal syslog messages can be really helpful.
>   

This benefit is a bit debatable on x86 due to the lack of non-volatile
fast memory with decent sizes. (Yes, you can store the log in the flash
ROM, but it will wear out.)
Coreboot can try to preserve RAM contents, but depending on the
particular hardware all bets are off.


>>> I think a quite bad part of this feature is that it renders the
>>> kernel's log_buf_len boot parameter (and its Kconfig defult)
>>> ineffective.  That's a pretty useful feature - people often set it to
>>> 1MB or more during problem diagnosis, and the new unalterable 16kbytes
>>> is very very small.
>>>       
>
> Yes, we are aware of this, but it's a compromise. Basicly it depends
> on hardware capabilities. In most cases, it is sufficient to keep the
> syslog buffer in RAM. But if you really have to guarantee that the
> content of the buffer remains stable, this cannot be used because
> during a processor reset there will be usually a too long pause until
> the memory controller gets re-initialized and RAM refresh continues -
> we might lose data in this time. In most cases it just works fine,
> but it's out of spec. In such situations, when such a guarantee is
> needed, we can use any SRAM that may be available on the board, or we
> can use some on-chip-memory (OCM, basicly also SRAM) that might be on
> the processor.
>
> The OCM on the processors in question is 16 kB, hence the limitation
> there.
>   

I'd like to have the buffer location at least configurable to use that
feature on regular desktop and server x86 machines.


> But note that the code is general enough to support the other options
> (other, bigger SRAM, or arbitrarily sized buffer in RAM), too.
>   

Good.


>> Agreed; however, I'm not sure how often this technique is employed in
>> embedded systems versus desktop systems.
>>     
>
> I haven't seen this ever used in desktop systems yet. And I doubt if
> there is any x86 with a boot loader that supports somehting like this.
>   

Coreboot v3 (free x86 firmware, formerly known as LinuxBIOS) does this.


> But there is for example a pretty large number of mobile cranes all
> over the world which are using this feature :-)
>
>   
>> I'd be happy to keep the existing log buffer and perform a prepend from the
>> boot buffer to it rather than an append of the kernel buffer to the boot
>> buffer. Thoughts?
>>     
>
> Please don't - this would implement only the way to pass the buffer
> from the boot loader too Linux, but not the way back.
>   

It would be great if the location of that buffer could be retrieved
either via ACPI or e820 tables on x86. Hardcoded locations do not make
sense for x86 because coreboot tries to keep memory as unfragmented as
possible and stores the buffer at the top of memory (well, you can
hardcode the buffer location in coreboot but that is just a hack).


Unless I misread the code, it assumes no wraparound has occured in the
firmware log buffer. With full verbosity in coreboot, such wraparounds
can happen.

The coreboot log buffer struct is really simple:
struct printk_buffer {
        int len;
        int readoffset;
        int writeoffset;
        char buffer[];
};

The buffer is implemented as a ring buffer with variable size. The size
is determined from a few runtime variables for minimum RAM waste, so
hardcoding it to some fixed value would be suboptimal.

Can we coordinate to make sure both U-Boot and coreboot can use the feature?


Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-30 22:28     ` Andrew Morton
@ 2009-01-31 10:01       ` Wolfgang Denk
  0 siblings, 0 replies; 11+ messages in thread
From: Wolfgang Denk @ 2009-01-31 10:01 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Grant Erickson, linux-kernel

Dear Andrew,

In message <20090130142825.e63ee797.akpm@linux-foundation.org> you wrote:
> 
> It shouldn't be too hard to cook up some way of copying some data into
> th log buffer before the kernel itself has put anything in there, if
> that's all which is needed.

Agreed, but we also want to get the data back into the shared log
buffer when the system crashes someone presses the reset button. The
log buffer shall survive reboots -that's the basic requirment of the
implementation, and this is what makes it a bit more complicated.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Microsoft Multitasking:
                     several applications can crash at the same time.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-30 22:01   ` Grant Erickson
  2009-01-30 22:28     ` Andrew Morton
@ 2009-01-31  9:59     ` Wolfgang Denk
  2009-02-03  1:08       ` Carl-Daniel Hailfinger
  1 sibling, 1 reply; 11+ messages in thread
From: Wolfgang Denk @ 2009-01-31  9:59 UTC (permalink / raw)
  To: Grant Erickson; +Cc: Andrew Morton, linux-kernel

Dear Grant & Andrew,

In message <C5A8B8CD.143B5%gerickson@nuovations.com> you wrote:
>
> > We have an external log buffer, into which stuff was written by (say)
> > the boot prom code.

Correct. Typically we use this in U-Boot to pass Power-On Self Test
results to applications running under Linux, using the standard syslog
tools.

Another are of use is to save the log buffer over a reset (crash)  of
the  system,  so  that  it can be read either rom the boot loader or,
after a reboot, from Linux.

> > We want that to come out on the console during bootup.

Not necessarily. The main function is to feed it into the syslog
processing.

> > So what we do is to divert the normal printk log buffer so that it
> > starts writing at the tail of that existing external buffer.  We emit
> > the entire buffer some time during bootup.  We henceforth continue to
> > use that external buffer in the normal manner. If so, why do it this way?

Do you have a better approach how to share  the  log  buffer  between
Linux  and  a boot loader in such a way that the content will survive
crahses and reboots?


> I believe this to be a useful feature for the embedded system space;
> however, I am not wedded in any way to the current implementation and am
> happy to evolve it as needed to meet the needs of key stakeholders.

Indeed we use this heavily in embedded systems, mainly to process POST
results under Linux (for example, to disable certain functions when
the POST results indicate hardware issues, etc.).

> > Why not just copy the external buffer's contents into the normal buffer during
> > bootup?  ie: printk("%s", external_buffer).

For two reasons:

1) Additional copy operation (ok, that's cheap).

2) We would lose the capability that the buffer content even survives 
   a reset / crash / reboot.

   This is a feature can be very useful in embedded systems where
   there is (at least normally) nobody looking at any console
   messages; being able to see the kernel's crash messages after a
   reboot in the normal syslog messages can be really helpful.

> > I think a quite bad part of this feature is that it renders the
> > kernel's log_buf_len boot parameter (and its Kconfig defult)
> > ineffective.  That's a pretty useful feature - people often set it to
> > 1MB or more during problem diagnosis, and the new unalterable 16kbytes
> > is very very small.

Yes, we are aware of this, but it's a compromise. Basicly it depends
on hardware capabilities. In most cases, it is sufficient to keep the
syslog buffer in RAM. But if you really have to guarantee that the
content of the buffer remains stable, this cannot be used because
during a processor reset there will be usually a too long pause until
the memory controller gets re-initialized and RAM refresh continues -
we might lose data in this time. In most cases it just works fine,
but it's out of spec. In such situations, when such a guarantee is
needed, we can use any SRAM that may be available on the board, or we
can use some on-chip-memory (OCM, basicly also SRAM) that might be on
the processor.

The OCM on the processors in question is 16 kB, hence the limitation
there.


But note that the code is general enough to support the other options
(other, bigger SRAM, or arbitrarily sized buffer in RAM), too.

> Agreed; however, I'm not sure how often this technique is employed in
> embedded systems versus desktop systems.

I haven't seen this ever used in desktop systems yet. And I doubt if
there is any x86 with a boot loader that supports somehting like this.

But there is for example a pretty large number of mobile cranes all
ove rthe workd which are using this feature :-)

> I'd be happy to keep the existing log buffer and perform a prepend from the
> boot buffer to it rather than an append of the kernel buffer to the boot
> buffer. Thoughts?

Please don't - this would implement only the way to pass the buffer
from the boot loader too Linux, but not the way back.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
What we anticipate seldom occurs;  what  we  least  expect  generally
happens.                                          - Bengamin Disraeli

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-30 22:01   ` Grant Erickson
@ 2009-01-30 22:28     ` Andrew Morton
  2009-01-31 10:01       ` Wolfgang Denk
  2009-01-31  9:59     ` Wolfgang Denk
  1 sibling, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2009-01-30 22:28 UTC (permalink / raw)
  To: Grant Erickson; +Cc: linux-kernel, wd

On Fri, 30 Jan 2009 14:01:49 -0800
Grant Erickson <gerickson@nuovations.com> wrote:

> Your summary is precisely the intent. As I
> mentioned in check-in notice, this isn't my work per se, but a fix of prior
> unsubmitted work. Consequently, I can only speculate on the original
> implementation choices of the primary authors (Yuri Tikhonov
> <yur@emcraft.com> on behalf of Denx from what I can ascertain from GIT
> logs). I've added Wolfgang in the event he can add background where
> appropriate.
> 
> I believe this to be a useful feature for the embedded system space;
> however, I am not wedded in any way to the current implementation and am
> happy to evolve it as needed to meet the needs of key stakeholders.
> 
> > Why not just copy the external buffer's contents into the normal buffer during
> > bootup?  ie: printk("%s", external_buffer).
> 
> Again, conjecturing, I suspect the goal was to ensure that the boot messages
> appear on the console or in the syslog before kernel messages of that
> particular boot cycle. Depending on the board and architecture in question,
> the kernel log buffer may have several messages in it by the time the log
> coalescing code runs. For example, on the board I tested this on:
> 
>     # dmesg
>     U-Boot 1.3.3 (Jan  9 2009 - 10:24:45)
>     POST memory PASSED
>     POST ecc PASSED
>     POST cache PASSED
>     POST ethernet PASSED
>     Using Kilauea machine description
>     log_buf=fdffc000
>     Linux version 2.6.27.7 (gerickson@ubuntu-fusion) (gcc version 4.0.0
> (DENX ELDK 4.1 4.0.0)) #1 Fri Jan 9 10:30:30 PST 2009

It shouldn't be too hard to cook up some way of copying some data into
th log buffer before the kernel itself has put anything in there, if
that's all which is needed.


something similar to this?

 include/linux/kernel.h |    2 ++
 init/main.c            |    6 ++++++
 kernel/printk.c        |    8 ++++++++
 3 files changed, 16 insertions(+)

diff -puN init/main.c~a init/main.c
--- a/init/main.c~a
+++ a/init/main.c
@@ -535,11 +535,17 @@ void __init __weak thread_info_cache_ini
 {
 }
 
+void __init __weak arch_copy_boot_messages(void)
+{
+}
+
 asmlinkage void __init start_kernel(void)
 {
 	char * command_line;
 	extern struct kernel_param __start___param[], __stop___param[];
 
+	arch_copy_boot_messages();
+
 	smp_setup_processor_id();
 
 	/*
diff -puN kernel/printk.c~a kernel/printk.c
--- a/kernel/printk.c~a
+++ a/kernel/printk.c
@@ -493,6 +493,14 @@ static void emit_log_char(char c)
 		logged_chars++;
 }
 
+void emit_log_string(const char *s)
+{
+	spin_lock_irqsave(&logbuf_lock);
+	while (*s)
+		emit_log_char(*s++);
+	spin_unlock_irqsave(&logbuf_lock);
+}
+
 /*
  * Zap console related locks when oopsing. Only zap at most once
  * every 10 seconds, to leave time for slow consoles to print a
diff -puN include/linux/kernel.h~a include/linux/kernel.h
--- a/include/linux/kernel.h~a
+++ a/include/linux/kernel.h
@@ -257,6 +257,8 @@ static inline bool printk_timed_ratelimi
 
 extern int printk_needs_cpu(int cpu);
 extern void printk_tick(void);
+extern void __init arch_copy_boot_messages(void);
+extern void emit_log_string(const char *s);
 
 extern void asmlinkage __attribute__((format(printf, 1, 2)))
 	early_printk(const char *fmt, ...);
_


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-30 19:51 ` Andrew Morton
@ 2009-01-30 22:01   ` Grant Erickson
  2009-01-30 22:28     ` Andrew Morton
  2009-01-31  9:59     ` Wolfgang Denk
  0 siblings, 2 replies; 11+ messages in thread
From: Grant Erickson @ 2009-01-30 22:01 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, Wolfgang Denx

On 1/30/09 11:51 AM, Andrew Morton wrote:
> On Wed, 21 Jan 2009 09:39:46 -0800
> Grant Erickson <gerickson@nuovations.com> wrote:
>> This merges support for the previously DENX-only kernel feature of
>> specifying an alternative, "external" buffer for kernel printk
>> messages and their associated metadata. In addition, this ports
>> architecture support for this feature from arch/ppc to arch/powerpc.
>> 
>> When this option is enabled, an architecture- or machine-specific log
>> buffer is used for all printk messages. This allows entities such as
>> boot loaders (e.g. U-Boot) to place printk-compatible messages into
>> this buffer and for the kernel to coalesce them with its normal
>> messages.
>> 
>> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
>> ---
>> 
>> The code has historically been used and proven to work on the LWMON5
>> platform under arch/ppc and is now used (by me) successfully on the
>> AMCC Haleakala and Kilauea platforms.
>> 
>> As implemented for arch/powerpc, two suboptions for the alternative
>> log buffer are supported. The buffer may be contiguous with the
>> metadata and message data colocated or the metadata and message
>> storage may be in discontiguous regions of memory (e.g. a set of
>> scratch registers and an SRAM buffer). On Kilauea and Haleakala, I
>> have used the former; whereas LWMON5 has traditionally used the latter.
>> 
>> The code here is, more or less, as-is from the DENX GIT tree. Comments
>> welcome. Some prior discussion from the linuxppc-dev mailing list is at
>> http://ozlabs.org/pipermail/linuxppc-dev/2008-November/065589.html.
>> 
> 
> I'm not completely clear what all this exactly does.  Let me try...
> 
> We have an external log buffer, into which stuff was written by (say)
> the boot prom code.
> 
> We want that to come out on the console during bootup.
> 
> So what we do is to divert the normal printk log buffer so that it
> starts writing at the tail of that existing external buffer.  We emit
> the entire buffer some time during bootup.  We henceforth continue to
> use that external buffer in the normal manner. If so, why do it this way?

Andrew,

Thanks for your feedback. Your summary is precisely the intent. As I
mentioned in check-in notice, this isn't my work per se, but a fix of prior
unsubmitted work. Consequently, I can only speculate on the original
implementation choices of the primary authors (Yuri Tikhonov
<yur@emcraft.com> on behalf of Denx from what I can ascertain from GIT
logs). I've added Wolfgang in the event he can add background where
appropriate.

I believe this to be a useful feature for the embedded system space;
however, I am not wedded in any way to the current implementation and am
happy to evolve it as needed to meet the needs of key stakeholders.

> Why not just copy the external buffer's contents into the normal buffer during
> bootup?  ie: printk("%s", external_buffer).

Again, conjecturing, I suspect the goal was to ensure that the boot messages
appear on the console or in the syslog before kernel messages of that
particular boot cycle. Depending on the board and architecture in question,
the kernel log buffer may have several messages in it by the time the log
coalescing code runs. For example, on the board I tested this on:

    # dmesg
    U-Boot 1.3.3 (Jan  9 2009 - 10:24:45)
    POST memory PASSED
    POST ecc PASSED
    POST cache PASSED
    POST ethernet PASSED
    Using Kilauea machine description
    log_buf=fdffc000
    Linux version 2.6.27.7 (gerickson@ubuntu-fusion) (gcc version 4.0.0
(DENX ELDK 4.1 4.0.0)) #1 Fri Jan 9 10:30:30 PST 2009

"Using Kilauea..." is already in the kernel buffer when the time comes
around to coalesce the buffer.

In the case of u-boot, it uses logging code nearly identical to Linux, so
employing the straightforward approach of 'printk("%s", external_buffer)'
would result in:

    # dmesg
    U-Boot 1.3.3 (Jan  9 2009 - 10:24:45)
    <4>POST memory PASSED
    <4>POST ecc PASSED
    <4>POST cache PASSED
    <4>POST ethernet PASSED
    Using Kilauea machine description

Not the end of the world, but not ideal either.

> I think a quite bad part of this feature is that it renders the
> kernel's log_buf_len boot parameter (and its Kconfig defult)
> ineffective.  That's a pretty useful feature - people often set it to
> 1MB or more during problem diagnosis, and the new unalterable 16kbytes
> is very very small.

Agreed; however, I'm not sure how often this technique is employed in
embedded systems versus desktop systems.

I'd be happy to keep the existing log buffer and perform a prepend from the
boot buffer to it rather than an append of the kernel buffer to the boot
buffer. Thoughts?

>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 3a2dc7e..60282f1 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/debugfs.h>
>>  #include <linux/irq.h>
>>  #include <linux/lmb.h>
>> +#include <linux/logbuff.h>
>>  
>>  #include <asm/prom.h>
>>  #include <asm/rtas.h>
>> @@ -61,6 +62,15 @@
>>  #define DBG(fmt...)
>>  #endif
>>  
>> +#ifdef CONFIG_LOGBUFFER
>> +#ifdef CONFIG_ALT_LB_LOCATION
> 
> CONFIG_ALT_LB_LOCATION depends upon CONFIG_LOGBUFFER, so we only need
> 
> #ifdef CONFIG_ALT_LB_LOCATION
> 
> here.  There are several instances of this in the patch.

On the PPC mailing list, there were some solid proposals using the device
tree that would make the configuration a bit cleaner. In those cases,
there's simply a board/architecture method/probe enabled by Kconfig that
would perform the aforementioned prepend operation.

Thoughts? 

>> +#ifndef _LOGBUFF_H_
>> +#define _LOGBUFF_H_
>> +
>> +#ifdef CONFIG_LOGBUFFER
>> +
>> +#define LOGBUFF_MAGIC  0xc0de4ced
> 
> What is the meaning of this value?  Is it random, or does it correspond
> to some powerpc firmware thing, or??

It's defined by u-boot and used by u-boot in relation to how it manages POST
status and its log buffer.

> Do we actually need this magic number?  All it appears to do is to
> cause a warning.
> 
> The magic number could/should be placed in magic.h.

Wolfgang?

> 
>> +#define LOGBUFF_LEN  16384
>> +#define LOGBUFF_OVERHEAD 4096
>> +#define LOGBUFF_RESERVE  (LOGBUFF_LEN + LOGBUFF_OVERHEAD)
> 
> What is LOGBUFF_OVERHEAD supposed to do?
> 
> afacit both LOGBUFF_OVERHEAD and LOGBUFF_RESERVE are unused and
> could/should be removed.

Again, I am simply conjecturing here; however, I would guess that for the
case of the co-located boot log metadata and buffer data, there was a desire
to provide a buffer of the same page-aligned size as the default kernel
buffer (16 KiB) and then a desire to use 5 words of metadata but, again,
keep things "easy" by just stealing/reserving a whole, additional page.

Wolfgang?

>> +#ifdef CONFIG_ALT_LB_LOCATION
>> +# define LOGBUFF_VOLATILE volatile
>> +#else
>> +# define LOGBUFF_VOLATILE
>> +#endif /* defined(CONFIG_ALT_LB_LOCATION) */
> 
> Why does this code use volatiles?  Please see
> Documentation/volatile-considered-harmful.txt

I've definitely read and understand it. Again, conjecturing here, but I
would guess that for the non-colocated buffer/metadata case, the original
author(s) were perhaps trying to avoid placing xe32_to_cpu or such in the
generic kernel logging code.

If we take the prepend-and-preserve-the-kernel-buffer tack, I think this
would all largely get cleaned-up and disappear. Prepend I/O would then be
relegated to architecture- or board-specific code.

Thoughts? Wolfgang?

>> lock_kernel();
>> tick_init();
>> boot_cpu_init();
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index f492f15..59884e2 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/security.h>
>>  #include <linux/bootmem.h>
>>  #include <linux/syscalls.h>
>> +#include <linux/logbuff.h>
>>  
>>  #include <asm/uaccess.h>
>>  
>> @@ -101,9 +102,39 @@ static DEFINE_SPINLOCK(logbuf_lock);
>>   * The indices into log_buf are not constrained to log_buf_len - they
>>   * must be masked before subscripting
>>   */
>> +#ifdef CONFIG_LOGBUFFER
>> +/* Indexes to the local log buffer */
>> +static unsigned long _log_start;
>> +static unsigned long _con_start;
>> +static unsigned long _log_end;
>> +static unsigned long _logged_chars;
>> +/* These will be switched to the external log buffer */
>> +#ifndef CONFIG_ALT_LB_LOCATION
>> +/* usual logbuffer location */
>> +static unsigned long *ext_log_start = &_log_start;
>> +static unsigned long *ext_con_start = &_con_start;
>> +static unsigned long *ext_log_end = &_log_end;
>> +static unsigned long *ext_logged_chars = &_logged_chars;
>> +#define log_start (*ext_log_start)
>> +#define con_start (*ext_con_start)
>> +#define log_end  (*ext_log_end)
>> +#define logged_chars (*ext_logged_chars)
>> +#else /* defined(CONFIG_ALT_LB_LOCATION) */
>> +/* alternative logbuffer location */
>> +static volatile unsigned long *ext_log_start = &_log_start;
>> +static volatile unsigned long *ext_con_start = &_con_start;
>> +static volatile unsigned long *ext_log_end = &_log_end;
>> +static volatile unsigned long *ext_logged_chars = &_logged_chars;
>> +#define log_start       (*((volatile u32 *)ext_log_start))
>> +#define con_start       (*((volatile u32 *)ext_con_start))
>> +#define log_end         (*((volatile u32 *)ext_log_end))
>> +#define logged_chars    (*((volatile u32 *)ext_logged_chars))
>> +#endif /* !defined(CONFIG_ALT_LB_LOCATION) */
>> +#else /* !defined(CONFIG_LOGBUFFER) */
>>  static unsigned log_start; /* Index into log_buf: next char to be read by
>> syslog() */
>>  static unsigned con_start; /* Index into log_buf: next char to be sent to
>> consoles */
>>  static unsigned log_end; /* Index into log_buf: most-recently-written-char +
>> 1 */
>> +#endif /* CONFIG_LOGBUFFER */
> 
> ick.

See comments above.

If you and Wolfgang are supportive of the prepend and preserve approach, I
think I can take another pass at this and eliminate a good measure of the
implementation concerns.

I welcome your further feedback.

Regards,

Grant



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-21 17:39 Grant Erickson
  2009-01-21 20:37 ` David Miller
  2009-01-27 13:36 ` Carl-Daniel Hailfinger
@ 2009-01-30 19:51 ` Andrew Morton
  2009-01-30 22:01   ` Grant Erickson
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Morton @ 2009-01-30 19:51 UTC (permalink / raw)
  To: Grant Erickson; +Cc: linux-kernel

On Wed, 21 Jan 2009 09:39:46 -0800
Grant Erickson <gerickson@nuovations.com> wrote:

> This merges support for the previously DENX-only kernel feature of
> specifying an alternative, "external" buffer for kernel printk
> messages and their associated metadata. In addition, this ports
> architecture support for this feature from arch/ppc to arch/powerpc.
> 
> When this option is enabled, an architecture- or machine-specific log
> buffer is used for all printk messages. This allows entities such as
> boot loaders (e.g. U-Boot) to place printk-compatible messages into
> this buffer and for the kernel to coalesce them with its normal
> messages.
> 
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> ---
> 
> The code has historically been used and proven to work on the LWMON5
> platform under arch/ppc and is now used (by me) successfully on the
> AMCC Haleakala and Kilauea platforms.
> 
> As implemented for arch/powerpc, two suboptions for the alternative
> log buffer are supported. The buffer may be contiguous with the
> metadata and message data colocated or the metadata and message
> storage may be in discontiguous regions of memory (e.g. a set of
> scratch registers and an SRAM buffer). On Kilauea and Haleakala, I
> have used the former; whereas LWMON5 has traditionally used the latter.
> 
> The code here is, more or less, as-is from the DENX GIT tree. Comments
> welcome. Some prior discussion from the linuxppc-dev mailing list is at
> http://ozlabs.org/pipermail/linuxppc-dev/2008-November/065589.html.
> 

I'm not completely clear what all this exactly does.  Let me try...

We have an external log buffer, into which stuff was written by (say)
the boot prom code.

We want that to come out on the console during bootup.

So what we do is to divert the normal printk log buffer so that it
starts writing at the tail of that existing external buffer.  We emit
the entire buffer some time during bootup.  We henceforth continue to
use that external buffer in the normal manner.

If so, why do it this way?  Why not just copy the external buffer's
contents into the normal buffer during bootup?  ie: printk("%s",
external_buffer).


I think a quite bad part of this feature is that it renders the
kernel's log_buf_len boot parameter (and its Kconfig defult)
ineffective.  That's a pretty useful feature - people often set it to
1MB or more during problem diagnosis, and the new unalterable 16kbytes
is very very small.


> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 3a2dc7e..60282f1 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -32,6 +32,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/irq.h>
>  #include <linux/lmb.h>
> +#include <linux/logbuff.h>
>  
>  #include <asm/prom.h>
>  #include <asm/rtas.h>
> @@ -61,6 +62,15 @@
>  #define DBG(fmt...)
>  #endif
>  
> +#ifdef CONFIG_LOGBUFFER
> +#ifdef CONFIG_ALT_LB_LOCATION

CONFIG_ALT_LB_LOCATION depends upon CONFIG_LOGBUFFER, so we only need

	#ifdef CONFIG_ALT_LB_LOCATION

here.  There are several instances of this in the patch.

> +# if !defined(BOARD_ALT_LH_ADDR) || !defined(BOARD_ALT_LB_ADDR)
> +#  error "Please specify BOARD_ALT_LH_ADDR & BOARD_ALT_LB_ADDR."
> +# endif
> +#else /* !CONFIG_ALT_LB_LOCATION */
> +static phys_addr_t ext_logbuff;
> +#endif /* CONFIG_ALT_LB_LOCATION */
> +#endif /* CONFIG_LOGBUFFER */
>  
>  static int __initdata dt_root_addr_cells;
>  static int __initdata dt_root_size_cells;
> @@ -1018,6 +1028,85 @@ static int __init early_init_dt_scan_memory(unsigned long node,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LOGBUFFER
> +#ifdef CONFIG_ALT_LB_LOCATION
> +/* Alternative external log buffer mapping: log metadata header & the
> + * character buffer are separated and allocated not in RAM but in some
> + * other memory-mapped I/O region (e.g. log head in unused registers,
> + * and log buffer in OCM memory)
> + */
> +int __init setup_ext_logbuff_mem(volatile logbuff_t **lhead, char **lbuf)
> +{
> +	void *h, *b;
> +
> +	if (unlikely(!lhead) || unlikely(!lbuf))
> +		return -EINVAL;

This test is unneeded.

> +	/* map log head */
> +	h = ioremap(BOARD_ALT_LH_ADDR, sizeof(logbuff_t));
> +	if (unlikely(!h))
> +		return -EFAULT;
> +
> +	/* map log buffer */
> +	b = ioremap(BOARD_ALT_LB_ADDR, LOGBUFF_LEN);
> +	if (unlikely(!b)) {
> +		iounmap(h);
> +		return -EFAULT;
> +	}
> +
> +	*lhead = h;
> +	*lbuf = b;
> +
> +	return 0;
> +}
> +#else /* !CONFIG_ALT_LB_LOCATION */
> +/* Usual external log-buffer mapping: log metadata header & the character
> + * buffer are both contiguous in system RAM.
> + */
> +int __init setup_ext_logbuff_mem(logbuff_t **lhead, char **lbuf)
> +{
> +	void *p;
> +
> +	if (unlikely(!lhead) || unlikely(!lbuf))
> +		return -EINVAL;

This test is unneeded.

> +	if (unlikely(!ext_logbuff) || !lmb_is_reserved(ext_logbuff))
> +		return -EFAULT;

Is this test needed?

> +	p = ioremap(ext_logbuff, LOGBUFF_RESERVE);
> +
> +	if (unlikely(!p))
> +		return -EFAULT;
> +
> +	*lhead = (logbuff_t *)(p + LOGBUFF_OVERHEAD -
> +			       sizeof(logbuff_t) +
> +			       sizeof(((logbuff_t *)0)->buf));

The cast is unneeded.

> +	*lbuf = (*lhead)->buf;
> +
> +	return 0;
> +}
> +
> +/* When the external log buffer configuration is used with the
> + * non-alternate location, the log head metadata and character buffer
> + * lie in the LOGBUFF_RESERVE bytes at the end of system RAM. Add this
> + * block of memory to the reserved memory pool so that it is not
> + * allocated for other purposes.
> + */
> +static void __init reserve_ext_logbuff_mem(void)
> +{
> +	phys_addr_t top = lmb_end_of_DRAM();
> +	phys_addr_t size = LOGBUFF_RESERVE;
> +	phys_addr_t base = top - size;
> +
> +	if (top > base) {
> +		ext_logbuff = base;
> +		DBG("reserving: %x -> %x\n", base, size);
> +		lmb_reserve(base, size);
> +	}
> +}
> +#endif /* CONFIG_ALT_LB_LOCATION */
> +#endif /* CONFIG_LOGBUFFER */
> +
>  static void __init early_reserve_mem(void)
>  {
>  	u64 base, size;
> @@ -1033,6 +1122,10 @@ static void __init early_reserve_mem(void)
>  	self_size = initial_boot_params->totalsize;
>  	lmb_reserve(self_base, self_size);
>  
> +#if defined(CONFIG_LOGBUFFER) && !defined(CONFIG_ALT_LB_LOCATION)
> +	reserve_ext_logbuff_mem();
> +#endif /* defined(CONFIG_LOGBUFFER) && !defined(CONFIG_ALT_LB_LOCATION) */

It would be cleaner and more maintainable to do

#else
static inline void reserve_ext_logbuff_mem(void)
{
}
#endif

above, then remove the ifdefs here.


> +
>  #ifdef CONFIG_BLK_DEV_INITRD
>  	/* then reserve the initrd, if any */
>  	if (initrd_start && (initrd_end > initrd_start))
> diff --git a/include/linux/logbuff.h b/include/linux/logbuff.h
> new file mode 100644
> index 0000000..22a51c0
> --- /dev/null
> +++ b/include/linux/logbuff.h
> @@ -0,0 +1,56 @@
> +/*
> + * (C) Copyright 2007
> + * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +#ifndef _LOGBUFF_H_
> +#define _LOGBUFF_H_
> +
> +#ifdef CONFIG_LOGBUFFER
> +
> +#define LOGBUFF_MAGIC		0xc0de4ced

What is the meaning of this value?  Is it random, or does it correspond
to some powerpc firmware thing, or??

Do we actually need this magic number?  All it appears to do is to
cause a warning.

The magic number could/should be placed in magic.h.

> +#define LOGBUFF_LEN		16384
> +#define LOGBUFF_OVERHEAD	4096
> +#define LOGBUFF_RESERVE		(LOGBUFF_LEN + LOGBUFF_OVERHEAD)

What is LOGBUFF_OVERHEAD supposed to do?

afacit both LOGBUFF_OVERHEAD and LOGBUFF_RESERVE are unused and
could/should be removed.

> +/* The mapping used here has to be the same as in logbuff_init_ptrs ()
> +   in u-boot/common/cmd_log.c */

Please prefer to format comments as

/*
 * The mapping used here has to be the same as in logbuff_init_ptrs ()
 * in u-boot/common/cmd_log.c 
 */

> +typedef struct {
> +	unsigned long	tag;
> +	unsigned long	start;
> +	unsigned long	con;	/* next char to be sent to consoles	*/
> +	unsigned long	end;
> +	unsigned long	chars;
> +	unsigned char	buf[0];
> +} logbuff_t;

Please avoid adding typedefs like this.  Just use `struct
ext_log_buffer' (or whatever) everywhere.

> +#ifdef CONFIG_ALT_LB_LOCATION
> +# define LOGBUFF_VOLATILE	volatile
> +#else
> +# define LOGBUFF_VOLATILE
> +#endif /* defined(CONFIG_ALT_LB_LOCATION) */

Why does this code use volatiles?  Please see
Documentation/volatile-considered-harmful.txt

> +extern void setup_ext_logbuff(void);
> +/* arch specific */
> +extern int setup_ext_logbuff_mem(LOGBUFF_VOLATILE logbuff_t **lhead, char **lbuf);
> +
> +#endif /* CONFIG_LOGBUFFER */
> +#endif /* _LOGBUFF_H_ */
> diff --git a/init/Kconfig b/init/Kconfig
> index f763762..e1a1b59 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -619,6 +619,31 @@ config PRINTK
>  	  very difficult to diagnose system problems, saying N here is
>  	  strongly discouraged.
>  
> +config LOGBUFFER
> +	bool "External logbuffer" if PRINTK
> +	default n
> +	help
> +	  This option enables support for an alternative, "external"
> +	  printk log buffer. When enabled, an architecture- or machine-
> +	  specific log buffer is used for all printk messages. This
> +	  allows entities such as boot loaders to place printk-compatible
> +	  messages into this buffer and for the kernel to coalesce them
> +	  with its normal messages.
> +
> +config ALT_LB_LOCATION
> +	bool "Alternative logbuffer" if LOGBUFFER
> +	default n
> +	help
> +	  When using an alternative, "external" printk log buffer, an
> +	  architecture- or machine-specific log buffer with contiguous
> +	  metadata and message storage is used. This option enables
> +	  support for discontiguous metadata and message storage
> +	  memory (e.g. a set of scratch registers and an SRAM
> +	  buffer). By saying Y here, you must also ensure your
> +	  architecture- or machine-code specify BOARD_ALT_LH_ADDR and
> +	  BOARD_ALT_LB_ADDR, for the metadata and message memory,
> +	  respectively.
> +
>  config BUG
>  	bool "BUG() support" if EMBEDDED
>  	default y
> diff --git a/init/main.c b/init/main.c
> index 7e117a2..5687b98 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -61,6 +61,7 @@
>  #include <linux/kthread.h>
>  #include <linux/sched.h>
>  #include <linux/signal.h>
> +#include <linux/logbuff.h>
>  #include <linux/idr.h>
>  #include <linux/ftrace.h>
>  
> @@ -563,6 +564,9 @@ asmlinkage void __init start_kernel(void)
>   * Interrupts are still disabled. Do necessary setups, then
>   * enable them
>   */
> +#ifdef CONFIG_LOGBUFFER
> +	setup_ext_logbuff();
> +#endif

Do 

#else
static inline void setup_ext_logbuff(void)
{
}
#endif		/* CONFIG_whatever */

in a header file, then remove these ifdefs.

>  	lock_kernel();
>  	tick_init();
>  	boot_cpu_init();
> diff --git a/kernel/printk.c b/kernel/printk.c
> index f492f15..59884e2 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -32,6 +32,7 @@
>  #include <linux/security.h>
>  #include <linux/bootmem.h>
>  #include <linux/syscalls.h>
> +#include <linux/logbuff.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -101,9 +102,39 @@ static DEFINE_SPINLOCK(logbuf_lock);
>   * The indices into log_buf are not constrained to log_buf_len - they
>   * must be masked before subscripting
>   */
> +#ifdef CONFIG_LOGBUFFER
> +/* Indexes to the local log buffer */
> +static unsigned long _log_start;
> +static unsigned long _con_start;
> +static unsigned long _log_end;
> +static unsigned long _logged_chars;
> +/* These will be switched to the external log buffer */
> +#ifndef CONFIG_ALT_LB_LOCATION
> +/* usual logbuffer location */
> +static unsigned long *ext_log_start = &_log_start;
> +static unsigned long *ext_con_start = &_con_start;
> +static unsigned long *ext_log_end = &_log_end;
> +static unsigned long *ext_logged_chars = &_logged_chars;
> +#define log_start	(*ext_log_start)
> +#define con_start	(*ext_con_start)
> +#define log_end		(*ext_log_end)
> +#define logged_chars	(*ext_logged_chars)
> +#else /* defined(CONFIG_ALT_LB_LOCATION) */
> +/* alternative logbuffer location */
> +static volatile unsigned long *ext_log_start = &_log_start;
> +static volatile unsigned long *ext_con_start = &_con_start;
> +static volatile unsigned long *ext_log_end = &_log_end;
> +static volatile unsigned long *ext_logged_chars = &_logged_chars;
> +#define log_start       (*((volatile u32 *)ext_log_start))
> +#define con_start       (*((volatile u32 *)ext_con_start))
> +#define log_end         (*((volatile u32 *)ext_log_end))
> +#define logged_chars    (*((volatile u32 *)ext_logged_chars))
> +#endif /* !defined(CONFIG_ALT_LB_LOCATION) */
> +#else /* !defined(CONFIG_LOGBUFFER) */
>  static unsigned log_start;	/* Index into log_buf: next char to be read by syslog() */
>  static unsigned con_start;	/* Index into log_buf: next char to be sent to consoles */
>  static unsigned log_end;	/* Index into log_buf: most-recently-written-char + 1 */
> +#endif /* CONFIG_LOGBUFFER */

ick.

>  /*
>   *	Array of consoles built from command line options (console=)
> @@ -134,10 +165,121 @@ static int console_may_schedule;
>  static char __log_buf[__LOG_BUF_LEN];
>  static char *log_buf = __log_buf;
>  static int log_buf_len = __LOG_BUF_LEN;
> +#ifndef CONFIG_LOGBUFFER
>  static unsigned logged_chars; /* Number of chars produced since last read+clear operation */
> +#endif /* !defined(CONFIG_LOGBUFFER) */
> +#ifdef CONFIG_LOGBUFFER
> +/* Sanity check the external log buffer metadata. When an the external
> + * log buffer is enabled, the log metadata is effectively non-volatile
> + * in that the values are preserved from reboot to reboot (until/unless
> + * the system loses power).
> + */
> +static void __init logbuff_check_metadata(LOGBUFF_VOLATILE logbuff_t *log)
> +{
> +	unsigned long chars;
> +
> +	/* Sanity check the producer and consumer indices. */
> +
> +	if (log->end - log->start > LOGBUFF_LEN)
> +		log->start = log->end - LOGBUFF_LEN;
> +
> +	if (log->end - log->con > LOGBUFF_LEN)
> +		log->con = log->end - LOGBUFF_LEN;
> +
> +	/* Occasionally, particularly following a reboot, the start
> +	 * consumer index is not properly caught up to the console
> +	 * consumer index. If this is the case, catch it up so that
> +	 * the log buffer doesn't start with, for example,
> +	 * "<0>Restarting system.\n" followed by the 'real' start of
> +	 * the log.
> +	 */
> +
> +	if (log->con > log->start)
> +		log->start = log->con;
> +
> +	/* Ensure that the number of characters logged reflects the
> +	 * characters actually logged based on the producer and
> +	 * consumer indices rather than all characters cumulatively
> +	 * logged across all reboots since a power-loss event.
> +	 */
> +
> +	chars = log->end - log->start;
> +
> +	if (log->chars > chars)
> +		log->chars = chars;
> +}
> +
> +/* Coalesce the current log bounded buffer and the external log
> + * bounded buffer by appending the former to the latter. Precedence is
> + * given to the external log buffer when there is more data to be
> + * appended than space exists, so the current log buffer is truncated
> + * instead of overwritting the external buffer.
> + */
> +static void __init logbuff_coalesce_buffers(LOGBUFF_VOLATILE logbuff_t *log)
> +{
> +	unsigned long dspace, ssize, len;
> +
> +	dspace = LOGBUFF_LEN - (log->end - log->start);
> +	ssize = log_end - log_start;
> +	len = min(dspace, ssize);
> +
> +	while (len-- > 0) {
> +		log->buf[log->end++ & (LOGBUFF_LEN-1)] = LOG_BUF(log_start++);
> +		log->chars++;
> +	}
> +}
>  
> +void __init setup_ext_logbuff(void)
> +{
> +	LOGBUFF_VOLATILE logbuff_t *log;
> +	char *ext_log_buf = NULL;
> +	unsigned long flags;
> +
> +	if (setup_ext_logbuff_mem(&log, &ext_log_buf) < 0) {
> +		printk(KERN_WARNING
> +		       "Failed to setup external logbuffer - ignoring it\n");
> +		return;
> +	}
> +
> +	/* When no properly setup buffer is found, reset pointers */

"set up"

> +	if (log->tag != LOGBUFF_MAGIC) {
> +		printk(KERN_WARNING
> +		       "Unexpected external log buffer magic number. "
> +		       "Got %08lx, expected %08x. Resetting pointers and using "
> +		       "the buffer anyway.\n",
> +		       log->tag, LOGBUFF_MAGIC);
> +		log->tag = LOGBUFF_MAGIC;
> +		log->start = log->end = log->con = log->chars = 0;
> +	}
> +
> +	spin_lock_irqsave(&logbuf_lock, flags);
> +
> +	logbuff_check_metadata(log);
> +	logbuff_coalesce_buffers(log);
> +
> +	/* Switch to the external log buffer */
> +	ext_log_start		= &log->start;
> +	ext_con_start		= &log->con;
> +	ext_log_end		= &log->end;
> +	ext_logged_chars	= &log->chars;
> +
> +	log_buf			= ext_log_buf;
> +	log_buf_len 		= LOGBUFF_LEN;
> +
> +	spin_unlock_irqrestore(&logbuf_lock, flags);
> +
> +	printk(KERN_NOTICE "log_buf=%p\n", log_buf);
> +}
> +#endif /* CONFIG_LOGBUFFER */
>  static int __init log_buf_len_setup(char *str)
>  {
> +#ifdef CONFIG_LOGBUFFER
> +	/* Log buffer size is LOGBUFF_LEN bytes */
> +	printk(KERN_NOTICE
> +	       "External log buffer configured; "
> +	       "ignoring log_buf_len param.\n");
> +	return 1;
> +#else

:(

>  	unsigned size = memparse(str, &str);
>  	unsigned long flags;
>  
> @@ -173,6 +315,7 @@ static int __init log_buf_len_setup(char *str)
>  	}
>  out:
>  	return 1;
> +#endif /* CONFIG_LOGBUFFER */
>  }
>  
>  __setup("log_buf_len=", log_buf_len_setup);
> @@ -230,7 +373,7 @@ static void boot_delay_msec(void)
>  static inline void boot_delay_msec(void)
>  {
>  }
> -#endif
> +#endif /* CONFIG_BOOT_PRINTK_DELAY */
>  
>  /*
>   * Commands to do_syslog:
> @@ -740,7 +883,7 @@ out_restore_irqs:
>  EXPORT_SYMBOL(printk);
>  EXPORT_SYMBOL(vprintk);
>  
> -#else
> +#else /* !CONFIG_PRINTK */
>  
>  asmlinkage long sys_syslog(int type, char __user *buf, int len)
>  {
> @@ -751,7 +894,7 @@ static void call_console_drivers(unsigned start, unsigned end)
>  {
>  }
>  
> -#endif
> +#endif /* CONFIG_PRINTK */
>  
>  static int __add_preferred_console(char *name, int idx, char *options,
>  				   char *brl_options)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-21 17:39 Grant Erickson
  2009-01-21 20:37 ` David Miller
@ 2009-01-27 13:36 ` Carl-Daniel Hailfinger
  2009-01-30 19:51 ` Andrew Morton
  2 siblings, 0 replies; 11+ messages in thread
From: Carl-Daniel Hailfinger @ 2009-01-27 13:36 UTC (permalink / raw)
  To: Grant Erickson; +Cc: linux-kernel, Coreboot

Hi,

[Sorry for the TOFU, I wanted to make sure people from the coreboot list
can join in without having to dig through the archives. The coreboot
list is moderated, but your mails will get through eventually.]

This is a feature which would benefit coreboot (formerly known as
LinuxBIOS) as well. coreboot v3 keeps such a log buffer in RAM and we
(coreboot developers) are interested in sharing that buffer with the
kernel. There might be some differences in the buffer format, though.

I haven't yet read the patch completely, but from what I can see struct
logbuff_t differs quite a bit between coreboot and U-Boot. There are
some conceptually common struct members, but each of the projects has
some info in there that's missing at the other project. It would be nice
if we could work out some common data structure combining the best of
both worlds.

Regards,
Carl-Daniel

On 21.01.2009 18:39, Grant Erickson wrote:
> This merges support for the previously DENX-only kernel feature of
> specifying an alternative, "external" buffer for kernel printk
> messages and their associated metadata. In addition, this ports
> architecture support for this feature from arch/ppc to arch/powerpc.
>
> When this option is enabled, an architecture- or machine-specific log
> buffer is used for all printk messages. This allows entities such as
> boot loaders (e.g. U-Boot) to place printk-compatible messages into
> this buffer and for the kernel to coalesce them with its normal
> messages.
>
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>
> ---
>
> The code has historically been used and proven to work on the LWMON5
> platform under arch/ppc and is now used (by me) successfully on the
> AMCC Haleakala and Kilauea platforms.
>
> As implemented for arch/powerpc, two suboptions for the alternative
> log buffer are supported. The buffer may be contiguous with the
> metadata and message data colocated or the metadata and message
> storage may be in discontiguous regions of memory (e.g. a set of
> scratch registers and an SRAM buffer). On Kilauea and Haleakala, I
> have used the former; whereas LWMON5 has traditionally used the latter.
>
> The code here is, more or less, as-is from the DENX GIT tree. Comments
> welcome. Some prior discussion from the linuxppc-dev mailing list is at
> http://ozlabs.org/pipermail/linuxppc-dev/2008-November/065589.html.
>
>  arch/powerpc/kernel/prom.c |   93 +++++++++++++++++++++++++++
>  include/linux/logbuff.h    |   56 ++++++++++++++++
>  init/Kconfig               |   25 +++++++
>  init/main.c                |    4 +
>  kernel/printk.c            |  149 +++++++++++++++++++++++++++++++++++++++++++-
>  5 files changed, 324 insertions(+), 3 deletions(-)
>  create mode 100644 include/linux/logbuff.h
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 3a2dc7e..60282f1 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -32,6 +32,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/irq.h>
>  #include <linux/lmb.h>
> +#include <linux/logbuff.h>
>  
>  #include <asm/prom.h>
>  #include <asm/rtas.h>
> @@ -61,6 +62,15 @@
>  #define DBG(fmt...)
>  #endif
>  
> +#ifdef CONFIG_LOGBUFFER
> +#ifdef CONFIG_ALT_LB_LOCATION
> +# if !defined(BOARD_ALT_LH_ADDR) || !defined(BOARD_ALT_LB_ADDR)
> +#  error "Please specify BOARD_ALT_LH_ADDR & BOARD_ALT_LB_ADDR."
> +# endif
> +#else /* !CONFIG_ALT_LB_LOCATION */
> +static phys_addr_t ext_logbuff;
> +#endif /* CONFIG_ALT_LB_LOCATION */
> +#endif /* CONFIG_LOGBUFFER */
>  
>  static int __initdata dt_root_addr_cells;
>  static int __initdata dt_root_size_cells;
> @@ -1018,6 +1028,85 @@ static int __init early_init_dt_scan_memory(unsigned long node,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_LOGBUFFER
> +#ifdef CONFIG_ALT_LB_LOCATION
> +/* Alternative external log buffer mapping: log metadata header & the
> + * character buffer are separated and allocated not in RAM but in some
> + * other memory-mapped I/O region (e.g. log head in unused registers,
> + * and log buffer in OCM memory)
> + */
> +int __init setup_ext_logbuff_mem(volatile logbuff_t **lhead, char **lbuf)
> +{
> +	void *h, *b;
> +
> +	if (unlikely(!lhead) || unlikely(!lbuf))
> +		return -EINVAL;
> +
> +	/* map log head */
> +	h = ioremap(BOARD_ALT_LH_ADDR, sizeof(logbuff_t));
> +	if (unlikely(!h))
> +		return -EFAULT;
> +
> +	/* map log buffer */
> +	b = ioremap(BOARD_ALT_LB_ADDR, LOGBUFF_LEN);
> +	if (unlikely(!b)) {
> +		iounmap(h);
> +		return -EFAULT;
> +	}
> +
> +	*lhead = h;
> +	*lbuf = b;
> +
> +	return 0;
> +}
> +#else /* !CONFIG_ALT_LB_LOCATION */
> +/* Usual external log-buffer mapping: log metadata header & the character
> + * buffer are both contiguous in system RAM.
> + */
> +int __init setup_ext_logbuff_mem(logbuff_t **lhead, char **lbuf)
> +{
> +	void *p;
> +
> +	if (unlikely(!lhead) || unlikely(!lbuf))
> +		return -EINVAL;
> +
> +	if (unlikely(!ext_logbuff) || !lmb_is_reserved(ext_logbuff))
> +		return -EFAULT;
> +
> +	p = ioremap(ext_logbuff, LOGBUFF_RESERVE);
> +
> +	if (unlikely(!p))
> +		return -EFAULT;
> +
> +	*lhead = (logbuff_t *)(p + LOGBUFF_OVERHEAD -
> +			       sizeof(logbuff_t) +
> +			       sizeof(((logbuff_t *)0)->buf));
> +	*lbuf = (*lhead)->buf;
> +
> +	return 0;
> +}
> +
> +/* When the external log buffer configuration is used with the
> + * non-alternate location, the log head metadata and character buffer
> + * lie in the LOGBUFF_RESERVE bytes at the end of system RAM. Add this
> + * block of memory to the reserved memory pool so that it is not
> + * allocated for other purposes.
> + */
> +static void __init reserve_ext_logbuff_mem(void)
> +{
> +	phys_addr_t top = lmb_end_of_DRAM();
> +	phys_addr_t size = LOGBUFF_RESERVE;
> +	phys_addr_t base = top - size;
> +
> +	if (top > base) {
> +		ext_logbuff = base;
> +		DBG("reserving: %x -> %x\n", base, size);
> +		lmb_reserve(base, size);
> +	}
> +}
> +#endif /* CONFIG_ALT_LB_LOCATION */
> +#endif /* CONFIG_LOGBUFFER */
> +
>  static void __init early_reserve_mem(void)
>  {
>  	u64 base, size;
> @@ -1033,6 +1122,10 @@ static void __init early_reserve_mem(void)
>  	self_size = initial_boot_params->totalsize;
>  	lmb_reserve(self_base, self_size);
>  
> +#if defined(CONFIG_LOGBUFFER) && !defined(CONFIG_ALT_LB_LOCATION)
> +	reserve_ext_logbuff_mem();
> +#endif /* defined(CONFIG_LOGBUFFER) && !defined(CONFIG_ALT_LB_LOCATION) */
> +
>  #ifdef CONFIG_BLK_DEV_INITRD
>  	/* then reserve the initrd, if any */
>  	if (initrd_start && (initrd_end > initrd_start))
> diff --git a/include/linux/logbuff.h b/include/linux/logbuff.h
> new file mode 100644
> index 0000000..22a51c0
> --- /dev/null
> +++ b/include/linux/logbuff.h
> @@ -0,0 +1,56 @@
> +/*
> + * (C) Copyright 2007
> + * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +#ifndef _LOGBUFF_H_
> +#define _LOGBUFF_H_
> +
> +#ifdef CONFIG_LOGBUFFER
> +
> +#define LOGBUFF_MAGIC		0xc0de4ced
> +#define LOGBUFF_LEN		16384
> +#define LOGBUFF_OVERHEAD	4096
> +#define LOGBUFF_RESERVE		(LOGBUFF_LEN + LOGBUFF_OVERHEAD)
> +
> +/* The mapping used here has to be the same as in logbuff_init_ptrs ()
> +   in u-boot/common/cmd_log.c */
> +
> +typedef struct {
> +	unsigned long	tag;
> +	unsigned long	start;
> +	unsigned long	con;	/* next char to be sent to consoles	*/
> +	unsigned long	end;
> +	unsigned long	chars;
> +	unsigned char	buf[0];
> +} logbuff_t;
> +
> +#ifdef CONFIG_ALT_LB_LOCATION
> +# define LOGBUFF_VOLATILE	volatile
> +#else
> +# define LOGBUFF_VOLATILE
> +#endif /* defined(CONFIG_ALT_LB_LOCATION) */
> +
> +extern void setup_ext_logbuff(void);
> +/* arch specific */
> +extern int setup_ext_logbuff_mem(LOGBUFF_VOLATILE logbuff_t **lhead, char **lbuf);
> +
> +#endif /* CONFIG_LOGBUFFER */
> +#endif /* _LOGBUFF_H_ */
> diff --git a/init/Kconfig b/init/Kconfig
> index f763762..e1a1b59 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -619,6 +619,31 @@ config PRINTK
>  	  very difficult to diagnose system problems, saying N here is
>  	  strongly discouraged.
>  
> +config LOGBUFFER
> +	bool "External logbuffer" if PRINTK
> +	default n
> +	help
> +	  This option enables support for an alternative, "external"
> +	  printk log buffer. When enabled, an architecture- or machine-
> +	  specific log buffer is used for all printk messages. This
> +	  allows entities such as boot loaders to place printk-compatible
> +	  messages into this buffer and for the kernel to coalesce them
> +	  with its normal messages.
> +
> +config ALT_LB_LOCATION
> +	bool "Alternative logbuffer" if LOGBUFFER
> +	default n
> +	help
> +	  When using an alternative, "external" printk log buffer, an
> +	  architecture- or machine-specific log buffer with contiguous
> +	  metadata and message storage is used. This option enables
> +	  support for discontiguous metadata and message storage
> +	  memory (e.g. a set of scratch registers and an SRAM
> +	  buffer). By saying Y here, you must also ensure your
> +	  architecture- or machine-code specify BOARD_ALT_LH_ADDR and
> +	  BOARD_ALT_LB_ADDR, for the metadata and message memory,
> +	  respectively.
> +
>  config BUG
>  	bool "BUG() support" if EMBEDDED
>  	default y
> diff --git a/init/main.c b/init/main.c
> index 7e117a2..5687b98 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -61,6 +61,7 @@
>  #include <linux/kthread.h>
>  #include <linux/sched.h>
>  #include <linux/signal.h>
> +#include <linux/logbuff.h>
>  #include <linux/idr.h>
>  #include <linux/ftrace.h>
>  
> @@ -563,6 +564,9 @@ asmlinkage void __init start_kernel(void)
>   * Interrupts are still disabled. Do necessary setups, then
>   * enable them
>   */
> +#ifdef CONFIG_LOGBUFFER
> +	setup_ext_logbuff();
> +#endif
>  	lock_kernel();
>  	tick_init();
>  	boot_cpu_init();
> diff --git a/kernel/printk.c b/kernel/printk.c
> index f492f15..59884e2 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -32,6 +32,7 @@
>  #include <linux/security.h>
>  #include <linux/bootmem.h>
>  #include <linux/syscalls.h>
> +#include <linux/logbuff.h>
>  
>  #include <asm/uaccess.h>
>  
> @@ -101,9 +102,39 @@ static DEFINE_SPINLOCK(logbuf_lock);
>   * The indices into log_buf are not constrained to log_buf_len - they
>   * must be masked before subscripting
>   */
> +#ifdef CONFIG_LOGBUFFER
> +/* Indexes to the local log buffer */
> +static unsigned long _log_start;
> +static unsigned long _con_start;
> +static unsigned long _log_end;
> +static unsigned long _logged_chars;
> +/* These will be switched to the external log buffer */
> +#ifndef CONFIG_ALT_LB_LOCATION
> +/* usual logbuffer location */
> +static unsigned long *ext_log_start = &_log_start;
> +static unsigned long *ext_con_start = &_con_start;
> +static unsigned long *ext_log_end = &_log_end;
> +static unsigned long *ext_logged_chars = &_logged_chars;
> +#define log_start	(*ext_log_start)
> +#define con_start	(*ext_con_start)
> +#define log_end		(*ext_log_end)
> +#define logged_chars	(*ext_logged_chars)
> +#else /* defined(CONFIG_ALT_LB_LOCATION) */
> +/* alternative logbuffer location */
> +static volatile unsigned long *ext_log_start = &_log_start;
> +static volatile unsigned long *ext_con_start = &_con_start;
> +static volatile unsigned long *ext_log_end = &_log_end;
> +static volatile unsigned long *ext_logged_chars = &_logged_chars;
> +#define log_start       (*((volatile u32 *)ext_log_start))
> +#define con_start       (*((volatile u32 *)ext_con_start))
> +#define log_end         (*((volatile u32 *)ext_log_end))
> +#define logged_chars    (*((volatile u32 *)ext_logged_chars))
> +#endif /* !defined(CONFIG_ALT_LB_LOCATION) */
> +#else /* !defined(CONFIG_LOGBUFFER) */
>  static unsigned log_start;	/* Index into log_buf: next char to be read by syslog() */
>  static unsigned con_start;	/* Index into log_buf: next char to be sent to consoles */
>  static unsigned log_end;	/* Index into log_buf: most-recently-written-char + 1 */
> +#endif /* CONFIG_LOGBUFFER */
>  
>  /*
>   *	Array of consoles built from command line options (console=)
> @@ -134,10 +165,121 @@ static int console_may_schedule;
>  static char __log_buf[__LOG_BUF_LEN];
>  static char *log_buf = __log_buf;
>  static int log_buf_len = __LOG_BUF_LEN;
> +#ifndef CONFIG_LOGBUFFER
>  static unsigned logged_chars; /* Number of chars produced since last read+clear operation */
> +#endif /* !defined(CONFIG_LOGBUFFER) */
> +#ifdef CONFIG_LOGBUFFER
> +/* Sanity check the external log buffer metadata. When an the external
> + * log buffer is enabled, the log metadata is effectively non-volatile
> + * in that the values are preserved from reboot to reboot (until/unless
> + * the system loses power).
> + */
> +static void __init logbuff_check_metadata(LOGBUFF_VOLATILE logbuff_t *log)
> +{
> +	unsigned long chars;
> +
> +	/* Sanity check the producer and consumer indices. */
> +
> +	if (log->end - log->start > LOGBUFF_LEN)
> +		log->start = log->end - LOGBUFF_LEN;
> +
> +	if (log->end - log->con > LOGBUFF_LEN)
> +		log->con = log->end - LOGBUFF_LEN;
> +
> +	/* Occasionally, particularly following a reboot, the start
> +	 * consumer index is not properly caught up to the console
> +	 * consumer index. If this is the case, catch it up so that
> +	 * the log buffer doesn't start with, for example,
> +	 * "<0>Restarting system.\n" followed by the 'real' start of
> +	 * the log.
> +	 */
> +
> +	if (log->con > log->start)
> +		log->start = log->con;
> +
> +	/* Ensure that the number of characters logged reflects the
> +	 * characters actually logged based on the producer and
> +	 * consumer indices rather than all characters cumulatively
> +	 * logged across all reboots since a power-loss event.
> +	 */
> +
> +	chars = log->end - log->start;
> +
> +	if (log->chars > chars)
> +		log->chars = chars;
> +}
> +
> +/* Coalesce the current log bounded buffer and the external log
> + * bounded buffer by appending the former to the latter. Precedence is
> + * given to the external log buffer when there is more data to be
> + * appended than space exists, so the current log buffer is truncated
> + * instead of overwritting the external buffer.
> + */
> +static void __init logbuff_coalesce_buffers(LOGBUFF_VOLATILE logbuff_t *log)
> +{
> +	unsigned long dspace, ssize, len;
> +
> +	dspace = LOGBUFF_LEN - (log->end - log->start);
> +	ssize = log_end - log_start;
> +	len = min(dspace, ssize);
> +
> +	while (len-- > 0) {
> +		log->buf[log->end++ & (LOGBUFF_LEN-1)] = LOG_BUF(log_start++);
> +		log->chars++;
> +	}
> +}
>  
> +void __init setup_ext_logbuff(void)
> +{
> +	LOGBUFF_VOLATILE logbuff_t *log;
> +	char *ext_log_buf = NULL;
> +	unsigned long flags;
> +
> +	if (setup_ext_logbuff_mem(&log, &ext_log_buf) < 0) {
> +		printk(KERN_WARNING
> +		       "Failed to setup external logbuffer - ignoring it\n");
> +		return;
> +	}
> +
> +	/* When no properly setup buffer is found, reset pointers */
> +	if (log->tag != LOGBUFF_MAGIC) {
> +		printk(KERN_WARNING
> +		       "Unexpected external log buffer magic number. "
> +		       "Got %08lx, expected %08x. Resetting pointers and using "
> +		       "the buffer anyway.\n",
> +		       log->tag, LOGBUFF_MAGIC);
> +		log->tag = LOGBUFF_MAGIC;
> +		log->start = log->end = log->con = log->chars = 0;
> +	}
> +
> +	spin_lock_irqsave(&logbuf_lock, flags);
> +
> +	logbuff_check_metadata(log);
> +	logbuff_coalesce_buffers(log);
> +
> +	/* Switch to the external log buffer */
> +	ext_log_start		= &log->start;
> +	ext_con_start		= &log->con;
> +	ext_log_end		= &log->end;
> +	ext_logged_chars	= &log->chars;
> +
> +	log_buf			= ext_log_buf;
> +	log_buf_len 		= LOGBUFF_LEN;
> +
> +	spin_unlock_irqrestore(&logbuf_lock, flags);
> +
> +	printk(KERN_NOTICE "log_buf=%p\n", log_buf);
> +}
> +#endif /* CONFIG_LOGBUFFER */
>  static int __init log_buf_len_setup(char *str)
>  {
> +#ifdef CONFIG_LOGBUFFER
> +	/* Log buffer size is LOGBUFF_LEN bytes */
> +	printk(KERN_NOTICE
> +	       "External log buffer configured; "
> +	       "ignoring log_buf_len param.\n");
> +	return 1;
> +#else
>  	unsigned size = memparse(str, &str);
>  	unsigned long flags;
>  
> @@ -173,6 +315,7 @@ static int __init log_buf_len_setup(char *str)
>  	}
>  out:
>  	return 1;
> +#endif /* CONFIG_LOGBUFFER */
>  }
>  
>  __setup("log_buf_len=", log_buf_len_setup);
> @@ -230,7 +373,7 @@ static void boot_delay_msec(void)
>  static inline void boot_delay_msec(void)
>  {
>  }
> -#endif
> +#endif /* CONFIG_BOOT_PRINTK_DELAY */
>  
>  /*
>   * Commands to do_syslog:
> @@ -740,7 +883,7 @@ out_restore_irqs:
>  EXPORT_SYMBOL(printk);
>  EXPORT_SYMBOL(vprintk);
>  
> -#else
> +#else /* !CONFIG_PRINTK */
>  
>  asmlinkage long sys_syslog(int type, char __user *buf, int len)
>  {
> @@ -751,7 +894,7 @@ static void call_console_drivers(unsigned start, unsigned end)
>  {
>  }
>  
> -#endif
> +#endif /* CONFIG_PRINTK */
>  
>  static int __add_preferred_console(char *name, int idx, char *options,
>  				   char *brl_options)
>   


-- 
http://www.hailfinger.org/


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC] Add Alternative Log Buffer Support for printk Messages
  2009-01-21 17:39 Grant Erickson
@ 2009-01-21 20:37 ` David Miller
  2009-01-27 13:36 ` Carl-Daniel Hailfinger
  2009-01-30 19:51 ` Andrew Morton
  2 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2009-01-21 20:37 UTC (permalink / raw)
  To: gerickson; +Cc: linux-kernel

From: Grant Erickson <gerickson@nuovations.com>
Date: Wed, 21 Jan 2009 09:39:46 -0800

> This merges support for the previously DENX-only kernel feature of
> specifying an alternative, "external" buffer for kernel printk
> messages and their associated metadata. In addition, this ports
> architecture support for this feature from arch/ppc to arch/powerpc.
> 
> When this option is enabled, an architecture- or machine-specific log
> buffer is used for all printk messages. This allows entities such as
> boot loaders (e.g. U-Boot) to place printk-compatible messages into
> this buffer and for the kernel to coalesce them with its normal
> messages.
> 
> Signed-off-by: Grant Erickson <gerickson@nuovations.com>

Nice.

This would also be useful on Sparc where it is possible to tell
the firmware to set aside a piece of physical memory which survives
soft resets and thus can be inspected post reboot.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC] Add Alternative Log Buffer Support for printk Messages
@ 2009-01-21 17:39 Grant Erickson
  2009-01-21 20:37 ` David Miller
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Grant Erickson @ 2009-01-21 17:39 UTC (permalink / raw)
  To: linux-kernel

This merges support for the previously DENX-only kernel feature of
specifying an alternative, "external" buffer for kernel printk
messages and their associated metadata. In addition, this ports
architecture support for this feature from arch/ppc to arch/powerpc.

When this option is enabled, an architecture- or machine-specific log
buffer is used for all printk messages. This allows entities such as
boot loaders (e.g. U-Boot) to place printk-compatible messages into
this buffer and for the kernel to coalesce them with its normal
messages.

Signed-off-by: Grant Erickson <gerickson@nuovations.com>
---

The code has historically been used and proven to work on the LWMON5
platform under arch/ppc and is now used (by me) successfully on the
AMCC Haleakala and Kilauea platforms.

As implemented for arch/powerpc, two suboptions for the alternative
log buffer are supported. The buffer may be contiguous with the
metadata and message data colocated or the metadata and message
storage may be in discontiguous regions of memory (e.g. a set of
scratch registers and an SRAM buffer). On Kilauea and Haleakala, I
have used the former; whereas LWMON5 has traditionally used the latter.

The code here is, more or less, as-is from the DENX GIT tree. Comments
welcome. Some prior discussion from the linuxppc-dev mailing list is at
http://ozlabs.org/pipermail/linuxppc-dev/2008-November/065589.html.

 arch/powerpc/kernel/prom.c |   93 +++++++++++++++++++++++++++
 include/linux/logbuff.h    |   56 ++++++++++++++++
 init/Kconfig               |   25 +++++++
 init/main.c                |    4 +
 kernel/printk.c            |  149 +++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 324 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/logbuff.h

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 3a2dc7e..60282f1 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -32,6 +32,7 @@
 #include <linux/debugfs.h>
 #include <linux/irq.h>
 #include <linux/lmb.h>
+#include <linux/logbuff.h>
 
 #include <asm/prom.h>
 #include <asm/rtas.h>
@@ -61,6 +62,15 @@
 #define DBG(fmt...)
 #endif
 
+#ifdef CONFIG_LOGBUFFER
+#ifdef CONFIG_ALT_LB_LOCATION
+# if !defined(BOARD_ALT_LH_ADDR) || !defined(BOARD_ALT_LB_ADDR)
+#  error "Please specify BOARD_ALT_LH_ADDR & BOARD_ALT_LB_ADDR."
+# endif
+#else /* !CONFIG_ALT_LB_LOCATION */
+static phys_addr_t ext_logbuff;
+#endif /* CONFIG_ALT_LB_LOCATION */
+#endif /* CONFIG_LOGBUFFER */
 
 static int __initdata dt_root_addr_cells;
 static int __initdata dt_root_size_cells;
@@ -1018,6 +1028,85 @@ static int __init early_init_dt_scan_memory(unsigned long node,
 	return 0;
 }
 
+#ifdef CONFIG_LOGBUFFER
+#ifdef CONFIG_ALT_LB_LOCATION
+/* Alternative external log buffer mapping: log metadata header & the
+ * character buffer are separated and allocated not in RAM but in some
+ * other memory-mapped I/O region (e.g. log head in unused registers,
+ * and log buffer in OCM memory)
+ */
+int __init setup_ext_logbuff_mem(volatile logbuff_t **lhead, char **lbuf)
+{
+	void *h, *b;
+
+	if (unlikely(!lhead) || unlikely(!lbuf))
+		return -EINVAL;
+
+	/* map log head */
+	h = ioremap(BOARD_ALT_LH_ADDR, sizeof(logbuff_t));
+	if (unlikely(!h))
+		return -EFAULT;
+
+	/* map log buffer */
+	b = ioremap(BOARD_ALT_LB_ADDR, LOGBUFF_LEN);
+	if (unlikely(!b)) {
+		iounmap(h);
+		return -EFAULT;
+	}
+
+	*lhead = h;
+	*lbuf = b;
+
+	return 0;
+}
+#else /* !CONFIG_ALT_LB_LOCATION */
+/* Usual external log-buffer mapping: log metadata header & the character
+ * buffer are both contiguous in system RAM.
+ */
+int __init setup_ext_logbuff_mem(logbuff_t **lhead, char **lbuf)
+{
+	void *p;
+
+	if (unlikely(!lhead) || unlikely(!lbuf))
+		return -EINVAL;
+
+	if (unlikely(!ext_logbuff) || !lmb_is_reserved(ext_logbuff))
+		return -EFAULT;
+
+	p = ioremap(ext_logbuff, LOGBUFF_RESERVE);
+
+	if (unlikely(!p))
+		return -EFAULT;
+
+	*lhead = (logbuff_t *)(p + LOGBUFF_OVERHEAD -
+			       sizeof(logbuff_t) +
+			       sizeof(((logbuff_t *)0)->buf));
+	*lbuf = (*lhead)->buf;
+
+	return 0;
+}
+
+/* When the external log buffer configuration is used with the
+ * non-alternate location, the log head metadata and character buffer
+ * lie in the LOGBUFF_RESERVE bytes at the end of system RAM. Add this
+ * block of memory to the reserved memory pool so that it is not
+ * allocated for other purposes.
+ */
+static void __init reserve_ext_logbuff_mem(void)
+{
+	phys_addr_t top = lmb_end_of_DRAM();
+	phys_addr_t size = LOGBUFF_RESERVE;
+	phys_addr_t base = top - size;
+
+	if (top > base) {
+		ext_logbuff = base;
+		DBG("reserving: %x -> %x\n", base, size);
+		lmb_reserve(base, size);
+	}
+}
+#endif /* CONFIG_ALT_LB_LOCATION */
+#endif /* CONFIG_LOGBUFFER */
+
 static void __init early_reserve_mem(void)
 {
 	u64 base, size;
@@ -1033,6 +1122,10 @@ static void __init early_reserve_mem(void)
 	self_size = initial_boot_params->totalsize;
 	lmb_reserve(self_base, self_size);
 
+#if defined(CONFIG_LOGBUFFER) && !defined(CONFIG_ALT_LB_LOCATION)
+	reserve_ext_logbuff_mem();
+#endif /* defined(CONFIG_LOGBUFFER) && !defined(CONFIG_ALT_LB_LOCATION) */
+
 #ifdef CONFIG_BLK_DEV_INITRD
 	/* then reserve the initrd, if any */
 	if (initrd_start && (initrd_end > initrd_start))
diff --git a/include/linux/logbuff.h b/include/linux/logbuff.h
new file mode 100644
index 0000000..22a51c0
--- /dev/null
+++ b/include/linux/logbuff.h
@@ -0,0 +1,56 @@
+/*
+ * (C) Copyright 2007
+ * Wolfgang Denk, DENX Software Engineering, wd@denx.de.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+#ifndef _LOGBUFF_H_
+#define _LOGBUFF_H_
+
+#ifdef CONFIG_LOGBUFFER
+
+#define LOGBUFF_MAGIC		0xc0de4ced
+#define LOGBUFF_LEN		16384
+#define LOGBUFF_OVERHEAD	4096
+#define LOGBUFF_RESERVE		(LOGBUFF_LEN + LOGBUFF_OVERHEAD)
+
+/* The mapping used here has to be the same as in logbuff_init_ptrs ()
+   in u-boot/common/cmd_log.c */
+
+typedef struct {
+	unsigned long	tag;
+	unsigned long	start;
+	unsigned long	con;	/* next char to be sent to consoles	*/
+	unsigned long	end;
+	unsigned long	chars;
+	unsigned char	buf[0];
+} logbuff_t;
+
+#ifdef CONFIG_ALT_LB_LOCATION
+# define LOGBUFF_VOLATILE	volatile
+#else
+# define LOGBUFF_VOLATILE
+#endif /* defined(CONFIG_ALT_LB_LOCATION) */
+
+extern void setup_ext_logbuff(void);
+/* arch specific */
+extern int setup_ext_logbuff_mem(LOGBUFF_VOLATILE logbuff_t **lhead, char **lbuf);
+
+#endif /* CONFIG_LOGBUFFER */
+#endif /* _LOGBUFF_H_ */
diff --git a/init/Kconfig b/init/Kconfig
index f763762..e1a1b59 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -619,6 +619,31 @@ config PRINTK
 	  very difficult to diagnose system problems, saying N here is
 	  strongly discouraged.
 
+config LOGBUFFER
+	bool "External logbuffer" if PRINTK
+	default n
+	help
+	  This option enables support for an alternative, "external"
+	  printk log buffer. When enabled, an architecture- or machine-
+	  specific log buffer is used for all printk messages. This
+	  allows entities such as boot loaders to place printk-compatible
+	  messages into this buffer and for the kernel to coalesce them
+	  with its normal messages.
+
+config ALT_LB_LOCATION
+	bool "Alternative logbuffer" if LOGBUFFER
+	default n
+	help
+	  When using an alternative, "external" printk log buffer, an
+	  architecture- or machine-specific log buffer with contiguous
+	  metadata and message storage is used. This option enables
+	  support for discontiguous metadata and message storage
+	  memory (e.g. a set of scratch registers and an SRAM
+	  buffer). By saying Y here, you must also ensure your
+	  architecture- or machine-code specify BOARD_ALT_LH_ADDR and
+	  BOARD_ALT_LB_ADDR, for the metadata and message memory,
+	  respectively.
+
 config BUG
 	bool "BUG() support" if EMBEDDED
 	default y
diff --git a/init/main.c b/init/main.c
index 7e117a2..5687b98 100644
--- a/init/main.c
+++ b/init/main.c
@@ -61,6 +61,7 @@
 #include <linux/kthread.h>
 #include <linux/sched.h>
 #include <linux/signal.h>
+#include <linux/logbuff.h>
 #include <linux/idr.h>
 #include <linux/ftrace.h>
 
@@ -563,6 +564,9 @@ asmlinkage void __init start_kernel(void)
  * Interrupts are still disabled. Do necessary setups, then
  * enable them
  */
+#ifdef CONFIG_LOGBUFFER
+	setup_ext_logbuff();
+#endif
 	lock_kernel();
 	tick_init();
 	boot_cpu_init();
diff --git a/kernel/printk.c b/kernel/printk.c
index f492f15..59884e2 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -32,6 +32,7 @@
 #include <linux/security.h>
 #include <linux/bootmem.h>
 #include <linux/syscalls.h>
+#include <linux/logbuff.h>
 
 #include <asm/uaccess.h>
 
@@ -101,9 +102,39 @@ static DEFINE_SPINLOCK(logbuf_lock);
  * The indices into log_buf are not constrained to log_buf_len - they
  * must be masked before subscripting
  */
+#ifdef CONFIG_LOGBUFFER
+/* Indexes to the local log buffer */
+static unsigned long _log_start;
+static unsigned long _con_start;
+static unsigned long _log_end;
+static unsigned long _logged_chars;
+/* These will be switched to the external log buffer */
+#ifndef CONFIG_ALT_LB_LOCATION
+/* usual logbuffer location */
+static unsigned long *ext_log_start = &_log_start;
+static unsigned long *ext_con_start = &_con_start;
+static unsigned long *ext_log_end = &_log_end;
+static unsigned long *ext_logged_chars = &_logged_chars;
+#define log_start	(*ext_log_start)
+#define con_start	(*ext_con_start)
+#define log_end		(*ext_log_end)
+#define logged_chars	(*ext_logged_chars)
+#else /* defined(CONFIG_ALT_LB_LOCATION) */
+/* alternative logbuffer location */
+static volatile unsigned long *ext_log_start = &_log_start;
+static volatile unsigned long *ext_con_start = &_con_start;
+static volatile unsigned long *ext_log_end = &_log_end;
+static volatile unsigned long *ext_logged_chars = &_logged_chars;
+#define log_start       (*((volatile u32 *)ext_log_start))
+#define con_start       (*((volatile u32 *)ext_con_start))
+#define log_end         (*((volatile u32 *)ext_log_end))
+#define logged_chars    (*((volatile u32 *)ext_logged_chars))
+#endif /* !defined(CONFIG_ALT_LB_LOCATION) */
+#else /* !defined(CONFIG_LOGBUFFER) */
 static unsigned log_start;	/* Index into log_buf: next char to be read by syslog() */
 static unsigned con_start;	/* Index into log_buf: next char to be sent to consoles */
 static unsigned log_end;	/* Index into log_buf: most-recently-written-char + 1 */
+#endif /* CONFIG_LOGBUFFER */
 
 /*
  *	Array of consoles built from command line options (console=)
@@ -134,10 +165,121 @@ static int console_may_schedule;
 static char __log_buf[__LOG_BUF_LEN];
 static char *log_buf = __log_buf;
 static int log_buf_len = __LOG_BUF_LEN;
+#ifndef CONFIG_LOGBUFFER
 static unsigned logged_chars; /* Number of chars produced since last read+clear operation */
+#endif /* !defined(CONFIG_LOGBUFFER) */
+#ifdef CONFIG_LOGBUFFER
+/* Sanity check the external log buffer metadata. When an the external
+ * log buffer is enabled, the log metadata is effectively non-volatile
+ * in that the values are preserved from reboot to reboot (until/unless
+ * the system loses power).
+ */
+static void __init logbuff_check_metadata(LOGBUFF_VOLATILE logbuff_t *log)
+{
+	unsigned long chars;
+
+	/* Sanity check the producer and consumer indices. */
+
+	if (log->end - log->start > LOGBUFF_LEN)
+		log->start = log->end - LOGBUFF_LEN;
+
+	if (log->end - log->con > LOGBUFF_LEN)
+		log->con = log->end - LOGBUFF_LEN;
+
+	/* Occasionally, particularly following a reboot, the start
+	 * consumer index is not properly caught up to the console
+	 * consumer index. If this is the case, catch it up so that
+	 * the log buffer doesn't start with, for example,
+	 * "<0>Restarting system.\n" followed by the 'real' start of
+	 * the log.
+	 */
+
+	if (log->con > log->start)
+		log->start = log->con;
+
+	/* Ensure that the number of characters logged reflects the
+	 * characters actually logged based on the producer and
+	 * consumer indices rather than all characters cumulatively
+	 * logged across all reboots since a power-loss event.
+	 */
+
+	chars = log->end - log->start;
+
+	if (log->chars > chars)
+		log->chars = chars;
+}
+
+/* Coalesce the current log bounded buffer and the external log
+ * bounded buffer by appending the former to the latter. Precedence is
+ * given to the external log buffer when there is more data to be
+ * appended than space exists, so the current log buffer is truncated
+ * instead of overwritting the external buffer.
+ */
+static void __init logbuff_coalesce_buffers(LOGBUFF_VOLATILE logbuff_t *log)
+{
+	unsigned long dspace, ssize, len;
+
+	dspace = LOGBUFF_LEN - (log->end - log->start);
+	ssize = log_end - log_start;
+	len = min(dspace, ssize);
+
+	while (len-- > 0) {
+		log->buf[log->end++ & (LOGBUFF_LEN-1)] = LOG_BUF(log_start++);
+		log->chars++;
+	}
+}
 
+void __init setup_ext_logbuff(void)
+{
+	LOGBUFF_VOLATILE logbuff_t *log;
+	char *ext_log_buf = NULL;
+	unsigned long flags;
+
+	if (setup_ext_logbuff_mem(&log, &ext_log_buf) < 0) {
+		printk(KERN_WARNING
+		       "Failed to setup external logbuffer - ignoring it\n");
+		return;
+	}
+
+	/* When no properly setup buffer is found, reset pointers */
+	if (log->tag != LOGBUFF_MAGIC) {
+		printk(KERN_WARNING
+		       "Unexpected external log buffer magic number. "
+		       "Got %08lx, expected %08x. Resetting pointers and using "
+		       "the buffer anyway.\n",
+		       log->tag, LOGBUFF_MAGIC);
+		log->tag = LOGBUFF_MAGIC;
+		log->start = log->end = log->con = log->chars = 0;
+	}
+
+	spin_lock_irqsave(&logbuf_lock, flags);
+
+	logbuff_check_metadata(log);
+	logbuff_coalesce_buffers(log);
+
+	/* Switch to the external log buffer */
+	ext_log_start		= &log->start;
+	ext_con_start		= &log->con;
+	ext_log_end		= &log->end;
+	ext_logged_chars	= &log->chars;
+
+	log_buf			= ext_log_buf;
+	log_buf_len 		= LOGBUFF_LEN;
+
+	spin_unlock_irqrestore(&logbuf_lock, flags);
+
+	printk(KERN_NOTICE "log_buf=%p\n", log_buf);
+}
+#endif /* CONFIG_LOGBUFFER */
 static int __init log_buf_len_setup(char *str)
 {
+#ifdef CONFIG_LOGBUFFER
+	/* Log buffer size is LOGBUFF_LEN bytes */
+	printk(KERN_NOTICE
+	       "External log buffer configured; "
+	       "ignoring log_buf_len param.\n");
+	return 1;
+#else
 	unsigned size = memparse(str, &str);
 	unsigned long flags;
 
@@ -173,6 +315,7 @@ static int __init log_buf_len_setup(char *str)
 	}
 out:
 	return 1;
+#endif /* CONFIG_LOGBUFFER */
 }
 
 __setup("log_buf_len=", log_buf_len_setup);
@@ -230,7 +373,7 @@ static void boot_delay_msec(void)
 static inline void boot_delay_msec(void)
 {
 }
-#endif
+#endif /* CONFIG_BOOT_PRINTK_DELAY */
 
 /*
  * Commands to do_syslog:
@@ -740,7 +883,7 @@ out_restore_irqs:
 EXPORT_SYMBOL(printk);
 EXPORT_SYMBOL(vprintk);
 
-#else
+#else /* !CONFIG_PRINTK */
 
 asmlinkage long sys_syslog(int type, char __user *buf, int len)
 {
@@ -751,7 +894,7 @@ static void call_console_drivers(unsigned start, unsigned end)
 {
 }
 
-#endif
+#endif /* CONFIG_PRINTK */
 
 static int __add_preferred_console(char *name, int idx, char *options,
 				   char *brl_options)
-- 
1.6.0.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2009-03-31 16:05 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <49D17E5D.30306@gmx.net>
2009-03-31 15:58 ` [RFC] Add Alternative Log Buffer Support for printk Messages Grant Erickson
2009-02-18 21:27 Timo Juhani Lindfors
  -- strict thread matches above, loose matches on Subject: below --
2009-01-21 17:39 Grant Erickson
2009-01-21 20:37 ` David Miller
2009-01-27 13:36 ` Carl-Daniel Hailfinger
2009-01-30 19:51 ` Andrew Morton
2009-01-30 22:01   ` Grant Erickson
2009-01-30 22:28     ` Andrew Morton
2009-01-31 10:01       ` Wolfgang Denk
2009-01-31  9:59     ` Wolfgang Denk
2009-02-03  1:08       ` Carl-Daniel Hailfinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).