Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 15 Apr 2022 14:56:54 +0000
From:   Yazen Ghannam <yazen.ghannam@amd.com>
To:     bp@alien8.de, tony.luck@intel.com
Cc:     "Luck, Tony" <tony.luck@intel.com>,
        Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>,
        linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        x86@kernel.org, hpa@zytor.com,
        Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v5 2/2] x86/mce: Add support for Extended Physical
 Address MCA changes
Message-ID: <YlmHtlKABn9W0pu5@yaz-ubuntu>
References: <20220412154038.261750-1-Smita.KoralahalliChannabasappa@amd.com>
 <20220412154038.261750-3-Smita.KoralahalliChannabasappa@amd.com>
 <YlakNe012hhErszh@zn.tnic>
 <YlbZ1k1cT1FVJj4W@yaz-ubuntu>
 <YlbkCK9LU2KdXZUG@zn.tnic>
 <YlbzbZO6AvxOqQb/@agluck-desk3.sc.intel.com>
 <Ylb3/4oi6KAjdsJW@zn.tnic>
 <YlcnN2q9ducdvsUZ@yaz-ubuntu>
 <YlflJfyQR/j/eRkn@zn.tnic>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YlflJfyQR/j/eRkn@zn.tnic>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?R9gTJy1H/9HuerZ1NT8q1XEACy+r57Ib/ypBL+FxzBaO5PyF6KbDQqD6g5wU?=
 =?us-ascii?Q?nZ9+grEet6ibb0qJJj6ytnaJ+yJUaLmK4xzzoz38gMkDcu+w7R9o/M0JFfJm?=
 =?us-ascii?Q?t85OtaUaZipzj6hS8UchmcIyArMm7BMkOR7Mf1Z+04aJsOCMCB1X0fjbM/GI?=
 =?us-ascii?Q?vqcHFFUopHRtD+ndElDCtJWiYtCl+CTxLP/5GXfqPTqFljyLj6zkz1xwRgZC?=
 =?us-ascii?Q?Xp8+PbsTWyWB/gYcmlvstLAsgnW+SOLeXYZjcuVbkPqba89/sLym8A4ApBWi?=
 =?us-ascii?Q?ci2BBVLbG/vIX6CBS/lc9Dp2BxvPrY2QbO3f7u/XxP/l30kRZij8Jkt+SlMu?=
 =?us-ascii?Q?8b1B/ji9Hv/8wcPZ8G25OnWvUKa4D7rSVRLu9HADWzlYEZeC/eMOlCc8c1pw?=
 =?us-ascii?Q?w7RrpOI77racGpwqUTX14o9PP66t5WfAeAJ4xHkmGwUHsuoFksA1ySFG4WUJ?=
 =?us-ascii?Q?fyfDR7fjaFUOkhefXszPIRxM3QZXf/uNkIkOEyRhvqDI+XptuFE5HyJsVyqr?=
 =?us-ascii?Q?9PxfXAItEYp4tbLOaE0CgC7Q0okYcjYCBnoJ9BUQ1TAfdroE3KV6pwJNhQoV?=
 =?us-ascii?Q?ySRn8Pn81+uTbjoRmJ7adqaiIc4ETS1A0rScAcKyPf9h8V2t/FCkp+GgolUC?=
 =?us-ascii?Q?eBPbSC8CuitwK0BvZfcwROIgWfRKF2hVimMKUCniFDg1kPbHSvJDUKvU1YBr?=
 =?us-ascii?Q?Hhl7BBu378+MH+SubjP3r4LOTGIKysf+nhLWDqSoL+rqXBLpaEVHopdRyOIA?=
 =?us-ascii?Q?oxP57Um5OuBXxKBilz5N17utZSQnefHPKUBKIxhnycXDxnQI5YVDOZMiq8vw?=
 =?us-ascii?Q?2p9Dyz25H9BJXo6GkqhPYzWtEJMfSyMM8tmAXpIBS5RpD0k/Ig/hmQykGEZS?=
 =?us-ascii?Q?g9F3IZpD4MdoJyJlPqMNnLpm7wD+ME0e9hmJbCWPu2W/5JjE3afh1TYbOLO5?=
 =?us-ascii?Q?lWqn9vfDWgj6nRvxHkJ23G3OffQYTt1XGB9HNq14pLDrKX4Os6gQZDstZrds?=
 =?us-ascii?Q?BmH/tia8FHAmbqFTjKhXghgHrFU6pX8k89kEoPha0YyATlL1jq9axpW1OpmR?=
 =?us-ascii?Q?GDJoxerrMP+s1oCCxNYCeHu9MJ1k5tbzL9CsZEcejwOlUE9sq4dTt1oePguX?=
 =?us-ascii?Q?jYG+J8Fr/15MlMu4KzGOtYm3wGDGSuNkz1dY4O4JkJHRAQWz1gayzL6cmgfu?=
 =?us-ascii?Q?eBjosnLCQpyUQRbvwK4XfEZllAvca+kmoym6+Nh4ZwWs5IWKJh3QZqz4kfKn?=
 =?us-ascii?Q?UOP6EPSI72n1wqyiHwRYTvE/3UHg+2glf0xNycHsysVTnFV9m+/1BMwLjdBx?=
 =?us-ascii?Q?mzJSFnyFdGqnO+T5Z7xi6ogjICrR0jkRHgqRvifHHhpM6thttqHrs4h1iKbT?=
 =?us-ascii?Q?PfODLZ6ulWOSKOnSfZXyLJX7XznQh4/9SriKwntlfu38ENGmTBADXku1Iv18?=
 =?us-ascii?Q?UU9zq9nGfODT4tkmte2NpG8Vd7KK29yse8hupi3PiecmYsaWHSbkNavGxSuY?=
 =?us-ascii?Q?1Ehyk7q7WL50KO/4S7iCCozIrLzWzbsrUkDk6qt1NNEgVKCjUW2l8kFMVe67?=
 =?us-ascii?Q?g1T51civVTdVuMmy3AvA2PdzNDKCfpjb5c10arT8ojqm+XutSbkKhXbLQ69v?=
 =?us-ascii?Q?lDOgpV7IVaqByHEq+jUicrbJRv8WvxHSsN81vRI+MNzdmJqG6UNOKNzdYRg8?=
 =?us-ascii?Q?p6YD4p8mV4PeMlb3RUGJ8HbPzIz9/jyWM/8HNougENToyprsHIE6APHodPR9?=
 =?us-ascii?Q?b0n7YX0EYw=3D=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9e300766-f173-4a56-ba4d-08da1ef03414
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3108.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Apr 2022 14:57:04.2666
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: OYQfrlztNPD4fnHXg2QbTEmP73RSshxE2aERxQo4+dWGa35U6uzIoS/+X0J+WufuKQNLrO+UUo5yYdeRAiMkQg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB5499
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Apr 14, 2022 at 11:11:01AM +0200, Borislav Petkov wrote:
> On Wed, Apr 13, 2022 at 07:40:39PM +0000, Yazen Ghannam wrote:
> > I think the init logic breaks here. MCE now gets enabled before clearing old
> > errors. So it's possible that the old errors get overwritten by new ones.
> 
> Err, I don't understand. CR4.MCE bit description has:
> 
> "Regardless of whether machine-check exceptions are enabled, the
> processor records enabled-errors when they occur."
> 
> I'm guessing enabled errors are those for which the respective bits in
> the MCi_CTL banks are set. And I think the CPU comes out of reset with
> those bits set.
> 
> So the overwriting will happen regardless.
> 
> The only difference here is that "[s]etting MCE to 1 enables the
> machine-check exception mechanism." So you'll get a #MC raised vs
> shutdown on a fatal error.
> 
> Or am I missing an angle?
>

I agree with you about the CR4.MCE statement. But MCi_CTL needs to be set by
system software. The reset value is '0' at least on AMD systems.

Here's a example scenario.

1) OS has fully booted.
   a) MCi_CTL, MCG_CTL, CR4.MCE are all enabled.
2) Fatal MCA error occurs causing hardware-initialzed reset. No OS handling.
   a) MCA state info is warm reset persistent.
   b) MCi_STATUS, MCi_ADDR, etc. have valid info.
   c) MCi_CTL, MCG_CTL, CR4.MCE are all set to reset value: '0'.
3) OS, or optionally BIOS, polls MCA banks and logs any valid errors.
   a) Since MCi_CTL, etc. are cleared due to reset, any errors detected are
      from before the reset.
4) MCi_STATUS is cleared to discard old error information.
5) MCA is initiliazed (MCi_CTL, MCG_CTL, CR4.MCE, etc.). Any error detected
   now is a new error from this session.
 
> > > @@ -1791,6 +1761,9 @@ static void __mcheck_cpu_check_banks(void)
> > >  		if (!b->init)
> > >  			continue;
> > >  
> > > +		wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl);
> > > +		wrmsrl(mca_msr_reg(i, MCA_STATUS), 0);
> > 
> > Same idea here. STATUS should be cleared before turning on reporting in a bank
> > using MCA_CTL.
> 
> Look at the current code. Called in this order:
> 
> __mcheck_cpu_init_clear_banks:
>         wrmsrl(mca_msr_reg(i, MCA_CTL), b->ctl);
>         wrmsrl(mca_msr_reg(i, MCA_STATUS), 0);
> __mcheck_cpu_check_banks
> 	rdmsrl(mca_msr_reg(i, MCA_CTL), msrval);
>         b->init = !!msrval;
> 
> STATUS *is* cleared after MCA_CTL now too.
> 
> If this ordering is wrong - and it sounds like it is - then this needs
> to be a separate patch and Cc: <stable@vger.kernel.org> and needs to go
> in now.
>

I agree. The Intel SDM and AMD APM have the following procedure, in summary.

1) Set MCG_CTL
2) Set MCi_CTL for all banks
3) Read MCi_STATUS and log valid errors.
4) Clear MCi_STATUS
5) Set CR4.MCE

I don't know of a reason why STATUS needs to be cleared after MCi_CTL is set.
The only thing I can think of is that enabling MCi_CTL may cause spurious info
logged in MCi_STATUS, and that needs to be cleared out. I'm asking AMD folks
about it.

Of course, this contradicts the flow I outlined above, and also the flow given
in the AMD Processor Programming Reference (PPR). I wonder if the
architectural documents have gotten stale compared to current guidelines. I'm
asking about this too.

Tony,
Do you have any thoughts on this?

> > One downside though is that the system goes longer with CR4.MCE cleared. So
> > there's greater risk of encountering a shutdown due to a machine check error.
> 
> Yeah, I don't think the couple of msecs matter.
>

Okay, yeah then maybe there should be another small patch to bring the init
flow closer to x86 documentation.

Thanks,
Yazen