[PATCH] x86, boot: add hex output for debugging

[PATCH] x86, boot: add hex output for debugging

[Kees Cook]

This is useful for reporting various addresses or other values while
debugging early boot.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/boot/compressed/misc.c | 19 +++++++++++++++++++
 arch/x86/boot/compressed/misc.h |  5 +++++
 2 files changed, 24 insertions(+)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 30dd59a9f0b4..1b105664f720 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -220,6 +220,25 @@ void __putstr(const char *s)
 	outb(0xff & (pos >> 1), vidport+1);
 }
 
+void __puthex(unsigned long value)
+{
+	char alpha[2] = "0";
+	int bits;
+	unsigned char byte;
+
+	__putstr("0x");
+	for (bits = sizeof(value) * 8 - 4; bits >= 0; bits -= 4) {
+		unsigned long digit = (value >> bits) & 0xf;
+
+		if (digit < 0xA)
+			alpha[0] = '0' + digit;
+		else
+			alpha[0] = 'a' + (digit - 0xA);
+
+		__putstr(alpha);
+	}
+}
+
 static void error(char *x)
 {
 	error_putstr("\n\n");
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 24e3e569a13c..6056d339fb17 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -34,16 +34,21 @@ extern memptr free_mem_ptr;
 extern memptr free_mem_end_ptr;
 extern struct boot_params *real_mode;		/* Pointer to real-mode data */
 void __putstr(const char *s);
+void __puthex(unsigned long value);
 #define error_putstr(__x)  __putstr(__x)
+#define error_puthex(__x)  __puthex(__x)
 
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 
 #define debug_putstr(__x)  __putstr(__x)
+#define debug_puthex(__x)  __puthex(__x)
 
 #else
 
 static inline void debug_putstr(const char *s)
 { }
+static inline void debug_puthex(const char *s)
+{ }
 
 #endif
 
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] x86, boot: add hex output for debugging

[Randy Dunlap]

On 10/31/14 09:20, Kees Cook wrote:
> This is useful for reporting various addresses or other values while
> debugging early boot.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/x86/boot/compressed/misc.c | 19 +++++++++++++++++++
>  arch/x86/boot/compressed/misc.h |  5 +++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 30dd59a9f0b4..1b105664f720 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -220,6 +220,25 @@ void __putstr(const char *s)
>  	outb(0xff & (pos >> 1), vidport+1);
>  }
>  
> +void __puthex(unsigned long value)
> +{
> +	char alpha[2] = "0";
> +	int bits;
> +	unsigned char byte;

what is 'byte' for?  (unused)

> +
> +	__putstr("0x");
> +	for (bits = sizeof(value) * 8 - 4; bits >= 0; bits -= 4) {
> +		unsigned long digit = (value >> bits) & 0xf;
> +
> +		if (digit < 0xA)
> +			alpha[0] = '0' + digit;
> +		else
> +			alpha[0] = 'a' + (digit - 0xA);
> +
> +		__putstr(alpha);
> +	}
> +}
> +
>  static void error(char *x)
>  {
>  	error_putstr("\n\n");
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 24e3e569a13c..6056d339fb17 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -34,16 +34,21 @@ extern memptr free_mem_ptr;
>  extern memptr free_mem_end_ptr;
>  extern struct boot_params *real_mode;		/* Pointer to real-mode data */
>  void __putstr(const char *s);
> +void __puthex(unsigned long value);
>  #define error_putstr(__x)  __putstr(__x)
> +#define error_puthex(__x)  __puthex(__x)
>  
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>  
>  #define debug_putstr(__x)  __putstr(__x)
> +#define debug_puthex(__x)  __puthex(__x)
>  
>  #else
>  
>  static inline void debug_putstr(const char *s)
>  { }
> +static inline void debug_puthex(const char *s)
> +{ }
>  
>  #endif
>  
> 


-- 
~Randy

Re: [PATCH] x86, boot: add hex output for debugging

[Andi Kleen]

> > +void __puthex(unsigned long value)
> > +{
> > +	char alpha[2] = "0";
> > +	int bits;
> > +	unsigned char byte;
> 
> what is 'byte' for?  (unused)

Well the whole function is unused. We don't normally add unused functions
to the code because they bitrot too easily.

If you need it please just patch it in yourself.

-Andi

Re: [PATCH] x86, boot: add hex output for debugging

[Vivek Goyal]

On Fri, Oct 31, 2014 at 10:42:51AM -0700, Andi Kleen wrote:
> > > +void __puthex(unsigned long value)
> > > +{
> > > +	char alpha[2] = "0";
> > > +	int bits;
> > > +	unsigned char byte;
> > 
> > what is 'byte' for?  (unused)
> 
> Well the whole function is unused. We don't normally add unused functions
> to the code because they bitrot too easily.

I think this is useful. I had to write similar code for printing out
values during early boot.

May be we can print some values if CONFIG_X86_VERBOSE_BOOTUP=y.
That way it will not be an unused code and others can reuse it easily
to print additional data during debugging.

Thanks
Vivek

Re: [PATCH] x86, boot: add hex output for debugging

[H. Peter Anvin]

On 10/31/2014 01:11 PM, Vivek Goyal wrote:
> On Fri, Oct 31, 2014 at 10:42:51AM -0700, Andi Kleen wrote:
>>>> +void __puthex(unsigned long value)
>>>> +{
>>>> +	char alpha[2] = "0";
>>>> +	int bits;
>>>> +	unsigned char byte;
>>>
>>> what is 'byte' for?  (unused)
>>
>> Well the whole function is unused. We don't normally add unused functions
>> to the code because they bitrot too easily.
> 
> I think this is useful. I had to write similar code for printing out
> values during early boot.
> 
> May be we can print some values if CONFIG_X86_VERBOSE_BOOTUP=y.
> That way it will not be an unused code and others can reuse it easily
> to print additional data during debugging.
> 

The various addresses involved in decompression (load address, relocated
address, etc.) might be good ideas.

	-hpa

Re: [PATCH] x86, boot: add hex output for debugging

[josh]

On Fri, Oct 31, 2014 at 09:20:37AM -0700, Kees Cook wrote:
> This is useful for reporting various addresses or other values while
> debugging early boot.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

You haven't provided any user of this function.  I don't think this
should get merged without a caller (nor should an artificial caller be
added).  What's your use case for adding this?

Also, while I realize __putstr already has this problem, ideally all the
printing functions in this file should go in some separate source file
that gets omitted when !CONFIG_PRINTK (or possibly
!CONFIG_EARLY_PRINTK).

- Josh Triplett

>  arch/x86/boot/compressed/misc.c | 19 +++++++++++++++++++
>  arch/x86/boot/compressed/misc.h |  5 +++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 30dd59a9f0b4..1b105664f720 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -220,6 +220,25 @@ void __putstr(const char *s)
>  	outb(0xff & (pos >> 1), vidport+1);
>  }
>  
> +void __puthex(unsigned long value)
> +{
> +	char alpha[2] = "0";
> +	int bits;
> +	unsigned char byte;
> +
> +	__putstr("0x");
> +	for (bits = sizeof(value) * 8 - 4; bits >= 0; bits -= 4) {
> +		unsigned long digit = (value >> bits) & 0xf;
> +
> +		if (digit < 0xA)
> +			alpha[0] = '0' + digit;
> +		else
> +			alpha[0] = 'a' + (digit - 0xA);
> +
> +		__putstr(alpha);
> +	}
> +}
> +
>  static void error(char *x)
>  {
>  	error_putstr("\n\n");
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 24e3e569a13c..6056d339fb17 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -34,16 +34,21 @@ extern memptr free_mem_ptr;
>  extern memptr free_mem_end_ptr;
>  extern struct boot_params *real_mode;		/* Pointer to real-mode data */
>  void __putstr(const char *s);
> +void __puthex(unsigned long value);
>  #define error_putstr(__x)  __putstr(__x)
> +#define error_puthex(__x)  __puthex(__x)
>  
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>  
>  #define debug_putstr(__x)  __putstr(__x)
> +#define debug_puthex(__x)  __puthex(__x)
>  
>  #else
>  
>  static inline void debug_putstr(const char *s)
>  { }
> +static inline void debug_puthex(const char *s)
> +{ }
>  
>  #endif
>  
> -- 
> 1.9.1
> 
> 
> -- 
> Kees Cook
> Chrome OS Security
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [PATCH] x86, boot: add hex output for debugging

[Kees Cook]

On Fri, Oct 31, 2014 at 1:33 PM,  <josh@joshtriplett.org> wrote:
> On Fri, Oct 31, 2014 at 09:20:37AM -0700, Kees Cook wrote:
>> This is useful for reporting various addresses or other values while
>> debugging early boot.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> You haven't provided any user of this function.  I don't think this
> should get merged without a caller (nor should an artificial caller be
> added).  What's your use case for adding this?

I'll fix that.

> Also, while I realize __putstr already has this problem, ideally all the
> printing functions in this file should go in some separate source file
> that gets omitted when !CONFIG_PRINTK (or possibly
> !CONFIG_EARLY_PRINTK).

Hm, I don't agree: we need error_putstr, not just debug_putstr, and
early_printk is just for serial console, where as __putstr works
against the boot block's defined video area too, IIUC.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] x86, boot: add hex output for debugging

[Josh Triplett]

On Fri, Oct 31, 2014 at 01:55:33PM -0700, Kees Cook wrote:
> On Fri, Oct 31, 2014 at 1:33 PM,  <josh@joshtriplett.org> wrote:
> > On Fri, Oct 31, 2014 at 09:20:37AM -0700, Kees Cook wrote:
> >> This is useful for reporting various addresses or other values while
> >> debugging early boot.
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >
> > You haven't provided any user of this function.  I don't think this
> > should get merged without a caller (nor should an artificial caller be
> > added).  What's your use case for adding this?
> 
> I'll fix that.

Thanks.

> > Also, while I realize __putstr already has this problem, ideally all the
> > printing functions in this file should go in some separate source file
> > that gets omitted when !CONFIG_PRINTK (or possibly
> > !CONFIG_EARLY_PRINTK).
> 
> Hm, I don't agree: we need error_putstr, not just debug_putstr, and
> early_printk is just for serial console, where as __putstr works
> against the boot block's defined video area too, IIUC.

OK, that seems like a good argument that it shouldn't go under
EARLY_PRINTK.  However, for size-constrained systems that have nowhere
for the output to go at all, it still makes sense to compile all of the
printing infrastructure out of the decompression stub, especially since
that code itself does not get compressed and thus has a higher impact
than code in vmlinux.

- Josh Triplett