Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 6 Dec 2018 17:23:53 +0100
From:   Ingo Molnar <mingo@kernel.org>
To:     Sean Christopherson <sean.j.christopherson@intel.com>
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
        linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Rik van Riel <riel@surriel.com>,
        Yu-cheng Yu <yu-cheng.yu@intel.com>
Subject: Re: [PATCH] x86/mm/fault: Streamline the fault error_code decoder
 some more
Message-ID: <20181206162353.GA119052@gmail.com>
References: <20181205163624.1842-1-sean.j.christopherson@intel.com>
 <20181206073409.GA64887@gmail.com>
 <20181206155336.GC31263@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181206155336.GC31263@linux.intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* Sean Christopherson <sean.j.christopherson@intel.com> wrote:

> On Thu, Dec 06, 2018 at 08:34:09AM +0100, Ingo Molnar wrote:
> > I like your '!' idea, but with a further simplification: how about using 
> > '-/+' differentiation and a single character and a fixed-length message.
> > 
> > The new output will be lines of:
> > 
> >   #PF error code: -P -W -U -S -I -K (0x00)
> >   ...
> >   #PF error code: -P -W +U +S -I -K (0x0c)
> >   ...
> >   #PF error code: +P +W +U +S +I +K (0x3f)
> > 
> > The symbol abbreviations are pretty self-explanatory:
> > 
> >   P = protection fault   (X86_PF_PROT)
> >   W = write access       (X86_PF_WRITE)
> >   U = user-mode access   (X86_PF_USER)
> >   S = supervisor mode    (X86_PF_RSVD)
> >   I = instruction fault  (X86_PF_INSTR)
> >   K = keys fault         (X86_PF_PK)
> > 
> > Misc notes:
> > 
> > - In principle the new text is now short enough to include it in one of 
> >   the existing output lines, further shortening the oops output - but I
> >   havent done that in this patch.
> > 
> > - Another question is the ordering of the bits: the symbolic display is 
> >   actually big endian, while the numeric hexa printout is little endian.
> > 
> >   I kind of still like it that way, not just because the decoding loop is 
> >   more natural, but because the bits are actually ordered by importance: 
> >   the PROT bits is more important than the INSTR or the PK bits - and the 
> >   more important bits are displayed first.
> 
> Hmm, my eyes tend to be drawn to the end of the line, e.g. having PROT
> be the last thing makes it stand out more than being buried in the middle
> of the line.  Inverting the ordering between raw and decoded also makes
> it very difficult to correlate the raw value with each bit.
> 
> > - Only build-tested the patch and looked at the generated assembly, but 
> >   it all looks sane enough so will obviously work just fine! ;-)
> 
> ...
> 
> >  /*
> > - * This helper function transforms the #PF error_code bits into
> > - * "[PROT] [USER]" type of descriptive, almost human-readable error strings:
> > + * This maps the somewhat obscure error_code number to symbolic text:
> > + *
> > + * P = protection fault   (X86_PF_PROT)
> > + * W = write access       (X86_PF_WRITE)
> > + * U = user-mode access   (X86_PF_USER)
> > + * S = supervisor mode    (X86_PF_RSVD)
> > + * I = instruction fault  (X86_PF_INSTR)
> > + * K = keys fault         (X86_PF_PK)
> >   */
> > -static void err_str_append(unsigned long error_code, char *buf, unsigned long mask, const char *txt)
> > +static const char error_code_chars[] = "PWUSIK";
> > +
> > +/*
> > + * This helper function transforms the #PF error_code bits into " +P -W +U -R -I -K"
> > + * type of descriptive, almost human-readable error strings:
> > + */
> > +static void show_error_code(struct pt_regs *regs, unsigned long error_code)
> 
> No need for @regs.
> 
> >  {
> > -     if (error_code & mask) {
> > -             if (buf[0])
> > -                     strcat(buf, " ");
> > -             strcat(buf, txt);
> > +     unsigned int bit, mask;
> > +     char err_txt[6*3+1]; /* Fixed length of 6 bits decoded plus zero at the end */
> 
> Assuming the error code bits are contiguous breaks if/when SGX gets added,
> which uses bit 15.  Oopsing on an SGX fault should be nigh impossible, but
> it might be nice to be able to reuse show_error_code in other places.
> 
> Hardcoding "6" is also a bit painful.
> 
> > +
> > +     /* We go from the X86_PF_PROT bit to the X86_PF_PK bit: */
> > +
> > +     for (bit = 0; bit < 6; bit++) {
> > +             unsigned int offset = bit*3;
> > +
> > +             err_txt[offset+0] = ' ';
> > +
> > +             mask = 1 << bit;
> > +             if (error_code & mask)
> > +                     err_txt[offset+1] = '+';
> > +             else
> > +                     err_txt[offset+1] = '-';
> 
> To me, using '!' contrasts better when side-by-side with '+'.
> 
> > +
> > +             err_txt[offset+2] = error_code_chars[bit];
> >       }
> > +
> > +     /* Close the string: */
> > +     err_txt[sizeof(err_txt)-1] = 0;
> > +
> > +     pr_alert("#PF error code: %s (%02lx)\n", err_txt, error_code);
> 
> The changelog example has a leading "0x" on the error code.  That being
> said, I actually like it without the "0x".
> 
> How about printing the raw value before the colon?  Having it at the end
> makes it look like extra noise.  And for me, seeing the raw code first
> (reading left to right) cue's my brain that it's about to decode some
> bits.
> 
> SGX will also break the two digit printing.  And for whatever reason four
> digits makes me think "this is an error code!".  IIRC the vectoring ucode
> makes a lot of assumptions about the error code being at most 16 bits, so
> in theory four digits is all we'll ever need.
> 
> E.g.
> 
> [    0.144247] #PF error code:  +P -W -U -S -I -K (01)
> [    0.144411] #PF error code:  +P +W -U -S -I -K (03)
> [    0.144826] #PF error code:  +P +W +U -S -I -K (07)
> [    0.145252] #PF error code:  +P -W +U -S -I +K (25)
> [    0.145706] #PF error code:  -P +W -U -S -I -K (02)
> [    0.146111] #PF error code:  -P -W +U -S -I -K (04)
> [    0.146521] #PF error code:  -P +W +U -S -I -K (06)
> [    0.146934] #PF error code:  -P -W +U -S +I -K (14)
> [    0.147348] #PF error code:  +P -W -U -S +I -K (11)
> [    0.147767] #PF error code:  -P -W -U -S -I -K (00)
> 
> vs. (with SGX added as 'G' for testing purposes)
> 
> [    0.158849] #PF error code(0001):  +P !W !U !S !I !K !G
> [    0.159292] #PF error code(0003):  +P +W !U !S !I !K !G
> [    0.159742] #PF error code(0007):  +P +W +U !S !I !K !G
> [    0.160190] #PF error code(0025):  +P !W +U !S !I +K !G
> [    0.160638] #PF error code(0002):  !P +W !U !S !I !K !G
> [    0.161087] #PF error code(0004):  !P !W +U !S !I !K !G
> [    0.161538] #PF error code(0006):  !P +W +U !S !I !K !G
> [    0.161992] #PF error code(0014):  !P !W +U !S +I !K !G
> [    0.162450] #PF error code(0011):  +P !W !U !S +I !K !G
> [    0.162667] #PF error code(8001):  +P !W !U !S !I !K +G
> [    0.162667] #PF error code(8003):  +P +W !U !S !I !K +G
> [    0.162667] #PF error code(8007):  +P +W +U !S !I !K +G
> [    0.162667] #PF error code(8025):  +P !W +U !S !I +K +G
> [    0.162667] #PF error code(8002):  !P +W !U !S !I !K +G
> [    0.162667] #PF error code(8004):  !P !W +U !S !I !K +G
> [    0.162667] #PF error code(8006):  !P +W +U !S !I !K +G
> [    0.162667] #PF error code(8014):  !P !W +U !S +I !K +G
> [    0.162667] #PF error code(8011):  +P !W !U !S +I !K +G
> [    0.162667] #PF error code(0000):  !P !W !U !S !I !K !G
> 
> vs. (with consistent ordering between raw and decoded)
> 
> [    0.153004] #PF error code(0001):  !K !I !S !U !W +P
> [    0.153004] #PF error code(0003):  !K !I !S !U +W +P
> [    0.153004] #PF error code(0007):  !K !I !S +U +W +P
> [    0.153004] #PF error code(0025):  +K !I !S +U !W +P
> [    0.153004] #PF error code(0002):  !K !I !S !U +W !P
> [    0.153004] #PF error code(0004):  !K !I !S +U !W !P
> [    0.153004] #PF error code(0006):  !K !I !S +U +W !P
> [    0.153362] #PF error code(0014):  !K +I !S +U !W !P
> [    0.153788] #PF error code(0011):  !K +I !S !U !W +P
> [    0.154104] #PF error code(0000):  !K !I !S !U !W !P

Ok, looks nice enough to me, with one request: please make it 0x prefixed 
as is common with hexa code: "0010" could be binary, octal or decimal as 
well. Since this is a separate line we don't lack the space.

Also some nits:

> A patch with the kitchen sink...
> ================================>
> 
> From 22e6d40e52b4341a424f065a138be54bc79d4db4 Mon Sep 17 00:00:00 2001
> From: Sean Christopherson <sean.j.christopherson@intel.com>
> Date: Thu, 6 Dec 2018 07:25:13 -0800
> Subject: [PATCH] x86/fault: Make show_error_code() more extensible and tweak
>  its formatting
> 
>   - Initialize each bit individually in the error_code_chars array.  This
>     allows for non-contiguous bits and is self-documenting.  Define a
>     macro to make the initialization code somewhatmore readable.
> 
>   - Reverse the decode order so it's consistent with the raw display.
> 
>   - Use ARRAY_SIZE instead of hardcoding '6' in multiple locations.
> 
>   - Use '!' for the negative case to better contrast against '+'.
> 
>   - Display four digits (was two) when printing the raw error code.
> 
>   - Remove @regs from show_error_code().
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/mm/fault.c | 47 ++++++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 23dc7163f6ac..cd28d058ed39 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -605,45 +605,48 @@ static void show_ldttss(const struct desc_ptr *gdt, const char *name, u16 index)
>  
>  /*
>   * This maps the somewhat obscure error_code number to symbolic text:
> - *
> - * P = protection fault   (X86_PF_PROT)
> - * W = write access       (X86_PF_WRITE)
> - * U = user-mode access   (X86_PF_USER)
> - * S = supervisor mode    (X86_PF_RSVD)
> - * I = instruction fault  (X86_PF_INSTR)
> - * K = keys fault         (X86_PF_PK)
>   */
> -static const char error_code_chars[] = "PWUSIK";
> +#define EC(x) ilog2(X86_PF_##x)
> +static const char error_code_chars[] = {
> +	[EC(PROT)]	= 'P',
> +	[EC(WRITE)]	= 'W',
> +	[EC(USER)]	= 'U',
> +	[EC(RSVD)]	= 'S',
> +	[EC(INSTR)]	= 'I',
> +	[EC(PK)]	= 'K',
> +};

Please use an extra newline between the #define and the variable 
definition.

>  
>  /*
> - * This helper function transforms the #PF error_code bits into " +P -W +U -R -I -K"
> + * This helper function transforms the #PF error_code bits into " +P !W +U !R !I !K"
>   * type of descriptive, almost human-readable error strings:
>   */
> -static void show_error_code(struct pt_regs *regs, unsigned long error_code)
> +static void show_error_code(unsigned long error_code)
>  {
> -	unsigned int bit, mask;
> -	char err_txt[6*3+1]; /* Fixed length of 6 bits decoded plus zero at the end */
> +	char err_txt[ARRAY_SIZE(error_code_chars)*3+1]; /* 3 chars per bit plus zero at the end */
> +	unsigned offset = 0;
> +	unsigned long mask;
> +	int i;
>  
> -	/* We go from the X86_PF_PROT bit to the X86_PF_PK bit: */
> -
> -	for (bit = 0; bit < 6; bit++) {
> -		unsigned int offset = bit*3;
> +	for (i = ARRAY_SIZE(error_code_chars) - 1; i >= 0; i--) {
> +		if (!error_code_chars[i])
> +			continue;
>  
>  		err_txt[offset+0] = ' ';
>  
> -		mask = 1 << bit;
> +		mask = 1 << i;
>  		if (error_code & mask)
>  			err_txt[offset+1] = '+';
>  		else
> -			err_txt[offset+1] = '-';
> +			err_txt[offset+1] = '!';
>  
> -		err_txt[offset+2] = error_code_chars[bit];
> +		err_txt[offset+2] = error_code_chars[i];
> +		offset += 3;
>  	}
>  
>  	/* Close the string: */
> -	err_txt[sizeof(err_txt)-1] = 0;
> +	err_txt[offset] = 0;
>  
> -	pr_alert("#PF error code: %s (%02lx)\n", err_txt, error_code);
> +	pr_alert("#PF error code(%04lx): %s\n", error_code, err_txt);

0x%04lx here, but other than that looks good to me!

Thanks,

	Ingo