Re: [PATCH v7 11/12] firmware: qcom: scm: clarify the comment in qcom_scm_pas_init_image()

From: Prasad Sodagudi
Date: Fri Mar 01 2024 - 13:52:51 EST



On 2/17/2024 7:50 PM, Bjorn Andersson wrote:
On Mon, Feb 05, 2024 at 07:28:09PM +0100, Bartosz Golaszewski wrote:
From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>

The "memory protection" mechanism mentioned in the comment is the SHM
Bridge. This is also the reason why we do not convert this call to using
the TZ memory allocator.

No, this mechanism predates shmbridge.
Yes. PIL calls are there for very long time and shm bridge concept is introduced later.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
Tested-by: Andrew Halaney <ahalaney@xxxxxxxxxx> # sc8280xp-lenovo-thinkpad-x13s
Tested-by: Deepti Jaggi <quic_djaggi@xxxxxxxxxxx> #sa8775p-ride
Reviewed-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
---
drivers/firmware/qcom/qcom_scm.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 839773270a21..7ba5cff6e4e7 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -563,9 +563,13 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
struct qcom_scm_res res;
/*
- * During the scm call memory protection will be enabled for the meta
- * data blob, so make sure it's physically contiguous, 4K aligned and
- * non-cachable to avoid XPU violations.
What this is saying is that the memory will be made unaccessible to
Linux, so it needs to be contiguous and aligned.

Based on my understanding,  this buffer has to be  physically contiguous, 4K aligned and non-cachable to avoid XPU violations.

We should keep this comment


+ * During the SCM call the hypervisor will make the buffer containing
+ * the program data into an SHM Bridge.
Are you saying that the hypervisor will convert this memory to a
shmbridge, and then pass it to TrustZone?
Yes.  Specifically for PIL calls hyp is creating shm bridge for buffers/regions on behalf of Linux/NS-EL1.

This is why we exceptionally
+ * must not use the TrustZone memory allocator here as - depending on
+ * Kconfig - it may already use the SHM Bridge mechanism internally.
+ *
"it may already"? You describe above that we shouldn't pass shmbridge
memory, and the other case never deals with shmbridges. So, I think you
can omit this part.

+ * If we pass a buffer that is already part of an SHM Bridge to this
+ * call, it will fail.
Could this be because the consumer of this buffer operates in EL2, and
not TZ?
These buffers are consumed by TZ only and not by EL2.  hyp creating the required bridges for pil buffers.

Regards,
Bjorn

*/
mdata_buf = dma_alloc_coherent(__scm->dev, size, &mdata_phys,
GFP_KERNEL);
--
2.40.1