| Message ID | 20241103-rework-qseecom-v1-2-1d75d4eedc1e@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: scm: rework allowlist to be more scalable | expand |
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
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?
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
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
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).
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
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(-)