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

[Maximilian Luz]

On 10/11/23 09:44, Bartosz Golaszewski wrote:
> On Wed, Oct 11, 2023 at 12:49 AM Andrew Halaney <ahalaney@xxxxxxxxxx> wrote:
> > On Mon, Oct 09, 2023 at 05:34:23PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>

[...]

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