Page MenuHomeFreeBSD

arm64: add swapuword8/32
ClosedPublic

Authored by kevans on Apr 26 2023, 8:03 PM.
Tags
None
Referenced Files
F102420025: D39837.diff
Tue, Nov 12, 12:58 AM
Unknown Object (File)
Tue, Oct 29, 2:10 PM
Unknown Object (File)
Sun, Oct 20, 9:13 AM
Unknown Object (File)
Sun, Oct 20, 9:13 AM
Unknown Object (File)
Oct 5 2024, 3:38 AM
Unknown Object (File)
Oct 5 2024, 3:38 AM
Unknown Object (File)
Oct 5 2024, 3:37 AM
Unknown Object (File)
Oct 5 2024, 3:35 AM

Details

Summary

Much like casueword*, except just a plain old swap. Maintains a similar
interface to casu(9)- return value -1 (fault), 0 (success), or 1 (fail),
and also both ll/sc and LSE variants are implemented.

These will be used to implement 32-bit swp/swpb emulation on aarch64.

Sponsored by: Stormshield
Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jrtc27 added inline comments.
sys/sys/systm.h
325 ↗(On Diff #121107)

If these are not expected to be used outside of AArch64 for emulating swp etc, can these prototypes live somewhere highly MD and opt-in lest they creep into being used by other code?

335 ↗(On Diff #121107)

Presumably we need interceptors for these?

sys/sys/systm.h
325 ↗(On Diff #121107)

Yeah, so I couldn't decide where to put them. I could just put them in undefined.c and we can re-scope it later if we find another arch-specific use that make sense?

Not sure if they should be called swapueword* to be consistent with the other userspace functions that return an error value rather than what was read.

sys/arm64/arm64/support.S
35

Needed?

66–69

It looks like this comment needs an update

66–69

This comment needs an update.

71
75–77

I don't think you need w5 in the llsc functions. w3 will contain the correct value to be returned.

79
83
108
137
166
sys/arm64/arm64/support.S
66–69

Hmm, phab hid the first comment after hitting the wrong button

kevans added inline comments.
sys/arm64/arm64/support.S
90

w5 use in _lse variants is also not necessary, we can only either succeed or fault -- removing those and just mov w0, #0 at the end.

sys/sys/systm.h
335 ↗(On Diff #121107)

I commented on IRC that your two suggestions conflict in an annoying way, but I *think* I'm actually going to implement the other and respectfully punt on this one with an XXX comment.

We don't currently support KMSAN (and I'm not aware of any WIP to) on aarch64 and the KASAN interceptor would presumably just check that val is valid in the shadow map, which is trivially true right now because in all existing callers it's a stack address.

kevans marked an inline comment as done.

Address review feedback:

  • swapuword -> swapueword ('e' indicates error return)
  • various issues with the implementation itself, stale comments, etc
  • hide the prototypes a little better and add a note with some caveats

What do you mean by ''fail"? A possible failure from store-conditional? It sounds unnatural to expose the property to consumers. For instance, C/C++ atomics do not provide weak variants of atomic_exchange, the operation always succeed.

In other words, I suspect that _llsc variant is better to behave same as _lse variant.

In D39837#906951, @kib wrote:

What do you mean by ''fail"? A possible failure from store-conditional? It sounds unnatural to expose the property to consumers. For instance, C/C++ atomics do not provide weak variants of atomic_exchange, the operation always succeed.

In other words, I suspect that _llsc variant is better to behave same as _lse variant.

Right after sending submit, I realized that you might want to handle issue from 30b3018d48e4441083b483ca1f? Is this needed? If yes, please ignore my comment.

In D39837#906953, @kib wrote:
In D39837#906951, @kib wrote:

What do you mean by ''fail"? A possible failure from store-conditional? It sounds unnatural to expose the property to consumers. For instance, C/C++ atomics do not provide weak variants of atomic_exchange, the operation always succeed.

In other words, I suspect that _llsc variant is better to behave same as _lse variant.

Right after sending submit, I realized that you might want to handle issue from 30b3018d48e4441083b483ca1f? Is this needed? If yes, please ignore my comment.

Correct- there's a little bit of context in D39667 where it was brought up in my earlier implementation that was purpose-made for swp/swpb (undefined.c:255 being where we handle it and maybe_yield() every so often)

This revision is now accepted and ready to land.Apr 27 2023, 9:20 AM
This revision was automatically updated to reflect the committed changes.