[Skiboot] [PATCH v2 3/9] opal-msg: Enhance opal-get-msg API

[Vasant Hegde]

On 05/06/2019 03:18 PM, Jeremy Kerr wrote:
> Hi Vasant,
> 
>> -int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
>> -		    void (*consumed)(void *data, int status),
>> -		    size_t num_params, const u64 *params)
>> +static int __opal_queue_msg(enum opal_msg_type msg_type, void *data,
>> +			    void (*consumed)(void *data, int status),
>> +			    size_t params_size, const void *params_data)
> 
> Why add the new __opal_queue_msg(), when we have _opal_queue_msg()?
> Can't we just remove the "/sizeof(u64)" from the opal_queue_msg macro
> instead, and use _opal_queue_msg() for the low-level function?

Yeah we can do that.. But _opal_queue_msg() is used in many places directly.
So we have to change all places to pass actual size.. something like below :

diff --git a/core/hmi.c b/core/hmi.c
index e81328600..ab079a95a 100644
--- a/core/hmi.c
+++ b/core/hmi.c
@@ -337,7 +337,7 @@ static int queue_hmi_event(struct OpalHMIEvent *hmi_evt, int 
recover, uint64_t *

         /* queue up for delivery to host. */
         return _opal_queue_msg(OPAL_MSG_HMI_EVT, NULL, NULL,
-                               num_params, (uint64_t *)hmi_evt);
+                               num_params * sizeof(u64), (uint64_t *)hmi_evt);
  }

To avoid these changes I created added function to pass buffer 
(opal_queue_msg_buf()).

If you still want I can change this. Please let me know.


> 
>> @@ -56,12 +62,14 @@ int _opal_queue_msg(enum opal_msg_type msg_type,
>> void *data,
>>   	entry->consumed = consumed;
>>   	entry->data = data;
>>   	entry->msg.msg_type = cpu_to_be32(msg_type);
>> +	entry->msg.size = cpu_to_be32(params_size);
>>   
>> -	if (num_params > ARRAY_SIZE(entry->msg.params)) {
>> -		prerror("Discarding extra parameters\n");
>> -		num_params = ARRAY_SIZE(entry->msg.params);
>> +	if (params_size > OPAL_MSG_FIXED_PARAMS_SIZE) {
>> +		/* msg.params[0] contains address of data */
>> +		entry->msg.params[0] = (uint64_t)params_data;
>> +	} else {
>> +		memcpy(entry->msg.params, params_data, params_size);
>>   	}
> 
> This concerns me a bit - now we have two different "formats" of message
> on the msg_pending_list, and it looks like there are now two different
> lifetime requirements on the params_data argument, depending on the
> size.
> 
> Could we unify this?

This is bit tricky one. In many places its just return code -OR-  some values
queued using opal_queue_msg.

Only way is to enforce on function usage level.
   opal_queue_msg_buf -> lifetime of buffer is until call back handler is called

Let know if you have any other better way to handle this.



> 
>> -	memcpy(entry->msg.params, params, num_params*sizeof(u64));
>>   
>>   	list_add_tail(&msg_pending_list, &entry->link);
>>   	opal_update_pending_evt(OPAL_EVENT_MSG_PENDING,
>> @@ -72,8 +80,29 @@ int _opal_queue_msg(enum opal_msg_type msg_type,
>> void *data,
>>   	return 0;
>>   }
>>   
>> +int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
>> +		    void (*consumed)(void *data, int status),
>> +		    size_t num_params, const u64 *params)
>> +{
>> +	/* Convert num_params to actual size */
>> +	return __opal_queue_msg(msg_type, data, consumed,
>> +				(num_params * sizeof(u64)), params);
>> +}
>> +
>> +int opal_queue_msg_buf(enum opal_msg_type msg_type, void *data,
>> +		       void (*consumed)(void *data, int status),
>> +		       size_t params_size, const void *params_data)
>> +{
>> +	return __opal_queue_msg(msg_type, data,
>> +				consumed, params_size, params_data);
>> +}
>> +
>>   static int64_t opal_get_msg(uint64_t *buffer, uint64_t size)
>>   {
>> +	int rc = OPAL_SUCCESS;
>> +	int params_size;
>> +	uint64_t msg_size;
>> +	struct opal_msg *msg = (struct opal_msg *)buffer;
> 
> Minor (but also on other patches in this series): reverse christmas-
> tree?

Not sure what you meant here. you mean variable order ?

-Vasant