Xen devel - Oct 2012 - [PATCH] x86/MCE: Implement clearbank callback for AMD

Implement clearbank callbank for AMD.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Ping?

On 10/12/12 16:46, Christoph Egger wrote:
> 
> Implement clearbank callbank for AMD.
> 
> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
> 

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

>>> On 19.10.12 at 15:11, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> Ping?
I have this on my radar to review (and commit if no significant
reason not to turns up), but I can''t currently tell when I''ll
be able
to get to it (some time not too early next week maybe).

Sorry, Jan
> On 10/12/12 16:46, Christoph Egger wrote:
>> 
>> Implement clearbank callbank for AMD.
>> 
>> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>
>> 
> 
> 
> -- 
> ---to satisfy European Law for business letters:
> Advanced Micro Devices GmbH
> Einsteinring 24, 85689 Dornach b. Muenchen
> Geschaeftsfuehrer: Alberto Bozzo
> Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
> Registergericht Muenchen, HRB Nr. 43632
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>+static int k8_need_clearbank_scan(enum mca_source who, uint64_t status)
>+{
>+	switch (who) {
>+	case MCA_MCE_SCAN:
>+	case MCA_MCE_HANDLER:
>+		break;
>+	default:
>+		return 1;
>+	}
>+
>+	/* For fatal error, it shouldn''t be cleared so that sticky bank
>+	 * have chance to be handled after reboot by polling.
>+	 */
>+	if ( (status & MCi_STATUS_UC) && (status & MCi_STATUS_PCC)
)
>+		return 0;
>+	/* Spurious need clear bank */
>+	if ( !(status & MCi_STATUS_OVER)
>+	    && (status & MCi_STATUS_UC) && !(status &
MCi_STATUS_EN))
>+		return 1;
>+
>+	return 1;
> }
So what''s the purpose of first conditionally returning 1, and then
also doing so unconditionally? Do anticipate to insert code between
the two parts within the very near future? Otherwise I''d drop the
whole if() construct.

(No need to re-submit, just let me know.)

Jan

On 10/24/12 14:18, Jan Beulich wrote:
>>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>> +static int k8_need_clearbank_scan(enum mca_source who, uint64_t
status)
>> +{
>> +	switch (who) {
>> +	case MCA_MCE_SCAN:
>> +	case MCA_MCE_HANDLER:
>> +		break;
>> +	default:
>> +		return 1;
>> +	}
>> +
>> +	/* For fatal error, it shouldn''t be cleared so that sticky
bank
>> +	 * have chance to be handled after reboot by polling.
>> +	 */
>> +	if ( (status & MCi_STATUS_UC) && (status &
MCi_STATUS_PCC) )
>> +		return 0;
>> +	/* Spurious need clear bank */
>> +	if ( !(status & MCi_STATUS_OVER)
>> +	    && (status & MCi_STATUS_UC) && !(status &
MCi_STATUS_EN))
>> +		return 1;
>> +
>> +	return 1;
>> }
> 
> So what''s the purpose of first conditionally returning 1, and then
> also doing so unconditionally? Do anticipate to insert code between
> the two parts within the very near future? Otherwise I''d drop the
> whole if() construct.
This function is derived from  intel_need_clearbank_scan().
I just took over the relevant parts for AMD.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

>>> On 25.10.12 at 10:18, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> On 10/24/12 14:18, Jan Beulich wrote:
>>>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>> +static int k8_need_clearbank_scan(enum mca_source who, uint64_t
status)
>>> +{
>>> +	switch (who) {
>>> +	case MCA_MCE_SCAN:
>>> +	case MCA_MCE_HANDLER:
>>> +		break;
>>> +	default:
>>> +		return 1;
>>> +	}
>>> +
>>> +	/* For fatal error, it shouldn''t be cleared so that
sticky bank
>>> +	 * have chance to be handled after reboot by polling.
>>> +	 */
>>> +	if ( (status & MCi_STATUS_UC) && (status &
MCi_STATUS_PCC) )
>>> +		return 0;
>>> +	/* Spurious need clear bank */
>>> +	if ( !(status & MCi_STATUS_OVER)
>>> +	    && (status & MCi_STATUS_UC) && !(status
& MCi_STATUS_EN))
>>> +		return 1;
>>> +
>>> +	return 1;
>>> }
>> 
>> So what''s the purpose of first conditionally returning 1, and
then
>> also doing so unconditionally? Do anticipate to insert code between
>> the two parts within the very near future? Otherwise I''d drop
the
>> whole if() construct.
> 
> This function is derived from  intel_need_clearbank_scan().
> I just took over the relevant parts for AMD.
But that would suggest that the final return be "return 0" rather
than "return 1".

Further, the Intel code does no extra checking for the
MCA_MCE_HANDLER case, so I''d like you to confirm that this is
indeed to be different for your CPUs.

Jan

On 10/25/12 10:55, Jan Beulich wrote:
>>>> On 25.10.12 at 10:18, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>> On 10/24/12 14:18, Jan Beulich wrote:
>>>>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>>> +static int k8_need_clearbank_scan(enum mca_source who,
uint64_t status)
>>>> +{
>>>> +	switch (who) {
>>>> +	case MCA_MCE_SCAN:
>>>> +	case MCA_MCE_HANDLER:
>>>> +		break;
>>>> +	default:
>>>> +		return 1;
>>>> +	}
>>>> +
>>>> +	/* For fatal error, it shouldn''t be cleared so that
sticky bank
>>>> +	 * have chance to be handled after reboot by polling.
>>>> +	 */
>>>> +	if ( (status & MCi_STATUS_UC) && (status &
MCi_STATUS_PCC) )
>>>> +		return 0;
>>>> +	/* Spurious need clear bank */
>>>> +	if ( !(status & MCi_STATUS_OVER)
>>>> +	    && (status & MCi_STATUS_UC) &&
!(status & MCi_STATUS_EN))
>>>> +		return 1;
>>>> +
>>>> +	return 1;
>>>> }
>>>
>>> So what''s the purpose of first conditionally returning 1,
and then
>>> also doing so unconditionally? Do anticipate to insert code between
>>> the two parts within the very near future? Otherwise I''d
drop the
>>> whole if() construct.
>>
>> This function is derived from  intel_need_clearbank_scan().
>> I just took over the relevant parts for AMD.
> 
> But that would suggest that the final return be "return 0" rather
> than "return 1".
> 
> Further, the Intel code does no extra checking for the
> MCA_MCE_HANDLER case, so I''d like you to confirm that this is
> indeed to be different for your CPUs.
I just noticed that MCA_MCE_HANDLER is not used at all and can be
removed entirely.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

>>> On 25.10.12 at 11:29, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> On 10/25/12 10:55, Jan Beulich wrote:
>>>>> On 25.10.12 at 10:18, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>> On 10/24/12 14:18, Jan Beulich wrote:
>>>>>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>>>> +static int k8_need_clearbank_scan(enum mca_source who,
uint64_t status)
>>>>> +{
>>>>> +	switch (who) {
>>>>> +	case MCA_MCE_SCAN:
>>>>> +	case MCA_MCE_HANDLER:
>>>>> +		break;
>>>>> +	default:
>>>>> +		return 1;
>>>>> +	}
>>>>> +
>>>>> +	/* For fatal error, it shouldn''t be cleared so
that sticky bank
>>>>> +	 * have chance to be handled after reboot by polling.
>>>>> +	 */
>>>>> +	if ( (status & MCi_STATUS_UC) && (status
& MCi_STATUS_PCC) )
>>>>> +		return 0;
>>>>> +	/* Spurious need clear bank */
>>>>> +	if ( !(status & MCi_STATUS_OVER)
>>>>> +	    && (status & MCi_STATUS_UC) &&
!(status & MCi_STATUS_EN))
>>>>> +		return 1;
>>>>> +
>>>>> +	return 1;
>>>>> }
>>>>
>>>> So what''s the purpose of first conditionally returning
1, and then
>>>> also doing so unconditionally? Do anticipate to insert code
between
>>>> the two parts within the very near future? Otherwise
I''d drop the
>>>> whole if() construct.
>>>
>>> This function is derived from  intel_need_clearbank_scan().
>>> I just took over the relevant parts for AMD.
>> 
>> But that would suggest that the final return be "return 0"
rather
>> than "return 1".
>> 
>> Further, the Intel code does no extra checking for the
>> MCA_MCE_HANDLER case, so I''d like you to confirm that this is
>> indeed to be different for your CPUs.
> 
> I just noticed that MCA_MCE_HANDLER is not used at all and can be
> removed entirely.
Will do. But how about the "return" related question above?

Jan

On 10/25/12 12:03, Jan Beulich wrote:
>>>> On 25.10.12 at 11:29, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>> On 10/25/12 10:55, Jan Beulich wrote:
>>>>>> On 25.10.12 at 10:18, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>>> On 10/24/12 14:18, Jan Beulich wrote:
>>>>>>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>>>>> +static int k8_need_clearbank_scan(enum mca_source who,
uint64_t status)
>>>>>> +{
>>>>>> +	switch (who) {
>>>>>> +	case MCA_MCE_SCAN:
>>>>>> +	case MCA_MCE_HANDLER:
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		return 1;
>>>>>> +	}
>>>>>> +
>>>>>> +	/* For fatal error, it shouldn''t be cleared
so that sticky bank
>>>>>> +	 * have chance to be handled after reboot by polling.
>>>>>> +	 */
>>>>>> +	if ( (status & MCi_STATUS_UC) && (status
& MCi_STATUS_PCC) )
>>>>>> +		return 0;
>>>>>> +	/* Spurious need clear bank */
>>>>>> +	if ( !(status & MCi_STATUS_OVER)
>>>>>> +	    && (status & MCi_STATUS_UC)
&& !(status & MCi_STATUS_EN))
>>>>>> +		return 1;
>>>>>> +
>>>>>> +	return 1;
>>>>>> }
>>>>>
>>>>> So what''s the purpose of first conditionally
returning 1, and then
>>>>> also doing so unconditionally? Do anticipate to insert code
between
>>>>> the two parts within the very near future? Otherwise
I''d drop the
>>>>> whole if() construct.
>>>>
>>>> This function is derived from  intel_need_clearbank_scan().
>>>> I just took over the relevant parts for AMD.
>>>
>>> But that would suggest that the final return be "return
0" rather
>>> than "return 1".
>>>
>>> Further, the Intel code does no extra checking for the
>>> MCA_MCE_HANDLER case, so I''d like you to confirm that this
is
>>> indeed to be different for your CPUs.
>>
>> I just noticed that MCA_MCE_HANDLER is not used at all and can be
>> removed entirely.
> 
> Will do.
Ok, thanks.
> But how about the "return" related question above?
I just run some tests.
Remove this part:
> +	/* Spurious need clear bank */
> +	if ( !(status & MCi_STATUS_OVER)
> +	    && (status & MCi_STATUS_UC) && !(status &
MCi_STATUS_EN))
> +		return 1;
Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

On 10/25/12 12:13, Christoph Egger wrote:
> On 10/25/12 12:03, Jan Beulich wrote:
>>>>> On 25.10.12 at 11:29, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>> On 10/25/12 10:55, Jan Beulich wrote:
>>>>>>> On 25.10.12 at 10:18, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>>>> On 10/24/12 14:18, Jan Beulich wrote:
>>>>>>>>> On 12.10.12 at 16:46, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>>>>>>> +static int k8_need_clearbank_scan(enum mca_source
who, uint64_t status)
>>>>>>> +{
>>>>>>> +	switch (who) {
>>>>>>> +	case MCA_MCE_SCAN:
>>>>>>> +	case MCA_MCE_HANDLER:
>>>>>>> +		break;
>>>>>>> +	default:
>>>>>>> +		return 1;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/* For fatal error, it shouldn''t be
cleared so that sticky bank
>>>>>>> +	 * have chance to be handled after reboot by
polling.
>>>>>>> +	 */
>>>>>>> +	if ( (status & MCi_STATUS_UC) &&
(status & MCi_STATUS_PCC) )
>>>>>>> +		return 0;
>>>>>>> +	/* Spurious need clear bank */
>>>>>>> +	if ( !(status & MCi_STATUS_OVER)
>>>>>>> +	    && (status & MCi_STATUS_UC)
&& !(status & MCi_STATUS_EN))
>>>>>>> +		return 1;
>>>>>>> +
>>>>>>> +	return 1;
>>>>>>> }
>>>>>>
>>>>>> So what''s the purpose of first conditionally
returning 1, and then
>>>>>> also doing so unconditionally? Do anticipate to insert
code between
>>>>>> the two parts within the very near future? Otherwise
I''d drop the
>>>>>> whole if() construct.
>>>>>
>>>>> This function is derived from  intel_need_clearbank_scan().
>>>>> I just took over the relevant parts for AMD.
>>>>
>>>> But that would suggest that the final return be "return
0" rather
>>>> than "return 1".
>>>>
>>>> Further, the Intel code does no extra checking for the
>>>> MCA_MCE_HANDLER case, so I''d like you to confirm that
this is
>>>> indeed to be different for your CPUs.
>>>
>>> I just noticed that MCA_MCE_HANDLER is not used at all and can be
>>> removed entirely.
>>
>> Will do.
> 
> Ok, thanks.
> 
>> But how about the "return" related question above?
> 
> I just run some tests.
> Remove this part:
> 
>> +	/* Spurious need clear bank */
>> +	if ( !(status & MCi_STATUS_OVER)
>> +	    && (status & MCi_STATUS_UC) && !(status &
MCi_STATUS_EN))
>> +		return 1;
> 
And I changed the switch (who) to:

   if (who != MCA_MCE_SCAN)
       return 1;

Christoph



-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

>>> On 25.10.12 at 12:23, Christoph Egger
<Christoph.Egger@amd.com> wrote:
> And I changed the switch (who) to:
> 
>    if (who != MCA_MCE_SCAN)
>        return 1;
That''s what I did too.

Jan

On 10/25/12 12:28, Jan Beulich wrote:
>>>> On 25.10.12 at 12:23, Christoph Egger
<Christoph.Egger@amd.com> wrote:
>> And I changed the switch (who) to:
>>
>>    if (who != MCA_MCE_SCAN)
>>        return 1;
> 
> That''s what I did too.
Thanks.

Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632