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

From: Ghannam, Yazen
Date: Tue Feb 27 2018 - 13:06:29 EST


> -----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