[Skiboot] [PATCH v2 2/3] opal: Check Core FIRs to find the reason for Malfunction Alert.

[Stewart Smith]

Mahesh Jagannath Salgaonkar <mahesh at linux.vnet.ibm.com> writes:
> On 02/12/2015 11:24 AM, Stewart Smith wrote:
>> Mahesh J Salgaonkar <mahesh at linux.vnet.ibm.com> writes:
>>> @@ -337,7 +454,7 @@ static int64_t opal_handle_hmi(void)
>>>  	struct OpalHMIEvent hmi_evt;
>>>  
>>>  	memset(&hmi_evt, 0, sizeof(struct OpalHMIEvent));
>>> -	hmi_evt.version = OpalHMIEvt_V1;
>>> +	hmi_evt.version = OpalHMIEvt_V2;
>>>  
>>>  	lock(&hmi_lock);
>>>  	hmer = mfspr(SPR_HMER);		/* Get HMER register value */
>> 
>> It would be better if you only sent v2 in the event of something not
>> supported by V1.
>
> Wouldn't that make code bit messy ? Also, would it be correct for OPAL
> layer to send two different versions of HMI event depending on what it
> contains ? V2 is a super set of v1, and can contain data pertaining to
> v1 and v2 both. V2 is backward compatible. Older Linux kernel receiving
> V2 event (containing only v1 data) can still print all the info that
> pertains to V1.

Okay - so we should *really* document this pretty carefully (in
doc/opal-api/opal-messages.txt ).

I've gone and filled out some of the basics of the forward/backwards
compatibility with struct opal_msg in doc/opal-api/opal-messages.txt,
I'd love it if you could complete the documentation on the HMI message.

I'd love to see BUILD_ASSERT(sizeof(struct opal_msg) >= sizeof(struct
OpalHMIEvent)) somewhere in the code too. I can imagine this getting
hairy to ensure at some point in the future.

Currently it seems that:
1) version cannot be < 1 (linux will bail out). So a version of 0 is
   the way to make a backwards-incompatible change. This should be
   documented.
2) All future versions of OPAL_MSG_HMI_EVT are backwards compatible with
   v1. It'd be great to explicitly document this somewhere.

   Looking carefully at the code, it's "safe" to read the extra words
   out of struct opal_msg as OPAL message is always at least this big.
   (the case for new kernel with a bigger struct OpalHMIEvent than the
   event it receives from an old OPAL)

   IF you decide to have all future versions compatible with v2 as well,
   then please also explicitly document this.


My initial concern with saying we should emit v1 where possible was
around exactly how everything fits together with OPAL_GET_MSG, and
after looking at things fairly closely and documenting some of it
(again, *please* expand the OpalHMIEvent docs) it looks like we'd be
safe - but we should really explicitly work out if being backwards
compatible with v2 as well as v1 *forever* is something we want to
do. I'm pretty sure it's okay - thoughts?

Please also document that when adding a struct to the union, the
version of HMIEvent *MUST* be bumped.