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

[Dmitry Baryshkov]

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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > ---
> >   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