stm32f7, stm32h7: Avoid speculative reads from QSPI

[icsys-omh]

As recommended in AN4760 the memory region where the QSPI flash can be memory mapped should be configured to be Strongly ordered memory. This works around an issue where a speculative read from the CPU may cause later problems with using the QSPI bus.

This avoids #57466.

I made this change in the standard Cortex-M arm_mpu_regions.c, even though it's applicable to only STM's Cortex-M7 chips, since we can use the device tree to make sure it's enabled only on devices where it's relevant. It also seems this perhaps should be done through the device tree now, but it was not clear to me how that could be done, especially not to set custom values to the regions.

Note that the change has only tested on STM32F7, but it seems from ST's docs that it should be done on both F7 and H7, so I'm including the equivalent changes for both.

[erwango]

I think the expected way of doing this is to add following properties in dts node:

                         zephyr,memory-region = "MY_NAME";
                         zephyr,memory-region-mpu = "PPB";

See https://github.com/zephyrproject-rtos/zephyr/blob/bee551ec1b74f856499de60290688ec89383db06/include/zephyr/linker/devicetree_regions.h#L215

Did you already had a try ?

[icsys-omh]

I think the expected way of doing this is to add following properties in dts node:

                         zephyr,memory-region = "MY_NAME";
                         zephyr,memory-region-mpu = "PPB";

See

https://github.com/zephyrproject-rtos/zephyr/blob/bee551ec1b74f856499de60290688ec89383db06/include/zephyr/linker/devicetree_regions.h#L215

Did you already had a try ?

No, I hadn't noticed that segment of the docs, so I hadn't tried it. Thanks for pointing it out to me! I do however see that this doesn't appear to make it possible to set NO_ACCESS, since it relies on the REGION_*_ATTR macros, all of which have something else. For some reason, even though the appnote just says that the region needs to be set to "Strongly ordered", it appears I also must set the region to No access to make it work - neither P_RW_U_NA_Msk (which is used in REGION_PPB_ATTR) and RO_Msk eliminates the issue. I'm not quite sure why that is, but perhaps there's something I'm missing?

I'm also not quite sure on which device tree node it'd make sense to add these attributes. If we add them to the quadspi node of for instance stm32f7.dtsi, wouldn't they be applied independent of whether that node is actually enabled or not? That might not be a problem in practice, though, since having an extra MPU region defined is probably ok. I am however unsure if it is at all possible to add them to that node, since its base address is different from the region we actually want to protect - QUADSPI is at 0xa0001000, while the region we want to protect is at 0x90000000-0x9FFFFFFF. If however we add these flags to a separate node, the user may need to explicitly enable this node, which would be easy to forget, and hence cause future developers pain.

With the way this PR is now the protection should be automatically applied whenever it's needed, which seems easier from a user-perspective. I am however not very well-versed in the device tree code, so I could very well be overlooking some nice way to do it, so please let me know if there is, and I'll be happy to change this.

[erwango]

The point of having mpu_regions.c files in soc/arm// is to avoid each soc putting its specifics into this common file.
So if we'd want to apply this to F7 series, we'd need to create a soc/arm/st_stm32/stm32f7/mpu_regions.c new file.
(And this is exactly to avoid this that #43119 was created)

[erwango]

See my comment

I do however see that this doesn't appear to make it possible to set NO_ACCESS, since it relies on the REGION_*_ATTR macros, all of which have something else.

I guess if more configurations are required, these can be added. Am I right @carlocaione ?

I'm also not quite sure on which device tree node it'd make sense to add these attributes [...]

Regarding the use of dt method, I think we should create a memory node reflecting the quadspi region we need to protect in .dtsi files . I agree this will uselessly add an extra memory region if qspi is not used, but this is always something people can fix on their application in the end if that's a concern. Alternatively, we can add it in .dts with the risk you identified of having it forgotten by some users.

[carlocaione]

Am I right @carlocaione ?

Yes. I only added the very basic ones. Feel free to add more if you need those.

[icsys-omh]

I'm also not quite sure on which device tree node it'd make sense to add these attributes [...]

Regarding the use of dt method, I think we should create a memory node reflecting the quadspi region we need to protect in .dtsi files . I agree this will uselessly add an extra memory region if qspi is not used, but this is always something people can fix on their application in the end if that's a concern. Alternatively, we can add it in .dts with the risk you identified of having it forgotten by some users.

Thanks for the comment! Yeah, I understood that the DT method was preferred, it just wasn't clear to me how it could be done in a nice way, but I think I see what we can do now. If I understand you correctly, we want to add a quadspi_memory region to the stm32f7.dtsi and stm32h7.dtsi files, with the relevant MPU configurations. I'll try see if I can get around to update this PR relatively shortly.

[icsys-omh]

I've now changed to do this in the DT-way, which seems to work just as well. I've made the change both to STM32H7 and STM32F7 since the appnote explicitly mentions both these series, but note that I've only tested this myself on an STM32F767ZI.

Please let me know if there are further changes needed.

[carlocaione]

From the DT prospective LGTM

[GeorgeCGV]

I am confused here. Doesn't NO_ACCESS_Msk completely restrict access to that region (All accesses generate a permission fault)?

[icsys-omh]

I am confused here. Doesn't NO_ACCESS_Msk completely restrict access to that region (All accesses generate a permission fault)?

That might be - I have honestly not tested accessing the area manually with this enabled, since this area isn't used for anything as long as memory mapping is not enabled (and that is not currently supported by the Zephyr QSPI driver). I first tried using P_RW_U_NA_Msk, but that didn't seem to fix the problem. With NO_ACCESS there was however no extra QSPI transactions, so it seems this configuration disables speculative read for this area in some way internally in the chip.

Note that it's highly likely that this will need further changes if support for memory-mapping is added, but in the meantime, I think this should work as needed.

ST's support also came back to me this week, and mentioned that this issue may also affect the FMC, but unfortunately I don't think we have any boards with any supported external memory, so I haven't investigated that any further.

[GeorgeCGV]

I see. Yes, it breaks mem-map. But it can be dealt with re-definition.