Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 367
Code to decode pipeline create messages payload#10265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
e855f3a34d1b4a51fd081c8e96740e9d17cfe86763759c15fa6d1975312c6e3File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -25,19 +25,19 @@ | ||
| LOG_MODULE_DECLARE(module_adapter, CONFIG_SOF_LOG_LEVEL); | ||
| static const struct ipc4_base_module_extended_cfg * | ||
| module_ext_init_decode(struct comp_dev *dev, struct module_config *dst, | ||
| const unsigned char *data, size_t *size) | ||
| int module_ext_init_decode(const struct comp_driver *drv, struct module_ext_init_data *ext_data, | ||
| struct ipc_config_process *spec) | ||
| { | ||
| const struct ipc4_module_init_ext_init *ext_init = | ||
| (const struct ipc4_module_init_ext_init *)data; | ||
| (const struct ipc4_module_init_ext_init *)spec->data; | ||
| bool last_object = !ext_init->data_obj_array; | ||
| const struct ipc4_module_init_ext_object *obj; | ||
| if (*size < sizeof(ext_init)) { | ||
| comp_err(dev, "Size too small for ext init %u < %u", | ||
| *size, sizeof(ext_init)); | ||
| return NULL; | ||
| assert(drv->type == SOF_COMP_MODULE_ADAPTER); | ||
| if (spec->size < sizeof(ext_init)) { | ||
| comp_cl_err(drv, "Size too small for ext init %zu < %zu", | ||
| spec->size, sizeof(ext_init)); | ||
| return -EINVAL; | ||
| } | ||
| /* TODO: Handle ext_init->gna_used and ext_init->rtos_domain here */ | ||
| /* Get the first obj struct right after ext_init struct */ | ||
| @@ -46,18 +46,18 @@ module_ext_init_decode(struct comp_dev *dev, struct module_config *dst, | ||
| const struct ipc4_module_init_ext_object *next_obj; | ||
| /* Check if there is space for the object header */ | ||
| if ((unsigned char *)(obj + 1) - data > *size) { | ||
| comp_err(dev, "ext init obj overflow, %u > %u", | ||
| (unsigned char *)(obj + 1) - data, *size); | ||
| return NULL; | ||
| if ((unsigned char *)(obj + 1) - spec->data > spec->size) { | ||
| comp_cl_err(drv, "ext init obj overflow, %u > %zu", | ||
| (unsigned char *)(obj + 1) - spec->data, spec->size); | ||
| return -EINVAL; | ||
| } | ||
| /* Calculate would be next object position and check if current object fits */ | ||
| next_obj = (const struct ipc4_module_init_ext_object *) | ||
| (((uint32_t *) (obj + 1)) + obj->object_words); | ||
| if ((unsigned char *)next_obj - data > *size) { | ||
| comp_err(dev, "ext init object array overflow, %u > %u", | ||
| (unsigned char *)obj - data, *size); | ||
| return NULL; | ||
| if ((unsigned char *)next_obj - spec->data > spec->size) { | ||
| comp_cl_err(drv, "ext init object array overflow, %u > %zu", | ||
| (unsigned char *)obj - spec->data, spec->size); | ||
| return -EINVAL; | ||
| } | ||
| switch (obj->object_id) { | ||
| case IPC4_MOD_INIT_DATA_ID_DP_DATA: | ||
| @@ -67,32 +67,34 @@ module_ext_init_decode(struct comp_dev *dev, struct module_config *dst, | ||
| (const struct ipc4_module_init_ext_obj_dp_data *)(obj + 1); | ||
| if (obj->object_words * sizeof(uint32_t) < sizeof(*dp_data)) { | ||
| comp_err(dev, "dp_data object too small %u < %u", | ||
| obj->object_words * sizeof(uint32_t), sizeof(*dp_data)); | ||
| return NULL; | ||
| comp_cl_warn(drv, "dp_data object too small %zu < %zu", | ||
| obj->object_words * sizeof(uint32_t), | ||
| sizeof(*dp_data)); | ||
| break; | ||
Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this not fatal any more? Also Collaborator There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks a bit suspicious, this goes on to access "dp_data->interim_heap_bytes" from the too small object. This does not look correct. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is mandatory if we go and redefine "struct ipc4_module_init_ext_obj_dp_data". Otherwise payloads sent by an older linux version would always fail dp module creation. But fear not, non of the data in the too short data object is used. Its all ignored as all of that code accessing the data is within the case block. And we need to continue the decoding loop in case there is some other objects there that we can decode correctly. | ||
| } | ||
| dst->domain_id = dp_data->domain_id; | ||
| dst->stack_bytes = dp_data->stack_bytes; | ||
| dst->heap_bytes = dp_data->heap_bytes; | ||
| comp_info(dev, "init_ext_obj_dp_data domain %u stack %u heap %u", | ||
| dp_data->domain_id, dp_data->stack_bytes, dp_data->heap_bytes); | ||
| ext_data->dp_data = dp_data; | ||
| comp_cl_info(drv, | ||
| "init_ext_obj_dp_data domain %u stack %u interim %u lifetime %u shared %u", | ||
| dp_data->domain_id, dp_data->stack_bytes, | ||
| dp_data->interim_heap_bytes, dp_data->lifetime_heap_bytes, | ||
| dp_data->shared_bytes); | ||
| break; | ||
| } | ||
| default: | ||
| comp_info(dev, "Unknown ext init object id %u of %u words", | ||
| obj->object_id, obj->object_words); | ||
| comp_cl_info(drv, "Unknown ext init object id %u of %u words", | ||
| obj->object_id, obj->object_words); | ||
| } | ||
| /* Read the last object flag from obj header */ | ||
| last_object = obj->last_object; | ||
| /* Move to next object */ | ||
| obj = next_obj; | ||
| } | ||
| /* Remove decoded ext_init payload from the size */ | ||
| *size -= (unsigned char *) obj - data; | ||
| /* Remove decoded ext_init payload from spec */ | ||
| spec->size -= (unsigned char *)obj - spec->data; | ||
| spec->data = (const unsigned char *)obj; | ||
| /* return remaining payload */ | ||
| return (const struct ipc4_base_module_extended_cfg *)obj; | ||
| return 0; | ||
| } | ||
| /* | ||
| @@ -114,10 +116,7 @@ int module_adapter_init_data(struct comp_dev *dev, | ||
| size_t cfgsz = args->size; | ||
| assert(dev->drv->type == SOF_COMP_MODULE_ADAPTER); | ||
| if (config->ipc_extended_init) | ||
| cfg = module_ext_init_decode(dev, dst, args->data, &cfgsz); | ||
| else | ||
| cfg = (const struct ipc4_base_module_extended_cfg *)args->data; | ||
| cfg = (const struct ipc4_base_module_extended_cfg *)args->data; | ||
| if (cfg == NULL) | ||
| return -EINVAL; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -69,6 +69,41 @@ enum ipc4_pipeline_state { | ||
| SOF_IPC4_PIPELINE_STATE_SAVED | ||
| }; | ||
| /* IDs for all pipeline ext data types in struct ipc4_pipeline_init_ext_object */ | ||
| enum ipc4_pipeline_ext_obj_id { | ||
| IPC4_GLB_PIPE_EXT_OBJ_ID_INVALID = 0, | ||
| IPC4_GLB_PIPE_EXT_OBJ_ID_MEM_DATA = 1, | ||
| IPC4_GLB_PIPE_EXT_OBJ_ID_MAX = IPC4_GLB_PIPE_EXT_OBJ_ID_MEM_DATA, | ||
| }; | ||
| /* data object for vendor bespoke data with ABI growth and backwards compat */ | ||
| struct ipc4_pipeline_ext_object { | ||
| uint32_t last_object : 1; /* object is last in array if 1 else object follows. */ | ||
| uint32_t object_id : 15; /* unique ID for this object or globally */ | ||
| uint32_t object_words : 16; /* size in dwords (excluding this structure) */ | ||
| } __packed __aligned(4); | ||
| /* the object data will be placed in memory here and will have size "object_words" */ | ||
| /* Ext array data object for pipeline instance's memory requirements */ | ||
| struct ipc4_pipeline_ext_obj_mem_data { | ||
| uint32_t domain_id; /* userspace domain ID */ | ||
| uint32_t stack_bytes; /* required stack size in bytes */ | ||
| uint32_t interim_heap_bytes; /* required interim heap size in bytes */ | ||
| uint32_t lifetime_heap_bytes; /* required lifetime heap size in bytes */ | ||
| uint32_t shared_bytes; /* required shared memory in bytes */ | ||
| } __packed __aligned(4); | ||
jsarha marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| /* | ||
| * Host Driver sends this message to create a new pipeline instance. | ||
| */ | ||
| struct ipc4_pipeline_ext_payload { | ||
| uint32_t payload_words : 24; /* size in dwords (excluding this structure) */ | ||
| uint32_t data_obj_array : 1; /* struct ipc4_pipeline_ext_object data */ | ||
| uint32_t rsvd0 : 7; | ||
| uint32_t rsvd1; | ||
| uint32_t rsvd2; | ||
| } __packed __aligned(4); | ||
| /*! | ||
| * lp - indicates whether the pipeline should be kept on running in low power | ||
| * mode. On BXT the driver should set this flag to 1 for WoV pipeline. | ||
| @@ -106,7 +141,8 @@ struct ipc4_pipeline_create { | ||
| uint32_t rsvd1 : 3; | ||
| uint32_t attributes : 16; | ||
| uint32_t core_id : 4; | ||
| uint32_t rsvd2 : 6; | ||
| uint32_t rsvd2 : 5; | ||
| uint32_t payload : 1; | ||
| uint32_t _reserved_2 : 2; | ||
| } r; | ||
| } extension; | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the commit message, I understand by "released" you mean it's already a part of the ABI? Maybe better use that wording, since I first thought "released" meant "freed" which was confusing. (unrelated to this commit) just noticing, that
module_ext_init_decode()returns a pointer of typestruct ipc4_base_module_extended_cfgwhich should only be appropriate for the base module? StrangeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix the commit message.
The reason is that the base config follows ext_init in the payload, and if there is anything left aftere ext_init, it should be base config. Anyway, this is going way when I move module_ext_init_decode()-call to module_adapter_new_ext().