Re: [PATCH v7 4/9] powerpc/pseries: Define MCE error event section.

[Mahesh Jagannath Salgaonkar <mahesh@linux.vnet.ibm.com>]

On 08/08/2018 08:12 PM, Michael Ellerman wrote:
> Hi Mahesh,
> 
> A few nitpicks.
> 
> Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com> writes:
>> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>>
>> On pseries, the machine check error details are part of RTAS extended
>> event log passed under Machine check exception section. This patch adds
>> the definition of rtas MCE event section and related helper
>> functions.
>>
>> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/include/asm/rtas.h |  111 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 111 insertions(+)
> 
> AFIACS none of this ever gets used outside of ras.c, should it should
> just go in there.

Since it was all rtas specific I thought rtas.h is better place. But
yes, I can move this into ras.c

> 
>> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
>> index 71e393c46a49..adc677c5e3a4 100644
>> --- a/arch/powerpc/include/asm/rtas.h
>> +++ b/arch/powerpc/include/asm/rtas.h
>> @@ -326,6 +334,109 @@ struct pseries_hp_errorlog {
>>  #define PSERIES_HP_ELOG_ID_DRC_COUNT	3
>>  #define PSERIES_HP_ELOG_ID_DRC_IC	4
>>  
>> +/* RTAS pseries MCE errorlog section */
>> +#pragma pack(push, 1)
>> +struct pseries_mc_errorlog {
>> +	__be32	fru_id;
>> +	__be32	proc_id;
>> +	uint8_t	error_type;
> 
> Please use kernel types, so u8.

Will do so.

> 
>> +	union {
>> +		struct {
>> +			uint8_t	ue_err_type;
>> +			/* XXXXXXXX
>> +			 * X		1: Permanent or Transient UE.
>> +			 *  X		1: Effective address provided.
>> +			 *   X		1: Logical address provided.
>> +			 *    XX	2: Reserved.
>> +			 *      XXX	3: Type of UE error.
>> +			 */
> 
> But which bit is bit 0? And is that the LSB or MSB?

RTAS errorlog data in BE format, the leftmost bit is MSB 0 (1: Permanent
or Transient UE.). I Will update the comment above that properly points
out which one is MSB 0.

> 
> 
>> +			uint8_t	reserved_1[6];
>> +			__be64	effective_address;
>> +			__be64	logical_address;
>> +		} ue_error;
>> +		struct {
>> +			uint8_t	soft_err_type;
>> +			/* XXXXXXXX
>> +			 * X		1: Effective address provided.
>> +			 *  XXXXX	5: Reserved.
>> +			 *       XX	2: Type of SLB/ERAT/TLB error.
>> +			 */
>> +			uint8_t	reserved_1[6];
>> +			__be64	effective_address;
>> +			uint8_t	reserved_2[8];
>> +		} soft_error;
>> +	} u;
>> +};
>> +#pragma pack(pop)
> 
> Why not __packed ?

Because when used __packed it added 1 byte extra padding between
reserved_1[6] and effective_address. That caused wrong effective address
to be printed on the console. Hence I switched to #pragma pack to force
1 byte alignment for this structure alone.

> 
>> +/* RTAS pseries MCE error types */
>> +#define PSERIES_MC_ERROR_TYPE_UE		0x00
>> +#define PSERIES_MC_ERROR_TYPE_SLB		0x01
>> +#define PSERIES_MC_ERROR_TYPE_ERAT		0x02
>> +#define PSERIES_MC_ERROR_TYPE_TLB		0x04
>> +#define PSERIES_MC_ERROR_TYPE_D_CACHE		0x05
>> +#define PSERIES_MC_ERROR_TYPE_I_CACHE		0x07
> 
> Once these are in ras.c they can have less unwieldy names, ie. the
> PSERIES at least can be dropped.

ok.

> 
>> +/* RTAS pseries MCE error sub types */
>> +#define PSERIES_MC_ERROR_UE_INDETERMINATE		0
>> +#define PSERIES_MC_ERROR_UE_IFETCH			1
>> +#define PSERIES_MC_ERROR_UE_PAGE_TABLE_WALK_IFETCH	2
>> +#define PSERIES_MC_ERROR_UE_LOAD_STORE			3
>> +#define PSERIES_MC_ERROR_UE_PAGE_TABLE_WALK_LOAD_STORE	4
>> +
>> +#define PSERIES_MC_ERROR_SLB_PARITY		0
>> +#define PSERIES_MC_ERROR_SLB_MULTIHIT		1
>> +#define PSERIES_MC_ERROR_SLB_INDETERMINATE	2
>> +
>> +#define PSERIES_MC_ERROR_ERAT_PARITY		1
>> +#define PSERIES_MC_ERROR_ERAT_MULTIHIT		2
>> +#define PSERIES_MC_ERROR_ERAT_INDETERMINATE	3
>> +
>> +#define PSERIES_MC_ERROR_TLB_PARITY		1
>> +#define PSERIES_MC_ERROR_TLB_MULTIHIT		2
>> +#define PSERIES_MC_ERROR_TLB_INDETERMINATE	3
>> +
>> +static inline uint8_t rtas_mc_error_type(const struct pseries_mc_errorlog *mlog)
>> +{
>> +	return mlog->error_type;
>> +}
> 
> Why not just access it directly?

sure.

> 
>> +static inline uint8_t rtas_mc_error_sub_type(
>> +					const struct pseries_mc_errorlog *mlog)
>> +{
>> +	switch (mlog->error_type) {
>> +	case	PSERIES_MC_ERROR_TYPE_UE:
>> +		return (mlog->u.ue_error.ue_err_type & 0x07);
>> +	case	PSERIES_MC_ERROR_TYPE_SLB:
>> +	case	PSERIES_MC_ERROR_TYPE_ERAT:
>> +	case	PSERIES_MC_ERROR_TYPE_TLB:
>> +		return (mlog->u.soft_error.soft_err_type & 0x03);
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +static inline uint64_t rtas_mc_get_effective_addr(
>> +					const struct pseries_mc_errorlog *mlog)
>> +{
>> +	uint64_t addr = 0;
> 
> That should be __be64.

Sure will do.

Thanks,
-Mahesh.