diff mbox series

[v3,11/15] firmware: qcom: qseecom: convert to using the TZ allocator

Message ID 20231009153427.20951-12-brgl@bgdev.pl (mailing list archive)
State New, archived
Headers show
Series arm64: qcom: add and enable SHM Bridge support | expand

Commit Message

Bartosz Golaszewski Oct. 9, 2023, 3:34 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
convert all users of it in the qseecom module to using the TZ allocator
for creating SCM call buffers. Together with using the cleanup macros,
it has the added benefit of a significant code shrink. As this is
largely a module separate from the SCM driver, let's use a separate
memory pool.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 .../firmware/qcom/qcom_qseecom_uefisecapp.c   | 260 +++++++-----------
 drivers/firmware/qcom/qcom_scm.c              |  30 +-
 include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
 3 files changed, 103 insertions(+), 191 deletions(-)

Comments

Andrew Halaney Oct. 10, 2023, 10:49 p.m. UTC | #1
On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> convert all users of it in the qseecom module to using the TZ allocator
> for creating SCM call buffers. Together with using the cleanup macros,
> it has the added benefit of a significant code shrink. As this is
> largely a module separate from the SCM driver, let's use a separate
> memory pool.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

<snip>

> @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>  		return EFI_INVALID_PARAMETER;
>  
>  	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->length < sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length < sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->status) {
>  		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>  		if (efi_status == EFI_BUFFER_TOO_SMALL)
>  			*name_size = rsp_data->name_size;
>  
> -		goto out_free;
> +		return efi_status;
>  	}
>  
> -	if (rsp_data->length > rsp_size) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length > rsp_size)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>  
> -	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>  
>  	if (rsp_data->name_size > *name_size) {
>  		*name_size = rsp_data->name_size;
> -		efi_status = EFI_BUFFER_TOO_SMALL;
> -		goto out_free;
> +		return EFI_BUFFER_TOO_SMALL;
>  	}
>  
> -	if (rsp_data->guid_size != sizeof(*guid)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->guid_size != sizeof(*guid))
> +		return EFI_DEVICE_ERROR;
>  
>  	memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
>  	status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
>  			      rsp_data->name_size / sizeof(*name));
>  	*name_size = rsp_data->name_size;
>  
> -	if (status < 0) {
> +	if (status < 0)
>  		/*
>  		 * Return EFI_DEVICE_ERROR here because the buffer size should
>  		 * have already been validated above, causing this function to
>  		 * bail with EFI_BUFFER_TOO_SMALL.
>  		 */
>  		return EFI_DEVICE_ERROR;
> -	}

Personally (no idea what the actual style guide says) leaving braces
around the multiline if statement would be nice.... that being said,
that's my opinion :)

<snip>
> @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
>  	if (max_variable_size)
>  		*max_variable_size = rsp_data->max_variable_size;
>  
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>  }
>  
>  /* -- Global efivar interface. ---------------------------------------------- */
> @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>  	if (status)
>  		qcuefi_set_reference(NULL);
>  
> +	qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);

Any particular reason for this size? Just curious, it was (one) of the
reasons I had not marked patch 4 yet (it looks good, but I wanted to get
through the series to digest the Kconfig as well).

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Bartosz Golaszewski Oct. 11, 2023, 7:44 a.m. UTC | #2
On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> > convert all users of it in the qseecom module to using the TZ allocator
> > for creating SCM call buffers. Together with using the cleanup macros,
> > it has the added benefit of a significant code shrink. As this is
> > largely a module separate from the SCM driver, let's use a separate
> > memory pool.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> <snip>
>
> > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
> >               return EFI_INVALID_PARAMETER;
> >
> >       status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> > -     if (status) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (status)
> > +             return EFI_DEVICE_ERROR;
> >
> > -     if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
> > +             return EFI_DEVICE_ERROR;
> >
> > -     if (rsp_data->length < sizeof(*rsp_data)) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (rsp_data->length < sizeof(*rsp_data))
> > +             return EFI_DEVICE_ERROR;
> >
> >       if (rsp_data->status) {
> >               dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
> >               if (efi_status == EFI_BUFFER_TOO_SMALL)
> >                       *name_size = rsp_data->name_size;
> >
> > -             goto out_free;
> > +             return efi_status;
> >       }
> >
> > -     if (rsp_data->length > rsp_size) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (rsp_data->length > rsp_size)
> > +             return EFI_DEVICE_ERROR;
> >
> > -     if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
> > +             return EFI_DEVICE_ERROR;
> >
> > -     if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
> > +             return EFI_DEVICE_ERROR;
> >
> >       if (rsp_data->name_size > *name_size) {
> >               *name_size = rsp_data->name_size;
> > -             efi_status = EFI_BUFFER_TOO_SMALL;
> > -             goto out_free;
> > +             return EFI_BUFFER_TOO_SMALL;
> >       }
> >
> > -     if (rsp_data->guid_size != sizeof(*guid)) {
> > -             efi_status = EFI_DEVICE_ERROR;
> > -             goto out_free;
> > -     }
> > +     if (rsp_data->guid_size != sizeof(*guid))
> > +             return EFI_DEVICE_ERROR;
> >
> >       memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
> >       status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
> >                             rsp_data->name_size / sizeof(*name));
> >       *name_size = rsp_data->name_size;
> >
> > -     if (status < 0) {
> > +     if (status < 0)
> >               /*
> >                * Return EFI_DEVICE_ERROR here because the buffer size should
> >                * have already been validated above, causing this function to
> >                * bail with EFI_BUFFER_TOO_SMALL.
> >                */
> >               return EFI_DEVICE_ERROR;
> > -     }
>
> Personally (no idea what the actual style guide says) leaving braces
> around the multiline if statement would be nice.... that being said,
> that's my opinion :)
>
> <snip>
> > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
> >       if (max_variable_size)
> >               *max_variable_size = rsp_data->max_variable_size;
> >
> > -out_free:
> > -     kfree(rsp_data);
> > -out_free_req:
> > -     kfree(req_data);
> > -out:
> > -     return efi_status;
> > +     return EFI_SUCCESS;
> >  }
> >
> >  /* -- Global efivar interface. ---------------------------------------------- */
> > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
> >       if (status)
> >               qcuefi_set_reference(NULL);
> >
> > +     qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
>
> Any particular reason for this size? Just curious, it was (one) of the
> reasons I had not marked patch 4 yet (it looks good, but I wanted to get
> through the series to digest the Kconfig as well).
>

I cannot test this. Do you know what the minimum correct size would be?

Bart

> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
>
Andrew Halaney Oct. 11, 2023, 1:55 p.m. UTC | #3
On Wed, Oct 11, 2023 at 09:44:54AM +0200, Bartosz Golaszewski wrote:
> On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote:
> >
> > On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> > > convert all users of it in the qseecom module to using the TZ allocator
> > > for creating SCM call buffers. Together with using the cleanup macros,
> > > it has the added benefit of a significant code shrink. As this is
> > > largely a module separate from the SCM driver, let's use a separate
> > > memory pool.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > <snip>
> >
> > > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
> > >               return EFI_INVALID_PARAMETER;
> > >
> > >       status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> > > -     if (status) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (status)
> > > +             return EFI_DEVICE_ERROR;
> > >
> > > -     if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
> > > +             return EFI_DEVICE_ERROR;
> > >
> > > -     if (rsp_data->length < sizeof(*rsp_data)) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (rsp_data->length < sizeof(*rsp_data))
> > > +             return EFI_DEVICE_ERROR;
> > >
> > >       if (rsp_data->status) {
> > >               dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> > > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
> > >               if (efi_status == EFI_BUFFER_TOO_SMALL)
> > >                       *name_size = rsp_data->name_size;
> > >
> > > -             goto out_free;
> > > +             return efi_status;
> > >       }
> > >
> > > -     if (rsp_data->length > rsp_size) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (rsp_data->length > rsp_size)
> > > +             return EFI_DEVICE_ERROR;
> > >
> > > -     if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
> > > +             return EFI_DEVICE_ERROR;
> > >
> > > -     if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
> > > +             return EFI_DEVICE_ERROR;
> > >
> > >       if (rsp_data->name_size > *name_size) {
> > >               *name_size = rsp_data->name_size;
> > > -             efi_status = EFI_BUFFER_TOO_SMALL;
> > > -             goto out_free;
> > > +             return EFI_BUFFER_TOO_SMALL;
> > >       }
> > >
> > > -     if (rsp_data->guid_size != sizeof(*guid)) {
> > > -             efi_status = EFI_DEVICE_ERROR;
> > > -             goto out_free;
> > > -     }
> > > +     if (rsp_data->guid_size != sizeof(*guid))
> > > +             return EFI_DEVICE_ERROR;
> > >
> > >       memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
> > >       status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
> > >                             rsp_data->name_size / sizeof(*name));
> > >       *name_size = rsp_data->name_size;
> > >
> > > -     if (status < 0) {
> > > +     if (status < 0)
> > >               /*
> > >                * Return EFI_DEVICE_ERROR here because the buffer size should
> > >                * have already been validated above, causing this function to
> > >                * bail with EFI_BUFFER_TOO_SMALL.
> > >                */
> > >               return EFI_DEVICE_ERROR;
> > > -     }
> >
> > Personally (no idea what the actual style guide says) leaving braces
> > around the multiline if statement would be nice.... that being said,
> > that's my opinion :)
> >
> > <snip>
> > > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
> > >       if (max_variable_size)
> > >               *max_variable_size = rsp_data->max_variable_size;
> > >
> > > -out_free:
> > > -     kfree(rsp_data);
> > > -out_free_req:
> > > -     kfree(req_data);
> > > -out:
> > > -     return efi_status;
> > > +     return EFI_SUCCESS;
> > >  }
> > >
> > >  /* -- Global efivar interface. ---------------------------------------------- */
> > > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
> > >       if (status)
> > >               qcuefi_set_reference(NULL);
> > >
> > > +     qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
> >
> > Any particular reason for this size? Just curious, it was (one) of the
> > reasons I had not marked patch 4 yet (it looks good, but I wanted to get
> > through the series to digest the Kconfig as well).
> >
> 
> I cannot test this. Do you know what the minimum correct size would be?

I've got no insight into these firmware interfaces unfortunately. Was
mostly curious if Qualcomm had provided a suggestion behind the scenes
or if this was picked as a "sufficiently large" pool size.

> 
> Bart
> 
> > Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> >
>
Bartosz Golaszewski Oct. 11, 2023, 2:37 p.m. UTC | #4
On Wed, Oct 11, 2023 at 3:56 PM Andrew Halaney <ahalaney@redhat.com> wrote:
>
> On Wed, Oct 11, 2023 at 09:44:54AM +0200, Bartosz Golaszewski wrote:
> > On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote:
> > >
> > > On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> > > > convert all users of it in the qseecom module to using the TZ allocator
> > > > for creating SCM call buffers. Together with using the cleanup macros,
> > > > it has the added benefit of a significant code shrink. As this is
> > > > largely a module separate from the SCM driver, let's use a separate
> > > > memory pool.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > <snip>
> > >
> > > > @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
> > > >               return EFI_INVALID_PARAMETER;
> > > >
> > > >       status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> > > > -     if (status) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (status)
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > > -     if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > > -     if (rsp_data->length < sizeof(*rsp_data)) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (rsp_data->length < sizeof(*rsp_data))
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > >       if (rsp_data->status) {
> > > >               dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> > > > @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
> > > >               if (efi_status == EFI_BUFFER_TOO_SMALL)
> > > >                       *name_size = rsp_data->name_size;
> > > >
> > > > -             goto out_free;
> > > > +             return efi_status;
> > > >       }
> > > >
> > > > -     if (rsp_data->length > rsp_size) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (rsp_data->length > rsp_size)
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > > -     if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > > -     if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > >       if (rsp_data->name_size > *name_size) {
> > > >               *name_size = rsp_data->name_size;
> > > > -             efi_status = EFI_BUFFER_TOO_SMALL;
> > > > -             goto out_free;
> > > > +             return EFI_BUFFER_TOO_SMALL;
> > > >       }
> > > >
> > > > -     if (rsp_data->guid_size != sizeof(*guid)) {
> > > > -             efi_status = EFI_DEVICE_ERROR;
> > > > -             goto out_free;
> > > > -     }
> > > > +     if (rsp_data->guid_size != sizeof(*guid))
> > > > +             return EFI_DEVICE_ERROR;
> > > >
> > > >       memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
> > > >       status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
> > > >                             rsp_data->name_size / sizeof(*name));
> > > >       *name_size = rsp_data->name_size;
> > > >
> > > > -     if (status < 0) {
> > > > +     if (status < 0)
> > > >               /*
> > > >                * Return EFI_DEVICE_ERROR here because the buffer size should
> > > >                * have already been validated above, causing this function to
> > > >                * bail with EFI_BUFFER_TOO_SMALL.
> > > >                */
> > > >               return EFI_DEVICE_ERROR;
> > > > -     }
> > >
> > > Personally (no idea what the actual style guide says) leaving braces
> > > around the multiline if statement would be nice.... that being said,
> > > that's my opinion :)
> > >
> > > <snip>
> > > > @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
> > > >       if (max_variable_size)
> > > >               *max_variable_size = rsp_data->max_variable_size;
> > > >
> > > > -out_free:
> > > > -     kfree(rsp_data);
> > > > -out_free_req:
> > > > -     kfree(req_data);
> > > > -out:
> > > > -     return efi_status;
> > > > +     return EFI_SUCCESS;
> > > >  }
> > > >
> > > >  /* -- Global efivar interface. ---------------------------------------------- */
> > > > @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
> > > >       if (status)
> > > >               qcuefi_set_reference(NULL);
> > > >
> > > > +     qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
> > >
> > > Any particular reason for this size? Just curious, it was (one) of the
> > > reasons I had not marked patch 4 yet (it looks good, but I wanted to get
> > > through the series to digest the Kconfig as well).
> > >
> >
> > I cannot test this. Do you know what the minimum correct size would be?
>
> I've got no insight into these firmware interfaces unfortunately. Was
> mostly curious if Qualcomm had provided a suggestion behind the scenes
> or if this was picked as a "sufficiently large" pool size.
>

No, I chose a small but reasonable value and intend to see if it
breaks anything. :)

But if anyone from QCom reading knows a better value - be it smaller
or larger, please let me know.

Bartosz

> >
> > Bart
> >
> > > Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> > >
> >
>
Maximilian Luz Oct. 11, 2023, 8:09 p.m. UTC | #5
On 10/9/23 17:34, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
> convert all users of it in the qseecom module to using the TZ allocator
> for creating SCM call buffers. Together with using the cleanup macros,
> it has the added benefit of a significant code shrink. As this is
> largely a module separate from the SCM driver, let's use a separate
> memory pool.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Tested-by: Maximilian Luz <luzmaximilian@gmail.com>

> ---
>   .../firmware/qcom/qcom_qseecom_uefisecapp.c   | 260 +++++++-----------
>   drivers/firmware/qcom/qcom_scm.c              |  30 +-
>   include/linux/firmware/qcom/qcom_qseecom.h    |   4 +-
>   3 files changed, 103 insertions(+), 191 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> index a33acdaf7b78..720cddd7c8c7 100644
> --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
> @@ -7,6 +7,7 @@
>    * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com>
>    */
>   
> +#include <linux/cleanup.h>
>   #include <linux/efi.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> @@ -18,6 +19,8 @@
>   #include <linux/ucs2_string.h>
>   
>   #include <linux/firmware/qcom/qcom_qseecom.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/firmware/qcom/qcom_tzmem.h>
>   
>   /* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */
>   
> @@ -253,6 +256,7 @@ struct qsee_rsp_uefi_query_variable_info {
>   struct qcuefi_client {
>   	struct qseecom_client *client;
>   	struct efivars efivars;
> +	struct qcom_tzmem_pool *mempool;
>   };
>   
>   static struct device *qcuefi_dev(struct qcuefi_client *qcuefi)
> @@ -272,11 +276,11 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>   					   const efi_guid_t *guid, u32 *attributes,
>   					   unsigned long *data_size, void *data)
>   {
> -	struct qsee_req_uefi_get_variable *req_data;
> -	struct qsee_rsp_uefi_get_variable *rsp_data;
> +	struct qsee_req_uefi_get_variable *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_get_variable *rsp_data __free(qcom_tzmem) = NULL;
>   	unsigned long buffer_size = *data_size;
> -	efi_status_t efi_status = EFI_SUCCESS;
>   	unsigned long name_length;
> +	efi_status_t efi_status;
>   	size_t guid_offs;
>   	size_t name_offs;
>   	size_t req_size;
> @@ -304,17 +308,13 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>   		__array(u8, buffer_size)
>   	);
>   
> -	req_data = kzalloc(req_size, GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
> -	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
>   	req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE;
>   	req_data->data_size = buffer_size;
> @@ -331,20 +331,14 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>   	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
>   
>   	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->length < sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length < sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>   
>   	if (rsp_data->status) {
>   		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> @@ -358,18 +352,14 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>   				*attributes = rsp_data->attributes;
>   		}
>   
> -		goto out_free;
> +		return efi_status;
>   	}
>   
> -	if (rsp_data->length > rsp_size) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length > rsp_size)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>   
>   	/*
>   	 * Note: We need to set attributes and data size even if the buffer is
> @@ -392,33 +382,23 @@ static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
>   	if (attributes)
>   		*attributes = rsp_data->attributes;
>   
> -	if (buffer_size == 0 && !data) {
> -		efi_status = EFI_SUCCESS;
> -		goto out_free;
> -	}
> +	if (buffer_size == 0 && !data)
> +		return EFI_SUCCESS;
>   
> -	if (buffer_size < rsp_data->data_size) {
> -		efi_status = EFI_BUFFER_TOO_SMALL;
> -		goto out_free;
> -	}
> +	if (buffer_size < rsp_data->data_size)
> +		return EFI_BUFFER_TOO_SMALL;
>   
>   	memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size);
>   
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>   }
>   
>   static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
>   					   const efi_guid_t *guid, u32 attributes,
>   					   unsigned long data_size, const void *data)
>   {
> -	struct qsee_req_uefi_set_variable *req_data;
> -	struct qsee_rsp_uefi_set_variable *rsp_data;
> -	efi_status_t efi_status = EFI_SUCCESS;
> +	struct qsee_req_uefi_set_variable *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_set_variable *rsp_data __free(qcom_tzmem) = NULL;
>   	unsigned long name_length;
>   	size_t name_offs;
>   	size_t guid_offs;
> @@ -448,17 +428,14 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
>   		__array_offs(u8, data_size, &data_offs)
>   	);
>   
> -	req_data = kzalloc(req_size, GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
> -	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data),
> +				    GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
>   	req_data->command_id = QSEE_CMD_UEFI_SET_VARIABLE;
>   	req_data->attributes = attributes;
> @@ -481,42 +458,31 @@ static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
>   
>   	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data,
>   				       sizeof(*rsp_data));
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->length != sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length != sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>   
>   	if (rsp_data->status) {
>   		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
>   			__func__, rsp_data->status);
> -		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
> +		return qsee_uefi_status_to_efi(rsp_data->status);
>   	}
>   
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>   }
>   
>   static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>   						unsigned long *name_size, efi_char16_t *name,
>   						efi_guid_t *guid)
>   {
> -	struct qsee_req_uefi_get_next_variable *req_data;
> -	struct qsee_rsp_uefi_get_next_variable *rsp_data;
> -	efi_status_t efi_status = EFI_SUCCESS;
> +	struct qsee_req_uefi_get_next_variable *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_get_next_variable *rsp_data __free(qcom_tzmem) = NULL;
> +	efi_status_t efi_status;
>   	size_t guid_offs;
>   	size_t name_offs;
>   	size_t req_size;
> @@ -541,17 +507,13 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>   		__array(*name, *name_size / sizeof(*name))
>   	);
>   
> -	req_data = kzalloc(req_size, GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
> -	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
>   	req_data->command_id = QSEE_CMD_UEFI_GET_NEXT_VARIABLE;
>   	req_data->guid_offset = guid_offs;
> @@ -567,20 +529,14 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>   		return EFI_INVALID_PARAMETER;
>   
>   	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->length < sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length < sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>   
>   	if (rsp_data->status) {
>   		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
> @@ -595,77 +551,59 @@ static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
>   		if (efi_status == EFI_BUFFER_TOO_SMALL)
>   			*name_size = rsp_data->name_size;
>   
> -		goto out_free;
> +		return efi_status;
>   	}
>   
> -	if (rsp_data->length > rsp_size) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length > rsp_size)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
> +		return EFI_DEVICE_ERROR;
>   
>   	if (rsp_data->name_size > *name_size) {
>   		*name_size = rsp_data->name_size;
> -		efi_status = EFI_BUFFER_TOO_SMALL;
> -		goto out_free;
> +		return EFI_BUFFER_TOO_SMALL;
>   	}
>   
> -	if (rsp_data->guid_size != sizeof(*guid)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->guid_size != sizeof(*guid))
> +		return EFI_DEVICE_ERROR;
>   
>   	memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
>   	status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
>   			      rsp_data->name_size / sizeof(*name));
>   	*name_size = rsp_data->name_size;
>   
> -	if (status < 0) {
> +	if (status < 0)
>   		/*
>   		 * Return EFI_DEVICE_ERROR here because the buffer size should
>   		 * have already been validated above, causing this function to
>   		 * bail with EFI_BUFFER_TOO_SMALL.
>   		 */
>   		return EFI_DEVICE_ERROR;
> -	}
>   
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>   }
>   
>   static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, u32 attr,
>   						  u64 *storage_space, u64 *remaining_space,
>   						  u64 *max_variable_size)
>   {
> -	struct qsee_req_uefi_query_variable_info *req_data;
> -	struct qsee_rsp_uefi_query_variable_info *rsp_data;
> -	efi_status_t efi_status = EFI_SUCCESS;
> +	struct qsee_req_uefi_query_variable_info *req_data __free(qcom_tzmem) = NULL;
> +	struct qsee_rsp_uefi_query_variable_info *rsp_data __free(qcom_tzmem) = NULL;
>   	int status;
>   
> -	req_data = kzalloc(sizeof(*req_data), GFP_KERNEL);
> -	if (!req_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out;
> -	}
> +	req_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*req_data),
> +				    GFP_KERNEL);
> +	if (!req_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
> -	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
> -	if (!rsp_data) {
> -		efi_status = EFI_OUT_OF_RESOURCES;
> -		goto out_free_req;
> -	}
> +	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data),
> +				    GFP_KERNEL);
> +	if (!rsp_data)
> +		return EFI_OUT_OF_RESOURCES;
>   
>   	req_data->command_id = QSEE_CMD_UEFI_QUERY_VARIABLE_INFO;
>   	req_data->attributes = attr;
> @@ -673,26 +611,19 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
>   
>   	status = qcom_qseecom_app_send(qcuefi->client, req_data, sizeof(*req_data), rsp_data,
>   				       sizeof(*rsp_data));
> -	if (status) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (status)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO)
> +		return EFI_DEVICE_ERROR;
>   
> -	if (rsp_data->length != sizeof(*rsp_data)) {
> -		efi_status = EFI_DEVICE_ERROR;
> -		goto out_free;
> -	}
> +	if (rsp_data->length != sizeof(*rsp_data))
> +		return EFI_DEVICE_ERROR;
>   
>   	if (rsp_data->status) {
>   		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
>   			__func__, rsp_data->status);
> -		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
> -		goto out_free;
> +		return qsee_uefi_status_to_efi(rsp_data->status);
>   	}
>   
>   	if (storage_space)
> @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
>   	if (max_variable_size)
>   		*max_variable_size = rsp_data->max_variable_size;
>   
> -out_free:
> -	kfree(rsp_data);
> -out_free_req:
> -	kfree(req_data);
> -out:
> -	return efi_status;
> +	return EFI_SUCCESS;
>   }
>   
>   /* -- Global efivar interface. ---------------------------------------------- */
> @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>   	if (status)
>   		qcuefi_set_reference(NULL);
>   
> +	qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
> +	if (IS_ERR(qcuefi->mempool))
> +		return PTR_ERR(qcuefi->mempool);
> +
>   	return status;
>   }
>   
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 3a6cefb4eb2e..318d7d398e5f 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1567,9 +1567,9 @@ 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:      Request buffer sent to the app (must be TZ memory)
>    * @req_size: Size of the request buffer.
> - * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
> + * @rsp:      Response buffer, written to by the app (must be TZ memory)
>    * @rsp_size: Size of the response buffer.
>    *
>    * Sends a request to the QSEE app associated with the given ID and read back
> @@ -1585,26 +1585,12 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>   {
>   	struct qcom_scm_qseecom_resp res = {};
>   	struct qcom_scm_desc desc = {};
> -	dma_addr_t req_phys;
> -	dma_addr_t rsp_phys;
> +	phys_addr_t req_phys;
> +	phys_addr_t rsp_phys;
>   	int status;
>   
> -	/* Map request buffer */
> -	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
> -	status = dma_mapping_error(__scm->dev, req_phys);
> -	if (status) {
> -		dev_err(__scm->dev, "qseecom: failed to map request buffer\n");
> -		return status;
> -	}
> -
> -	/* Map response buffer */
> -	rsp_phys = dma_map_single(__scm->dev, rsp, rsp_size, DMA_FROM_DEVICE);
> -	status = dma_mapping_error(__scm->dev, rsp_phys);
> -	if (status) {
> -		dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
> -		dev_err(__scm->dev, "qseecom: failed to map response buffer\n");
> -		return status;
> -	}
> +	req_phys = qcom_tzmem_to_phys(req);
> +	rsp_phys = qcom_tzmem_to_phys(rsp);
>   
>   	/* Set up SCM call data */
>   	desc.owner = QSEECOM_TZ_OWNER_TZ_APPS;
> @@ -1622,10 +1608,6 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
>   	/* Perform call */
>   	status = qcom_scm_qseecom_call(&desc, &res);
>   
> -	/* Unmap buffers */
> -	dma_unmap_single(__scm->dev, rsp_phys, rsp_size, DMA_FROM_DEVICE);
> -	dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
> -
>   	if (status)
>   		return status;
>   
> diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
> index b531547e1dc9..26af1e778f00 100644
> --- a/include/linux/firmware/qcom/qcom_qseecom.h
> +++ b/include/linux/firmware/qcom/qcom_qseecom.h
> @@ -23,9 +23,9 @@ struct qseecom_client {
>   /**
>    * qcom_qseecom_app_send() - Send to and receive data from a given QSEE app.
>    * @client:   The QSEECOM client associated with the target app.
> - * @req:      Request buffer sent to the app (must be DMA-mappable).
> + * @req:      Request buffer sent to the app (must be TZ memory).
>    * @req_size: Size of the request buffer.
> - * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
> + * @rsp:      Response buffer, written to by the app (must be TZ memory).
>    * @rsp_size: Size of the response buffer.
>    *
>    * Sends a request to the QSEE app associated with the given client and read
Maximilian Luz Oct. 11, 2023, 8:47 p.m. UTC | #6
On 10/11/23 09:44, Bartosz Golaszewski wrote:
> On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@redhat.com> wrote:
>>
>> On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> Drop the DMA mapping operations from qcom_scm_qseecom_app_send() and
>>> convert all users of it in the qseecom module to using the TZ allocator
>>> for creating SCM call buffers. Together with using the cleanup macros,
>>> it has the added benefit of a significant code shrink. As this is
>>> largely a module separate from the SCM driver, let's use a separate
>>> memory pool.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

[...]

>>> @@ -704,12 +635,7 @@ static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
>>>        if (max_variable_size)
>>>                *max_variable_size = rsp_data->max_variable_size;
>>>
>>> -out_free:
>>> -     kfree(rsp_data);
>>> -out_free_req:
>>> -     kfree(req_data);
>>> -out:
>>> -     return efi_status;
>>> +     return EFI_SUCCESS;
>>>   }
>>>
>>>   /* -- Global efivar interface. ---------------------------------------------- */
>>> @@ -838,6 +764,10 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>>>        if (status)
>>>                qcuefi_set_reference(NULL);
>>>
>>> +     qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
>>
>> Any particular reason for this size? Just curious, it was (one) of the
>> reasons I had not marked patch 4 yet (it looks good, but I wanted to get
>> through the series to digest the Kconfig as well).
>>
> 
> I cannot test this. Do you know what the minimum correct size would be?

Unfortunately, I don't know a specific size either.

We can try to roughly estimate that though: At most, we have some rather
negligible overhead for the argument struct and GUID, the name buffer,
and the data buffer (get/set variable). The name is limited to 1024
utf-16 characters (although that's a limit that we set in our driver,
not necessarily of the firmware). The thing that's more difficult to
gauge is the maximum data size. Also I think we can reach the alloc code
with multiple threads (unless the EFI subsystem is doing some locking).
Only the actual SCM call is locked on the qseecom side.

The efivar_query_variable_info() call could help with the data size part
(it can return the maximum valid size of a single variable).
Unfortunately it's not directly exposed, but I could code something up
to read it out. The next best thing is `df -h` (which uses the same call
under the hood) to get the total storage space available for EFI
variables. On my Surface Pro X, that's 60K. So I guess overall, 64K
should be enough for a single call. That being said, the biggest variable
stored right now is about 4K in size.

Given that that's a sample-size of one device though and that we might
want to future-proof things, I think 256K is a good choice. But we could
probably go with less if we really want to save some memory.

Regards,
Max
kernel test robot Oct. 12, 2023, 11:21 p.m. UTC | #7
Hi Bartosz,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20231009]
[cannot apply to arm64/for-next/core krzk-dt/for-next linus/master v6.6-rc5 v6.6-rc4 v6.6-rc3 v6.6-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bartosz-Golaszewski/firmware-qcom-move-Qualcomm-code-into-its-own-directory/20231009-233826
base:   next-20231009
patch link:    https://lore.kernel.org/r/20231009153427.20951-12-brgl%40bgdev.pl
patch subject: [PATCH v3 11/15] firmware: qcom: qseecom: convert to using the TZ allocator
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20231013/202310130731.YHBtmaQU-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231013/202310130731.YHBtmaQU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310130731.YHBtmaQU-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/firmware/qcom/qcom_qseecom_uefisecapp.c: In function 'qcom_uefisecapp_probe':
>> drivers/firmware/qcom/qcom_qseecom_uefisecapp.c:767:67: error: 'SZ_256K' undeclared (first use in this function)
     767 |         qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
         |                                                                   ^~~~~~~
   drivers/firmware/qcom/qcom_qseecom_uefisecapp.c:767:67: note: each undeclared identifier is reported only once for each function it appears in


vim +/SZ_256K +767 drivers/firmware/qcom/qcom_qseecom_uefisecapp.c

   745	
   746	static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
   747					 const struct auxiliary_device_id *aux_dev_id)
   748	{
   749		struct qcuefi_client *qcuefi;
   750		int status;
   751	
   752		qcuefi = devm_kzalloc(&aux_dev->dev, sizeof(*qcuefi), GFP_KERNEL);
   753		if (!qcuefi)
   754			return -ENOMEM;
   755	
   756		qcuefi->client = container_of(aux_dev, struct qseecom_client, aux_dev);
   757	
   758		auxiliary_set_drvdata(aux_dev, qcuefi);
   759		status = qcuefi_set_reference(qcuefi);
   760		if (status)
   761			return status;
   762	
   763		status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
   764		if (status)
   765			qcuefi_set_reference(NULL);
   766	
 > 767		qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
   768		if (IS_ERR(qcuefi->mempool))
   769			return PTR_ERR(qcuefi->mempool);
   770	
   771		return status;
   772	}
   773
diff mbox series

Patch

diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
index a33acdaf7b78..720cddd7c8c7 100644
--- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
+++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
@@ -7,6 +7,7 @@ 
  * Copyright (C) 2023 Maximilian Luz <luzmaximilian@gmail.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/efi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -18,6 +19,8 @@ 
 #include <linux/ucs2_string.h>
 
 #include <linux/firmware/qcom/qcom_qseecom.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/firmware/qcom/qcom_tzmem.h>
 
 /* -- Qualcomm "uefisecapp" interface definitions. -------------------------- */
 
@@ -253,6 +256,7 @@  struct qsee_rsp_uefi_query_variable_info {
 struct qcuefi_client {
 	struct qseecom_client *client;
 	struct efivars efivars;
+	struct qcom_tzmem_pool *mempool;
 };
 
 static struct device *qcuefi_dev(struct qcuefi_client *qcuefi)
@@ -272,11 +276,11 @@  static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
 					   const efi_guid_t *guid, u32 *attributes,
 					   unsigned long *data_size, void *data)
 {
-	struct qsee_req_uefi_get_variable *req_data;
-	struct qsee_rsp_uefi_get_variable *rsp_data;
+	struct qsee_req_uefi_get_variable *req_data __free(qcom_tzmem) = NULL;
+	struct qsee_rsp_uefi_get_variable *rsp_data __free(qcom_tzmem) = NULL;
 	unsigned long buffer_size = *data_size;
-	efi_status_t efi_status = EFI_SUCCESS;
 	unsigned long name_length;
+	efi_status_t efi_status;
 	size_t guid_offs;
 	size_t name_offs;
 	size_t req_size;
@@ -304,17 +308,13 @@  static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
 		__array(u8, buffer_size)
 	);
 
-	req_data = kzalloc(req_size, GFP_KERNEL);
-	if (!req_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out;
-	}
+	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
+	if (!req_data)
+		return EFI_OUT_OF_RESOURCES;
 
-	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
-	if (!rsp_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out_free_req;
-	}
+	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL);
+	if (!rsp_data)
+		return EFI_OUT_OF_RESOURCES;
 
 	req_data->command_id = QSEE_CMD_UEFI_GET_VARIABLE;
 	req_data->data_size = buffer_size;
@@ -331,20 +331,14 @@  static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
 	memcpy(((void *)req_data) + req_data->guid_offset, guid, req_data->guid_size);
 
 	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
-	if (status) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (status)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_VARIABLE)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->length < sizeof(*rsp_data)) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->length < sizeof(*rsp_data))
+		return EFI_DEVICE_ERROR;
 
 	if (rsp_data->status) {
 		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
@@ -358,18 +352,14 @@  static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
 				*attributes = rsp_data->attributes;
 		}
 
-		goto out_free;
+		return efi_status;
 	}
 
-	if (rsp_data->length > rsp_size) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->length > rsp_size)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->data_offset + rsp_data->data_size > rsp_data->length)
+		return EFI_DEVICE_ERROR;
 
 	/*
 	 * Note: We need to set attributes and data size even if the buffer is
@@ -392,33 +382,23 @@  static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const e
 	if (attributes)
 		*attributes = rsp_data->attributes;
 
-	if (buffer_size == 0 && !data) {
-		efi_status = EFI_SUCCESS;
-		goto out_free;
-	}
+	if (buffer_size == 0 && !data)
+		return EFI_SUCCESS;
 
-	if (buffer_size < rsp_data->data_size) {
-		efi_status = EFI_BUFFER_TOO_SMALL;
-		goto out_free;
-	}
+	if (buffer_size < rsp_data->data_size)
+		return EFI_BUFFER_TOO_SMALL;
 
 	memcpy(data, ((void *)rsp_data) + rsp_data->data_offset, rsp_data->data_size);
 
-out_free:
-	kfree(rsp_data);
-out_free_req:
-	kfree(req_data);
-out:
-	return efi_status;
+	return EFI_SUCCESS;
 }
 
 static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name,
 					   const efi_guid_t *guid, u32 attributes,
 					   unsigned long data_size, const void *data)
 {
-	struct qsee_req_uefi_set_variable *req_data;
-	struct qsee_rsp_uefi_set_variable *rsp_data;
-	efi_status_t efi_status = EFI_SUCCESS;
+	struct qsee_req_uefi_set_variable *req_data __free(qcom_tzmem) = NULL;
+	struct qsee_rsp_uefi_set_variable *rsp_data __free(qcom_tzmem) = NULL;
 	unsigned long name_length;
 	size_t name_offs;
 	size_t guid_offs;
@@ -448,17 +428,14 @@  static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
 		__array_offs(u8, data_size, &data_offs)
 	);
 
-	req_data = kzalloc(req_size, GFP_KERNEL);
-	if (!req_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out;
-	}
+	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
+	if (!req_data)
+		return EFI_OUT_OF_RESOURCES;
 
-	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
-	if (!rsp_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out_free_req;
-	}
+	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data),
+				    GFP_KERNEL);
+	if (!rsp_data)
+		return EFI_OUT_OF_RESOURCES;
 
 	req_data->command_id = QSEE_CMD_UEFI_SET_VARIABLE;
 	req_data->attributes = attributes;
@@ -481,42 +458,31 @@  static efi_status_t qsee_uefi_set_variable(struct qcuefi_client *qcuefi, const e
 
 	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data,
 				       sizeof(*rsp_data));
-	if (status) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (status)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->command_id != QSEE_CMD_UEFI_SET_VARIABLE)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->length != sizeof(*rsp_data)) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->length != sizeof(*rsp_data))
+		return EFI_DEVICE_ERROR;
 
 	if (rsp_data->status) {
 		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
 			__func__, rsp_data->status);
-		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
+		return qsee_uefi_status_to_efi(rsp_data->status);
 	}
 
-out_free:
-	kfree(rsp_data);
-out_free_req:
-	kfree(req_data);
-out:
-	return efi_status;
+	return EFI_SUCCESS;
 }
 
 static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
 						unsigned long *name_size, efi_char16_t *name,
 						efi_guid_t *guid)
 {
-	struct qsee_req_uefi_get_next_variable *req_data;
-	struct qsee_rsp_uefi_get_next_variable *rsp_data;
-	efi_status_t efi_status = EFI_SUCCESS;
+	struct qsee_req_uefi_get_next_variable *req_data __free(qcom_tzmem) = NULL;
+	struct qsee_rsp_uefi_get_next_variable *rsp_data __free(qcom_tzmem) = NULL;
+	efi_status_t efi_status;
 	size_t guid_offs;
 	size_t name_offs;
 	size_t req_size;
@@ -541,17 +507,13 @@  static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
 		__array(*name, *name_size / sizeof(*name))
 	);
 
-	req_data = kzalloc(req_size, GFP_KERNEL);
-	if (!req_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out;
-	}
+	req_data = qcom_tzmem_alloc(qcuefi->mempool, req_size, GFP_KERNEL);
+	if (!req_data)
+		return EFI_OUT_OF_RESOURCES;
 
-	rsp_data = kzalloc(rsp_size, GFP_KERNEL);
-	if (!rsp_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out_free_req;
-	}
+	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, rsp_size, GFP_KERNEL);
+	if (!rsp_data)
+		return EFI_OUT_OF_RESOURCES;
 
 	req_data->command_id = QSEE_CMD_UEFI_GET_NEXT_VARIABLE;
 	req_data->guid_offset = guid_offs;
@@ -567,20 +529,14 @@  static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
 		return EFI_INVALID_PARAMETER;
 
 	status = qcom_qseecom_app_send(qcuefi->client, req_data, req_size, rsp_data, rsp_size);
-	if (status) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (status)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->command_id != QSEE_CMD_UEFI_GET_NEXT_VARIABLE)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->length < sizeof(*rsp_data)) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->length < sizeof(*rsp_data))
+		return EFI_DEVICE_ERROR;
 
 	if (rsp_data->status) {
 		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
@@ -595,77 +551,59 @@  static efi_status_t qsee_uefi_get_next_variable(struct qcuefi_client *qcuefi,
 		if (efi_status == EFI_BUFFER_TOO_SMALL)
 			*name_size = rsp_data->name_size;
 
-		goto out_free;
+		return efi_status;
 	}
 
-	if (rsp_data->length > rsp_size) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->length > rsp_size)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->name_offset + rsp_data->name_size > rsp_data->length)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->guid_offset + rsp_data->guid_size > rsp_data->length)
+		return EFI_DEVICE_ERROR;
 
 	if (rsp_data->name_size > *name_size) {
 		*name_size = rsp_data->name_size;
-		efi_status = EFI_BUFFER_TOO_SMALL;
-		goto out_free;
+		return EFI_BUFFER_TOO_SMALL;
 	}
 
-	if (rsp_data->guid_size != sizeof(*guid)) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->guid_size != sizeof(*guid))
+		return EFI_DEVICE_ERROR;
 
 	memcpy(guid, ((void *)rsp_data) + rsp_data->guid_offset, rsp_data->guid_size);
 	status = ucs2_strscpy(name, ((void *)rsp_data) + rsp_data->name_offset,
 			      rsp_data->name_size / sizeof(*name));
 	*name_size = rsp_data->name_size;
 
-	if (status < 0) {
+	if (status < 0)
 		/*
 		 * Return EFI_DEVICE_ERROR here because the buffer size should
 		 * have already been validated above, causing this function to
 		 * bail with EFI_BUFFER_TOO_SMALL.
 		 */
 		return EFI_DEVICE_ERROR;
-	}
 
-out_free:
-	kfree(rsp_data);
-out_free_req:
-	kfree(req_data);
-out:
-	return efi_status;
+	return EFI_SUCCESS;
 }
 
 static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi, u32 attr,
 						  u64 *storage_space, u64 *remaining_space,
 						  u64 *max_variable_size)
 {
-	struct qsee_req_uefi_query_variable_info *req_data;
-	struct qsee_rsp_uefi_query_variable_info *rsp_data;
-	efi_status_t efi_status = EFI_SUCCESS;
+	struct qsee_req_uefi_query_variable_info *req_data __free(qcom_tzmem) = NULL;
+	struct qsee_rsp_uefi_query_variable_info *rsp_data __free(qcom_tzmem) = NULL;
 	int status;
 
-	req_data = kzalloc(sizeof(*req_data), GFP_KERNEL);
-	if (!req_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out;
-	}
+	req_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*req_data),
+				    GFP_KERNEL);
+	if (!req_data)
+		return EFI_OUT_OF_RESOURCES;
 
-	rsp_data = kzalloc(sizeof(*rsp_data), GFP_KERNEL);
-	if (!rsp_data) {
-		efi_status = EFI_OUT_OF_RESOURCES;
-		goto out_free_req;
-	}
+	rsp_data = qcom_tzmem_alloc(qcuefi->mempool, sizeof(*rsp_data),
+				    GFP_KERNEL);
+	if (!rsp_data)
+		return EFI_OUT_OF_RESOURCES;
 
 	req_data->command_id = QSEE_CMD_UEFI_QUERY_VARIABLE_INFO;
 	req_data->attributes = attr;
@@ -673,26 +611,19 @@  static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
 
 	status = qcom_qseecom_app_send(qcuefi->client, req_data, sizeof(*req_data), rsp_data,
 				       sizeof(*rsp_data));
-	if (status) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (status)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->command_id != QSEE_CMD_UEFI_QUERY_VARIABLE_INFO)
+		return EFI_DEVICE_ERROR;
 
-	if (rsp_data->length != sizeof(*rsp_data)) {
-		efi_status = EFI_DEVICE_ERROR;
-		goto out_free;
-	}
+	if (rsp_data->length != sizeof(*rsp_data))
+		return EFI_DEVICE_ERROR;
 
 	if (rsp_data->status) {
 		dev_dbg(qcuefi_dev(qcuefi), "%s: uefisecapp error: 0x%x\n",
 			__func__, rsp_data->status);
-		efi_status = qsee_uefi_status_to_efi(rsp_data->status);
-		goto out_free;
+		return qsee_uefi_status_to_efi(rsp_data->status);
 	}
 
 	if (storage_space)
@@ -704,12 +635,7 @@  static efi_status_t qsee_uefi_query_variable_info(struct qcuefi_client *qcuefi,
 	if (max_variable_size)
 		*max_variable_size = rsp_data->max_variable_size;
 
-out_free:
-	kfree(rsp_data);
-out_free_req:
-	kfree(req_data);
-out:
-	return efi_status;
+	return EFI_SUCCESS;
 }
 
 /* -- Global efivar interface. ---------------------------------------------- */
@@ -838,6 +764,10 @@  static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
 	if (status)
 		qcuefi_set_reference(NULL);
 
+	qcuefi->mempool = devm_qcom_tzmem_pool_new(&aux_dev->dev, SZ_256K);
+	if (IS_ERR(qcuefi->mempool))
+		return PTR_ERR(qcuefi->mempool);
+
 	return status;
 }
 
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 3a6cefb4eb2e..318d7d398e5f 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1567,9 +1567,9 @@  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:      Request buffer sent to the app (must be TZ memory)
  * @req_size: Size of the request buffer.
- * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
+ * @rsp:      Response buffer, written to by the app (must be TZ memory)
  * @rsp_size: Size of the response buffer.
  *
  * Sends a request to the QSEE app associated with the given ID and read back
@@ -1585,26 +1585,12 @@  int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
 {
 	struct qcom_scm_qseecom_resp res = {};
 	struct qcom_scm_desc desc = {};
-	dma_addr_t req_phys;
-	dma_addr_t rsp_phys;
+	phys_addr_t req_phys;
+	phys_addr_t rsp_phys;
 	int status;
 
-	/* Map request buffer */
-	req_phys = dma_map_single(__scm->dev, req, req_size, DMA_TO_DEVICE);
-	status = dma_mapping_error(__scm->dev, req_phys);
-	if (status) {
-		dev_err(__scm->dev, "qseecom: failed to map request buffer\n");
-		return status;
-	}
-
-	/* Map response buffer */
-	rsp_phys = dma_map_single(__scm->dev, rsp, rsp_size, DMA_FROM_DEVICE);
-	status = dma_mapping_error(__scm->dev, rsp_phys);
-	if (status) {
-		dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
-		dev_err(__scm->dev, "qseecom: failed to map response buffer\n");
-		return status;
-	}
+	req_phys = qcom_tzmem_to_phys(req);
+	rsp_phys = qcom_tzmem_to_phys(rsp);
 
 	/* Set up SCM call data */
 	desc.owner = QSEECOM_TZ_OWNER_TZ_APPS;
@@ -1622,10 +1608,6 @@  int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size, void *rsp,
 	/* Perform call */
 	status = qcom_scm_qseecom_call(&desc, &res);
 
-	/* Unmap buffers */
-	dma_unmap_single(__scm->dev, rsp_phys, rsp_size, DMA_FROM_DEVICE);
-	dma_unmap_single(__scm->dev, req_phys, req_size, DMA_TO_DEVICE);
-
 	if (status)
 		return status;
 
diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
index b531547e1dc9..26af1e778f00 100644
--- a/include/linux/firmware/qcom/qcom_qseecom.h
+++ b/include/linux/firmware/qcom/qcom_qseecom.h
@@ -23,9 +23,9 @@  struct qseecom_client {
 /**
  * qcom_qseecom_app_send() - Send to and receive data from a given QSEE app.
  * @client:   The QSEECOM client associated with the target app.
- * @req:      Request buffer sent to the app (must be DMA-mappable).
+ * @req:      Request buffer sent to the app (must be TZ memory).
  * @req_size: Size of the request buffer.
- * @rsp:      Response buffer, written to by the app (must be DMA-mappable).
+ * @rsp:      Response buffer, written to by the app (must be TZ memory).
  * @rsp_size: Size of the response buffer.
  *
  * Sends a request to the QSEE app associated with the given client and read