Re: [PATCH v4 3/8] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>]

On 16/07/2025 22:13, Bjorn Andersson wrote:
> On Wed, Jun 25, 2025 at 01:53:22AM +0300, Dmitry Baryshkov wrote:
>> For some platforms (e.g. Lenovo Yoga C630) we don't yet know a way to
>> update variables in the permanent storage. However being able to read
>> the vars is still useful as it allows us to get e.g. RTC offset.
>>
>> Add a quirk for QSEECOM specifying that UEFI variables for this platform
>> should be registered in read-only mode.
>>
> 
> In order to implement UEFI variable storage on any device where the OS
> owns the one storage device requires some form of bridge service running
> in the OS.
 > > We should expect that such devices will exist in the future as well 
(due
> to BOM cost) and as such a solution for this will have to present itself
> in a upstream compliant fashion.

Sure.

> 
> There's a lot of infrastructure being introduced here to convey a single
> boolean flag which I'm hoping to be dead code sooner rather than later.


I think we might have more quirks in future, but I'm fine with just 
forcing R/O uefi vars for SDM845 and MSM8998 (I assume SC8180X should 
also have the same issue, but it was enabled unconditionally).


> 
> Now there's some differences in your wording between the patches and the
> responses. In some places you're talking about the C630 crashing, in
> others you describe it as EFI variable writes aren't persistent. That
> makes me wonder if we have two problems, or what you're refering to here
> is just the same problem we see on all UFS-based systems (Qualcomm and
> others).


Those are separate issues:
- C630 doesn't persist variables across reboots
- Enabling R/O UEFI variables uncovers a crash at the rtc_pm8xxx / 
efivars code.


> 
> Regards,
> Bjorn
> 
>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>> ---
>>   drivers/firmware/qcom/qcom_qseecom_uefisecapp.c | 18 +++++++++++++++++-
>>   include/linux/firmware/qcom/qcom_qseecom.h      |  2 ++
>>   2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>> index 98a463e9774bf04f2deb0f7fa1318bd0d2edfa49..05f700dcb8cf3189f640237ff0e045564abb8264 100644
>> --- a/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>> +++ b/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c
>> @@ -792,6 +792,12 @@ static efi_status_t qcuefi_query_variable_info(u32 attr, u64 *storage_space, u64
>>   	return status;
>>   }
>>   
>> +static const struct efivar_operations qcom_efivars_ro_ops = {
>> +	.get_variable = qcuefi_get_variable,
>> +	.get_next_variable = qcuefi_get_next_variable,
>> +	.query_variable_info = qcuefi_query_variable_info,
>> +};
>> +
>>   static const struct efivar_operations qcom_efivar_ops = {
>>   	.get_variable = qcuefi_get_variable,
>>   	.set_variable = qcuefi_set_variable,
>> @@ -804,7 +810,9 @@ static const struct efivar_operations qcom_efivar_ops = {
>>   static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>>   				 const struct auxiliary_device_id *aux_dev_id)
>>   {
>> +	unsigned long *quirks = aux_dev->dev.platform_data;
>>   	struct qcom_tzmem_pool_config pool_config;
>> +	const struct efivar_operations *ops;
>>   	struct qcuefi_client *qcuefi;
>>   	int status;
>>   
>> @@ -829,7 +837,15 @@ static int qcom_uefisecapp_probe(struct auxiliary_device *aux_dev,
>>   	if (status)
>>   		return status;
>>   
>> -	status = efivars_register(&qcuefi->efivars, &qcom_efivar_ops);
>> +	if (quirks &&
>> +	    *quirks & QCOM_QSEECOM_QUIRK_RO_UEFIVARS) {
>> +		dev_dbg(&aux_dev->dev, "R/O UEFI vars implementation\n");
>> +		ops = &qcom_efivars_ro_ops;
>> +	} else {
>> +		ops = &qcom_efivar_ops;
>> +	}
>> +
>> +	status = efivars_register(&qcuefi->efivars, ops);
>>   	if (status)
>>   		qcuefi_set_reference(NULL);
>>   
>> diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
>> index 3387897bf36843cccd0bd933dd562390bf674b14..8d6d660e854fdb0fabbef10ab5ee6ff23ad79826 100644
>> --- a/include/linux/firmware/qcom/qcom_qseecom.h
>> +++ b/include/linux/firmware/qcom/qcom_qseecom.h
>> @@ -51,4 +51,6 @@ static inline int qcom_qseecom_app_send(struct qseecom_client *client,
>>   	return qcom_scm_qseecom_app_send(client->app_id, req, req_size, rsp, rsp_size);
>>   }
>>   
>> +#define QCOM_QSEECOM_QUIRK_RO_UEFIVARS		BIT(0)
>> +
>>   #endif /* __QCOM_QSEECOM_H */
>>
>> -- 
>> 2.39.5
>>


-- 
With best wishes
Dmitry