linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Smita Koralahalli Channabasappa <skoralah@amd.com>
To: Punit Agrawal <punit1.agrawal@toshiba.co.jp>,
	Borislav Petkov <bp@alien8.de>
Cc: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-edac@vger.kernel.org,
	linux-efi@vger.kernel.org, linux-acpi@vger.kernel.org,
	devel@acpica.org, Tony Luck <tony.luck@intel.com>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Len Brown <len.brown@intel.com>, Ard Biesheuvel <ardb@kernel.org>,
	Yazen Ghannam <yazen.ghannam@amd.com>
Subject: Re: [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA handling chain
Date: Thu, 24 Sep 2020 12:23:27 -0500	[thread overview]
Message-ID: <52c50f37-a86c-57ad-30e0-dac0857e4ef7@amd.com> (raw)
In-Reply-To: <87pn6chwil.fsf@kokedama.swc.toshiba.co.jp>

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&amp;data=02%7C01%7CSmita.KoralahalliChannabasappa%40amd.com%7C1e8d8042158141af2c0a08d8601d31d7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637365025808391248&amp;sdata=C71Gp1ZNQhtckegVJbYPA%2FTNi6np%2Fl1Xl4BvI4kGX4Y%3D&amp;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


  reply	other threads:[~2020-09-24 17:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-04 14:04 [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA handling chain Smita Koralahalli
2020-09-23 10:07 ` Punit Agrawal
2020-09-23 14:05   ` Borislav Petkov
2020-09-23 14:52     ` Ard Biesheuvel
2020-09-23 15:39       ` Borislav Petkov
2020-09-23 18:24         ` Ard Biesheuvel
2020-09-24  0:02     ` Punit Agrawal
2020-09-24 17:23       ` Smita Koralahalli Channabasappa [this message]
2020-09-24 17:50         ` Borislav Petkov
2020-09-25  0:54           ` Punit Agrawal
2020-09-25  7:07             ` Borislav Petkov
2020-09-25 16:19             ` Yazen Ghannam
2020-09-25 16:27               ` Borislav Petkov
2020-09-28  8:06               ` Punit Agrawal

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52c50f37-a86c-57ad-30e0-dac0857e4ef7@amd.com \
    --to=skoralah@amd.com \
    --cc=Smita.KoralahalliChannabasappa@amd.com \
    --cc=ardb@kernel.org \
    --cc=bp@alien8.de \
    --cc=devel@acpica.org \
    --cc=len.brown@intel.com \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=punit1.agrawal@toshiba.co.jp \
    --cc=rjw@rjwysocki.net \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --cc=yazen.ghannam@amd.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).