Re: [PATCH v3 08/15] firmware: qcom: scm: make qcom_scm_ice_set_key() use the TZ allocator

[Andrew Halaney]

On Mon, Oct 09, 2023 at 05:34:20PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> 
> Let's use the new TZ memory allocator to obtain a buffer for this call
> instead of using dma_alloc_coherent().
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 754f6056b99f..31071a714cf1 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1197,32 +1197,21 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>  		.args[4] = data_unit_size,
>  		.owner = ARM_SMCCC_OWNER_SIP,
>  	};
> -	void *keybuf;
> -	dma_addr_t key_phys;
> +
>  	int ret;
>  
> -	/*
> -	 * 'key' may point to vmalloc()'ed memory, but we need to pass a
> -	 * physical address that's been properly flushed.  The sanctioned way to
> -	 * do this is by using the DMA API.  But as is best practice for crypto
> -	 * keys, we also must wipe the key after use.  This makes kmemdup() +
> -	 * dma_map_single() not clearly correct, since the DMA API can use
> -	 * bounce buffers.  Instead, just use dma_alloc_coherent().  Programming
> -	 * keys is normally rare and thus not performance-critical.
> -	 */
> -
> -	keybuf = dma_alloc_coherent(__scm->dev, key_size, &key_phys,
> -				    GFP_KERNEL);
> +	void *keybuf __free(qcom_tzmem) = qcom_tzmem_alloc(__scm->mempool,
> +							   key_size,
> +							   GFP_KERNEL);

At the risk of sounding like a broken record, the same nit about
declaration being moved, I'll just mention that one last time here in
the series and then accept the outcome of that discussion across the
series :) Also a bummer to lose that comment, but I guess oh well.

Reviewed-by: Andrew Halaney <ahalaney@xxxxxxxxxx>

>  	if (!keybuf)
>  		return -ENOMEM;
>  	memcpy(keybuf, key, key_size);
> -	desc.args[1] = key_phys;
> +	desc.args[1] = qcom_tzmem_to_phys(keybuf);
>  
>  	ret = qcom_scm_call(__scm->dev, &desc, NULL);
>  
>  	memzero_explicit(keybuf, key_size);
>  
> -	dma_free_coherent(__scm->dev, key_size, keybuf, key_phys);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
> -- 
> 2.39.2
>