Re: [PATCH v8 11/14] firmware: qcom_scm: Add qcom_scm_pas_get_rsc_table() to get resource table

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 11/24/25 4:25 PM, Mukesh Ojha wrote:
> On Mon, Nov 24, 2025 at 12:48:31PM +0100, Konrad Dybcio wrote:
>> On 11/21/25 12:01 PM, Mukesh Ojha wrote:
>>> Qualcomm remote processor may rely on Static and Dynamic resources for
>>> it to be functional. Static resources are fixed like for example,
>>> memory-mapped addresses required by the subsystem and dynamic
>>> resources, such as shared memory in DDR etc., are determined at
>>> runtime during the boot process.
>>>
>>> For most of the Qualcomm SoCs, when run with Gunyah or older QHEE
>>> hypervisor, all the resources whether it is static or dynamic, is
>>> managed by the hypervisor. Dynamic resources if it is present for a
>>> remote processor will always be coming from secure world via SMC call
>>> while static resources may be present in remote processor firmware
>>> binary or it may be coming qcom_scm_pas_get_rsc_table() SMC call along
>>> with dynamic resources.
>>>
>>> Some of the remote processor drivers, such as video, GPU, IPA, etc., do
>>> not check whether resources are present in their remote processor
>>> firmware binary. In such cases, the caller of this function should set
>>> input_rt and input_rt_size as NULL and zero respectively. Remoteproc
>>> framework has method to check whether firmware binary contain resources
>>> or not and they should be pass resource table pointer to input_rt and
>>> resource table size to input_rt_size and this will be forwarded to
>>> TrustZone for authentication. TrustZone will then append the dynamic
>>> resources and return the complete resource table in output_rt
>>>
>>> More about documentation on resource table format can be found in
>>> include/linux/remoteproc.h
>>>
>>> Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
>>> ---
>>
>> [...]
>>
>>> +int qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *ctx, void *input_rt,
>>> +			       size_t input_rt_size, void **output_rt,
>>> +			       size_t *output_rt_size)
>>> +{
>>> +	unsigned int retry_num = 5;
>>> +	int ret;
>>> +
>>> +	do {
>>> +		*output_rt = kzalloc(*output_rt_size, GFP_KERNEL);
>>> +		if (!*output_rt)
>>> +			return -ENOMEM;
>>> +
>>> +		ret = __qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt,
>>> +						   input_rt_size, output_rt,
>>> +						   output_rt_size);
>>> +		if (ret)
>>> +			kfree(*output_rt);
>>> +
>>> +	} while (ret == -EAGAIN && --retry_num);
>>
>> Will firmware return -EAGAIN as a result, or is this to handle the
>> "buffer too small case"?
> 
> The latter one where a re-attempt could pass..
> 
>>
>> I think the latter should use a different errno (EOVERFLOW?) and print
>> a message since we decided that it's the caller that suggests a suitable
>> output buffer size
> 
> Agree with error code..
> 
> This is kept on the caller side keeping future in mind. where we can have
> resource table coming from the client and it needs to go to TZ for
> authentication.
> 
> Are you suggesting to move this retry on the caller side ?

I think we got confused in the review of the previous iterations and made
qcom_scm_pas_get_rsc_table() retry 5 times (on the basis that "some" error
could happen in firmware), but if it's specifically "buf too small", we should
only need to call it utmost twice - once to get the required larger size (or
succeed and exit) and another one with a now-correctly sized buffer.

Looking at it again, do we really need to be so stringent about the maximum
resource table size? Can we just push the currently defined SZ_16K inside
qcom_scm_pas_get_rsc_table() as a constant and bump it up as necessary in
the future?

> Just for information, once video patches becomes ready, we may bring this
> qcom_mdt_pas_map_devmem_rscs()[1] helper for video or any other client
> should be do this here as well ?
> 
> I wanted to optimize this path, where caller is making a guess and
> gets the updated output size in return.

We always end up allocating in __qcom_scm_pas_get_rsc_table() so I think
guessing a number like SZ_16K which is plenty for a effectively small u64[]
in this file is ok too. Perhaps we could even shrink it down a bit..

> [1]
> https://lore.kernel.org/lkml/20250819165447.4149674-9-mukesh.ojha@oss.qualcomm.com/#t
> 
>>
>> In case it doesn't make sense for the caller to share their expectations,
>> the buffer could be allocated (and perhaps resized if necessary) internally
>> with some validation (i.e. a rsctable likely won't be 5 GiB)
> 
> Are you saying output_size as well should be checked and it should not be
> greater than something like UINT_MAX or something.. ?
> 
> +	*output_rt_size = res.result[2];

Yeah we should probably make sure this doesn't exceed a large-but-not-
entirely-unreasonable value

Konrad