Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Tue, 10 Oct 2023 17:25:12 -0500
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Bartosz Golaszewski <brgl@bgdev.pl>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Elliot Berman <quic_eberman@quicinc.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        Guru Das Srinagesh <quic_gurus@quicinc.com>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        Alex Elder <elder@linaro.org>,
        Srini Kandagatla <srinivas.kandagatla@linaro.org>,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, kernel@quicinc.com,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [PATCH v3 08/15] firmware: qcom: scm: make
 qcom_scm_ice_set_key() use the TZ allocator
Message-ID: <25rend34es2ayrgbyawoz6tfpweba3drvdrwgiflxhkd7lipma@lj6xolgwwjpt>
References: <20231009153427.20951-1-brgl@bgdev.pl>
 <20231009153427.20951-9-brgl@bgdev.pl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231009153427.20951-9-brgl@bgdev.pl>
Precedence: bulk

On Mon, Oct 09, 2023 at 05:34:20PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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@linaro.org>
> ---
>  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@redhat.com>

>  	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
>