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 6:23 AM
> 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 Mon, Feb 26, 2018 at 01:38:58PM -0600, Yazen Ghannam wrote:
> > + * We don't need a "CPER_IA" prefix since these are all locally defined.
> > + * This will save us a lot of line space.
> > + */
> > +#define VALID_LAPIC_ID			BIT_ULL(0)
> > +#define VALID_CPUID_INFO		BIT_ULL(1)
> > +
> > +void cper_print_proc_ia(const char *pfx, const struct cper_sec_proc_ia
> *proc)
> > +{
> > +	printk("%sValidation Bits: 0x%016llx\n", pfx, proc->validation_bits);
> 
> Ok, so this...
> 
> > +
> > +	if (proc->validation_bits & VALID_LAPIC_ID)
> > +		printk("%sLocal APIC_ID: 0x%llx\n", pfx, proc->lapic_id);
> > +
> > +	if (proc->validation_bits & VALID_CPUID_INFO) {
> > +		printk("%sCPUID Info:\n", pfx);
> > +		print_hex_dump(pfx, "", DUMP_PREFIX_OFFSET, 16, 4, proc-
> >cpuid,
> > +			       sizeof(proc->cpuid), 0);
> 
> ... and this are semi-decoded information bits which I'd have to go open
> the spec and continue decoding.
> 
> Can we please change the whole approach of not simply dumping such
> fields but decode them fully. We want that error information to be
> helpful to the user and she should be able to immediately understand
> what type of error it is.
> 
> Validation Bits and a CPUID hexdump simply makes you go look at the spec
> again and such dumps are only useful as a debugging aid but nothing
> more.
> 
> The APIC ID dump is how this should be done - properly and fully decoded
> error info which can be used immediately for diagnosing the error.
> 

I decided to print the Validation Bits as a sanity check for whomever is looking
at this. Since we only print fields with a valid bit, it may be confusing for users
who don't know why fields are missing. They can check the Validation Bits to see
that the fields printed have a valid bit set and those not printed don't have valid
bits set.

Also, I don't think we should be interpreting the spec for the user. We should
print the fields as they and users can refer back to the spec for more information.

The more detailed info about the error is printed later. Even just printing the fields
at that point should be enough for most users.

Thanks,
Yazen