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

[Mahesh Jagannath Salgaonkar]

On 08/16/2018 09:44 AM, Michael Ellerman wrote:
> Mahesh Jagannath Salgaonkar <mahesh at linux.vnet.ibm.com> writes:
>> On 08/08/2018 08:12 PM, Michael Ellerman wrote:
> ...
>>>
>>>> +	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.
> 
> OK, that's weird.
> 
> Do we really need to bother with all the union stuff? The only
> difference is the field names, and whether logical address has a value

Also the bit fields for UE and other sub errors differ. Yeah but we can
do away with union stuff.

> or not. What about:
> 
> struct pseries_mc_errorlog {
> 	__be32	fru_id;
> 	__be32	proc_id;
> 	u8	error_type;
> 	u8	sub_error_type;
> 	u8	reserved_1[6];
> 	__be64	effective_address;
> 	__be64	logical_address;
> } __packed;

Sure will do.

Thanks
-Mahesh.

> 
> cheers
>