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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Sep 2020 16:05:12 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Punit Agrawal <punit1.agrawal@...hiba.co.jp>
Cc:     Smita Koralahalli <Smita.KoralahalliChannabasappa@....com>,
        x86@...nel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-edac@...r.kernel.org,
        linux-efi@...r.kernel.org, linux-acpi@...r.kernel.org,
        devel@...ica.org, Tony Luck <tony.luck@...el.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Len Brown <len.brown@...el.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Yazen Ghannam <yazen.ghannam@....com>
Subject: Re: [PATCH v4] cper, apei, mce: Pass x86 CPER through the MCA
 handling chain

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?

> > + * 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@amd.com

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

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette