On 4/2/24 2:30 PM, Johan Hovold wrote:
On Fri, Mar 29, 2024 at 09:18:25PM +0100, Maximilian Luz wrote:
It turns out that while the QSEECOM APP_SEND command has specific fields
for request and response buffers, uefisecapp expects them both to be in
a single memory region. Failure to adhere to this has (so far) resulted
in either no response being written to the response buffer (causing an
EIO to be emitted down the line), the SCM call to fail with EINVAL
(i.e., directly from TZ/firmware), or the device to be hard-reset.
While this issue can be triggered deterministically, in the current form
it seems to happen rather sporadically (which is why it has gone
unnoticed during earlier testing). This is likely due to the two
kzalloc() calls (for request and response) being directly after each
other. Which means that those likely return consecutive regions most of
the time, especially when not much else is going on in the system.
Fix this by allocating a single memory region for both request and
response buffers, properly aligning both structs inside it. This
unfortunately also means that the qcom_scm_qseecom_app_send() interface
needs to be restructured, as it should no longer map the DMA regions
separately. Therefore, move the responsibility of DMA allocation (or
mapping) to the caller.
Fixes: 759e7a2b62eb ("firmware: Add support for Qualcomm UEFI Secure Application")
Signed-off-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>
Thanks for tracking this down. I can confirm that this fixes the
hypervisor reset I can trigger by repeatedly reading an EFI variable on
the X13s. Before it triggered within minutes, now I ran it for 3 hours
without any issues:
Tested-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
Even if the patch is a bit large it's straight-forward and fixes a
critical bug so I think this needs to go to stable as well:
Cc: stable@xxxxxxxxxxxxxxx # 6.7
A couple of comments below.
@@ -277,10 +296,15 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
unsigned long buffer_size = *data_size;
efi_status_t efi_status = EFI_SUCCESS;
unsigned long name_length;
+ dma_addr_t cmd_buf_phys;
Throughout the patch, could you please refer to DMA addresses as DMA
addresses rather than physical addresses, for example, by using a "_dma"
rather than "_phys" suffix here?
Sure. I've applied this and the changes below (as well as similar ones
for qcom_qseecom.h) for v2.
Thanks!
Best regards,
Max
+ size_t cmd_buf_size;
+ void *cmd_buf_virt;
I'd also prefer if you dropped the "_virt" suffix from the buffer
pointers.
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 49ddbcab06800..fc59688227f43 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1579,46 +1579,26 @@ EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_get_id);
/**
* qcom_scm_qseecom_app_send() - Send to and receive data from a given QSEE app.
* @app_id: The ID of the target app.
- * @req: Request buffer sent to the app (must be DMA-mappable).
+ * @req: Physical address of the request buffer sent to the app.
DMA address
* @req_size: Size of the request buffer.
- * @rsp: Response buffer, written to by the app (must be DMA-mappable).
+ * @rsp: Physical address of the response buffer, written to by the app.
DMA address
* @rsp_size: Size of the response buffer.
*
* Sends a request to the QSEE app associated with the given ID and read back
- * its response. The caller must provide two DMA memory regions, one for the
- * request and one for the response, and fill out the @req region with the
+ * its response. The caller must provide two physical memory regions, one for
DMA memory
+ * the request and one for the response, and fill out the @req region with the
* respective (app-specific) request data. The QSEE app reads this and returns
* its response in the @rsp region.
*
* Return: Zero on success, nonzero on failure.
*/
-int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
- size_t rsp_size)
+int qcom_scm_qseecom_app_send(u32 app_id, dma_addr_t req, size_t req_size,
+ dma_addr_t rsp, size_t rsp_size)
And similar throughout.
With that fixed, feel free to add:
Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
Johan
