[PATCH v4 0/8] firmware: qcom: enable UEFI variables on Lenovo Yoga C630

[PATCH v4 0/8] 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 v4:
- Fixed comment describing how to disable QSEECOM for a particular
  platform (Konrad)
- Link to v3: https://lore.kernel.org/r/20250624-more-qseecom-v3-0-95205cd88cc2@oss.qualcomm.com

Changes in v3:
- Picked up patches from another series, reworking the QSEECOM
  allowtable
- Added a fix for efivar_set_variable() crashing the machine if R/O UEFI
  vars are provided (Johan)
- Link to v2: https://lore.kernel.org/r/20250621-more-qseecom-v2-0-6e8f635640c5@oss.qualcomm.com
- Link to the merged series: https://lore.kernel.org/r/20241103-rework-qseecom-v1-0-1d75d4eedc1e@linaro.org/

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 (8):
      efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case
      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
      firmware; qcom: scm: enable QSEECOM on SC8280XP CRD
      firmware: qcom: scm: add modparam to control QSEECOM enablement
      firmware: qcom: scm: rework QSEECOM allowlist
      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/efi/vars.c                        |  2 +
 drivers/firmware/qcom/qcom_qseecom.c               |  6 +-
 drivers/firmware/qcom/qcom_qseecom_uefisecapp.c    | 18 +++-
 drivers/firmware/qcom/qcom_scm.c                   | 99 +++++++++++++---------
 include/linux/firmware/qcom/qcom_qseecom.h         |  3 +
 7 files changed, 93 insertions(+), 41 deletions(-)
---
base-commit: 2ae2aaafb21454f4781c30734959cf223ab486ef
change-id: 20240725-more-qseecom-379933b9c769

Best regards,
-- 
With best wishes
Dmitry

[PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Dmitry Baryshkov]

If efivar implementation doesn't provide write support, then calling
efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
RTC offset) will crash the system. Prevent that by checking that
set_variable callback is actually provided and fail with an
EFI_WRITE_PROTECTED if it is not.

Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
Reported-by: Johan Hovold <johan@kernel.org>
Closes: https://lore.kernel.org/r/aFlps9iUcD42vN4w@hovoldconsulting.com
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/efi/vars.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 3700e98697676d8e6f04f061f447391503f9abba..11c5f785c09364f61642d82416822cb2e1a027fd 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -227,6 +227,8 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
 	setvar = __efivars->ops->set_variable_nonblocking;
 	if (!setvar || !nonblocking)
 		 setvar = __efivars->ops->set_variable;
+	if (!setvar)
+		return EFI_WRITE_PROTECTED;
 
 	return setvar(name, vendor, attr, data_size, data);
 }

-- 
2.39.5

[PATCH v4 2/8] 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.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
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..fc2ed02dbd30b389b5058f5cac70c184df7ca873 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)
+		*quirks = *(unsigned long *)(match->data);
+	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 v4 3/8] 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.

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

[PATCH v4 4/8] 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.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
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 fc2ed02dbd30b389b5058f5cac70c184df7ca873..dbb77c3f69ddaa931e7faa73911207a83634bda1 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 v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Dmitry Baryshkov]

As reported by Johan, this platform also doesn't currently support
updating of the UEFI variables. In preparation to reworking match list
for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
variables.

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

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index dbb77c3f69ddaa931e7faa73911207a83634bda1..27ef2497089e11b5a902d949de2e16b7443a2ca4 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -2005,6 +2005,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
 	{ .compatible = "microsoft,romulus13", },
 	{ .compatible = "microsoft,romulus15", },
 	{ .compatible = "qcom,sc8180x-primus" },
+	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
 	{ .compatible = "qcom,x1e001de-devkit" },
 	{ .compatible = "qcom,x1e80100-crd" },
 	{ .compatible = "qcom,x1e80100-qcp" },

-- 
2.39.5

[PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Dmitry Baryshkov]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

In preparation to enabling QSEECOM for the platforms rather than
individual machines provide a mechanism for the user to override default
selection. Allow users to use qcom_scm.qseecom modparam.

Setting it to 'force' will enable QSEECOM even if it disabled or not
handled by the allowlist.

Setting it to 'off' will forcibly disable the QSEECOM interface,
allowing incompatible machines to function.

Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
variables read-only.

All other values mean 'auto', trusting the allowlist in the module.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 27ef2497089e11b5a902d949de2e16b7443a2ca4..5bf59eba2a863ba16e59df7fa2de1c50b0a218d0 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1983,9 +1983,14 @@ EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_send);
 
 static unsigned long qcom_qseecom_ro_uefi = QCOM_QSEECOM_QUIRK_RO_UEFIVARS;
 
+static char *qseecom = "auto";
+MODULE_PARM_DESC(qseecom, "Enable QSEECOM interface (force | roefivars | off | auto)");
+module_param(qseecom, charp, 0);
+
 /*
  * 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.
+ * any potential issues with this, only allow validated machines for now. Users
+ * still can manually enable or disable it via the qcom_scm.qseecom modparam.
  */
 static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
 	{ .compatible = "asus,vivobook-s15" },
@@ -2013,11 +2018,27 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
 	{ }
 };
 
-static bool qcom_scm_qseecom_machine_is_allowed(unsigned long *quirks)
+static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev,
+						unsigned long *quirks)
 {
 	const struct of_device_id *match;
 	struct device_node *np;
 
+	if (!strcmp(qseecom, "off")) {
+		dev_info(scm_dev, "qseecom: disabled by modparam\n");
+		return false;
+	} else if (!strcmp(qseecom, "force")) {
+		dev_info(scm_dev, "qseecom: forcibly enabled\n");
+		*quirks = 0;
+		return true;
+	} else if (!strcmp(qseecom, "roefivars")) {
+		dev_info(scm_dev, "qseecom: enabling with R/O UEFI variables\n");
+		*quirks = QCOM_QSEECOM_QUIRK_RO_UEFIVARS;
+		return true;
+	} else if (strcmp(qseecom, "auto")) {
+		dev_warn(scm_dev, "qseecom: invalid value for the modparam, ignoring\n");
+	}
+
 	np = of_find_node_by_path("/");
 	if (!np)
 		return false;
@@ -2065,7 +2086,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(&quirks)) {
+	if (!qcom_scm_qseecom_machine_is_allowed(scm->dev, &quirks)) {
 		dev_info(scm->dev, "qseecom: untested machine, skipping\n");
 		return 0;
 	}

-- 
2.39.5

[PATCH v4 7/8] firmware: qcom: scm: rework QSEECOM allowlist

[Dmitry Baryshkov]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Listing individual machines in qcom_scm_qseecom_allowlist doesn't scale.
Allow it to function as allow and disallow list at the same time by the
means of the match->data and list the SoC families instead of devices.

In case a particular device has buggy or incompatible firmware user
still can disable QSEECOM by specifying qcom_scm.qseecom=off kernel
param and (in the longer term) adding machine-specific entry to the
qcom_scm_qseecom_allowlist table.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/firmware/qcom/qcom_scm.c           | 49 ++++++++++++++----------------
 include/linux/firmware/qcom/qcom_qseecom.h |  1 +
 2 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 5bf59eba2a863ba16e59df7fa2de1c50b0a218d0..025f834e95b537b76d41b59b63871a4ce5bed717 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1981,6 +1981,7 @@ 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_disable = QCOM_QSEECOM_QUIRK_DISABLE;
 static unsigned long qcom_qseecom_ro_uefi = QCOM_QSEECOM_QUIRK_RO_UEFIVARS;
 
 static char *qseecom = "auto";
@@ -1989,32 +1990,20 @@ module_param(qseecom, charp, 0);
 
 /*
  * 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. Users
+ * any potential issues with this, only allow validated platforms for now. Users
  * still can manually enable or disable it via the qcom_scm.qseecom modparam.
+ *
+ * To disable QSEECOM for a particular machine, add compatible entry and set
+ * data to &qcom_qseecom_disable.
  */
 static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
-	{ .compatible = "asus,vivobook-s15" },
-	{ .compatible = "asus,zenbook-a14-ux3407qa" },
-	{ .compatible = "asus,zenbook-a14-ux3407ra" },
-	{ .compatible = "dell,xps13-9345" },
-	{ .compatible = "hp,elitebook-ultra-g1q" },
-	{ .compatible = "hp,omnibook-x14" },
-	{ .compatible = "huawei,gaokun3" },
-	{ .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" },
-	{ .compatible = "microsoft,romulus13", },
-	{ .compatible = "microsoft,romulus15", },
-	{ .compatible = "qcom,sc8180x-primus" },
+	{ .compatible = "qcom,sc8180x", },
+	{ .compatible = "qcom,sc8280xp", },
 	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
-	{ .compatible = "qcom,x1e001de-devkit" },
-	{ .compatible = "qcom,x1e80100-crd" },
-	{ .compatible = "qcom,x1e80100-qcp" },
-	{ .compatible = "qcom,x1p42100-crd" },
+	{ .compatible = "qcom,sdm845", .data = &qcom_qseecom_disable, },
+	{ .compatible = "qcom,x1e80100", },
+	{ .compatible = "qcom,x1p42100", },
 	{ }
 };
 
@@ -2046,12 +2035,22 @@ static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev,
 	match = of_match_node(qcom_scm_qseecom_allowlist, np);
 	of_node_put(np);
 
-	if (match && match->data)
+	if (!match) {
+		dev_info(scm_dev, "qseecom: untested machine, skipping\n");
+		return false;
+	}
+
+	if (match->data)
 		*quirks = *(unsigned long *)(match->data);
 	else
 		*quirks = 0;
 
-	return match;
+	if (*quirks & QCOM_QSEECOM_QUIRK_DISABLE) {
+		dev_info(scm_dev, "qseecom: disabled by the quirk\n");
+		return false;
+	}
+
+	return true;
 }
 
 static void qcom_scm_qseecom_free(void *data)
@@ -2086,10 +2085,8 @@ 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(scm->dev, &quirks)) {
-		dev_info(scm->dev, "qseecom: untested machine, skipping\n");
+	if (!qcom_scm_qseecom_machine_is_allowed(scm->dev, &quirks))
 		return 0;
-	}
 
 	/*
 	 * Set up QSEECOM interface device. All application clients will be
diff --git a/include/linux/firmware/qcom/qcom_qseecom.h b/include/linux/firmware/qcom/qcom_qseecom.h
index 8d6d660e854fdb0fabbef10ab5ee6ff23ad79826..d48044ece20cc9ebac3357a642dc671c349d4343 100644
--- a/include/linux/firmware/qcom/qcom_qseecom.h
+++ b/include/linux/firmware/qcom/qcom_qseecom.h
@@ -52,5 +52,6 @@ static inline int qcom_qseecom_app_send(struct qseecom_client *client,
 }
 
 #define QCOM_QSEECOM_QUIRK_RO_UEFIVARS		BIT(0)
+#define QCOM_QSEECOM_QUIRK_DISABLE		BIT(1)
 
 #endif /* __QCOM_QSEECOM_H */

-- 
2.39.5

[PATCH v4 8/8] 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.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
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 v4 7/8] firmware: qcom: scm: rework QSEECOM allowlist

[Johan Hovold]

On Wed, Jun 25, 2025 at 01:53:26AM +0300, Dmitry Baryshkov wrote:
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Listing individual machines in qcom_scm_qseecom_allowlist doesn't scale.
> Allow it to function as allow and disallow list at the same time by the
> means of the match->data and list the SoC families instead of devices.
> 
> In case a particular device has buggy or incompatible firmware user
> still can disable QSEECOM by specifying qcom_scm.qseecom=off kernel
> param and (in the longer term) adding machine-specific entry to the
> qcom_scm_qseecom_allowlist table.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

>  /*
>   * 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. Users
> + * any potential issues with this, only allow validated platforms for now. Users
>   * still can manually enable or disable it via the qcom_scm.qseecom modparam.
> + *
> + * To disable QSEECOM for a particular machine, add compatible entry and set
> + * data to &qcom_qseecom_disable.
>   */
>  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> -	{ .compatible = "asus,vivobook-s15" },
> -	{ .compatible = "asus,zenbook-a14-ux3407qa" },
> -	{ .compatible = "asus,zenbook-a14-ux3407ra" },
> -	{ .compatible = "dell,xps13-9345" },
> -	{ .compatible = "hp,elitebook-ultra-g1q" },
> -	{ .compatible = "hp,omnibook-x14" },
> -	{ .compatible = "huawei,gaokun3" },
> -	{ .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" },
> -	{ .compatible = "microsoft,romulus13", },
> -	{ .compatible = "microsoft,romulus15", },
> -	{ .compatible = "qcom,sc8180x-primus" },
> +	{ .compatible = "qcom,sc8180x", },
> +	{ .compatible = "qcom,sc8280xp", },
>  	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },

You need to have the machine specific entries before the SoC fallbacks
for this to work.

Perhaps this should be made more clear in the table by adding a
separator comment before the SoC entries or similar.

> -	{ .compatible = "qcom,x1e001de-devkit" },
> -	{ .compatible = "qcom,x1e80100-crd" },
> -	{ .compatible = "qcom,x1e80100-qcp" },
> -	{ .compatible = "qcom,x1p42100-crd" },
> +	{ .compatible = "qcom,sdm845", .data = &qcom_qseecom_disable, },
> +	{ .compatible = "qcom,x1e80100", },
> +	{ .compatible = "qcom,x1p42100", },
>  	{ }
>  };
>  
> @@ -2046,12 +2035,22 @@ static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev,
>  	match = of_match_node(qcom_scm_qseecom_allowlist, np);
>  	of_node_put(np);
>  
> -	if (match && match->data)
> +	if (!match) {
> +		dev_info(scm_dev, "qseecom: untested machine, skipping\n");
> +		return false;
> +	}
> +
> +	if (match->data)
>  		*quirks = *(unsigned long *)(match->data);
>  	else
>  		*quirks = 0;
>  
> -	return match;
> +	if (*quirks & QCOM_QSEECOM_QUIRK_DISABLE) {
> +		dev_info(scm_dev, "qseecom: disabled by the quirk\n");

Not sure this is needed since it presumably has been disabled because it
has been tested and found not to work. No need to spam the logs with
that on every boot.

In any case I don't think you should be referring to "the quirk" which
makes little sense without looking at the implementation.

> +		return false;
> +	}
> +
> +	return true;
>  }

Johan

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Johan Hovold]

On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> If efivar implementation doesn't provide write support, then calling
> efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> RTC offset) will crash the system. Prevent that by checking that
> set_variable callback is actually provided and fail with an
> EFI_WRITE_PROTECTED if it is not.
> 
> Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")

I don't think a fixes tag is warranted here as it currently appears to
be expected that the callers check if setvar is supported before calling
this helper (e.g. by calling efivar_supports_writes() as efivarfs does).

So should perhaps be fixed in the RTC driver if we agree that supporting
read-only offsets is indeed something we want.

Are there any other current user that may possibly benefit from
something like this?

> Reported-by: Johan Hovold <johan@kernel.org>
> Closes: https://lore.kernel.org/r/aFlps9iUcD42vN4w@hovoldconsulting.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/firmware/efi/vars.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index 3700e98697676d8e6f04f061f447391503f9abba..11c5f785c09364f61642d82416822cb2e1a027fd 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -227,6 +227,8 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
>  	setvar = __efivars->ops->set_variable_nonblocking;
>  	if (!setvar || !nonblocking)
>  		 setvar = __efivars->ops->set_variable;
> +	if (!setvar)
> +		return EFI_WRITE_PROTECTED;
>  
>  	return setvar(name, vendor, attr, data_size, data);
>  }

Johan

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Johan Hovold]

On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> In preparation to enabling QSEECOM for the platforms rather than
> individual machines provide a mechanism for the user to override default
> selection. Allow users to use qcom_scm.qseecom modparam.
> 
> Setting it to 'force' will enable QSEECOM even if it disabled or not
> handled by the allowlist.
> 
> Setting it to 'off' will forcibly disable the QSEECOM interface,
> allowing incompatible machines to function.
> 
> Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> variables read-only.
> 
> All other values mean 'auto', trusting the allowlist in the module.

I don't see the need for this. The kernel should just provide sensible
defaults.

Johan

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

[Johan Hovold]

On Wed, Jun 25, 2025 at 01:53:27AM +0300, 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.

This is not a fix so please drop that word from Subject.

I'd also expect you to mention that the RTC would be heavily crippled on
this machine as the efi variables cannot be updated.

Is there even a UEFI setup setting for this so that users that have
blown away Windows can ever set the time (which may become totally off
due to drift)?

I'm still not convinced that this is something we want, especially since
you could have a fully functional RTC by providing an SDAM offset
instead like we do on the sc8280xp CRD.

Johan

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 12:04:30PM +0200, Johan Hovold wrote:
> On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> > If efivar implementation doesn't provide write support, then calling
> > efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> > RTC offset) will crash the system. Prevent that by checking that
> > set_variable callback is actually provided and fail with an
> > EFI_WRITE_PROTECTED if it is not.
> > 
> > Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
> 
> I don't think a fixes tag is warranted here as it currently appears to
> be expected that the callers check if setvar is supported before calling
> this helper (e.g. by calling efivar_supports_writes() as efivarfs does).

It is not documented as such. So, I think, we'd better not crash the
callers.

> So should perhaps be fixed in the RTC driver if we agree that supporting
> read-only offsets is indeed something we want.
> 
> Are there any other current user that may possibly benefit from
> something like this?

efi-pstore comes to my mind.

> 
> > Reported-by: Johan Hovold <johan@kernel.org>
> > Closes: https://lore.kernel.org/r/aFlps9iUcD42vN4w@hovoldconsulting.com
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> >  drivers/firmware/efi/vars.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 3700e98697676d8e6f04f061f447391503f9abba..11c5f785c09364f61642d82416822cb2e1a027fd 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -227,6 +227,8 @@ efi_status_t efivar_set_variable_locked(efi_char16_t *name, efi_guid_t *vendor,
> >  	setvar = __efivars->ops->set_variable_nonblocking;
> >  	if (!setvar || !nonblocking)
> >  		 setvar = __efivars->ops->set_variable;
> > +	if (!setvar)
> > +		return EFI_WRITE_PROTECTED;
> >  
> >  	return setvar(name, vendor, attr, data_size, data);
> >  }
> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > In preparation to enabling QSEECOM for the platforms rather than
> > individual machines provide a mechanism for the user to override default
> > selection. Allow users to use qcom_scm.qseecom modparam.
> > 
> > Setting it to 'force' will enable QSEECOM even if it disabled or not
> > handled by the allowlist.
> > 
> > Setting it to 'off' will forcibly disable the QSEECOM interface,
> > allowing incompatible machines to function.
> > 
> > Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> > variables read-only.
> > 
> > All other values mean 'auto', trusting the allowlist in the module.
> 
> I don't see the need for this. The kernel should just provide sensible
> defaults.

It does provide _defaults_. However with the next commit we mass-enable
QSEECOM for SoC families, which includes untested WoA devices. If the
user observes a misbehaviour of the UEFI vars or any other
QSEECOM-related driver on those platforms, it is much easier to let
users test and workaround UEFI misbehaviour.

I can probably add an explicit message that if the modparam is used, it
must be reported to linux-arm-msm@.

> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH v4 7/8] firmware: qcom: scm: rework QSEECOM allowlist

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 11:56:01AM +0200, Johan Hovold wrote:
> On Wed, Jun 25, 2025 at 01:53:26AM +0300, Dmitry Baryshkov wrote:
> > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > Listing individual machines in qcom_scm_qseecom_allowlist doesn't scale.
> > Allow it to function as allow and disallow list at the same time by the
> > means of the match->data and list the SoC families instead of devices.
> > 
> > In case a particular device has buggy or incompatible firmware user
> > still can disable QSEECOM by specifying qcom_scm.qseecom=off kernel
> > param and (in the longer term) adding machine-specific entry to the
> > qcom_scm_qseecom_allowlist table.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> 
> >  /*
> >   * 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. Users
> > + * any potential issues with this, only allow validated platforms for now. Users
> >   * still can manually enable or disable it via the qcom_scm.qseecom modparam.
> > + *
> > + * To disable QSEECOM for a particular machine, add compatible entry and set
> > + * data to &qcom_qseecom_disable.
> >   */
> >  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> > -	{ .compatible = "asus,vivobook-s15" },
> > -	{ .compatible = "asus,zenbook-a14-ux3407qa" },
> > -	{ .compatible = "asus,zenbook-a14-ux3407ra" },
> > -	{ .compatible = "dell,xps13-9345" },
> > -	{ .compatible = "hp,elitebook-ultra-g1q" },
> > -	{ .compatible = "hp,omnibook-x14" },
> > -	{ .compatible = "huawei,gaokun3" },
> > -	{ .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" },
> > -	{ .compatible = "microsoft,romulus13", },
> > -	{ .compatible = "microsoft,romulus15", },
> > -	{ .compatible = "qcom,sc8180x-primus" },
> > +	{ .compatible = "qcom,sc8180x", },
> > +	{ .compatible = "qcom,sc8280xp", },
> >  	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> 
> You need to have the machine specific entries before the SoC fallbacks
> for this to work.

I don't think so. It's not how OF matching works.

> 
> Perhaps this should be made more clear in the table by adding a
> separator comment before the SoC entries or similar.
> 
> > -	{ .compatible = "qcom,x1e001de-devkit" },
> > -	{ .compatible = "qcom,x1e80100-crd" },
> > -	{ .compatible = "qcom,x1e80100-qcp" },
> > -	{ .compatible = "qcom,x1p42100-crd" },
> > +	{ .compatible = "qcom,sdm845", .data = &qcom_qseecom_disable, },
> > +	{ .compatible = "qcom,x1e80100", },
> > +	{ .compatible = "qcom,x1p42100", },
> >  	{ }
> >  };
> >  
> > @@ -2046,12 +2035,22 @@ static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev,
> >  	match = of_match_node(qcom_scm_qseecom_allowlist, np);
> >  	of_node_put(np);
> >  
> > -	if (match && match->data)
> > +	if (!match) {
> > +		dev_info(scm_dev, "qseecom: untested machine, skipping\n");
> > +		return false;
> > +	}
> > +
> > +	if (match->data)
> >  		*quirks = *(unsigned long *)(match->data);
> >  	else
> >  		*quirks = 0;
> >  
> > -	return match;
> > +	if (*quirks & QCOM_QSEECOM_QUIRK_DISABLE) {
> > +		dev_info(scm_dev, "qseecom: disabled by the quirk\n");
> 
> Not sure this is needed since it presumably has been disabled because it
> has been tested and found not to work. No need to spam the logs with
> that on every boot.
> 
> In any case I don't think you should be referring to "the quirk" which
> makes little sense without looking at the implementation.

ack

> 
> > +		return false;
> > +	}
> > +
> > +	return true;
> >  }
> 
> Johan

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 12:16:49PM +0200, Johan Hovold wrote:
> On Wed, Jun 25, 2025 at 01:53:27AM +0300, 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.
> 
> This is not a fix so please drop that word from Subject.

ack

> 
> I'd also expect you to mention that the RTC would be heavily crippled on
> this machine as the efi variables cannot be updated.

ack

> 
> Is there even a UEFI setup setting for this so that users that have
> blown away Windows can ever set the time (which may become totally off
> due to drift)?

There is a UEFI setup UI.

> I'm still not convinced that this is something we want, especially since
> you could have a fully functional RTC by providing an SDAM offset
> instead like we do on the sc8280xp CRD.

No, I don't want to go that way.

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Johan Hovold]

On Thu, Jun 26, 2025 at 02:03:44PM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 12:04:30PM +0200, Johan Hovold wrote:
> > On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> > > If efivar implementation doesn't provide write support, then calling
> > > efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> > > RTC offset) will crash the system. Prevent that by checking that
> > > set_variable callback is actually provided and fail with an
> > > EFI_WRITE_PROTECTED if it is not.
> > > 
> > > Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
> > 
> > I don't think a fixes tag is warranted here as it currently appears to
> > be expected that the callers check if setvar is supported before calling
> > this helper (e.g. by calling efivar_supports_writes() as efivarfs does).
> 
> It is not documented as such. So, I think, we'd better not crash the
> callers.

You need to look at the backstory to determine that before jumping to
conclusions (e.g. start by looking at f88814cc2578 ("efi/efivars: Expose
RT service availability via efivars abstraction")).

> > So should perhaps be fixed in the RTC driver if we agree that supporting
> > read-only offsets is indeed something we want.
> > 
> > Are there any other current user that may possibly benefit from
> > something like this?
> 
> efi-pstore comes to my mind.

No, that driver is also disabled when efivar_supports_writes() returns
false.

Johan

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Dmitry Baryshkov]

On Thu, 26 Jun 2025 at 15:51, Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, Jun 26, 2025 at 02:03:44PM +0300, Dmitry Baryshkov wrote:
> > On Thu, Jun 26, 2025 at 12:04:30PM +0200, Johan Hovold wrote:
> > > On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> > > > If efivar implementation doesn't provide write support, then calling
> > > > efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> > > > RTC offset) will crash the system. Prevent that by checking that
> > > > set_variable callback is actually provided and fail with an
> > > > EFI_WRITE_PROTECTED if it is not.
> > > >
> > > > Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
> > >
> > > I don't think a fixes tag is warranted here as it currently appears to
> > > be expected that the callers check if setvar is supported before calling
> > > this helper (e.g. by calling efivar_supports_writes() as efivarfs does).
> >
> > It is not documented as such. So, I think, we'd better not crash the
> > callers.
>
> You need to look at the backstory to determine that before jumping to
> conclusions (e.g. start by looking at f88814cc2578 ("efi/efivars: Expose
> RT service availability via efivars abstraction")).

_documented_. I'll update documentation for efivar_set_variable() in
the next iteration and add a check to the RTC driver. However I still
think that this patch is valid.

>
> > > So should perhaps be fixed in the RTC driver if we agree that supporting
> > > read-only offsets is indeed something we want.
> > >
> > > Are there any other current user that may possibly benefit from
> > > something like this?
> >
> > efi-pstore comes to my mind.
>
> No, that driver is also disabled when efivar_supports_writes() returns
> false.

Good.


-- 
With best wishes
Dmitry

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Johan Hovold]

On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> > On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > In preparation to enabling QSEECOM for the platforms rather than
> > > individual machines provide a mechanism for the user to override default
> > > selection. Allow users to use qcom_scm.qseecom modparam.
> > > 
> > > Setting it to 'force' will enable QSEECOM even if it disabled or not
> > > handled by the allowlist.
> > > 
> > > Setting it to 'off' will forcibly disable the QSEECOM interface,
> > > allowing incompatible machines to function.
> > > 
> > > Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> > > variables read-only.
> > > 
> > > All other values mean 'auto', trusting the allowlist in the module.
> > 
> > I don't see the need for this. The kernel should just provide sensible
> > defaults.
> 
> It does provide _defaults_. However with the next commit we mass-enable
> QSEECOM for SoC families, which includes untested WoA devices. If the
> user observes a misbehaviour of the UEFI vars or any other
> QSEECOM-related driver on those platforms, it is much easier to let
> users test and workaround UEFI misbehaviour.

You basically know by now which machines supports qseecom and which do
not, right (e.g. UFS storage means non-persistent EFI vars)?

And it's a pretty bad user experience to have people trying to write
efivariables when setting up a machine and then spend hours trying to
debug why they don't persist after a reboot.

I don't think that's fair to users.

Let whoever brings up a new machine figure this out. It's just one
entry, no scaling issues, and we get accurate information (unless
Qualcomm, who sits on the documentation, is willing to provide it
upfront).

Johan

Re: [PATCH v4 7/8] firmware: qcom: scm: rework QSEECOM allowlist

[Johan Hovold]

On Thu, Jun 26, 2025 at 02:09:08PM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 11:56:01AM +0200, Johan Hovold wrote:
> > On Wed, Jun 25, 2025 at 01:53:26AM +0300, Dmitry Baryshkov wrote:

> > > +	{ .compatible = "qcom,sc8180x", },
> > > +	{ .compatible = "qcom,sc8280xp", },
> > >  	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> > 
> > You need to have the machine specific entries before the SoC fallbacks
> > for this to work.
> 
> I don't think so. It's not how OF matching works.

Ah, right, too used to USB matching.

Johan

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Dmitry Baryshkov]

On Thu, Jun 26, 2025 at 02:58:52PM +0200, Johan Hovold wrote:
> On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> > On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> > > On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > 
> > > > In preparation to enabling QSEECOM for the platforms rather than
> > > > individual machines provide a mechanism for the user to override default
> > > > selection. Allow users to use qcom_scm.qseecom modparam.
> > > > 
> > > > Setting it to 'force' will enable QSEECOM even if it disabled or not
> > > > handled by the allowlist.
> > > > 
> > > > Setting it to 'off' will forcibly disable the QSEECOM interface,
> > > > allowing incompatible machines to function.
> > > > 
> > > > Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> > > > variables read-only.
> > > > 
> > > > All other values mean 'auto', trusting the allowlist in the module.
> > > 
> > > I don't see the need for this. The kernel should just provide sensible
> > > defaults.
> > 
> > It does provide _defaults_. However with the next commit we mass-enable
> > QSEECOM for SoC families, which includes untested WoA devices. If the
> > user observes a misbehaviour of the UEFI vars or any other
> > QSEECOM-related driver on those platforms, it is much easier to let
> > users test and workaround UEFI misbehaviour.
> 
> You basically know by now which machines supports qseecom and which do
> not, right (e.g. UFS storage means non-persistent EFI vars)?
> 
> And it's a pretty bad user experience to have people trying to write
> efivariables when setting up a machine and then spend hours trying to
> debug why they don't persist after a reboot.
> 
> I don't think that's fair to users.

So, is it a user or a developer, trying to port Linux to a new hardware?
Also, R/O implementation makes it obvious, that the variables do not
persist.

> 
> Let whoever brings up a new machine figure this out. It's just one
> entry, no scaling issues, and we get accurate information (unless
> Qualcomm, who sits on the documentation, is willing to provide it
> upfront).

And that's not really scallable. All other parts of a particular device
are described by the DT only (that's especially true on the PMIC GLINK
machines). If we are to support new laptop in e.g. distro kernel, we
need to provide a DT... and a patch for qcom-scm driver. I'd very much
prefer to do it other way around: provide a DT and patch qcom-scm if the
laptop is any way different from other laptops. E.g. we know that all
X1Elite laptops support R/W EFI variables. Except for X1-CRD, which
deserves an entry in the driver.

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Konrad Dybcio]

On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
> As reported by Johan, this platform also doesn't currently support
> updating of the UEFI variables. In preparation to reworking match list
> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> variables.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index dbb77c3f69ddaa931e7faa73911207a83634bda1..27ef2497089e11b5a902d949de2e16b7443a2ca4 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -2005,6 +2005,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
>  	{ .compatible = "microsoft,romulus13", },
>  	{ .compatible = "microsoft,romulus15", },
>  	{ .compatible = "qcom,sc8180x-primus" },
> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },

R/W works for me (tm).. the META version may be (inconclusive) 2605

Konrad

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Dmitry Baryshkov]

On Fri, Jun 27, 2025 at 01:34:56AM +0200, Konrad Dybcio wrote:
> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
> > As reported by Johan, this platform also doesn't currently support
> > updating of the UEFI variables. In preparation to reworking match list
> > for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> > variables.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > ---
> >  drivers/firmware/qcom/qcom_scm.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index dbb77c3f69ddaa931e7faa73911207a83634bda1..27ef2497089e11b5a902d949de2e16b7443a2ca4 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -2005,6 +2005,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> >  	{ .compatible = "microsoft,romulus13", },
> >  	{ .compatible = "microsoft,romulus15", },
> >  	{ .compatible = "qcom,sc8180x-primus" },
> > +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> 
> R/W works for me (tm).. the META version may be (inconclusive) 2605

I'd let you sort this out with Johan. He wrote that variables didn't
persist across reboots.

> 
> Konrad

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Konrad Dybcio]

On 6/27/25 1:34 AM, Konrad Dybcio wrote:
> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
>> As reported by Johan, this platform also doesn't currently support
>> updating of the UEFI variables. In preparation to reworking match list
>> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
>> variables.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>> ---
>>  drivers/firmware/qcom/qcom_scm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>> index dbb77c3f69ddaa931e7faa73911207a83634bda1..27ef2497089e11b5a902d949de2e16b7443a2ca4 100644
>> --- a/drivers/firmware/qcom/qcom_scm.c
>> +++ b/drivers/firmware/qcom/qcom_scm.c
>> @@ -2005,6 +2005,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
>>  	{ .compatible = "microsoft,romulus13", },
>>  	{ .compatible = "microsoft,romulus15", },
>>  	{ .compatible = "qcom,sc8180x-primus" },
>> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> 
> R/W works for me (tm).. the META version may be (inconclusive) 2605

Looked at the wrong SoC META table.. the build date is 05/25/2023

Konrad

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Johan Hovold]

On Fri, Jun 27, 2025 at 01:54:37AM +0200, Konrad Dybcio wrote:
> On 6/27/25 1:34 AM, Konrad Dybcio wrote:
> > On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
> >> As reported by Johan, this platform also doesn't currently support
> >> updating of the UEFI variables. In preparation to reworking match list
> >> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> >> variables.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> >> ---
> >>  drivers/firmware/qcom/qcom_scm.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> >> index dbb77c3f69ddaa931e7faa73911207a83634bda1..27ef2497089e11b5a902d949de2e16b7443a2ca4 100644
> >> --- a/drivers/firmware/qcom/qcom_scm.c
> >> +++ b/drivers/firmware/qcom/qcom_scm.c
> >> @@ -2005,6 +2005,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> >>  	{ .compatible = "microsoft,romulus13", },
> >>  	{ .compatible = "microsoft,romulus15", },
> >>  	{ .compatible = "qcom,sc8180x-primus" },
> >> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> > 
> > R/W works for me (tm).. the META version may be (inconclusive) 2605
> 
> Looked at the wrong SoC META table.. the build date is 05/25/2023

Could be that my machine was not provisioned properly. Do you boot from
UFS or NVMe?

My fw is also older: 01/10/2022.

Johan

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Konrad Dybcio]

On 6/27/25 2:23 PM, Johan Hovold wrote:
> On Fri, Jun 27, 2025 at 01:54:37AM +0200, Konrad Dybcio wrote:
>> On 6/27/25 1:34 AM, Konrad Dybcio wrote:
>>> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
>>>> As reported by Johan, this platform also doesn't currently support
>>>> updating of the UEFI variables. In preparation to reworking match list
>>>> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
>>>> variables.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>> ---
>>>>  drivers/firmware/qcom/qcom_scm.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>>>> index dbb77c3f69ddaa931e7faa73911207a83634bda1..27ef2497089e11b5a902d949de2e16b7443a2ca4 100644
>>>> --- a/drivers/firmware/qcom/qcom_scm.c
>>>> +++ b/drivers/firmware/qcom/qcom_scm.c
>>>> @@ -2005,6 +2005,7 @@ static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
>>>>  	{ .compatible = "microsoft,romulus13", },
>>>>  	{ .compatible = "microsoft,romulus15", },
>>>>  	{ .compatible = "qcom,sc8180x-primus" },
>>>> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
>>>
>>> R/W works for me (tm).. the META version may be (inconclusive) 2605
>>
>> Looked at the wrong SoC META table.. the build date is 05/25/2023
> 
> Could be that my machine was not provisioned properly. Do you boot from
> UFS or NVMe?
> 
> My fw is also older: 01/10/2022.

The machine has UFS, NVME and SPINOR, however the boot log definitely says:

S - Boot Interface: SPI

Konrad

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Johan Hovold]

On Thu, Jun 26, 2025 at 03:54:11PM +0300, Dmitry Baryshkov wrote:
> On Thu, 26 Jun 2025 at 15:51, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Jun 26, 2025 at 02:03:44PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Jun 26, 2025 at 12:04:30PM +0200, Johan Hovold wrote:
> > > > On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> > > > > If efivar implementation doesn't provide write support, then calling
> > > > > efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> > > > > RTC offset) will crash the system. Prevent that by checking that
> > > > > set_variable callback is actually provided and fail with an
> > > > > EFI_WRITE_PROTECTED if it is not.
> > > > >
> > > > > Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
> > > >
> > > > I don't think a fixes tag is warranted here as it currently appears to
> > > > be expected that the callers check if setvar is supported before calling
> > > > this helper (e.g. by calling efivar_supports_writes() as efivarfs does).
> > >
> > > It is not documented as such. So, I think, we'd better not crash the
> > > callers.
> >
> > You need to look at the backstory to determine that before jumping to
> > conclusions (e.g. start by looking at f88814cc2578 ("efi/efivars: Expose
> > RT service availability via efivars abstraction")).
> 
> _documented_. I'll update documentation for efivar_set_variable() in
> the next iteration and add a check to the RTC driver. However I still
> think that this patch is valid.

Still depends on *how* we want to address this.

> > > > So should perhaps be fixed in the RTC driver if we agree that supporting
> > > > read-only offsets is indeed something we want.
> > > >
> > > > Are there any other current user that may possibly benefit from
> > > > something like this?
> > >
> > > efi-pstore comes to my mind.
> >
> > No, that driver is also disabled when efivar_supports_writes() returns
> > false.
> 
> Good.

Ok, so then there are no current drivers that will benefit from your
change, but you may (or may not) need it if you enable RO efivars on
this particular platform. That is, this patch is not actually fixing
anything that is broken currently.

Johan

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Johan Hovold]

On Fri, Jun 27, 2025 at 02:33:27AM +0300, Dmitry Baryshkov wrote:
> On Thu, Jun 26, 2025 at 02:58:52PM +0200, Johan Hovold wrote:
> > On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> > > > On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> > > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > 
> > > > > In preparation to enabling QSEECOM for the platforms rather than
> > > > > individual machines provide a mechanism for the user to override default
> > > > > selection. Allow users to use qcom_scm.qseecom modparam.
> > > > > 
> > > > > Setting it to 'force' will enable QSEECOM even if it disabled or not
> > > > > handled by the allowlist.
> > > > > 
> > > > > Setting it to 'off' will forcibly disable the QSEECOM interface,
> > > > > allowing incompatible machines to function.
> > > > > 
> > > > > Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> > > > > variables read-only.
> > > > > 
> > > > > All other values mean 'auto', trusting the allowlist in the module.
> > > > 
> > > > I don't see the need for this. The kernel should just provide sensible
> > > > defaults.
> > > 
> > > It does provide _defaults_. However with the next commit we mass-enable
> > > QSEECOM for SoC families, which includes untested WoA devices. If the
> > > user observes a misbehaviour of the UEFI vars or any other
> > > QSEECOM-related driver on those platforms, it is much easier to let
> > > users test and workaround UEFI misbehaviour.
> > 
> > You basically know by now which machines supports qseecom and which do
> > not, right (e.g. UFS storage means non-persistent EFI vars)?

Do you have a theory about why on some platforms, like the one you're
currently adding support for, writing UEFI variables does not work?

Can you please include that information in the series so we can consider
alternate routes for replacing the current whitelist with this black and
white thing you're going for.

Is it related to UFS at all, for example?

> > And it's a pretty bad user experience to have people trying to write
> > efivariables when setting up a machine and then spend hours trying to
> > debug why they don't persist after a reboot.
> > 
> > I don't think that's fair to users.
> 
> So, is it a user or a developer, trying to port Linux to a new hardware?
> Also, R/O implementation makes it obvious, that the variables do not
> persist.

A developer enabling support for a new platform can patch the driver and
does not need a command line option.

If you enable it by default, suddenly a bunch of end-users are going to
have to debug why storing efi variables silently fails. That would not
be fair to them.

> > Let whoever brings up a new machine figure this out. It's just one
> > entry, no scaling issues, and we get accurate information (unless
> > Qualcomm, who sits on the documentation, is willing to provide it
> > upfront).
> 
> And that's not really scallable. All other parts of a particular device
> are described by the DT only (that's especially true on the PMIC GLINK
> machines). If we are to support new laptop in e.g. distro kernel, we
> need to provide a DT... and a patch for qcom-scm driver. I'd very much
> prefer to do it other way around: provide a DT and patch qcom-scm if the
> laptop is any way different from other laptops. E.g. we know that all
> X1Elite laptops support R/W EFI variables.

But this is just kicking the can and putting the burden on someone else.
Now a user or distro would need to pass command line parameters after
spending time debugging why efi variable updates do not persist after a
reboot.

If we know with reasonable certainty that all, say X1E, devices works,
then that that's one thing.

But if this series now enables broken EFI variable support on every
other device then I don't think that's ok (even if you provide a command
line parameter that each user now have to pass).

Then I'd rather see a proposal for how to determine which machines
support this or not, information which was not available when this
interface was reverse engineered and where a conservative whitelist
approach made perfect sense.

> Except for X1-CRD, which deserves an entry in the driver.

I think you meant my sc8280xp CRD here.

Johan

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Johan Hovold]

On Fri, Jun 27, 2025 at 02:26:41PM +0200, Konrad Dybcio wrote:
> On 6/27/25 2:23 PM, Johan Hovold wrote:
> > On Fri, Jun 27, 2025 at 01:54:37AM +0200, Konrad Dybcio wrote:
> >> On 6/27/25 1:34 AM, Konrad Dybcio wrote:
> >>> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:

> >>>> As reported by Johan, this platform also doesn't currently support
> >>>> updating of the UEFI variables. In preparation to reworking match list
> >>>> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> >>>> variables.

> >>>> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> >>>
> >>> R/W works for me (tm).. the META version may be (inconclusive) 2605
> >>
> >> Looked at the wrong SoC META table.. the build date is 05/25/2023
> > 
> > Could be that my machine was not provisioned properly. Do you boot from
> > UFS or NVMe?
> > 
> > My fw is also older: 01/10/2022.
> 
> The machine has UFS, NVME and SPINOR, however the boot log definitely says:
> 
> S - Boot Interface: SPI

Mine says:

S - Boot Interface: UFS

Johan

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Dmitry Baryshkov]

On Fri, Jun 27, 2025 at 02:50:26PM +0200, Johan Hovold wrote:
> On Fri, Jun 27, 2025 at 02:26:41PM +0200, Konrad Dybcio wrote:
> > On 6/27/25 2:23 PM, Johan Hovold wrote:
> > > On Fri, Jun 27, 2025 at 01:54:37AM +0200, Konrad Dybcio wrote:
> > >> On 6/27/25 1:34 AM, Konrad Dybcio wrote:
> > >>> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
> 
> > >>>> As reported by Johan, this platform also doesn't currently support
> > >>>> updating of the UEFI variables. In preparation to reworking match list
> > >>>> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> > >>>> variables.
> 
> > >>>> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> > >>>
> > >>> R/W works for me (tm).. the META version may be (inconclusive) 2605
> > >>
> > >> Looked at the wrong SoC META table.. the build date is 05/25/2023
> > > 
> > > Could be that my machine was not provisioned properly. Do you boot from
> > > UFS or NVMe?
> > > 
> > > My fw is also older: 01/10/2022.
> > 
> > The machine has UFS, NVME and SPINOR, however the boot log definitely says:
> > 
> > S - Boot Interface: SPI
> 
> Mine says:
> 
> S - Boot Interface: UFS

Is this META even supported? I think it's recommended to update
firmware to the latest releases.

-- 
With best wishes
Dmitry

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Dmitry Baryshkov]

On Fri, Jun 27, 2025 at 02:46:38PM +0200, Johan Hovold wrote:
> On Fri, Jun 27, 2025 at 02:33:27AM +0300, Dmitry Baryshkov wrote:
> > On Thu, Jun 26, 2025 at 02:58:52PM +0200, Johan Hovold wrote:
> > > On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> > > > On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> > > > > On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> > > > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > 
> > > > > > In preparation to enabling QSEECOM for the platforms rather than
> > > > > > individual machines provide a mechanism for the user to override default
> > > > > > selection. Allow users to use qcom_scm.qseecom modparam.
> > > > > > 
> > > > > > Setting it to 'force' will enable QSEECOM even if it disabled or not
> > > > > > handled by the allowlist.
> > > > > > 
> > > > > > Setting it to 'off' will forcibly disable the QSEECOM interface,
> > > > > > allowing incompatible machines to function.
> > > > > > 
> > > > > > Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> > > > > > variables read-only.
> > > > > > 
> > > > > > All other values mean 'auto', trusting the allowlist in the module.
> > > > > 
> > > > > I don't see the need for this. The kernel should just provide sensible
> > > > > defaults.
> > > > 
> > > > It does provide _defaults_. However with the next commit we mass-enable
> > > > QSEECOM for SoC families, which includes untested WoA devices. If the
> > > > user observes a misbehaviour of the UEFI vars or any other
> > > > QSEECOM-related driver on those platforms, it is much easier to let
> > > > users test and workaround UEFI misbehaviour.
> > > 
> > > You basically know by now which machines supports qseecom and which do
> > > not, right (e.g. UFS storage means non-persistent EFI vars)?
> 
> Do you have a theory about why on some platforms, like the one you're
> currently adding support for, writing UEFI variables does not work?
> 
> Can you please include that information in the series so we can consider
> alternate routes for replacing the current whitelist with this black and
> white thing you're going for.
> 
> Is it related to UFS at all, for example?

Strictly speaking I have no confirmation (yet), but there are two
theories:

- UFS vs SPI-NOR
- a edk2 PCD which controls whether SetVariable commits immediately or
  whether it just buffers data until EBS (or other call).

> 
> > > And it's a pretty bad user experience to have people trying to write
> > > efivariables when setting up a machine and then spend hours trying to
> > > debug why they don't persist after a reboot.
> > > 
> > > I don't think that's fair to users.
> > 
> > So, is it a user or a developer, trying to port Linux to a new hardware?
> > Also, R/O implementation makes it obvious, that the variables do not
> > persist.
> 
> A developer enabling support for a new platform can patch the driver and
> does not need a command line option.

Yes. But it's easier to debug things this way. Consider all ACPI-related
or UEFI-related kernel options that we have.

> 
> If you enable it by default, suddenly a bunch of end-users are going to
> have to debug why storing efi variables silently fails. That would not
> be fair to them.

I'm enabling this only for platforms where all existing devices are
listed in the current whitelist.

> 
> > > Let whoever brings up a new machine figure this out. It's just one
> > > entry, no scaling issues, and we get accurate information (unless
> > > Qualcomm, who sits on the documentation, is willing to provide it
> > > upfront).
> > 
> > And that's not really scallable. All other parts of a particular device
> > are described by the DT only (that's especially true on the PMIC GLINK
> > machines). If we are to support new laptop in e.g. distro kernel, we
> > need to provide a DT... and a patch for qcom-scm driver. I'd very much
> > prefer to do it other way around: provide a DT and patch qcom-scm if the
> > laptop is any way different from other laptops. E.g. we know that all
> > X1Elite laptops support R/W EFI variables.
> 
> But this is just kicking the can and putting the burden on someone else.
> Now a user or distro would need to pass command line parameters after
> spending time debugging why efi variable updates do not persist after a
> reboot.

The original developer for new DTS will have to do that anyway, if
something fails. And once it is done, we can add a quirk for that pure
platform. However the majority of the case can go without extra quirks.
As you can see, all X-Elite / X-Plus and majority of SC8280XP platforms
already are in the whitelist. Once we sort out SC8280XP-CRD issue, all
SC8280XP platforms supported upstream will have an entry in the
allowlist, which means we can convert them to the wildcard + quirks.

> If we know with reasonable certainty that all, say X1E, devices works,
> then that that's one thing.

Yes, we do. You can hand-compare the lists too (I did).

> But if this series now enables broken EFI variable support on every
> other device then I don't think that's ok (even if you provide a command
> line parameter that each user now have to pass).
> 
> Then I'd rather see a proposal for how to determine which machines
> support this or not, information which was not available when this
> interface was reverse engineered and where a conservative whitelist
> approach made perfect sense.

WIP

> 
> > Except for X1-CRD, which deserves an entry in the driver.
> 
> I think you meant my sc8280xp CRD here.

Yes.

> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Dmitry Baryshkov]

On Fri, Jun 27, 2025 at 02:27:46PM +0200, Johan Hovold wrote:
> On Thu, Jun 26, 2025 at 03:54:11PM +0300, Dmitry Baryshkov wrote:
> > On Thu, 26 Jun 2025 at 15:51, Johan Hovold <johan@kernel.org> wrote:
> > >
> > > On Thu, Jun 26, 2025 at 02:03:44PM +0300, Dmitry Baryshkov wrote:
> > > > On Thu, Jun 26, 2025 at 12:04:30PM +0200, Johan Hovold wrote:
> > > > > On Wed, Jun 25, 2025 at 01:53:20AM +0300, Dmitry Baryshkov wrote:
> > > > > > If efivar implementation doesn't provide write support, then calling
> > > > > > efivar_set_variable() (e.g. when PM8xxx RTC driver tries to update the
> > > > > > RTC offset) will crash the system. Prevent that by checking that
> > > > > > set_variable callback is actually provided and fail with an
> > > > > > EFI_WRITE_PROTECTED if it is not.
> > > > > >
> > > > > > Fixes: 472831d4c4b2 ("efi: vars: Add thin wrapper around EFI get/set variable interface")
> > > > >
> > > > > I don't think a fixes tag is warranted here as it currently appears to
> > > > > be expected that the callers check if setvar is supported before calling
> > > > > this helper (e.g. by calling efivar_supports_writes() as efivarfs does).
> > > >
> > > > It is not documented as such. So, I think, we'd better not crash the
> > > > callers.
> > >
> > > You need to look at the backstory to determine that before jumping to
> > > conclusions (e.g. start by looking at f88814cc2578 ("efi/efivars: Expose
> > > RT service availability via efivars abstraction")).
> > 
> > _documented_. I'll update documentation for efivar_set_variable() in
> > the next iteration and add a check to the RTC driver. However I still
> > think that this patch is valid.
> 
> Still depends on *how* we want to address this.

I'd prefer to address it in both places.

> > > > > So should perhaps be fixed in the RTC driver if we agree that supporting
> > > > > read-only offsets is indeed something we want.
> > > > >
> > > > > Are there any other current user that may possibly benefit from
> > > > > something like this?
> > > >
> > > > efi-pstore comes to my mind.
> > >
> > > No, that driver is also disabled when efivar_supports_writes() returns
> > > false.
> > 
> > Good.
> 
> Ok, so then there are no current drivers that will benefit from your
> change, but you may (or may not) need it if you enable RO efivars on
> this particular platform. That is, this patch is not actually fixing
> anything that is broken currently.

I'd leave that to a discretion of EFI / EFI vars maintainers. RTC driver
definitely is broken in its current state.

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/8] efi: efivars: don't crash in efivar_set_variable{,_locked} in r/o case

[Johan Hovold]

On Sat, Jun 28, 2025 at 06:05:51PM +0300, Dmitry Baryshkov wrote:
> On Fri, Jun 27, 2025 at 02:27:46PM +0200, Johan Hovold wrote:

> > Ok, so then there are no current drivers that will benefit from your
> > change, but you may (or may not) need it if you enable RO efivars on
> > this particular platform. That is, this patch is not actually fixing
> > anything that is broken currently.
> 
> I'd leave that to a discretion of EFI / EFI vars maintainers. RTC driver
> definitely is broken in its current state.

Again, no. We only need this when you start enabling RO efivars on
Qualcomm platforms. So you're not fixing anything that is currently
broken.

Johan

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Johan Hovold]

On Sat, Jun 28, 2025 at 05:50:49PM +0300, Dmitry Baryshkov wrote:
> On Fri, Jun 27, 2025 at 02:50:26PM +0200, Johan Hovold wrote:
> > On Fri, Jun 27, 2025 at 02:26:41PM +0200, Konrad Dybcio wrote:
> > > On 6/27/25 2:23 PM, Johan Hovold wrote:
> > > > On Fri, Jun 27, 2025 at 01:54:37AM +0200, Konrad Dybcio wrote:
> > > >> On 6/27/25 1:34 AM, Konrad Dybcio wrote:
> > > >>> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
> > 
> > > >>>> As reported by Johan, this platform also doesn't currently support
> > > >>>> updating of the UEFI variables. In preparation to reworking match list
> > > >>>> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> > > >>>> variables.
> > 
> > > >>>> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> > > >>>
> > > >>> R/W works for me (tm).. the META version may be (inconclusive) 2605
> > > >>
> > > >> Looked at the wrong SoC META table.. the build date is 05/25/2023
> > > > 
> > > > Could be that my machine was not provisioned properly. Do you boot from
> > > > UFS or NVMe?
> > > > 
> > > > My fw is also older: 01/10/2022.
> > > 
> > > The machine has UFS, NVME and SPINOR, however the boot log definitely says:
> > > 
> > > S - Boot Interface: SPI
> > 
> > Mine says:
> > 
> > S - Boot Interface: UFS
> 
> Is this META even supported? I think it's recommended to update
> firmware to the latest releases.

It most likely has nothing to do with the meta version, but whether you
boot from SPI-NOR or UFS.

Johan

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Johan Hovold]

On Sat, Jun 28, 2025 at 06:03:40PM +0300, Dmitry Baryshkov wrote:
> On Fri, Jun 27, 2025 at 02:46:38PM +0200, Johan Hovold wrote:
> > On Fri, Jun 27, 2025 at 02:33:27AM +0300, Dmitry Baryshkov wrote:
> > > On Thu, Jun 26, 2025 at 02:58:52PM +0200, Johan Hovold wrote:
> > > > On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> > > > > On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:

> > > > You basically know by now which machines supports qseecom and which do
> > > > not, right (e.g. UFS storage means non-persistent EFI vars)?
> > 
> > Do you have a theory about why on some platforms, like the one you're
> > currently adding support for, writing UEFI variables does not work?
> > 
> > Can you please include that information in the series so we can consider
> > alternate routes for replacing the current whitelist with this black and
> > white thing you're going for.
> > 
> > Is it related to UFS at all, for example?
> 
> Strictly speaking I have no confirmation (yet), but there are two
> theories:
> 
> - UFS vs SPI-NOR

Someone with time and the sc8280xp and x1e CRDs should be able to set
them up for booting from either UFS or SPI-NOR and see if that makes a
difference to confirm this.

So far my sc8280xp CRD with UFS fails, while Konrad's work with SPI-NOR
(NVMe).

My x1e CRD works but also boots from SPI-NOR (NVMe).

The Yoga C630 booting from UFS is also known to fail.

> - a edk2 PCD which controls whether SetVariable commits immediately or
>   whether it just buffers data until EBS (or other call).
> 
> > 
> > > > And it's a pretty bad user experience to have people trying to write
> > > > efivariables when setting up a machine and then spend hours trying to
> > > > debug why they don't persist after a reboot.
> > > > 
> > > > I don't think that's fair to users.
> > > 
> > > So, is it a user or a developer, trying to port Linux to a new hardware?
> > > Also, R/O implementation makes it obvious, that the variables do not
> > > persist.
> > 
> > A developer enabling support for a new platform can patch the driver and
> > does not need a command line option.
> 
> Yes. But it's easier to debug things this way. Consider all ACPI-related
> or UEFI-related kernel options that we have.

That's because there is a common kernel implementation used across a
host of fw implementations.

Here it's just Qualcomm doing something funny that affects their own
platforms. We should be able to figure this out without forcing users or
distros to pass command line parameters.

> > If you enable it by default, suddenly a bunch of end-users are going to
> > have to debug why storing efi variables silently fails. That would not
> > be fair to them.
> 
> I'm enabling this only for platforms where all existing devices are
> listed in the current whitelist.

Do we know if there are any sc8280xp or x1e machines that boot off UFS?

If not (even with the exception of the CRDs) then it should be fine to
just whitelist the SoCs without any command line parameters.

> > > > Let whoever brings up a new machine figure this out. It's just one
> > > > entry, no scaling issues, and we get accurate information (unless
> > > > Qualcomm, who sits on the documentation, is willing to provide it
> > > > upfront).
> > > 
> > > And that's not really scallable. All other parts of a particular device
> > > are described by the DT only (that's especially true on the PMIC GLINK
> > > machines). If we are to support new laptop in e.g. distro kernel, we
> > > need to provide a DT... and a patch for qcom-scm driver. I'd very much
> > > prefer to do it other way around: provide a DT and patch qcom-scm if the
> > > laptop is any way different from other laptops. E.g. we know that all
> > > X1Elite laptops support R/W EFI variables.
> > 
> > But this is just kicking the can and putting the burden on someone else.
> > Now a user or distro would need to pass command line parameters after
> > spending time debugging why efi variable updates do not persist after a
> > reboot.
> 
> The original developer for new DTS will have to do that anyway, if
> something fails. And once it is done, we can add a quirk for that pure
> platform. However the majority of the case can go without extra quirks.

Adding to a blacklist is bound to be overlooked, while adding to a
whitelist is not.

> As you can see, all X-Elite / X-Plus and majority of SC8280XP platforms
> already are in the whitelist. Once we sort out SC8280XP-CRD issue, all
> SC8280XP platforms supported upstream will have an entry in the
> allowlist, which means we can convert them to the wildcard + quirks.

I'd rather see you get to the bottom of the UFS boot issue and whether
there is some way to determine this programmatically.

> > If we know with reasonable certainty that all, say X1E, devices works,
> > then that that's one thing.
> 
> Yes, we do. You can hand-compare the lists too (I did).

If everything that's currently upstream boots from NVMe that may not
necessarily mean it works for devices using UFS.

> > But if this series now enables broken EFI variable support on every
> > other device then I don't think that's ok (even if you provide a command
> > line parameter that each user now have to pass).
> > 
> > Then I'd rather see a proposal for how to determine which machines
> > support this or not, information which was not available when this
> > interface was reverse engineered and where a conservative whitelist
> > approach made perfect sense.
> 
> WIP

Good. We can manage with adding new entries for a while still while you
guys at Qualcomm work this out.

Johan

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Dmitry Baryshkov]

On Mon, Jun 30, 2025 at 02:42:06PM +0200, Johan Hovold wrote:
> On Sat, Jun 28, 2025 at 06:03:40PM +0300, Dmitry Baryshkov wrote:
> > On Fri, Jun 27, 2025 at 02:46:38PM +0200, Johan Hovold wrote:
> > > On Fri, Jun 27, 2025 at 02:33:27AM +0300, Dmitry Baryshkov wrote:
> > > > On Thu, Jun 26, 2025 at 02:58:52PM +0200, Johan Hovold wrote:
> > > > > On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> > > > > > On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> 
> > > > > You basically know by now which machines supports qseecom and which do
> > > > > not, right (e.g. UFS storage means non-persistent EFI vars)?
> > > 
> > > Do you have a theory about why on some platforms, like the one you're
> > > currently adding support for, writing UEFI variables does not work?
> > > 
> > > Can you please include that information in the series so we can consider
> > > alternate routes for replacing the current whitelist with this black and
> > > white thing you're going for.
> > > 
> > > Is it related to UFS at all, for example?
> > 
> > Strictly speaking I have no confirmation (yet), but there are two
> > theories:
> > 
> > - UFS vs SPI-NOR
> 
> Someone with time and the sc8280xp and x1e CRDs should be able to set
> them up for booting from either UFS or SPI-NOR and see if that makes a
> difference to confirm this.
> 
> So far my sc8280xp CRD with UFS fails, while Konrad's work with SPI-NOR
> (NVMe).
> 
> My x1e CRD works but also boots from SPI-NOR (NVMe).
> 
> The Yoga C630 booting from UFS is also known to fail.
> 
> > - a edk2 PCD which controls whether SetVariable commits immediately or
> >   whether it just buffers data until EBS (or other call).
> > 
> > > 
> > > > > And it's a pretty bad user experience to have people trying to write
> > > > > efivariables when setting up a machine and then spend hours trying to
> > > > > debug why they don't persist after a reboot.
> > > > > 
> > > > > I don't think that's fair to users.
> > > > 
> > > > So, is it a user or a developer, trying to port Linux to a new hardware?
> > > > Also, R/O implementation makes it obvious, that the variables do not
> > > > persist.
> > > 
> > > A developer enabling support for a new platform can patch the driver and
> > > does not need a command line option.
> > 
> > Yes. But it's easier to debug things this way. Consider all ACPI-related
> > or UEFI-related kernel options that we have.
> 
> That's because there is a common kernel implementation used across a
> host of fw implementations.
> 
> Here it's just Qualcomm doing something funny that affects their own
> platforms. We should be able to figure this out without forcing users or
> distros to pass command line parameters.

This is not intended for the normal working course, but for the initial
bringup / nailing out issues after the bringup (e.g. after firmware
upgrade).

> 
> > > If you enable it by default, suddenly a bunch of end-users are going to
> > > have to debug why storing efi variables silently fails. That would not
> > > be fair to them.
> > 
> > I'm enabling this only for platforms where all existing devices are
> > listed in the current whitelist.
> 
> Do we know if there are any sc8280xp or x1e machines that boot off UFS?
> 
> If not (even with the exception of the CRDs) then it should be fine to
> just whitelist the SoCs without any command line parameters.

I'm not aware of such platforms.

> 
> > > > > Let whoever brings up a new machine figure this out. It's just one
> > > > > entry, no scaling issues, and we get accurate information (unless
> > > > > Qualcomm, who sits on the documentation, is willing to provide it
> > > > > upfront).
> > > > 
> > > > And that's not really scallable. All other parts of a particular device
> > > > are described by the DT only (that's especially true on the PMIC GLINK
> > > > machines). If we are to support new laptop in e.g. distro kernel, we
> > > > need to provide a DT... and a patch for qcom-scm driver. I'd very much
> > > > prefer to do it other way around: provide a DT and patch qcom-scm if the
> > > > laptop is any way different from other laptops. E.g. we know that all
> > > > X1Elite laptops support R/W EFI variables.
> > > 
> > > But this is just kicking the can and putting the burden on someone else.
> > > Now a user or distro would need to pass command line parameters after
> > > spending time debugging why efi variable updates do not persist after a
> > > reboot.
> > 
> > The original developer for new DTS will have to do that anyway, if
> > something fails. And once it is done, we can add a quirk for that pure
> > platform. However the majority of the case can go without extra quirks.
> 
> Adding to a blacklist is bound to be overlooked, while adding to a
> whitelist is not.

You can't overlook it since it is required as a part of almost any
distro setup - point UEFI boot sequence to your new bootloader entry.

> 
> > As you can see, all X-Elite / X-Plus and majority of SC8280XP platforms
> > already are in the whitelist. Once we sort out SC8280XP-CRD issue, all
> > SC8280XP platforms supported upstream will have an entry in the
> > allowlist, which means we can convert them to the wildcard + quirks.
> 
> I'd rather see you get to the bottom of the UFS boot issue and whether
> there is some way to determine this programmatically.

I don't see a good way to do that - UFS might be probed very late, it
might be unused for the boot at all, etc.

> 
> > > If we know with reasonable certainty that all, say X1E, devices works,
> > > then that that's one thing.
> > 
> > Yes, we do. You can hand-compare the lists too (I did).
> 
> If everything that's currently upstream boots from NVMe that may not
> necessarily mean it works for devices using UFS.

And? I don't care that much about theoretical devices here.

> 
> > > But if this series now enables broken EFI variable support on every
> > > other device then I don't think that's ok (even if you provide a command
> > > line parameter that each user now have to pass).
> > > 
> > > Then I'd rather see a proposal for how to determine which machines
> > > support this or not, information which was not available when this
> > > interface was reverse engineered and where a conservative whitelist
> > > approach made perfect sense.
> > 
> > WIP
> 
> Good. We can manage with adding new entries for a while still while you
> guys at Qualcomm work this out.

You (we) guys at Linaro could have figured that out too ;-)

-- 
With best wishes
Dmitry

Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Johan Hovold]

On Tue, Jul 01, 2025 at 02:10:49PM +0300, Dmitry Baryshkov wrote:
> On Mon, Jun 30, 2025 at 02:42:06PM +0200, Johan Hovold wrote:

> > Here it's just Qualcomm doing something funny that affects their own
> > platforms. We should be able to figure this out without forcing users or
> > distros to pass command line parameters.
> 
> This is not intended for the normal working course, but for the initial
> bringup / nailing out issues after the bringup (e.g. after firmware
> upgrade).

And for that you do not need a module parameter either.

> > Do we know if there are any sc8280xp or x1e machines that boot off UFS?
> > 
> > If not (even with the exception of the CRDs) then it should be fine to
> > just whitelist the SoCs without any command line parameters.
> 
> I'm not aware of such platforms.

Then go for it.

> > Adding to a blacklist is bound to be overlooked, while adding to a
> > whitelist is not.
> 
> You can't overlook it since it is required as a part of almost any
> distro setup - point UEFI boot sequence to your new bootloader entry.

The distros don't do bring ups of these machines themselves.

> > I'd rather see you get to the bottom of the UFS boot issue and whether
> > there is some way to determine this programmatically.
> 
> I don't see a good way to do that - UFS might be probed very late, it
> might be unused for the boot at all, etc.

How about asking the Qualcomm firmware team?

Again, there's no rush here. Whitelisting is perfectly fine until then.

> > If everything that's currently upstream boots from NVMe that may not
> > necessarily mean it works for devices using UFS.
> 
> And? I don't care that much about theoretical devices here.

It's not theoretical. We know that the UEFI vars on the CRDs are not
persistent when booting off UFS. Not to mention your Yoga.

> > > > But if this series now enables broken EFI variable support on every
> > > > other device then I don't think that's ok (even if you provide a command
> > > > line parameter that each user now have to pass).
> > > > 
> > > > Then I'd rather see a proposal for how to determine which machines
> > > > support this or not, information which was not available when this
> > > > interface was reverse engineered and where a conservative whitelist
> > > > approach made perfect sense.
> > > 
> > > WIP
> > 
> > Good. We can manage with adding new entries for a while still while you
> > guys at Qualcomm work this out.
> 
> You (we) guys at Linaro could have figured that out too ;-)

Linaro relies on Qualcomm to provide details on things like this. As you
know.

Johan

Re: [PATCH v4 5/8] firmware; qcom: scm: enable QSEECOM on SC8280XP CRD

[Bjorn Andersson]

On Mon, Jun 30, 2025 at 02:16:58PM +0200, Johan Hovold wrote:
> On Sat, Jun 28, 2025 at 05:50:49PM +0300, Dmitry Baryshkov wrote:
> > On Fri, Jun 27, 2025 at 02:50:26PM +0200, Johan Hovold wrote:
> > > On Fri, Jun 27, 2025 at 02:26:41PM +0200, Konrad Dybcio wrote:
> > > > On 6/27/25 2:23 PM, Johan Hovold wrote:
> > > > > On Fri, Jun 27, 2025 at 01:54:37AM +0200, Konrad Dybcio wrote:
> > > > >> On 6/27/25 1:34 AM, Konrad Dybcio wrote:
> > > > >>> On 6/25/25 12:53 AM, Dmitry Baryshkov wrote:
> > > 
> > > > >>>> As reported by Johan, this platform also doesn't currently support
> > > > >>>> updating of the UEFI variables. In preparation to reworking match list
> > > > >>>> for QSEECOM mark this platform as supporting QSEECOM with R/O UEFI
> > > > >>>> variables.
> > > 
> > > > >>>> +	{ .compatible = "qcom,sc8280xp-crd", .data = &qcom_qseecom_ro_uefi, },
> > > > >>>
> > > > >>> R/W works for me (tm).. the META version may be (inconclusive) 2605
> > > > >>
> > > > >> Looked at the wrong SoC META table.. the build date is 05/25/2023
> > > > > 
> > > > > Could be that my machine was not provisioned properly. Do you boot from
> > > > > UFS or NVMe?
> > > > > 
> > > > > My fw is also older: 01/10/2022.
> > > > 
> > > > The machine has UFS, NVME and SPINOR, however the boot log definitely says:
> > > > 
> > > > S - Boot Interface: SPI
> > > 
> > > Mine says:
> > > 
> > > S - Boot Interface: UFS
> > 
> > Is this META even supported? I think it's recommended to update
> > firmware to the latest releases.
> 
> It most likely has nothing to do with the meta version, but whether you
> boot from SPI-NOR or UFS.
> 

It would make sense that the UFS firmware then acts as the firwmare on
any other UFS-based device relies on UFS for EFI variable storage -
using yet to be implemented mechanisms. This would explain why you don't
have persistent storage on your device after ExitBootServices...

Regards,
Bjorn

> Johan

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

[Bjorn Andersson]

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.

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.



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).

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
> 

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