Re: [RFC PATCH 1/7] powerpc/book3s: Move machine check event structure to opal-api.h

On 02/21/2017 08:05 AM, Nicholas Piggin wrote:

On Tue, 21 Feb 2017 07:21:56 +0530
Mahesh J Salgaonkar [off-list ref] wrote:

quoted

+enum MCE_TlbErrorType {
+	MCE_TLB_ERROR_INDETERMINATE = 0,
+	MCE_TLB_ERROR_PARITY = 1,
+	MCE_TLB_ERROR_MULTIHIT = 2,
+	MCE_TLB_ERROR_TLBIEL_PROG_ERROR = 3,
+};

The new TLBIE error isn't really a TLB error as such. Not a hardware error.
I added a new "user" type for it.

I don't think we can handle it just by flushing TLB because it can also be
raised in response to invalid non-local tlbie. We could flush all TLBs maybe
but I think also have to advance nip to return to.

ok got it.

quoted

+
+enum MCE_NestErrorType {
+	MCE_NEST_ERROR_ABRT_IFETCH = 0,
+	MCE_NEST_ERROR_ABRT_IFETCH_TABLEWALK = 1,
+	MCE_NEST_ERROR_ABRT_LOAD = 2,
+	MCE_NEST_ERROR_ABRT_LOAD_TABLEWALK = 3,
+};
+
+enum MCE_CrespErrorType {
+	MCE_CRESP_ERROR_BAD_RADDR_IFETCH = 0,
+	MCE_CRESP_ERROR_BAD_RADDR_IFETCH_TABLEWALK = 1,
+	MCE_CRESP_ERROR_BAD_RADDR_LOAD = 2,
+	MCE_CRESP_ERROR_BAD_RADDR_LOAD_TABLEWALK = 3,
+};
+
+enum MCE_FspaceErrorType {
+	MCE_FSPACE_ERROR_IFETCH = 0,
+	MCE_FSPACE_ERROR_IFETCH_TABLEWALK = 1,
+	MCE_FSPACE_ERROR_RADDR_TRANSLATION = 2,
+	MCE_FSPACE_ERROR_RADDR_LOAD = 3,
+};
+
+enum MCE_AsyncErrorType {
+	MCE_ASYNC_ERROR_REAL_ADDR_STORE = 0,
+	MCE_ASYNC_ERROR_NEST_ABRT_STORE = 1,
+};
+
+struct OpalMachineCheckEvent {

Can we have more of a think about this structure and error types
before making it an OPAL API?

Agree. I was just thinking how about we can just replace the entire
union as below:

	uint8_t 	specific_error_type;		/* 0x20 */
	uint8_t		effective_address_provided;	/* 0x21 */
	uint8_t		physical_address_provided;	/* 0x22 */
	uint8_t		reserved_1[5];			/* 0x23 */
	uint64_t	effective_address;		/* 0x28 */
	uint64_t	physical_address;		/* 0x30 */
	uint8_t		reserved_2[8];			/* 0x38 */
};

What do you say ? May increase few more bytes as reserved for future.

Errors don't always fit neatly into a simple classification like
this. For example "async" is not really an error. It's a property
of how the error is reported. The error is a timeout or real
address error. And it's caused by a store. And initiated by nest
or cResp... Other errors are caused by a table walk that was
caused by a store, etc.

I shoehorned these async errors into realaddr/link types in my
patch along with a different severity (i.e., not SYNC). But I
think we can do a lot better with a clean slate for OPAL.

I see.

More general thing is, I wonder how much we need to know of the
implementation details in this API? This still seems like it's
unnecessarily split between OS and FW. I think it would be much
nicer if we just return a set of things that the OS can usefully
respond to and have firmware construct the detailed messages for
logging.

That way we'll have much fewer new types of errors we don't know
how to handle, and never have to report unknown error.

Makes sense. That would make linux MCE error printing much simpler and
we may never have to modify it to add new strings. We can probably add
char buffer to machine check struct or send it as separate string buffer.

Thanks,
-Mahesh.