Messages in this thread | | | Date | Thu, 9 Mar 2023 21:44:29 +0100 | Subject | Re: [PATCH v3 4/4] firmware: Add support for Qualcomm UEFI Secure Application | From | Maximilian Luz <> |
| |
On 3/9/23 09:36, Dmitry Baryshkov wrote: > On 08/03/2023 17:02, Maximilian Luz wrote: >> On 3/7/23 16:51, Dmitry Baryshkov wrote: >>> On 05/03/2023 04:21, Maximilian Luz wrote: >>>> On platforms using the Qualcomm UEFI Secure Application (uefisecapp),
[...]
>> >> As I've elaborated on a previous version: I'm a bit wary of using >> qseecom_app_get_id() in this way, since the Windows driver I've got this from >> expects the app to be present when calling that function. So I don't know much >> about the failure cases, especially when it isn't present. >> >> At this point, I'm just assuming that "res.status != QSEECOM_RESULT_SUCCESS" >> means the app isn't present, but I don't know whether this can fail in other >> ways. For a proper detection system I'd prefer if we can differentiate between >> "some internal failure" and "not-present" cases. > > Please take a look at https://git.codelinaro.org/clo/la/kernel/msm-5.10/-/blob/KERNEL.PLATFORM.1.0.r1-13000-kernel.0/drivers/misc/qseecom.c#L2683 > > Note, the driver supports loading and unloading applications, we can ignore that for now. >
Thanks! That looks quite helpful.
[...]
>>>> +static efi_status_t qsee_uefi_get_variable(struct qcuefi_client *qcuefi, const efi_char16_t *name, >>>> + 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 qseecom_dma dma_req; >>>> + struct qseecom_dma dma_rsp; >>>> + unsigned long name_size = utf16_strsize(name); >>>> + unsigned long buffer_size = *data_size; >>>> + unsigned long size; >>>> + efi_status_t efi_status; >>>> + int status; >>>> + >>>> + /* Validation: We need a name and GUID. */ >>>> + if (!name || !guid) >>>> + return EFI_INVALID_PARAMETER; >>>> + >>>> + /* Validation: We need a buffer if the buffer_size is nonzero. */ >>>> + if (buffer_size && !data) >>>> + return EFI_INVALID_PARAMETER; >>>> + >>>> + /* Compute required size (upper limit with alignments). */ >>>> + size = sizeof(*req_data) + sizeof(*guid) + name_size /* Inputs. */ >>>> + + sizeof(*rsp_data) + buffer_size /* Outputs. */ >>>> + + 2 * (QSEECOM_DMA_ALIGNMENT - 1) /* Input parameter alignments. */ >>>> + + 1 * (QSEECOM_DMA_ALIGNMENT - 1); /* Output parameter alignments. */ >>> >>> Do we need to pack everything into a single DMA buffer? Otherwise it would be better to add qseecom_dma_alloc_aligned function, which will take care of the alignment for a single data piece. >> >> It may be possible to split this into two buffers, one for input and one for >> output, but packing of input parameters would still be required (see the >> assignments to req_data below). >> >> For the input, you essentially provide one buffer (address) to qseecom, >> starting with req_data describing the layout in it. This description is >> offset-based, so there's no way to specify multiple addresses/buffers as input. >> The output behaves similarly, it's just the secure OS that does the packing. >> >> And since we already have to take care of aligning the input parameters, I'm >> not sure that it makes sense to split this into two. > > I see, thanks for the explanation. Maybe you can add a wrapping call that will take the sizes of required arguments (as a variadic array?) and will return prepared req and all the pointers and/or offsets? I think that having to specify these alignment 'extras' is errror prone.
I'll give that a try.
[...]
>>>> +static int qcom_uefisecapp_probe(struct platform_device *pdev) >>>> +{ >>>> + struct qcuefi_client *qcuefi; >>>> + int status; >>>> + >>>> + /* Allocate driver data. */ >>>> + qcuefi = devm_kzalloc(&pdev->dev, sizeof(*qcuefi), GFP_KERNEL); >>>> + if (!qcuefi) >>>> + return -ENOMEM; >>>> + >>>> + qcuefi->dev = &pdev->dev; >>>> + >>>> + /* We expect the parent to be the QSEECOM device. */ >>>> + qcuefi->qsee = dev_get_drvdata(pdev->dev.parent); >>>> + if (!qcuefi->qsee) >>>> + return -EINVAL; >>>> + >>>> + /* Get application id for uefisecapp. */ >>>> + status = qseecom_app_get_id(qcuefi->qsee, QSEE_UEFISEC_APP_NAME, &qcuefi->app_id); >>>> + if (status) { >>>> + dev_err(&pdev->dev, "failed to query app ID: %d\n", status); >>>> + return status; >>>> + } >>>> + >>>> + /* Set up DMA. One page should be plenty to start with. */ >>> >>> one page? >> >> The driver I've reverse-engineered this from allocates the DMA memory for >> interaction with qseecom in multiples of PAGE_SIZE. I'm following that in this >> driver, as I don't know whether that's a hard requirement (at least on some >> platforms) or not. So I pre-allocate one page (1x PAGE_SIZE bytes) here. But as >> you've mentioned above, it might be better to allocate this on-demand in each >> call. > > Probably the comment was misplaced. It talks about 1 page, but it is placed right before a call to dma_set_mask rather than dma_alloc.
Ah, it was intended for both the dma_set_mask() and the qseecom_dma_alloc() below. I see how that is a bit confusing, will fix that.
>>>> + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(64))) { >>>> + dev_warn(&pdev->dev, "no suitable DMA available\n"); >>>> + return -EFAULT; >>>> + } >>>> + >>>> + status = qseecom_dma_alloc(&pdev->dev, &qcuefi->dma, PAGE_SIZE, GFP_KERNEL); >>>> + if (status) >>>> + return status; >>>> +
[...]
Regards Max
| |