Re: [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA handling chain

[Punit Agrawal]

Borislav Petkov <bp@xxxxxxxxx> writes:

> Smita,
>
> pls sync the time of the box where you create the patch:
>
>  Date: Fri,  4 Sep 2020 09:04:44 -0500
>
> but your mail headers have:
>
>  Received: from ... with mapi id 15.20.3370.019; Fri, 18 Sep 2020 14:49:12 +0000
>  						^^^^^^^^^^^^^^^^^^
>
> On Wed, Sep 23, 2020 at 07:07:17PM +0900, Punit Agrawal wrote:
>> I know Boris asked you to add the reason for the Reported-by, but
>> usually we don't track version differences in the committed patch.
>> 
>> Boris, can you confirm if you want the Reported-by to be retained?
>
> How else would you explain what the Reported-by: tag is for on a patch
> which adds a feature?

As Ard clarified, I was questioning the inclusion of the Reported-by:
tag in the patch itself. But I also don't have enough of a strong
opinion to obsess about it.

[ Aside: One interesting consequence of this though is that by the same
argument, changes resulting from comments on earlier versions are also
legitimate content for the final patch. Not saying I agree. ]

>
>> > + * The first expected register in the register layout of MCAX address space.
>> > + * The address defined must match with the first MSR address extracted from
>> > + * BERT which in SMCA systems is the bank's MCA_STATUS register.
>> > + *
>> > + * Note that the decoding of the raw MSR values in BERT is implementation
>> > + * specific and follows register offset order of MCAX address space.
>> > + */
>> > +#define MASK_MCA_STATUS 0xC0002001
>> 
>> The macro value is already defined in mce.h as
>> MSR_AMD64_SMCA_MC0_STATUS.  Is there any reason to not use it?
>
> Good point.
>
>> You can move the comment to where you check the status register.
>
> No need if he really wants to use the first MCi_STATUS address.
>
>> > +	m.apicid = lapic_id;
>> > +	m.bank = (ctx_info->msr_addr >> 4) & 0xFF;
>> > +	m.status = *i_mce;
>> > +	m.addr = *(i_mce + 1);
>> > +	m.misc = *(i_mce + 2);
>> > +	/* Skipping MCA_CONFIG */
>> > +	m.ipid = *(i_mce + 4);
>> > +	m.synd = *(i_mce + 5);
>> 
>> Instead of using the raw pointer arithmetic, it is better to define a
>> structure for the MCA registers? Something like -
>> 
>>     struct {
>>         u64 addr;
>>         u64 misc;
>>         u64 config;
>>         u64 ipid;
>>         ...
>>     }
>> 
>> Checking back, this was mentioned in the previous review comments as
>> well. Please address all comments before posting a new version - either
>> by following the suggestion or explaining why it is not a good idea.
>
> Well, that was addressed in his reply last time:
>
> https://lkml.kernel.org/r/a28aa613-8353-0052-31f6-34bc733abf59@xxxxxxx

Oops. My bad - sorry I missed the response.

Copying the relevant comment here for discussion -

>>> The registers here are implementation specific and applies only for
>>> SMCA systems. So I have used pointer arithmetic as it is not defined
>>> in the spec.

Even though it's not defined in the UEFI spec, it doesn't mean a
structure definition cannot be created. After all, the patch is relying
on some guarantee of the meaning of the values and their ordering.

If the patch is relying on the definitions in the SMCA spec it is a good
idea to reference it here - both for review and providing relevant
context for future developers.

> You might've missed it because you weren't CCed directly.

Indeed, I missed it. Thanks for the pointer.

Cheers,
Punit