Re: [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA handling chain
From: Smita Koralahalli Channabasappa
Date: Thu Sep 24 2020 - 13:23:44 EST
On 9/23/20 7:02 PM, Punit Agrawal wrote:
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
^^^^^^^^^^^^^^^^^^
Sorry for the trouble. I have fixed this.
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.
I indeed missed it. thanks!
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.
Okay!
+ 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.kernel.org%2Fr%2Fa28aa613-8353-0052-31f6-34bc733abf59%40amd.com&data=02%7C01%7CSmita.KoralahalliChannabasappa%40amd.com%7C1e8d8042158141af2c0a08d8601d31d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637365025808391248&sdata=C71Gp1ZNQhtckegVJbYPA%2FTNi6np%2Fl1Xl4BvI4kGX4Y%3D&reserved=0
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.
Okay, I agree the structure definition will make the code less arbitrary
and provides relevant context compared to pointer arithmetic. I did not
think this way. I can try this out if no objections.
You might've missed it because you weren't CCed directly.
Indeed, I missed it. Thanks for the pointer.
Sorry, I missed including you on CC. Will include henceforth!
Thanks,
Smita