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

Mahesh Jagannath Salgaonkar [off-list ref] writes:

On 08/08/2018 08:12 PM, Michael Ellerman wrote:

...

quoted

quoted

+	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.

quoted

quoted

+			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
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;

cheers