| Message ID | 20230828192507.117334-1-bartosz.golaszewski@linaro.org |
|---|---|
| Headers | show |
| Series | arm64: qcom: add and enable SHM Bridge support | expand |
On 28/08/2023 21:24, Bartosz Golaszewski wrote: > For easier maintenance order the included headers in qcom_scm.c > alphabetically. I assume they are all needed. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 28/08/2023 21:24, Bartosz Golaszewski wrote: > Checking for the availability of SCM bridge can happen from any context. > It's only by chance that we haven't run into concurrency issues but with > the upcoming SHM Bridge driver that will be initiated at the same > initcall level, we need to assure the assignment and readback of the > __scm pointer is atomic. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/firmware/qcom_scm.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index 980fcfa20b9f..422de70faff8 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > */ > bool qcom_scm_is_available(void) > { > - return !!__scm; > + return !!READ_ONCE(__scm); > } > EXPORT_SYMBOL(qcom_scm_is_available); > > @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev) > if (ret) > return ret; > > - __scm = scm; > - __scm->dev = &pdev->dev; > + scm->dev = &pdev->dev; > + WRITE_ONCE(__scm, scm); Your re-ordering is not effective here, I think. I don't understand it's purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it can be reordered: "compiler is also forbidden from reordering successive instances of READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder other stuff. "Ensuring that the compiler does not fold, spindle, or otherwise mutilate accesses that either do not require ordering or that interact" <- which means you do not require ordering here. Best regards, Krzysztof
On Tue, 29 Aug 2023 at 09:59, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 28/08/2023 21:24, Bartosz Golaszewski wrote: > > Checking for the availability of SCM bridge can happen from any context. > > It's only by chance that we haven't run into concurrency issues but with > > the upcoming SHM Bridge driver that will be initiated at the same > > initcall level, we need to assure the assignment and readback of the > > __scm pointer is atomic. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/firmware/qcom_scm.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > > index 980fcfa20b9f..422de70faff8 100644 > > --- a/drivers/firmware/qcom_scm.c > > +++ b/drivers/firmware/qcom_scm.c > > @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > > */ > > bool qcom_scm_is_available(void) > > { > > - return !!__scm; > > + return !!READ_ONCE(__scm); > > } > > EXPORT_SYMBOL(qcom_scm_is_available); > > > > @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev) > > if (ret) > > return ret; > > > > - __scm = scm; > > - __scm->dev = &pdev->dev; > > + scm->dev = &pdev->dev; > > + WRITE_ONCE(__scm, scm); > > Your re-ordering is not effective here, I think. I don't understand it's > purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it > can be reordered: > > "compiler is also forbidden from reordering successive instances of > READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder > other stuff. > > "Ensuring that the compiler does not fold, spindle, or otherwise > mutilate accesses that either do not require ordering or that interact" > <- which means you do not require ordering here. > Hmm, I had the list_add() implementation in mind as well as examples from https://www.kernel.org/doc/Documentation/memory-barriers.txt and was under the impression that WRITE_ONCE() here is enough. I need to double check it. Bart
On 29/08/2023 14:31, Bartosz Golaszewski wrote: > On Tue, 29 Aug 2023 at 09:59, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 28/08/2023 21:24, Bartosz Golaszewski wrote: >>> Checking for the availability of SCM bridge can happen from any context. >>> It's only by chance that we haven't run into concurrency issues but with >>> the upcoming SHM Bridge driver that will be initiated at the same >>> initcall level, we need to assure the assignment and readback of the >>> __scm pointer is atomic. >>> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> --- >>> drivers/firmware/qcom_scm.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c >>> index 980fcfa20b9f..422de70faff8 100644 >>> --- a/drivers/firmware/qcom_scm.c >>> +++ b/drivers/firmware/qcom_scm.c >>> @@ -1331,7 +1331,7 @@ static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) >>> */ >>> bool qcom_scm_is_available(void) >>> { >>> - return !!__scm; >>> + return !!READ_ONCE(__scm); >>> } >>> EXPORT_SYMBOL(qcom_scm_is_available); >>> >>> @@ -1477,8 +1477,8 @@ static int qcom_scm_probe(struct platform_device *pdev) >>> if (ret) >>> return ret; >>> >>> - __scm = scm; >>> - __scm->dev = &pdev->dev; >>> + scm->dev = &pdev->dev; >>> + WRITE_ONCE(__scm, scm); >> >> Your re-ordering is not effective here, I think. I don't understand it's >> purpose exactly, but scm->dev assignment is not WRITE_ONCE(), thus it >> can be reordered: >> >> "compiler is also forbidden from reordering successive instances of >> READ_ONCE and WRITE_ONCE" <- so compiler is not forbidden to reorder >> other stuff. >> >> "Ensuring that the compiler does not fold, spindle, or otherwise >> mutilate accesses that either do not require ordering or that interact" >> <- which means you do not require ordering here. >> > > Hmm, I had the list_add() implementation in mind as well as examples > from https://www.kernel.org/doc/Documentation/memory-barriers.txt and > was under the impression that WRITE_ONCE() here is enough. I need to > double check it. It all depends what do you want to achieve. If strict ordering of both writes, then all the examples and descriptions from memory-barriers.txt say that you need WRITE_ONCE in both places. If you want to achieve something else, like scm->dev visible for other CPUs before __scm=scm then you would need full barriers, IMHO. Best regards, Krzysztof
On Mon, 28 Aug 2023 at 23:24, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Mon, 28 Aug 2023 at 22:29, Bartosz Golaszewski > <bartosz.golaszewski@linaro.org> wrote: > > > > SHM Bridge is a mechanism allowing to map limited areas of kernel's > > virtual memory to physical addresses and share those with the > > trustzone in order to not expose the entire RAM for SMC calls. > > > > This series adds support for Qualcomm SHM Bridge in form of a platform > > driver and library functions available to users. It enables SHM Bridge > > support for three platforms and contains a bunch of cleanups for > > qcom-scm. > > Which users do you expect for this API? > This series adds a single user: the SCM driver. We have another user almost ready for upstream in the form of the scminvoke driver and I learned today, I can already convert another user upstream right now that I will try to get ready for v2. > Also, could you please describe your design a bit more? Why have you > implemented the shm-bridge as a separate driver rather than a part of > the SCM driver? > It's self-contained enough to be put into a separate module and not all platforms support it so in order to avoid unnecessary ifdeffery in the scm driver, I made it separate. Bart > > > > Bartosz Golaszewski (11): > > firmware: qcom-scm: drop unneeded 'extern' specifiers > > firmware: qcom-scm: order includes alphabetically > > firmware: qcom-scm: atomically assign and read the global __scm > > pointer > > firmware: qcom-scm: add support for SHM bridge operations > > dt-bindings: document the Qualcomm TEE Shared Memory Bridge > > firmware: qcom-shm-bridge: new driver > > firmware: qcom-scm: use SHM bridge if available > > arm64: defconfig: enable Qualcomm SHM bridge module > > arm64: dts: qcom: sm8450: enable SHM bridge > > arm64: dts: qcom: sa8775p: enable SHM bridge > > arm64: dts: qcom: sm8150: enable SHM bridge > > > > .../bindings/firmware/qcom,shm-bridge.yaml | 36 ++ > > arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 + > > arch/arm64/boot/dts/qcom/sm8150.dtsi | 4 + > > arch/arm64/boot/dts/qcom/sm8450.dtsi | 4 + > > arch/arm64/configs/defconfig | 1 + > > drivers/firmware/Kconfig | 8 + > > drivers/firmware/Makefile | 1 + > > drivers/firmware/qcom-shm-bridge.c | 452 ++++++++++++++++++ > > drivers/firmware/qcom_scm-smc.c | 20 +- > > drivers/firmware/qcom_scm.c | 106 +++- > > drivers/firmware/qcom_scm.h | 3 + > > include/linux/firmware/qcom/qcom_scm.h | 109 +++-- > > include/linux/firmware/qcom/shm-bridge.h | 32 ++ > > 13 files changed, 712 insertions(+), 68 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml > > create mode 100644 drivers/firmware/qcom-shm-bridge.c > > create mode 100644 include/linux/firmware/qcom/shm-bridge.h > > > > -- > > 2.39.2 > > > > > -- > With best wishes > Dmitry
On Tue, 29 Aug 2023 at 22:03, Bartosz Golaszewski <bartosz.golaszewski@linaro.org> wrote: > > On Mon, 28 Aug 2023 at 23:24, Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Mon, 28 Aug 2023 at 22:29, Bartosz Golaszewski > > <bartosz.golaszewski@linaro.org> wrote: > > > > > > SHM Bridge is a mechanism allowing to map limited areas of kernel's > > > virtual memory to physical addresses and share those with the > > > trustzone in order to not expose the entire RAM for SMC calls. > > > > > > This series adds support for Qualcomm SHM Bridge in form of a platform > > > driver and library functions available to users. It enables SHM Bridge > > > support for three platforms and contains a bunch of cleanups for > > > qcom-scm. > > > > Which users do you expect for this API? > > > > This series adds a single user: the SCM driver. We have another user > almost ready for upstream in the form of the scminvoke driver and I > learned today, I can already convert another user upstream right now > that I will try to get ready for v2. > > > Also, could you please describe your design a bit more? Why have you > > implemented the shm-bridge as a separate driver rather than a part of > > the SCM driver? > > > > It's self-contained enough to be put into a separate module and not > all platforms support it so in order to avoid unnecessary ifdeffery in > the scm driver, I made it separate. Judging from other reviews, I'm not the only one who questioned this design. I still suppose that it might be better to move it into the SCM driver. You can put ifdef's to the header file defining the interface between SCM and SHM bridge part.
On Mon, 28 Aug 2023 21:24:56 +0200, Bartosz Golaszewski wrote: > SHM Bridge is a mechanism allowing to map limited areas of kernel's > virtual memory to physical addresses and share those with the > trustzone in order to not expose the entire RAM for SMC calls. > > This series adds support for Qualcomm SHM Bridge in form of a platform > driver and library functions available to users. It enables SHM Bridge > support for three platforms and contains a bunch of cleanups for > qcom-scm. > > [...] Applied, thanks! [01/11] firmware: qcom-scm: drop unneeded 'extern' specifiers commit: 2758ac3a11d78af56e6969af04dec611806a62de [02/11] firmware: qcom-scm: order includes alphabetically commit: bc7fbb5ea701b22c09c0fa5acbc122207283366a Best regards,
SHM Bridge is a mechanism allowing to map limited areas of kernel's virtual memory to physical addresses and share those with the trustzone in order to not expose the entire RAM for SMC calls. This series adds support for Qualcomm SHM Bridge in form of a platform driver and library functions available to users. It enables SHM Bridge support for three platforms and contains a bunch of cleanups for qcom-scm. Bartosz Golaszewski (11): firmware: qcom-scm: drop unneeded 'extern' specifiers firmware: qcom-scm: order includes alphabetically firmware: qcom-scm: atomically assign and read the global __scm pointer firmware: qcom-scm: add support for SHM bridge operations dt-bindings: document the Qualcomm TEE Shared Memory Bridge firmware: qcom-shm-bridge: new driver firmware: qcom-scm: use SHM bridge if available arm64: defconfig: enable Qualcomm SHM bridge module arm64: dts: qcom: sm8450: enable SHM bridge arm64: dts: qcom: sa8775p: enable SHM bridge arm64: dts: qcom: sm8150: enable SHM bridge .../bindings/firmware/qcom,shm-bridge.yaml | 36 ++ arch/arm64/boot/dts/qcom/sa8775p.dtsi | 4 + arch/arm64/boot/dts/qcom/sm8150.dtsi | 4 + arch/arm64/boot/dts/qcom/sm8450.dtsi | 4 + arch/arm64/configs/defconfig | 1 + drivers/firmware/Kconfig | 8 + drivers/firmware/Makefile | 1 + drivers/firmware/qcom-shm-bridge.c | 452 ++++++++++++++++++ drivers/firmware/qcom_scm-smc.c | 20 +- drivers/firmware/qcom_scm.c | 106 +++- drivers/firmware/qcom_scm.h | 3 + include/linux/firmware/qcom/qcom_scm.h | 109 +++-- include/linux/firmware/qcom/shm-bridge.h | 32 ++ 13 files changed, 712 insertions(+), 68 deletions(-) create mode 100644 Documentation/devicetree/bindings/firmware/qcom,shm-bridge.yaml create mode 100644 drivers/firmware/qcom-shm-bridge.c create mode 100644 include/linux/firmware/qcom/shm-bridge.h