[gbernatxintel]

Added support for ipc4 query no 16.
This makes it possible to get information about the current basefw library.

[lgirdwood]

Will need to additionally pack our ABI

[lgirdwood]

will need packed too

[gbernatxintel]

SOFCI TEST

[softwarecki]

This condition is surprising. What is it supposed to check, apart from a header file is included?

[gbernatxintel]

This is exactly what he is supposed to check. Generally it is that in uses the definition which is platform specific. The idea is to check if there is this definition which is in the code. Do you think it's better to replace it with CONFIG_INTEL?

[lgirdwood]

@kv2019i and I were discussing a src/ipc/ipc4/intel.c where we could put a lot of the vendor specific code and wrap it with Kconfigs, have it read values from Zephyr DT later too.

It would seem we could use that here too and rather than using the #ifdef ZEPHYR_SOC_INTEL_ADSP_MEMORY_H_ we use a Kconfig CONFIG_SOME_INTEL_MEMORY_OPTION, so lets use Kconfig now and next step move into an intel specific file.

[softwarecki]

My question arises from the fact that in another place in the code where the IMR_BOOT_LDR_MANIFEST_BASE definition is used, the condition looks different. It would be good to check it the same way in both places.

sof/src/ipc/ipc4/helper.c

Lines 959 to 961 in 14c4e86

	#ifdef RIMAGE_MANIFEST
	desc = (const struct sof_man_fw_desc *)IMR_BOOT_LDR_MANIFEST_BASE;
	#else

[kv2019i]

I was looking at the same @softwarecki but later realized RIMAGE_MANIFEST is locally defined in helper.c. However, #9012 was now merged, so we now have a place to put Intel/vendor specific bits of code and don't need to ifdefs like RIMAGE_MANIFEST anymore.

E.g. I'd propose to make getting the base_fw manifest a platform specific call. Something like:

``` `
if (lib_id == 0)
desc = platform_base_fw_get_manifest()
else
desc = lib_manager_get_library_manifest()

And add platform_base_fw_get_manifest() to the newly added base_fw_platform.h and add implementation for Intel ACE and a dummy one for posix.

[lgirdwood]

Lets use Kconfig

[lgirdwood]

@kv2019i and I were discussing a src/ipc/ipc4/intel.c where we could put a lot of the vendor specific code and wrap it with Kconfigs, have it read values from Zephyr DT later too.

It would seem we could use that here too and rather than using the #ifdef ZEPHYR_SOC_INTEL_ADSP_MEMORY_H_ we use a Kconfig CONFIG_SOME_INTEL_MEMORY_OPTION, so lets use Kconfig now and next step move into an intel specific file.

[kv2019i]

Proposal how to handle the ifdef added inline

[kv2019i]

I was looking at the same @softwarecki but later realized RIMAGE_MANIFEST is locally defined in helper.c. However, #9012 was now merged, so we now have a place to put Intel/vendor specific bits of code and don't need to ifdefs like RIMAGE_MANIFEST anymore.

E.g. I'd propose to make getting the base_fw manifest a platform specific call. Something like:

``` `
if (lib_id == 0)
desc = platform_base_fw_get_manifest()
else
desc = lib_manager_get_library_manifest()

And add platform_base_fw_get_manifest() to the newly added base_fw_platform.h and add implementation for Intel ACE and a dummy one for posix.

[kv2019i]

Thanks @gbernatxintel , looks good now!

[softwarecki]

In my opinion, there is a missing check (eg. static assert) to make sure that sizeof(struct ipc4_libraries_info) + LIB_MANAGER_MAX_LIBS * sizeof(struct ipc4_library_props) not exceed the SOF_IPC_MSG_MAX_SIZE. Under unfavorable circumstances, this may result in a response buffer overflow.

[softwarecki]

I would like change the libraries filed declaration to struct ipc4_library_props libraries[0]; and then simplify this calculation to *data_offset = sizeof(libs_info) + libs_info->library_count * sizeof(libs_info->libraries[0]);

Comments addressed

[kv2019i]

@softwarecki Ok for you?

[softwarecki]

sizeof(uint32_t) -> sizeof(struct ipc4_libraries_info)