[Skiboot] [PATCH 2/7] opal-msg: Add get_opal_msg2

[Jeremy Kerr]

Hi Vasant,

Overall, looks pretty good to me. Since we're altering the OPAL API
here, I have a few comments inline:

> Linux uses get_opal_msg (GET_OPAL_MSG) API to get OPAL messages. This
> interface
> supports upto 8 params (64 bytes). Also OPAL doesn't return actual
> message.
> 
> We have a requirement to send bigger data to Linux. Hence this patch
> introduces
> new API - GET_OPAL_MSG2.

All of these are OPAL_GET_MSG*, not GET_OPAL_MSG*, right? Might be best
to update the documentation so that it's easily grep-able.

> - get_opal_msg2(uint64_t *buffer, uint64_t *size)
>   Linux will allocate and send buffer and buffer size to OPAL.
>   OPAL copies opal_msg to buffer and updates size with actual message
> size.
> 
> - Replaced `reserved` feild in "struct opal_msg" with `size`. So that
> Linux  side get_opal_msg2 user can detect message data size.

Doesn't the kernel already have the size, through the (output) size
value of the opal_get_msg2 call?

> - Add new internal function (opal_queue_msg_extended()) to queue
> opal_msg with bigger data.

I would prefer not to call these messages "extended", as they're all
using the same channel as the existing messages. How about we have:

  opal_queue_msg()

    - for the __VA_ARGS__ variant

and:

  opal_queue_msg_buf()

    - for the pointer + size variant

(then opal_queue_msg can just be a macro for opal_queue_msg_buf()).

>  
> +	/* Check whether opal_get_msg2 is supported by kernel */
> +	if (!opal_msg2_supported &&
> +	    params_size > OPAL_MSG_FIXED_PARAMS_SIZE) {
> +		prerror("Discarding extra parameters\n");
> +		params_size = OPAL_MSG_FIXED_PARAMS_SIZE;
> +	}

So this potentially truncates the message? Shouldn't we reject the
enqueue instead?

[Also, we may need to think about what happens when opal_msg2_supported
is unset when we already have a large message enqueued. Maybe we need to
return the error through the complete() callback?]

> +static int64_t __opal_get_msg(uint64_t *buffer, uint64_t *size)
> +{
> +	int rc = OPAL_SUCCESS;
> +	int msg_size;
> +	struct opal_msg *msg = (struct opal_msg *)buffer;
>  	struct opal_msg_entry *entry;
>  	void (*callback)(void *data);
>  	void *data;
>  
> -	if (size < sizeof(struct opal_msg) || !buffer)
> +	if (*size < sizeof(struct opal_msg) || !buffer)
>  		return OPAL_PARAMETER;
>  
>  	if (!opal_addr_valid(buffer))
> @@ -92,7 +128,25 @@ static int64_t opal_get_msg(uint64_t *buffer,
> uint64_t size)
>  		return OPAL_RESOURCE;
>  	}
>  
> -	memcpy(buffer, &entry->msg, sizeof(entry->msg));
> +	msg_size = offsetof(struct opal_msg, params) +
> +				be32_to_cpu(entry->msg.size);
> +	/* check whether params[0] contains address or actual data */

So we have different formats of message on the queue? Can we unify the
queue just to have one type, regardless of size?

> +	if (be32_to_cpu(entry->msg.size) > OPAL_MSG_FIXED_PARAMS_SIZE) {
> +		if (*size < msg_size) {
> +			/* Send error message to Linux */
> +			rc = OPAL_PARAMETER;
> +		} else {
> +			msg->msg_type = entry->msg.msg_type;
> +			msg->size = entry->msg.size;
> +			memcpy((void *)msg->params,
> +			       (void *)entry->msg.params[0],
> +			       be32_to_cpu(entry->msg.size));
> +		}
> +	} else {
> +		memcpy(buffer, &entry->msg, sizeof(entry->msg));
> +	}
> +

[...]

> diff --git a/doc/opal-api/opal-get-msg2-170.rst b/doc/opal-api/opal-
> get-msg2-170.rst
> new file mode 100644
> index 000000000..42af5e2b3
> --- /dev/null
> +++ b/doc/opal-api/opal-get-msg2-170.rst
> @@ -0,0 +1,30 @@
> +OPAL_GET_MSG2
> +=============
> +
> +OPAL_GET_MSG2 will get the next pending OPAL Message (see :ref:`opal-
> messages`).
> +
> +Parameters: ::
> +
> +	buffer to copy message into
> +	sizeof buffer to copy message into, OPAL will return actual
> message size
> +
> +The maximum size of an opal message is specified in the device tree
> passed
> +to the host OS: ::
> +
> +  ibm,opal {
> +            opal-msg-size = <0x10000>;
> +  }
> +
> +A host OS *SHOULD* always supply a buffer to OPAL_GET_MSG2 of "opal-
> msg-size"
> +bytes.

I think that opal_get_msg2() should always return an error if the size
is less than that specified in the device tree - ie., make this a MUST.
This should help to prevent misuse of the API.

Cheers,


Jeremy