[PATCH 0/2] firmware: scm: rework allowlist to be more scalable

[PATCH 0/2] firmware: scm: rework allowlist to be more scalable

[Dmitry Baryshkov]

Listing individual devices in the qcom_scm_qseecom_allowlist table
doesn't really scale. For each new device we have to add both DT and an
entry in the table. This way handling new devices by distributions
becomes more complicated: it's not enough to add new DT, the qcom_scm
also has to be patched.

Replace a machine-based allowlist with the platform-based table. If a
particular device has buggy or incompatible firmware, it still can be
disabled in the table. And while the patch to disable it is pending,
make it possible to use qcom_scm.qseecom kernel argument to forcebly
enable or disable usage of QSEECOM.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (2):
      firmware: qcom: scm: add modparam to control QSEECOM enablement
      firmware: qcom: scm: rework QSEECOM allowlist

 drivers/firmware/qcom/qcom_scm.c | 54 ++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 18 deletions(-)
---
base-commit: c88416ba074a8913cf6d61b789dd834bbca6681c
change-id: 20241103-rework-qseecom-a6c8ceffc424

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/2] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Dmitry Baryshkov]

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 forcebly disable the QSEECOM
interface, allowing incompatible machines to function. All other values
mean 'auto', trusting the allowlist in the module.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/firmware/qcom/qcom_scm.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 47853860422525da79a249824afb45f6801151fd..9fed03d0a4b7e5709edf2db9a58b5326301008b4 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1737,9 +1737,14 @@ int qcom_scm_qseecom_app_send(u32 app_id, void *req, size_t req_size,
 }
 EXPORT_SYMBOL_GPL(qcom_scm_qseecom_app_send);
 
+static char *qseecom = "auto";
+MODULE_PARM_DESC(qseecom, "Enable QSEECOM interface (force | 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 = "dell,xps13-9345" },
@@ -1756,11 +1761,21 @@ 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(struct device *scm_dev)
 {
 	struct device_node *np;
 	bool match;
 
+	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: forcebly enabled\n");
+		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;
@@ -1802,7 +1817,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(scm->dev)) {
 		dev_info(scm->dev, "qseecom: untested machine, skipping\n");
 		return 0;
 	}

-- 
2.39.5

Re: [PATCH 1/2] firmware: qcom: scm: add modparam to control QSEECOM enablement

[Konrad Dybcio]

On 3.11.2024 4:37 PM, Dmitry Baryshkov wrote:
> 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 forcebly disable the QSEECOM
> interface, allowing incompatible machines to function. All other values
> mean 'auto', trusting the allowlist in the module.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

[...]

>  
> -static bool qcom_scm_qseecom_machine_is_allowed(void)
> +static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev)
>  {
>  	struct device_node *np;
>  	bool match;
>  
> +	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: forcebly enabled\n");

forcibly

may also be useful to say "by modparam" here as well

Konrad

[PATCH 2/2] firmware: qcom: scm: rework QSEECOM allowlist

[Dmitry Baryshkov]

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>
---
 drivers/firmware/qcom/qcom_scm.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
index 9fed03d0a4b7e5709edf2db9a58b5326301008b4..6f70fbb0ddfbf88542ff2b3ed2bc372c2f3ce9eb 100644
--- a/drivers/firmware/qcom/qcom_scm.c
+++ b/drivers/firmware/qcom/qcom_scm.c
@@ -1743,28 +1743,23 @@ 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 (void *)false.
  */
 static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
-	{ .compatible = "dell,xps13-9345" },
-	{ .compatible = "lenovo,flex-5g" },
-	{ .compatible = "lenovo,thinkpad-t14s" },
-	{ .compatible = "lenovo,thinkpad-x13s", },
-	{ .compatible = "lenovo,yoga-slim7x" },
-	{ .compatible = "microsoft,arcata", },
-	{ .compatible = "microsoft,romulus13", },
-	{ .compatible = "microsoft,romulus15", },
-	{ .compatible = "qcom,sc8180x-primus" },
-	{ .compatible = "qcom,x1e80100-crd" },
-	{ .compatible = "qcom,x1e80100-qcp" },
+	{ .compatible = "qcom,sc8180x", .data = (void *)true },
+	{ .compatible = "qcom,sc8280xp", .data = (void *)true },
+	{ .compatible = "qcom,x1e80100", .data = (void *)true },
 	{ }
 };
 
 static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev)
 {
 	struct device_node *np;
-	bool match;
+	const struct of_device_id *match;
 
 	if (!strcmp(qseecom, "off")) {
 		dev_info(scm_dev, "qseecom: disabled by modparam\n");
@@ -1783,7 +1778,17 @@ 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);
 
-	return match;
+	if (!match) {
+		dev_info(scm_dev, "qseecom: untested machine, skipping\n");
+		return false;
+	}
+
+	if (!match->data) {
+		dev_info(scm_dev, "qseecom: disabled by the allowlist\n");
+		return false;
+	}
+
+	return true;
 }
 
 static void qcom_scm_qseecom_free(void *data)
@@ -1817,10 +1822,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)) {
-		dev_info(scm->dev, "qseecom: untested machine, skipping\n");
+	if (!qcom_scm_qseecom_machine_is_allowed(scm->dev))
 		return 0;
-	}
 
 	/*
 	 * Set up QSEECOM interface device. All application clients will be

-- 
2.39.5

Re: [PATCH 2/2] firmware: qcom: scm: rework QSEECOM allowlist

[Konrad Dybcio]

On 3.11.2024 4:37 PM, Dmitry Baryshkov wrote:
> 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>
> ---
>  drivers/firmware/qcom/qcom_scm.c | 37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 9fed03d0a4b7e5709edf2db9a58b5326301008b4..6f70fbb0ddfbf88542ff2b3ed2bc372c2f3ce9eb 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1743,28 +1743,23 @@ 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
                                                       ^ a

> + * data to (void *)false.
>   */
>  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> -	{ .compatible = "dell,xps13-9345" },
> -	{ .compatible = "lenovo,flex-5g" },
> -	{ .compatible = "lenovo,thinkpad-t14s" },
> -	{ .compatible = "lenovo,thinkpad-x13s", },
> -	{ .compatible = "lenovo,yoga-slim7x" },
> -	{ .compatible = "microsoft,arcata", },
> -	{ .compatible = "microsoft,romulus13", },
> -	{ .compatible = "microsoft,romulus15", },
> -	{ .compatible = "qcom,sc8180x-primus" },
> -	{ .compatible = "qcom,x1e80100-crd" },
> -	{ .compatible = "qcom,x1e80100-qcp" },
> +	{ .compatible = "qcom,sc8180x", .data = (void *)true },
> +	{ .compatible = "qcom,sc8280xp", .data = (void *)true },
> +	{ .compatible = "qcom,x1e80100", .data = (void *)true },
>  	{ }
>  };

+ Steev I think you had some unhappy machine

And maybe 8180 Primus?

>  
>  static bool qcom_scm_qseecom_machine_is_allowed(struct device *scm_dev)
>  {
>  	struct device_node *np;
> -	bool match;
> +	const struct of_device_id *match;

Reverse-Christmas-tree?

Konrad

Re: [PATCH 2/2] firmware: qcom: scm: rework QSEECOM allowlist

[Dmitry Baryshkov]

On Mon, 4 Nov 2024 at 11:24, Konrad Dybcio
<konrad.dybcio@oss.qualcomm.com> wrote:
>
> On 3.11.2024 4:37 PM, Dmitry Baryshkov wrote:
> > 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>
> > ---
> >  drivers/firmware/qcom/qcom_scm.c | 37 ++++++++++++++++++++-----------------
> >  1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> > index 9fed03d0a4b7e5709edf2db9a58b5326301008b4..6f70fbb0ddfbf88542ff2b3ed2bc372c2f3ce9eb 100644
> > --- a/drivers/firmware/qcom/qcom_scm.c
> > +++ b/drivers/firmware/qcom/qcom_scm.c
> > @@ -1743,28 +1743,23 @@ 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
>                                                        ^ a
>
> > + * data to (void *)false.
> >   */
> >  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> > -     { .compatible = "dell,xps13-9345" },
> > -     { .compatible = "lenovo,flex-5g" },
> > -     { .compatible = "lenovo,thinkpad-t14s" },
> > -     { .compatible = "lenovo,thinkpad-x13s", },
> > -     { .compatible = "lenovo,yoga-slim7x" },
> > -     { .compatible = "microsoft,arcata", },
> > -     { .compatible = "microsoft,romulus13", },
> > -     { .compatible = "microsoft,romulus15", },
> > -     { .compatible = "qcom,sc8180x-primus" },
> > -     { .compatible = "qcom,x1e80100-crd" },
> > -     { .compatible = "qcom,x1e80100-qcp" },
> > +     { .compatible = "qcom,sc8180x", .data = (void *)true },
> > +     { .compatible = "qcom,sc8280xp", .data = (void *)true },
> > +     { .compatible = "qcom,x1e80100", .data = (void *)true },
> >       { }
> >  };
>
> + Steev I think you had some unhappy machine
>
> And maybe 8180 Primus?

I don't think I understand this comment, could you please explain?

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] firmware: qcom: scm: rework QSEECOM allowlist

[Konrad Dybcio]

On 4.11.2024 12:34 PM, Dmitry Baryshkov wrote:
> On Mon, 4 Nov 2024 at 11:24, Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 3.11.2024 4:37 PM, Dmitry Baryshkov wrote:
>>> 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>
>>> ---
>>>  drivers/firmware/qcom/qcom_scm.c | 37 ++++++++++++++++++++-----------------
>>>  1 file changed, 20 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
>>> index 9fed03d0a4b7e5709edf2db9a58b5326301008b4..6f70fbb0ddfbf88542ff2b3ed2bc372c2f3ce9eb 100644
>>> --- a/drivers/firmware/qcom/qcom_scm.c
>>> +++ b/drivers/firmware/qcom/qcom_scm.c
>>> @@ -1743,28 +1743,23 @@ 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
>>                                                        ^ a
>>
>>> + * data to (void *)false.
>>>   */
>>>  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
>>> -     { .compatible = "dell,xps13-9345" },
>>> -     { .compatible = "lenovo,flex-5g" },
>>> -     { .compatible = "lenovo,thinkpad-t14s" },
>>> -     { .compatible = "lenovo,thinkpad-x13s", },
>>> -     { .compatible = "lenovo,yoga-slim7x" },
>>> -     { .compatible = "microsoft,arcata", },
>>> -     { .compatible = "microsoft,romulus13", },
>>> -     { .compatible = "microsoft,romulus15", },
>>> -     { .compatible = "qcom,sc8180x-primus" },
>>> -     { .compatible = "qcom,x1e80100-crd" },
>>> -     { .compatible = "qcom,x1e80100-qcp" },
>>> +     { .compatible = "qcom,sc8180x", .data = (void *)true },
>>> +     { .compatible = "qcom,sc8280xp", .data = (void *)true },
>>> +     { .compatible = "qcom,x1e80100", .data = (void *)true },
>>>       { }
>>>  };
>>
>> + Steev I think you had some unhappy machine
>>
>> And maybe 8180 Primus?
> 
> I don't think I understand this comment, could you please explain?

"maybe 8180-primus had some issues, too"

Konrad

Re: [PATCH 2/2] firmware: qcom: scm: rework QSEECOM allowlist

[Johan Hovold]

On Mon, Nov 04, 2024 at 12:23:57PM +0100, Konrad Dybcio wrote:
> On 3.11.2024 4:37 PM, Dmitry Baryshkov wrote:

> >  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> > -	{ .compatible = "dell,xps13-9345" },
> > -	{ .compatible = "lenovo,flex-5g" },
> > -	{ .compatible = "lenovo,thinkpad-t14s" },
> > -	{ .compatible = "lenovo,thinkpad-x13s", },
> > -	{ .compatible = "lenovo,yoga-slim7x" },
> > -	{ .compatible = "microsoft,arcata", },
> > -	{ .compatible = "microsoft,romulus13", },
> > -	{ .compatible = "microsoft,romulus15", },
> > -	{ .compatible = "qcom,sc8180x-primus" },
> > -	{ .compatible = "qcom,x1e80100-crd" },
> > -	{ .compatible = "qcom,x1e80100-qcp" },
> > +	{ .compatible = "qcom,sc8180x", .data = (void *)true },
> > +	{ .compatible = "qcom,sc8280xp", .data = (void *)true },
> > +	{ .compatible = "qcom,x1e80100", .data = (void *)true },
> >  	{ }
> >  };
> 
> + Steev I think you had some unhappy machine
> 
> And maybe 8180 Primus?

I have a sc8280xp crd here where variables can only be read, not stored
(e.g. similar to the Lenovo Yoga C630). In it's current configuration
the machine boots from UFS and this could possibly be related to how it
has been provisioned, but this is the reason why "qcom,sc8280xp-crd" is
not already in the above list.

Johan

Re: [PATCH 2/2] firmware: qcom: scm: rework QSEECOM allowlist

[Dmitry Baryshkov]

On Mon, Nov 04, 2024 at 01:15:41PM +0100, Johan Hovold wrote:
> On Mon, Nov 04, 2024 at 12:23:57PM +0100, Konrad Dybcio wrote:
> > On 3.11.2024 4:37 PM, Dmitry Baryshkov wrote:
> 
> > >  static const struct of_device_id qcom_scm_qseecom_allowlist[] __maybe_unused = {
> > > -	{ .compatible = "dell,xps13-9345" },
> > > -	{ .compatible = "lenovo,flex-5g" },
> > > -	{ .compatible = "lenovo,thinkpad-t14s" },
> > > -	{ .compatible = "lenovo,thinkpad-x13s", },
> > > -	{ .compatible = "lenovo,yoga-slim7x" },
> > > -	{ .compatible = "microsoft,arcata", },
> > > -	{ .compatible = "microsoft,romulus13", },
> > > -	{ .compatible = "microsoft,romulus15", },
> > > -	{ .compatible = "qcom,sc8180x-primus" },
> > > -	{ .compatible = "qcom,x1e80100-crd" },
> > > -	{ .compatible = "qcom,x1e80100-qcp" },
> > > +	{ .compatible = "qcom,sc8180x", .data = (void *)true },
> > > +	{ .compatible = "qcom,sc8280xp", .data = (void *)true },
> > > +	{ .compatible = "qcom,x1e80100", .data = (void *)true },
> > >  	{ }
> > >  };
> > 
> > + Steev I think you had some unhappy machine
> > 
> > And maybe 8180 Primus?
> 
> I have a sc8280xp crd here where variables can only be read, not stored
> (e.g. similar to the Lenovo Yoga C630). In it's current configuration
> the machine boots from UFS and this could possibly be related to how it
> has been provisioned, but this is the reason why "qcom,sc8280xp-crd" is
> not already in the above list.

Ok, so we need to add RO support first. Good point (that was pending for
c630 too, as you remember).

-- 
With best wishes
Dmitry