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

[Vasant Hegde]

On 05/10/2019 12:28 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.
> 
> I think it'd be simpler to reduce the number of functions here,
> especially as the difference between them is so subtle.
> 
> But as you say, it's not super important.

Correct. May be I will relook into it later.

> 
>>>> @@ -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.
> 
> I'm not sure I understand? Do you mean that not all callsites (ie, in
> the "just return code" case) are guaranteeing that their buffers remain
> valid until the callback?

Yes. Most places caller won't guarantee that their buffer remain valid until 
callback is called.

> 
> This sounds like a source of future bugs. For example, if we ever change
> the threshold between the two methods of message queuing.
> 
> Can we just use the memcpy() implementation and only have the one type
> of params storage instead?

I had thought about this.. But this will increase memory allocation (upto 64K).
May be this is better than two different implementation. I can do this.



> 
>>> Minor (but also on other patches in this series): reverse christmas-
>>> tree?
>>
>> Not sure what you meant here. you mean variable order ?
> 
> Yeah, this seems to be an ad-hoc convention that we've adopted, where
> the longer declarations go first:

Yeah. May be I will reoder.

-Vasant