| Message ID | 20190409114929.17080-3-hegdevasant@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/9] core/opal: Increase opal-msg-size size | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (050d8165ab05b6d9cdf4bfee42d9776969c77029) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
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? > @@ -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? > - 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? Cheers, Jeremy
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
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. > > > @@ -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? 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? > > 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: I'm not sure if anyone else is particularly fussed about it. Cheers, Jeremy
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
On 05/10/2019 02:17 PM, Vasant Hegde wrote: > 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. It turned out that its not too big changes. I have fixed opal_queue_msg/_opal_queue_msg function in v3 and removed opal_queue_msg_buf(). -Vasant
diff --git a/core/opal-msg.c b/core/opal-msg.c index d3dd2ae3c..a34cc29c0 100644 --- a/core/opal-msg.c +++ b/core/opal-msg.c @@ -34,12 +34,18 @@ static LIST_HEAD(msg_pending_list); static struct lock opal_msg_lock = LOCK_UNLOCKED; -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) { struct opal_msg_entry *entry; + if ((params_size + OPAL_MSG_HDR_SIZE) > OPAL_MSG_SIZE) { + prlog(PR_DEBUG, "param_size (0x%x) > opal_msg param size (0x%x)\n", + (u32)params_size, (u32)(OPAL_MSG_SIZE - OPAL_MSG_HDR_SIZE)); + return OPAL_PARAMETER; + } + lock(&opal_msg_lock); entry = list_pop(&msg_free_list, struct opal_msg_entry, link); @@ -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); } - 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; struct opal_msg_entry *entry; void (*callback)(void *data, int status); void *data; @@ -92,7 +121,30 @@ static int64_t opal_get_msg(uint64_t *buffer, uint64_t size) return OPAL_RESOURCE; } - memcpy(buffer, &entry->msg, sizeof(entry->msg)); + /* Check whether params[0] contains address or actual data */ + if (be32_to_cpu(entry->msg.size) > OPAL_MSG_FIXED_PARAMS_SIZE) { + + params_size = be32_to_cpu(entry->msg.size); + msg_size = OPAL_MSG_HDR_SIZE + params_size; + + if (size < msg_size) { + /* Send partial data to Linux */ + params_size = size - OPAL_MSG_HDR_SIZE; + prlog(PR_NOTICE, "Sending partial data [msg_type :" + " 0x%x, param_size : 0x%x, buf_size : 0x%x]\n", + be32_to_cpu(entry->msg.msg_type), + be32_to_cpu(entry->msg.size), params_size); + rc = OPAL_PARTIAL; + } + + msg->msg_type = entry->msg.msg_type; + msg->size = cpu_to_be32(params_size); + memcpy((void *)msg->params, + (void *)entry->msg.params[0], params_size); + } else { + memcpy(buffer, &entry->msg, sizeof(entry->msg)); + } + callback = entry->consumed; data = entry->data; @@ -103,9 +155,9 @@ static int64_t opal_get_msg(uint64_t *buffer, uint64_t size) unlock(&opal_msg_lock); if (callback) - callback(data, OPAL_SUCCESS); + callback(data, rc); - return OPAL_SUCCESS; + return rc; } opal_call(OPAL_GET_MSG, opal_get_msg, 2); diff --git a/core/test/run-msg.c b/core/test/run-msg.c index 08e1a019b..839074d61 100644 --- a/core/test/run-msg.c +++ b/core/test/run-msg.c @@ -135,7 +135,7 @@ int main(void) assert(list_count(&msg_free_list) == --nfree); r = opal_get_msg(m_ptr, sizeof(m)); - assert(r == 0); + assert(r == OPAL_PARTIAL); assert(list_count(&msg_pending_list) == --npending); assert(list_count(&msg_free_list) == ++nfree); diff --git a/doc/opal-api/opal-messages.rst b/doc/opal-api/opal-messages.rst index e4e813aad..25acaec2c 100644 --- a/doc/opal-api/opal-messages.rst +++ b/doc/opal-api/opal-messages.rst @@ -11,7 +11,7 @@ An opal_msg is: :: struct opal_msg { __be32 msg_type; - __be32 reserved; + __be32 size; __be64 params[8]; }; diff --git a/include/opal-api.h b/include/opal-api.h index 36aaf54e5..c8d04167a 100644 --- a/include/opal-api.h +++ b/include/opal-api.h @@ -553,7 +553,7 @@ enum opal_msg_type { struct opal_msg { __be32 msg_type; - __be32 reserved; + __be32 size; __be64 params[8]; }; diff --git a/include/opal-msg.h b/include/opal-msg.h index 9be70334d..462d66da1 100644 --- a/include/opal-msg.h +++ b/include/opal-msg.h @@ -30,6 +30,11 @@ /* Max size of struct opal_msg */ #define OPAL_MSG_SIZE (64 * 1024) +/* opal_msg fixed parameters size */ +#define OPAL_MSG_HDR_SIZE (offsetof(struct opal_msg, params)) +#define OPAL_MSG_FIXED_PARAMS_SIZE \ + (sizeof(struct opal_msg) - OPAL_MSG_HDR_SIZE) + 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); @@ -39,6 +44,10 @@ int _opal_queue_msg(enum opal_msg_type msg_type, void *data, sizeof((u64[]) {__VA_ARGS__})/sizeof(u64), \ (u64[]) {__VA_ARGS__}); +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); + void opal_init_msg(void); #endif /* __OPALMSG_H */
Linux uses opal_get_msg (OPAL_GET_MSG) API to get OPAL messages. This interface supports upto 8 params (64 bytes). We have a requirement to send bigger data to Linux. This patch enhances OPAL to send bigger data to Linux. - Linux will use "opal-msg-size" device tree property to allocate memory for OPAL messages (Previous patch increased "opal-msg-size" to 64K). - Replaced `reserved` field in "struct opal_msg" with `size`. So that Linux side opal_get_msg user can detect actual data size. - If buffer size < actual message size, then opal_get_msg will copy partial data and return OPAL_PARTIAL to Linux. - Add new internal function (opal_queue_msg_buf()) to queue opal_msg with bigger data. Cc: Jeremy Kerr <jk@ozlabs.org> Cc: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com> Cc: Oliver O'Halloran <oohall@gmail.com> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com> --- core/opal-msg.c | 72 ++++++++++++++++++++++++++++++++++++------ core/test/run-msg.c | 2 +- doc/opal-api/opal-messages.rst | 2 +- include/opal-api.h | 2 +- include/opal-msg.h | 9 ++++++ 5 files changed, 74 insertions(+), 13 deletions(-)