[PATCH v2 0/4] firmware: qcom: enable UEFI variables on Lenovo Yoga C630

[PATCH v2 0/4] firmware: qcom: enable UEFI variables on Lenovo Yoga C630

[Dmitry Baryshkov]

Lenovo Yoga C630 is a WoA / WoS laptop, which uses a "standard" QSEECOM /
uefisecapp application in order to implement UEFI variables. However as
this platform has only one storage (UFS) shared between Linux and
SecureOS world, uefisecapp can not update variables directly. It
requires some additional steps in order to update variables, which are
not yet reverse engineered.

However even with the current driver it is possible to implement R/O
UEFI vars access, which e.g. lets the RTC driver to read RTC offset,
providing Linux with a correct time.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
Changes in v2:
- Added QSEECOM quirks in order to make UEFI vars r/o on C630.
- Added DT patch, specifying the use of UEFI vars for RTC offset.
- Link to v1: https://lore.kernel.org/r/20240725-more-qseecom-v1-1-a55a3553d1fe@linaro.org

---
Dmitry Baryshkov (4):
      firmware: qcom: scm: allow specifying quirks for QSEECOM implementations
      firmware: qcom: uefisecapp: add support for R/O UEFI vars
      firmware: qcom: enable QSEECOM on Lenovo Yoga C630
      arm64: dts: qcom: sdm850-lenovo-yoga-c630: fix RTC offset info

 arch/arm64/boot/dts/qcom/pm8998.dtsi               |  2 +-
 .../boot/dts/qcom/sdm850-lenovo-yoga-c630.dts      |  4 +++
 drivers/firmware/qcom/qcom_qseecom.c               |  6 +++-
 drivers/firmware/qcom/qcom_qseecom_uefisecapp.c    | 18 +++++++++++-
 drivers/firmware/qcom/qcom_scm.c                   | 32 ++++++++++++----------
 include/linux/firmware/qcom/qcom_qseecom.h         |  2 ++
 6 files changed, 47 insertions(+), 17 deletions(-)
---
base-commit: 5d4809e25903ab8e74034c1f23c787fd26d52934
change-id: 20240725-more-qseecom-379933b9c769

Best regards,
-- 
With best wishes
Dmitry

[PATCH v2 1/4] firmware: qcom: scm: allow specifying quirks for QSEECOM implementations

[Dmitry Baryshkov]

Some of QSEECOM implementations might need additional quirks (e.g. some
of the platforms don't (yet) support read-write UEFI variables access).
Pass the quirks to the QSEECOM driver and down to individual app
drivers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_qseecom.c |  6 +++++-
 drivers/firmware/qcom/qcom_scm.c     | 28 ++++++++++++++--------------
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_qseecom.c b/drivers/firmware/qcom/qcom_qseecom.c
index 731e6d5719f9e3e9e698f5de0117540f51ebab63..aab0d61f0420c4f3d6c1a73e384195b9513f3ef9 100644
--- a/drivers/firmware/qcom/qcom_qseecom.c
+++ b/drivers/firmware/qcom/qcom_qseecom.c
@@ -36,6 +36,7 @@ static void qseecom_client_remove(void *data)
 }
 
 static int qseecom_client_register(struct platform_device *qseecom_dev,
+				   void *data,
 				   const struct qseecom_app_desc *desc)
 {
 	struct qseecom_client *client;
@@ -56,6 +57,7 @@ static int qseecom_client_register(struct platform_device *qseecom_dev,
 
 	client->aux_dev.name = desc->dev_name;
 	client->aux_dev.dev.parent = &qseecom_dev->dev;
+	client->aux_dev.dev.platform_data = data;
 	client->aux_dev.dev.release = qseecom_client_release;
 	client->app_id = app_id;
 
@@ -89,12 +91,14 @@ static const struct qseecom_app_desc qcom_qseecom_apps[] = {
 
 static int qcom_qseecom_probe(struct platform_device *qseecom_dev)
 {
+	void *data = dev_get_platdata(&qseecom_dev->dev);
 	int ret;
 	int i;
 
 	/* Set up client devices for each base application */
 	for (i = 0; i < ARRAY_SIZE(qcom_qseecom_apps); i++) {
-		ret = qseecom_client_register(qseecom_dev, &qcom_qseecom_apps[i]);
+		ret = qseecom_client_register(qseecom_dev, data,
+					      &qcom_qseecom_apps[i]);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index f63b716be5b027550ae3a987e784f0814ea6d678..fa7a3c4c8f006599dbf6deec0a060e1158c91586 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -2008,10 +2008,10 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
 	{ }
 };
 
-static bool qcom_scm_qseecom_machine_is_allowed(void)
+static bool qcom_scm_qseecom_machine_is_allowed(unsigned long *quirks)
 {
+	const struct of_device_id *match;
 	struct device_node *np;
-	bool match;
 
 	np = of_find_node_by_path("/");
 	if (!np)
@@ -2020,6 +2020,11 @@ static bool qcom_scm_qseecom_machine_is_allowed(void)
 	match = of_match_node(qcom_scm_qseecom_allowlist, np);
 	of_node_put(np);
 
+	if (match && match->data)
+		memcpy(quirks, match->data, sizeof(*quirks));
+	else
+		*quirks = 0;
+
 	return match;
 }
 
@@ -2034,6 +2039,7 @@ static void qcom_scm_qseecom_free(void *data)
 static int qcom_scm_qseecom_init(struct qcom_scm *scm)
 {
 	struct platform_device *qseecom_dev;
+	unsigned long quirks;
 	u32 version;
 	int ret;
 
@@ -2054,7 +2060,7 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm)
 
 	dev_info(scm->dev, "qseecom: found qseecom with version 0x%x\n", version);
 
-	if (!qcom_scm_qseecom_machine_is_allowed()) {
+	if (!qcom_scm_qseecom_machine_is_allowed(&quirks)) {
 		dev_info(scm->dev, "qseecom: untested machine, skipping\n");
 		return 0;
 	}
@@ -2063,17 +2069,11 @@ static int qcom_scm_qseecom_init(struct qcom_scm *scm)
 	 * Set up QSEECOM interface device. All application clients will be
 	 * set up and managed by the corresponding driver for it.
 	 */
-	qseecom_dev = platform_device_alloc("qcom_qseecom", -1);
-	if (!qseecom_dev)
-		return -ENOMEM;
-
-	qseecom_dev->dev.parent = scm->dev;
-
-	ret = platform_device_add(qseecom_dev);
-	if (ret) {
-		platform_device_put(qseecom_dev);
-		return ret;
-	}
+	qseecom_dev = platform_device_register_data(scm->dev,
+						    "qcom_qseecom", -1,
+						    &quirks, sizeof(quirks));
+	if (IS_ERR(qseecom_dev))
+		return PTR_ERR(qseecom_dev);
 
 	return devm_add_action_or_reset(scm->dev, qcom_scm_qseecom_free, qseecom_dev);
 }

-- 
2.39.5

[PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Dmitry Baryshkov]

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.

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

[PATCH v2 3/4] firmware: qcom: enable QSEECOM on Lenovo Yoga C630

[Dmitry Baryshkov]

QSEECOM driver end UEFI vars access works on the Lenovo Yoga C630. This
platform has only one storage (UFS) shared between Linux and SecureOS
world, uefisecapp can not update variables directly. It requires some
additional steps in order to update variables, which are not yet reverse
engineered.

Enable the QSEECOM device on that laptop and set up a quirk, making UEFI
vars read-only.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index fa7a3c4c8f006599dbf6deec0a060e1158c91586..3b1fbdbe79cdfe9dbb1a4ff5afae53b469441ed5 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -13,6 +13,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/export.h>
+#include <linux/firmware/qcom/qcom_qseecom.h>
 #include <linux/firmware/qcom/qcom_scm.h>
 #include <linux/firmware/qcom/qcom_tzmem.h>
 #include <linux/init.h>
@@ -1980,6 +1981,8 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size,
 }
 EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_send);
 
+static unsigned long qcom_qseecom_ro_uefi = QCOM_QSEECOM_QUIRK_RO_UEFIVARS;
+
 /*
  * We do not yet support re-entrant calls via the qseecom interface. To prevent
  + any potential issues with this, only allow validated machines for now.
@@ -1995,6 +1998,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
 	{ .compatible = "lenovo,flex-5g" },
 	{ .compatible = "lenovo,thinkpad-t14s" },
 	{ .compatible = "lenovo,thinkpad-x13s", },
+	{ .compatible = "lenovo,yoga-c630", .data = &qcom_qseecom_ro_uefi, },
 	{ .compatible = "lenovo,yoga-slim7x" },
 	{ .compatible = "microsoft,arcata", },
 	{ .compatible = "microsoft,blackrock" },

-- 
2.39.5

[PATCH v2 4/4] arm64: dts: qcom: sdm850-lenovo-yoga-c630: fix RTC offset info

[Dmitry Baryshkov]

Lenovo Yoga C630 as most of the other WoA devices stores RTC offset in
the UEFI variable. Add corresponding property to the RTC device in order
to make RTC driver wait for UEFI variables to become available and then
read offset value from the corresponding variable.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 arch/arm64/boot/dts/qcom/pm8998.dtsi                 | 2 +-
 arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 3ecb330590e59a6640f833a0bf4d2c62f40de17d..50b41942b06cf1a3f43f9c754b3bf2e1eaa4d353 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -101,7 +101,7 @@ pm8998_adc_tm: adc-tm@3400 {
 			status = "disabled";
 		};
 
-		rtc@6000 {
+		pm8998_rtc: rtc@6000 {
 			compatible = "qcom,pm8941-rtc";
 			reg = <0x6000>, <0x6100>;
 			reg-names = "rtc", "alarm";
diff --git a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
index 8ef6db3be6e3dffe4ec819288193a183b32db8e8..c0c007ce8682cacd1cbfe816ddb975c0a099ac89 100644
--- a/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
+++ b/arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dts
@@ -592,6 +592,10 @@ sw_edp_1p2_en: pm8998-gpio9-state {
 	};
 };
 
+&pm8998_rtc {
+	qcom,uefi-rtc-info;
+};
+
 &qup_i2c10_default {
 	drive-strength = <2>;
 	bias-disable;

-- 
2.39.5

Re: [PATCH v2 1/4] firmware: qcom: scm: allow specifying quirks for QSEECOM implementations

[Konrad Dybcio]

On 6/21/25 9:56 PM, Dmitry Baryshkov wrote:
> Some of QSEECOM implementations might need additional quirks (e.g. some
> of the platforms don't (yet) support read-write UEFI variables access).
> Pass the quirks to the QSEECOM driver and down to individual app
> drivers.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---

[...]

> -static bool qcom_scm_qseecom_machine_is_allowed(void)
> +static bool qcom_scm_qseecom_machine_is_allowed(unsigned long *quirks)
>  {
> +	const struct of_device_id *match;
>  	struct device_node *np;
> -	bool match;
>  
>  	np = of_find_node_by_path("/");
>  	if (!np)
> @@ -2020,6 +2020,11 @@ static bool qcom_scm_qseecom_machine_is_allowed(void)
>  	match = of_match_node(qcom_scm_qseecom_allowlist, np);
>  	of_node_put(np);
>  
> +	if (match && match->data)
> +		memcpy(quirks, match->data, sizeof(*quirks));

bit weird to use memcpy since it's just an UL

Konrad

Re: [PATCH v2 3/4] firmware: qcom: enable QSEECOM on Lenovo Yoga C630

[Konrad Dybcio]

On 6/21/25 9:56 PM, Dmitry Baryshkov wrote:
> QSEECOM driver end UEFI vars access works on the Lenovo Yoga C630. This
> platform has only one storage (UFS) shared between Linux and SecureOS
> world, uefisecapp can not update variables directly. It requires some
> additional steps in order to update variables, which are not yet reverse
> engineered.
> 
> Enable the QSEECOM device on that laptop and set up a quirk, making UEFI
> vars read-only.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v2 4/4] arm64: dts: qcom: sdm850-lenovo-yoga-c630: fix RTC offset info

[Konrad Dybcio]

On 6/21/25 9:56 PM, Dmitry Baryshkov wrote:
> Lenovo Yoga C630 as most of the other WoA devices stores RTC offset in
> the UEFI variable. Add corresponding property to the RTC device in order
> to make RTC driver wait for UEFI variables to become available and then
> read offset value from the corresponding variable.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---


Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Sat, Jun 21, 2025 at 10:56:11PM +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.
> 
> 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,
> +};

It looks like the efivars implementation does not support read-only
efivars and this will lead to NULL pointer dereferences whenever you try
to write a variable.

Also not sure how useful it is to only be able to read variables,
including for the RTC where you'll end up with an RTC that's always
slightly off due to drift (even if you can set it when booting into
Windows or possibly from the UEFI setup).

Don't you have any SDAM blocks in the PMICs that you can use instead for
a proper functioning RTC on these machines?

Johan

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Konrad Dybcio]

On 6/23/25 4:45 PM, Johan Hovold wrote:
> On Sat, Jun 21, 2025 at 10:56:11PM +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.
>>
>> 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,
>> +};
> 
> It looks like the efivars implementation does not support read-only
> efivars and this will lead to NULL pointer dereferences whenever you try
> to write a variable.

There's efivar_supports_writes() that is used to set the EFIVAR_OPS_RDONLY
flag which then sets SB_RDONLY on all efivarfs superblocks

Konrad

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> On Sat, Jun 21, 2025 at 10:56:11PM +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.
> > 
> > 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,
> > +};
> 
> It looks like the efivars implementation does not support read-only
> efivars and this will lead to NULL pointer dereferences whenever you try
> to write a variable.

Ok, efivarfs seems to support it, but you'd crash when setting a
variable from the kernel (e.g. from the RTC driver).

> Also not sure how useful it is to only be able to read variables,
> including for the RTC where you'll end up with an RTC that's always
> slightly off due to drift (even if you can set it when booting into
> Windows or possibly from the UEFI setup).
> 
> Don't you have any SDAM blocks in the PMICs that you can use instead for
> a proper functioning RTC on these machines?

Johan

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Mon, Jun 23, 2025 at 04:49:22PM +0200, Konrad Dybcio wrote:
> On 6/23/25 4:45 PM, Johan Hovold wrote:

> > It looks like the efivars implementation does not support read-only
> > efivars and this will lead to NULL pointer dereferences whenever you try
> > to write a variable.
> 
> There's efivar_supports_writes() that is used to set the EFIVAR_OPS_RDONLY
> flag which then sets SB_RDONLY on all efivarfs superblocks

Yep, but it doesn't help when attempting to store the RTC offset from
the kernel (since that does not use efivarfs).

Johan

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Konrad Dybcio]

On 6/23/25 4:52 PM, Johan Hovold wrote:
> On Mon, Jun 23, 2025 at 04:49:22PM +0200, Konrad Dybcio wrote:
>> On 6/23/25 4:45 PM, Johan Hovold wrote:
> 
>>> It looks like the efivars implementation does not support read-only
>>> efivars and this will lead to NULL pointer dereferences whenever you try
>>> to write a variable.
>>
>> There's efivar_supports_writes() that is used to set the EFIVAR_OPS_RDONLY
>> flag which then sets SB_RDONLY on all efivarfs superblocks
> 
> Yep, but it doesn't help when attempting to store the RTC offset from
> the kernel (since that does not use efivarfs).

You're right, that hole needs patching

Konrad

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Dmitry Baryshkov]

On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> > On Sat, Jun 21, 2025 at 10:56:11PM +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.
> > > 
> > > 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,
> > > +};
> > 
> > It looks like the efivars implementation does not support read-only
> > efivars and this will lead to NULL pointer dereferences whenever you try
> > to write a variable.
> 
> Ok, efivarfs seems to support it, but you'd crash when setting a
> variable from the kernel (e.g. from the RTC driver).

Ack, I'll fix it.

> 
> > Also not sure how useful it is to only be able to read variables,
> > including for the RTC where you'll end up with an RTC that's always
> > slightly off due to drift (even if you can set it when booting into
> > Windows or possibly from the UEFI setup).
> > 
> > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > a proper functioning RTC on these machines?

I'd rather not poke into an SDAM, especially since we don't have docs
which SDAM blocks are used and which are not.

I think the slightly drifted RTC is still much better than ending up
with an RTC value which is significantly off, because it was set via the
file modification time.

Anyway, let me pick up some more patches in the next revision, maybe it
would be more obvious why I'd like to get R/O support.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:

> > > Also not sure how useful it is to only be able to read variables,
> > > including for the RTC where you'll end up with an RTC that's always
> > > slightly off due to drift (even if you can set it when booting into
> > > Windows or possibly from the UEFI setup).
> > > 
> > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > a proper functioning RTC on these machines?
> 
> I'd rather not poke into an SDAM, especially since we don't have docs
> which SDAM blocks are used and which are not.

You're with Qualcomm now so you should be able to dig up this
information like we did for the X13s (even if I'm quite aware that it
may still be easier said than done).

> I think the slightly drifted RTC is still much better than ending up
> with an RTC value which is significantly off, because it was set via the
> file modification time.

I measured drift of 1 second every 3.5 h on the X13s, so having an
almost correct time with massive drift that cannot be corrected for may
not necessarily be better.

> Anyway, let me pick up some more patches in the next revision, maybe it
> would be more obvious why I'd like to get R/O support.

I'll try to take a look.

Johan

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> > On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> 
> > > > Also not sure how useful it is to only be able to read variables,
> > > > including for the RTC where you'll end up with an RTC that's always
> > > > slightly off due to drift (even if you can set it when booting into
> > > > Windows or possibly from the UEFI setup).
> > > > 
> > > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > > a proper functioning RTC on these machines?
> > 
> > I'd rather not poke into an SDAM, especially since we don't have docs
> > which SDAM blocks are used and which are not.
> 
> You're with Qualcomm now so you should be able to dig up this
> information like we did for the X13s (even if I'm quite aware that it
> may still be easier said than done).

I'd rather try to find information on how to update UEFI vars on the
storage. Moreover, using the UEFI variable doesn't send the wrong
message to other developers (if I remember correctly, I've seen patches
poking to semi-random SDAM just because it seemed to be unused).

> > I think the slightly drifted RTC is still much better than ending up
> > with an RTC value which is significantly off, because it was set via the
> > file modification time.
> 
> I measured drift of 1 second every 3.5 h on the X13s, so having an
> almost correct time with massive drift that cannot be corrected for may
> not necessarily be better.

For me it provided a better user experience. Yes, I'm using C630 from
time to time, including the kernel development. A drifted but ticking
RTC is better than the RTC that rolls backs by several months at a
reboot, because of the missing RTC offset info.

> 
> > Anyway, let me pick up some more patches in the next revision, maybe it
> > would be more obvious why I'd like to get R/O support.
> 
> I'll try to take a look.
> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> > On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> > > On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > > > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> > 
> > > > > Also not sure how useful it is to only be able to read variables,
> > > > > including for the RTC where you'll end up with an RTC that's always
> > > > > slightly off due to drift (even if you can set it when booting into
> > > > > Windows or possibly from the UEFI setup).
> > > > > 
> > > > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > > > a proper functioning RTC on these machines?
> > > 
> > > I'd rather not poke into an SDAM, especially since we don't have docs
> > > which SDAM blocks are used and which are not.
> > 
> > You're with Qualcomm now so you should be able to dig up this
> > information like we did for the X13s (even if I'm quite aware that it
> > may still be easier said than done).
> 
> I'd rather try to find information on how to update UEFI vars on the
> storage.

You can do both, especially if it turns out you won't be able to have
persistent variables on these machines.

> Moreover, using the UEFI variable doesn't send the wrong
> message to other developers (if I remember correctly, I've seen patches
> poking to semi-random SDAM just because it seemed to be unused).

That's for the Qualcomm maintainers, and the rest of us, to catch during
review. And people putting random values into devicetrees is
unfortunately not limited to SDAM addresses.

Furthermore, getting an allocated block of addresses in SDAM for Linux
could be useful for other things too.
 
> > > I think the slightly drifted RTC is still much better than ending up
> > > with an RTC value which is significantly off, because it was set via the
> > > file modification time.
> > 
> > I measured drift of 1 second every 3.5 h on the X13s, so having an
> > almost correct time with massive drift that cannot be corrected for may
> > not necessarily be better.
> 
> For me it provided a better user experience. Yes, I'm using C630 from
> time to time, including the kernel development. A drifted but ticking
> RTC is better than the RTC that rolls backs by several months at a
> reboot, because of the missing RTC offset info.

Does it have to roll back? Can't you just keep it running after whatever
semi-random date it started at? And there is ntp and services like
fake-hwclock which saves the time on shutdown too.

Anyway, I still do no understand why you seem so reluctant to having a
proper functioning RTC using an SDAM offset.

Johan

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Konrad Dybcio]

On 6/26/25 3:13 PM, Johan Hovold wrote:
> On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
>> On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
>>> On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
>>>> On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
>>>>> On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
>>>
>>>>>> Also not sure how useful it is to only be able to read variables,
>>>>>> including for the RTC where you'll end up with an RTC that's always
>>>>>> slightly off due to drift (even if you can set it when booting into
>>>>>> Windows or possibly from the UEFI setup).
>>>>>>
>>>>>> Don't you have any SDAM blocks in the PMICs that you can use instead for
>>>>>> a proper functioning RTC on these machines?
>>>>
>>>> I'd rather not poke into an SDAM, especially since we don't have docs
>>>> which SDAM blocks are used and which are not.
>>>
>>> You're with Qualcomm now so you should be able to dig up this
>>> information like we did for the X13s (even if I'm quite aware that it
>>> may still be easier said than done).
>>
>> I'd rather try to find information on how to update UEFI vars on the
>> storage.
> 
> You can do both, especially if it turns out you won't be able to have
> persistent variables on these machines.

The danger here is that we only know what Qualcomm uses these cells
for, not necessarily what the vendors with a similar idea could
have come up with.

This is especially important since (unfortunately without going into
detail), you *really* don't want to mess up some existing values in
there.

Konrad

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Thu, Jun 26, 2025 at 03:49:32PM +0200, Konrad Dybcio wrote:
> On 6/26/25 3:13 PM, Johan Hovold wrote:
> > On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
> >> On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> >>> On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> >>>> On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> >>>>> On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> >>>
> >>>>>> Also not sure how useful it is to only be able to read variables,
> >>>>>> including for the RTC where you'll end up with an RTC that's always
> >>>>>> slightly off due to drift (even if you can set it when booting into
> >>>>>> Windows or possibly from the UEFI setup).
> >>>>>>
> >>>>>> Don't you have any SDAM blocks in the PMICs that you can use instead for
> >>>>>> a proper functioning RTC on these machines?
> >>>>
> >>>> I'd rather not poke into an SDAM, especially since we don't have docs
> >>>> which SDAM blocks are used and which are not.
> >>>
> >>> You're with Qualcomm now so you should be able to dig up this
> >>> information like we did for the X13s (even if I'm quite aware that it
> >>> may still be easier said than done).
> >>
> >> I'd rather try to find information on how to update UEFI vars on the
> >> storage.
> > 
> > You can do both, especially if it turns out you won't be able to have
> > persistent variables on these machines.
> 
> The danger here is that we only know what Qualcomm uses these cells
> for, not necessarily what the vendors with a similar idea could
> have come up with.

Hmm. Good point.

But at least the address we used on sc8280xp appears to be cleared on
hard reset (holding down the power button) so it can't be used for
anything that useful it seems.

> This is especially important since (unfortunately without going into
> detail), you *really* don't want to mess up some existing values in
> there.

Yeah, I wouldn't pick a random address without getting an ack from
Qualcomm first.

Johan

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 03:13:42PM +0200, Johan Hovold wrote:
> On Thu, Jun 26, 2025 at 02:15:26PM +0300, Dmitry Baryshkov wrote:
> > On Thu, Jun 26, 2025 at 11:42:50AM +0200, Johan Hovold wrote:
> > > On Tue, Jun 24, 2025 at 04:13:34AM +0300, Dmitry Baryshkov wrote:
> > > > On Mon, Jun 23, 2025 at 04:50:27PM +0200, Johan Hovold wrote:
> > > > > On Mon, Jun 23, 2025 at 04:45:30PM +0200, Johan Hovold wrote:
> > > 
> > > > > > Also not sure how useful it is to only be able to read variables,
> > > > > > including for the RTC where you'll end up with an RTC that's always
> > > > > > slightly off due to drift (even if you can set it when booting into
> > > > > > Windows or possibly from the UEFI setup).
> > > > > > 
> > > > > > Don't you have any SDAM blocks in the PMICs that you can use instead for
> > > > > > a proper functioning RTC on these machines?
> > > > 
> > > > I'd rather not poke into an SDAM, especially since we don't have docs
> > > > which SDAM blocks are used and which are not.
> > > 
> > > You're with Qualcomm now so you should be able to dig up this
> > > information like we did for the X13s (even if I'm quite aware that it
> > > may still be easier said than done).
> > 
> > I'd rather try to find information on how to update UEFI vars on the
> > storage.
> 
> You can do both, especially if it turns out you won't be able to have
> persistent variables on these machines.
> 
> > Moreover, using the UEFI variable doesn't send the wrong
> > message to other developers (if I remember correctly, I've seen patches
> > poking to semi-random SDAM just because it seemed to be unused).
> 
> That's for the Qualcomm maintainers, and the rest of us, to catch during
> review. And people putting random values into devicetrees is
> unfortunately not limited to SDAM addresses.

Yes. But here it might be more fun.

> Furthermore, getting an allocated block of addresses in SDAM for Linux
> could be useful for other things too.

Yes, but this obviously can't happen for released platforms.

>  
> > > > I think the slightly drifted RTC is still much better than ending up
> > > > with an RTC value which is significantly off, because it was set via the
> > > > file modification time.
> > > 
> > > I measured drift of 1 second every 3.5 h on the X13s, so having an
> > > almost correct time with massive drift that cannot be corrected for may
> > > not necessarily be better.
> > 
> > For me it provided a better user experience. Yes, I'm using C630 from
> > time to time, including the kernel development. A drifted but ticking
> > RTC is better than the RTC that rolls backs by several months at a
> > reboot, because of the missing RTC offset info.
> 
> Does it have to roll back? Can't you just keep it running after whatever
> semi-random date it started at?

It rolls back after reboot.

> And there is ntp and services like
> fake-hwclock which saves the time on shutdown too.

Likewise I can plug in the USB RTC or do something else. NTP requires
network access, which is fun to have if you are debugging modem of WiFi.

> Anyway, I still do no understand why you seem so reluctant to having a
> proper functioning RTC using an SDAM offset.

Because that would be a one-off solution for this particular laptop,
etc. I want something that other laptops can use without having to find
another magic value which suits a particular laptop instance.

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/4] firmware: qcom: uefisecapp: add support for R/O UEFI vars

[Johan Hovold]

On Fri, Jun 27, 2025 at 02:09:51AM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 03:13:42PM +0200, Johan Hovold wrote:

> > Furthermore, getting an allocated block of addresses in SDAM for Linux
> > could be useful for other things too.
> 
> Yes, but this obviously can't happen for released platforms.

We managed to get one for sc8280xp post release (was harder for x1e for
some reason).

> > > > > I think the slightly drifted RTC is still much better than ending up
> > > > > with an RTC value which is significantly off, because it was set via the
> > > > > file modification time.
> > > > 
> > > > I measured drift of 1 second every 3.5 h on the X13s, so having an
> > > > almost correct time with massive drift that cannot be corrected for may
> > > > not necessarily be better.
> > > 
> > > For me it provided a better user experience. Yes, I'm using C630 from
> > > time to time, including the kernel development. A drifted but ticking
> > > RTC is better than the RTC that rolls backs by several months at a
> > > reboot, because of the missing RTC offset info.
> > 
> > Does it have to roll back? Can't you just keep it running after whatever
> > semi-random date it started at?
> 
> It rolls back after reboot.

That's odd. Doesn't happen here with the X1E CRD if I drop the uefi
offset property in DT. I'm back in the seventies but time is strictly
increasing also after reboots.

Perhaps you have some user space setting that resets it?

> > And there is ntp and services like
> > fake-hwclock which saves the time on shutdown too.
> 
> Likewise I can plug in the USB RTC or do something else. NTP requires
> network access, which is fun to have if you are debugging modem of WiFi.
>
> > Anyway, I still do no understand why you seem so reluctant to having a
> > proper functioning RTC using an SDAM offset.
> 
> Because that would be a one-off solution for this particular laptop,
> etc. I want something that other laptops can use without having to find
> another magic value which suits a particular laptop instance.

My point is that it's not really a solution. These machines still do not
have persistent UEFI variables, and now they have an apparently
functional but still broken RTC.

I added support for the UEFI offset to the driver so that the time could
be set on Qualcomm machines. With this series that is still not the
case, even if people may now initially get the impression that it works
since the time is only off by a few seconds (until it becomes minutes
and hours and you starting missing your trains).

Johan