Messages in this thread | | | Subject | Re: [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA handling chain | From | Smita Koralahalli Channabasappa <> | Date | Thu, 24 Sep 2020 12:23:27 -0500 |
| |
On 9/23/20 7:02 PM, Punit Agrawal wrote:
> Borislav Petkov <bp@alien8.de> 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
| |