* Sean Christopherson <sean.j.christopher...@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.christopher...@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.christopher...@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