RE: [PATCH v2 2/8] efi: Decode IA32/X64 Processor Error Section

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@xxxxxxx]
> Sent: Tuesday, February 27, 2018 12:45 PM
> To: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx>
> Cc: linux-efi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> ard.biesheuvel@xxxxxxxxxx; x86@xxxxxxxxxx
> Subject: Re: [PATCH v2 2/8] efi: Decode IA32/X64 Processor Error Section
> 
> On Tue, Feb 27, 2018 at 05:27:39PM +0000, Ghannam, Yazen wrote:
> > Sure, we can print the fields unconditionally if Ard is okay with that.
> >
> > The issue is that the x86 behavior will be different from all the others, so
> that
> > might be confusing.
> 
> Confusing for whom?
> 
> Are we sharing tools with other arches or what am I missing?
> 

It's not just about arches but record types. A single platform can report
using arch-specific records, memory records, PCIe records, etc.

So all the other record types only show fields with a valid bit set and this
has always been the case. There may be users or tools who expect that
same behavior.

> > This set does decode everything fully so that people can read the error.
> 
> No, it doesn't. It dumps
> 
>         printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
> 
>         printk("%sError Structure Type: %pUl\n", newpfx,
>                 &err_info->err_type);
> 
>         printk("%sCheck Information: 0x%016llx\n", newpfx,
>                  err_info->check_info);
> 
> and so on which are half-baked.
> 
> Think of it this way: if you look at the error record and you still need
> to look at the spec to decode it, then it is still not good enough.
> 
> Users don't care about validation bits, or unreadable GUIDs or WTF is
> "Check Information" even?
> 
> "This section details the layout of the Processor Error Information
> Structure and the detailed check information which is contained within."
> 
> And that "Check Information" thing is mentioned only twice in the whole
> spec.
> 
> StructureErrorType only there in the table.
> 
> Very informative that.
> 
> So no, users don't care about any of that internal crap - they wanna
> know what is wrong with their box and that should be written in plain
> english.
> 

Please see the other patches where these are decoded further. As I
mentioned the cover letter, the decoding is applied incrementally rather
than having one large patch.

Also, like I said in another thread, we should always print the raw value
followed by the decoding. That way the raw info is there in case a user
wants to send the data to the vendor or other expert party. 

> > I already mentioned in the Context info patch that there could be
> > further decoding which we can do in the future.
> 
> Then decode only those pieces fully now which you can do now and when
> you add something in the future, add it then with the full decoding
> functionality. No fields which need additional decoding with the spec
> opened on one side.
> 

Okay.

Thanks,
Yazen