Unsafe register access option.

[pellico]

Following the definition of unsafety as described in Ferrocene spec and Rust reference, unsafe code may result in undefined behavior.

In our microcontrollers we can trigger an undefined behavior for some peripherals if some write/read order is not followed.
Therefore I think that all write access and read access that has side effect (SVD support this attribute) shall be declared as unsafe just because the HW could have some undefined behavior.
HAL or Low Level Driver in Rust shall solve the safety issue by providing API that forbid to trigger undefined behavior.

Moreover I find someway a contradiction that presently all register access (with some exception) is considered safe while if I call a low level driver implemented in C is considered unsafe. I see a clear similarity between register access and C API.

Do I miss something ?

Proposal:

Provide a svd2rust option to mark all all write access and read access that has side effect as unsafe.

This will not break backward compatibility and it will let to migrate to a safer implementation.

PS. Someone in embassy team share the same concerns.

[jonathanpallant]

SVD support this attribute
Provide a svd2rust option

Seems reasonable to me.

[jannic]

I think this decision should not be made with only the individual registers in mind. You are of course right that on most (all?) microcontrollers, there are registers which are dangerous to access and should not be accessed carelessly. It should better not be possible for random application code to access them outside of unsafe blocks.

However, in practice this is already the case: The registers are only accessible via owned singletons, and code with access to those singletons is usually limited to some HAL and a relatively small amount of initialization code.

Therefore, I think there are mainly two sensible choices:

• owned singletons & pac access without unsafe blocks (ie. what svd2rust currently does)
• everybody can access the pac directly, but every access is unsafe (ie. what embassy does)

[jonathanpallant]

The Rust docs note:

if unsafe code can be misused by safe code to exhibit undefined behavior, it is unsound

If it is safe to obtain a singleton, and it is safe to write to a register using it, but that write performs undefined behaviour in certain scenarios (per the manufacturer's specification), then isn't the svd2rust API unsound regardless of whether access to such objects is 'limited' to whoever calls the safe API first?

We already distinguish between w.field().bits(1) (unsafe) and w.field().enabled() (safe). So the OP could obtain the same result by removing the relevant field attributes from the SVD and forcing the use of bits() on the unsafe registers/fields.

However I think the proposed third option is reasonable:

• owned singletons & pac access where some register fields are unsafe and some are safe (according to the SVD attributes for each register/field)

[adamgreig]

owned singletons & pac access where some register fields are unsafe and some are safe (according to the SVD attributes for each register/field)

This is what we have for writes today, using the enumeratedValues and/or writeConstraint attributes. Note that bits() is also safe if a writeConstraint says all possible values for the field width are permissible. It's up to the PAC author to ensure that any fields that might cause UB if written to are unsafe-only (generally this is limited to DMA peripheral address registers but maybe some fields to do with memory access on some microcontrollers too). We don't currently have a way to specify enumeratedValues and have a field be unsafe, but sadly the SVD spec doesn't include a "is this field/value safe" attribute so our hands are a bit tied there. I'm not aware of any other relevant attribute we could use to determine if a write should be safe.

Currently I don't believe read() is ever marked unsafe in svd2rust, so we could consider using the readAction attribute to mark some reads as unsafe when they have side effects - however we'd probably have to rely on enumeratedValues to decide when it's safe. My guess is there are very few registers where reading can lead to UB even if it has side effects, though. As an implementation nuisance, currently read() is generic across all registers, so breaking it into sometimes-unsafe might be a bit annoying.

But, @pellico, if I understand your suggestion correctly, you're saying "all write access" and also "all read access with side effects" should be unsafe - in other words, the only safe operations would be reads without side effects, is that right? I assume you are referring to readAction attribute for deciding which reads are safe. I think if we did go down that route it would be at the same time as removing the owned singletons entirely, and let the HAL handle ownership too, in which case we can simply make all access unsafe including reads.

[jannic]

If it is safe to obtain a singleton, and it is safe to write to a register using it, but that write performs undefined behaviour in certain scenarios (per the manufacturer's specification), then isn't the svd2rust API unsound regardless of whether access to such objects is 'limited' to whoever calls the safe API first?

I don't think it's so easy. UB is a construct defined by the programming language, specified in terms of an abstract machine. It doesn't even address interactions with hardware, and for a good reason: It's difficult enough to define UB inside of this relatively simple abstraction.

If you extend the notion of UB to include hardware outside of that abstract machine, and try to apply the rule you cited in a strict way, so many APIs would need to become unsafe that it would become impossible to write meaningful software in safe rust. If taken to the extreme, just blinking a LED could cause UB unless you first make sure that the power supply is strong enough to handle the additional load. Otherwise, the voltage could drop and the CPU could stop working correctly, causing UB.

Of course, the LED example is ridiculous. It was meant to be, obviously. But where do you draw the line? There are a lot of reasonable choices. And when there is a discussion where it's not sure if some hardware interaction should be marked as unsafe or not, the argument "this could cause UB in some situation, so it must be unsafe" isn't sufficient to decide, as shown by the LED example.

IMHO doing an individual assessment of every single register and its possible values is a gigantic amount of work, and not justified by the gain in safety or ease of use. (And every time we decide that a decision was wrong and a register or value should be marked as unsafe, it's a breaking change!)

So I'd prefer to find easy rules, knowing that they can't cover every detail in the best way possible. Which is why I like the idea "every register access is unsafe". The current handling, which as far as I understand makes writes unsafe unless the PAC lists allowed values, is fine as well. But it's already both more complex and opening more room for discussions.

[burrbull]

Therefore, I think there are mainly two sensible choices:

* owned singletons & pac access without unsafe blocks (ie. what svd2rust currently does)

* everybody can access the pac directly, but every access is unsafe (ie. what embassy does)

I'm agree. All or nothing.

For fields with readAction svd2rust adds note about risks in field docs (#605).

[jonathanpallant]

IMHO doing an individual assessment of every single register and its possible values is a gigantic amount of work

I agree, but that's not work that svd2rust has to do. It's work that the silicon vendor or other maintainer of the PAC can elect to do if it suits them - especially if they are looking to use the svd2rust generated output in a safety critical system. This is assuming such attributes exist in the SVD standard - I'm not an expert on that.

LED example

I agree it's not Undefined Behaviour, but as the author of an API, if I had a function would would crash the chip (e.g. turn_on_big_load()) if some other API hadn't been called first (turn_on_boost_convertor()) I would totally design my API to make doing the right thing safe, and make those raw APIs unsafe. The software is only there to model and operate the larger system after all.

What I think is novel here is that Infineon are offering a first-party svd2rust generated API for their microcontrollers and so customers, rather than community HAL maintainers, have to use it - hopefully correctly.

I'm agree. All or nothing.

There is an attractiveness to this position that I do understand. Maybe it's simply not appropriate to have these rules encoded in the types and APIs generated by svd2rust output, and we just say that some extra level of API would need to be layered over the top. You just put a big note at the top that says "Hey, just because the PAC has a safe API, doesn't mean you it's right for you to poke any given register at any given moment".

[pellico]

@adamgreig

But, @pellico, if I understand your suggestion correctly, you're saying "all write access" and also "all read access with side effects" should be unsafe - in other words, the only safe operations would be reads without side effects, is that right?

Yes this correct !
My proposal was relaing on SVD information. The SVD readaction is used by debuggers to skip reading and avoid to disturb the device. However declaring everything unsafe is fine to me because it copies the same approach as for Rust C API interface (all C api are by default unsafe).

[pellico]

@jannic

I don't think it's so easy. UB is a construct defined by the programming language, specified in terms of an abstract machine. It doesn't even address interactions with hardware, and for a good reason: It's difficult enough to define UB inside of this relatively simple abstraction.

Could you link where is described that UB is referring to the abstract machine?

From Rust Reference

Warning: The following list is not exhaustive. There is no formal model of Rust's semantics for what is and is not allowed in unsafe code, so there may be more behavior considered unsafe. The following list is just what we know for sure is undefined behavior. Please read the Rustonomicon before writing unsafe code.

Moreover is listed here as unsafe operations:

• Invoking undefined behavior via compiler intrinsics.
• Executing code compiled with platform features that the current platform does not support (see target_feature), except if the platform explicitly documents this to be safe.

I think here is someway involved the hardware behavior (many intrinsics are hardware specific instructions and some of them are used to write registers)
So I see that the hardware behaviors is already involved in the UB definition.

At the end of day this boils down to have a definition of unsafety and comply with this definition. I would be fine if in Rust Spec and Reference Manual will be written that undefined behavior of HW peripheral could happens in Rust safe code.

My proposal for where to draw the line of UB in HW is:
API that access HW block are safe if they don't trigger any known UB (as described in HW spec/user manual)

So I'd prefer to find easy rules, knowing that they can't cover every detail in the best way possible. Which is why I like the idea "every register access is unsafe".

👍

[jannic]

Could you link where is described that UB is referring to the abstract machine?

Sorry, I don't have a quote at hand, and I'm not sure if it's explicitly written somewhere.
But I also don't find any mention interactions with machine-specific runtime environments. The whole definition of UB and safe/unsafe code just doesn't cover much of the environment the program executes within. There's also the whole area of atomics and thread safety which often refers to the C++ memory model, which is defined based on an abstract machine, see https://en.cppreference.com/w/cpp/language/memory_model.

So technically I may have been wrong when I wrote "UB is a construct defined by the programming language, specified in terms of an abstract machine." But I think the conclusions are still valid.

As an example of something which doesn't seem to be covered by the definition of UB, see totally_safe_transmute, https://blog.yossarian.net/2021/03/16/totally_safe_transmute-line-by-line, which relies on access to /proc/self/mem.
Note that access to that device is very similar to triggering some DMA transfer. Of course, it's extremely dangerous. But it seems like it's far enough outside of the realm of what the rust language defines, that nobody(?) wants to mark filesystem access as unsafe.

Also noted that UB is a far reaching concept. It doesn't only mean "the machine may crash somehow". Instead, the compiler can do any code transformation under the assumption that UB will never happen. So you can't argue your way out of UB. (Like in: "I don't mind that there is a race condition, because I don't really care if the result is wrong every now and then, when some unfortunate timing happens." No, if the compiler can see the UB, it can just assume that the code will never be executed and for example replace it with some constant value, which will always be wrong. Or worse.)

As I understand it, this is not what we are talking about, here. From the compiler's point of view, those memory-mapped registers are just some random memory locations without specific meaning. As long as you observe the rules which also apply to regular memory, the compiler doesn't care what you do to them. Yes, turning of your cpu clock while your program is running probably isn't a good idea. But it also is not UB (like in "the compiler might ignore it"), but it will halt your program in a very (hardware-)defined way.

[Emilgardis]

For context, rusts stance on /proc/self/mem is found here https://doc.rust-lang.org/std/os/unix/io/index.html#procselfmem-and-similar-os-features

[pellico]

Yes, turning of your cpu clock while your program is running probably isn't a good idea. But it also is not UB (like in "the compiler might ignore it"), but it will halt your program in a very (hardware-)defined way.

We have in microcontrollers with behaviors that are more undefined and not consistently reproducible than writing out of an array boundary. When a behavior is declared undefined in a microcontroller user manual, it means that nobody has tested the condition in simulation or in real device. The result of the undefined operation is unpredictable. The behavior can change for every device or even for same device at different point in time (this is typical for power control, some PLL control circuit or other mixed signal peripherals).

About /proc/self/mem Example

Regarding the example of access of /proc/self/mem from the various discussions:

• https://users.rust-lang.org/t/proposal-to-organize-bounty-for-linux-kernel-patch-to-restrict-access-to-processs-own-memory-via-file-system/58303/11
• A soundness bug in std::fs rust-lang/rust#32670
• Document Rust's stance on /proc/self/mem rust-lang/rust#97837

It looks like that the real argument was: too difficult to create a safe open api.

However if I follow the argument described in https://doc.rust-lang.org/std/os/unix/io/index.html#procselfmem-and-similar-os-features (external world shall not be considered) I am not understanding the following:

• Why bits() method in PAL is unsafe if the range of valid values is smaller that the width of bitfield? I expect always safe. The argument is not bad from memory handling it is just bad for the peripheral.
• Why there is so much effort to implement safe DMA usage in embedonomicon? I expect DMA API access is always safe because it the same case as /proc/self/mem as @jannic has highlighted.

Bottom-line

I think that we could agree which are the requirements to be fulfilled to consider an API are not well defined and there is not a common understanding in the community and practice.

Why is important to have well defined requirements for safe API ?

As embedded developer that shall provide to a customer a Rust safe and sound library I want to match exactly his expectation but at the same time I don't want to put more effort than required.

In other words, safe property is a contract on the API that I want to fulfill at minimum.
So I need a coherent, unambiguous and widely accepted definition of what is a safe API at least for embedded applications.

Temporary proposal until proper definition of safe API is available

I would propose to have this command line switch that will allows semiconductor vendor to provide unsafe PAL. This will not affect any backward compatibility because anyway the PAL of a device is not portable and a standard HAL need to implemented on top.

[adamgreig]

You're right, it's not consistent to say you don't need unsafe to change memory using /proc/self/mem, but that DMA does -- they're basically the same thing! A DMA peripheral is exactly the sort of "entities outside the program" that the Rust docs talk about. Despite that, it's a popular opinion that DMA peripherals shouldn't be safe to access directly, while for instance almost no-one wants setting a GPIO pin to be unsafe even though if it shorted the power rail it could easily glitch memory or reset the program or whatever. I think it's just that on balance writing a register to a DMA peripheral is very likely to lead to UB while toggling a GPIO is both very useful and very unlikely to corrupt memory.

Why bits() method in PAL is unsafe if the range of valid values is smaller that the width of bitfield? I expect always safe. The argument is not bad from memory handling it is just bad for the peripheral.

This is just a peculiarity of svd2rust. It defaults to being unsafe to write any register, if specific enumeratedValues are provided then those are safe, and if those values cover all possible bit patterns then bits() is also safe. The idea was that fields could be something like a DMA address register, or a "turn this bit off to disable SRAM" or whatever that quickly leads to UB, and that by documenting them (with enumeratedValues) the PAC maintainer is saying "these are safe to write, actually". I don't know how useful that idea is in the end.

I think that we could agree which are the requirements to be fulfilled to consider an API are not well defined and there is not a common understanding in the community and practice.

👍

I would propose to have this command line switch that will allows semiconductor vendor to provide unsafe PAL.

My preference would be for such a switch to not only make all access unsafe, but to entirely remove the concept of owned singletons for peripherals, so you can access any register from anywhere in your program without needing to own it, but all access is unsafe. However, this is a bigger change than keeping the singletons and just making all use of them unsafe, so I don't know if it's too big a change to put behind a switch. What do people think?

[thejpster]

Maybe the 'unsafe' API can just give address to the RegisterBlock objects? It's a raw pointer to a struct so it's all unsafe, and you can read/modify/write the registers as you wish. We'd just need to expose constants for the shifts and masks required to access each field in each register, exactly like the C API. Or was the idea to have an 'unsafe_write' etc which took a closure with a proxy arg that only had unsafe methods?

An svd2rust flag would often mean building and publishing both variants which is more work.

A feature flag in the generated code might work?

[burrbull]

Maybe the 'unsafe' API can just give address to the RegisterBlock objects?

USART1::ptr() gives unsafe access to usart RegisterBlock

[adamgreig]

Yea, we already have unsafe direct pointers to the registers - this issue is mainly about removing the safe API. Just, if that was the main mode of interaction then we'd want a much more ergonomic unsafe API than we have now. I think it would need a command-line flag as the whole point is to remove safe access; having a (default, presumably?) feature to enable the safe API means HALs couldn't rely on it not being enabled, while making a feature to remove the safe API is the opposite of how features are supposed to work and still doesn't compose well. It's more like a choice the PAC author makes - they don't need to build and publish both variants if they don't want to.

[pellico]

My preference would be for such a switch to not only make all access unsafe, but to entirely remove the concept of owned singletons for peripherals, so you can access any register from anywhere in your program without needing to own it, but all access is unsafe.

I think that removing the concept of owned singletons has some deep impact of Low Level Driver architecture that may be undesirable.

Owned singleton allows composition of driver that are coming from different sources.

Sometime ago we developed a tool where different developers can create peripheral driver components that can be instantiated multiple times (component ~ class). Each component shall declare which kind of HW resource group it needs (HW resource group is modeled by group of register bitfields that implements the same functionality). The tool exclusively assign to each instance a different group of register bitfields based on connectivity requested and which pin end user want to use. Due to the lack ownership concept in C we had to relay on component developer to use in code generator template only the register bitfields assigned to the component instance.

This allowed to combine in the same project different kind of drivers for the same peripherals e.g. one UART was consumed by a basic driver instance, another UART plus a DMA channel was consumed by a more advanced component.

Without PAL ownership the low level drivers shall come from the same package to avoid multiple independent access of same registers.

This is still durable because in my experience the developers will end up in using the LLD provided by silicon vendor (the ones that expose all functionality) and with this one they develop higher abstraction layers e.g. standardized HAL. I am assuming silicon vendor will sometime offer Rust LLD :-)

The removal of ownership would simplify the work of PAL provider but I don't fully understand all benefits for PAL user. If the problem is the too coarse granularity I think it would be not too difficult for semiconductor vendor to split in smaller units that still make sense from functionality point of view.
E.g. Not only one big DMA singleton but the DMA can be split in multiple DMA channels. Therefore each DMA channel can be independently consumed by LLD.

[Dirbaio]

+1 for removing peripheral singletons and making all register access unsafe. I've found they have a few severe downsides, I've written about them here.

Embassy's HALs use this approach and define their own singletons at the HAL layer. It has turned out to work fine, and completely solves these downsides. (embassy-stm32 and embassy-rp use chiptool-based PACs, embassy-nrf uses the nrf-rs svd2rust-based PACs, but pretends the PAC singletons don't exist)