Page MenuHomeFreeBSD

arm64: fix mask in atomic_testand{set,clear}_64
ClosedPublic

Authored by rlibby on Jan 2 2021, 5:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Oct 3 2024, 7:51 AM
Unknown Object (File)
Oct 3 2024, 1:20 AM
Unknown Object (File)
Oct 1 2024, 8:35 AM
Unknown Object (File)
Sep 30 2024, 2:07 AM
Unknown Object (File)
Sep 26 2024, 11:00 PM
Unknown Object (File)
Sep 26 2024, 8:43 PM
Unknown Object (File)
Sep 23 2024, 7:35 AM
Unknown Object (File)
Sep 23 2024, 5:30 AM
Subscribers

Details

Summary

These macros generate both the 32- and 64-bit ops, but the mask was hard
coded for 32-bit ops, causing the 64-bit ops always to affect only the
low 32 bits.

Reported by: mmel

Test Plan
qemu-system-aarch64 -M virt -m 512m -cpu cortex-a57 -kernel obj/usr/src/freebsd/arm64.aarch64/sys/GENERIC/kernel.bin -nographic

After 942951ba46ecd5ebab18de006a24dc52e2d3f745 and before this patch, this panicked very early in boot (just after the "WITNESS option enabled" message), and now it makes it to mountroot.

Diff Detail

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

Event Timeline

rlibby requested review of this revision.Jan 2 2021, 5:09 AM

Good catch. It never occurred to me that we could have a bug in atomics on two architectures at the same time, so I wasn't looking for a problem in my own garden :) I will try to fix arm by myself.
Tested on real HW, everything OK.

This revision is now accepted and ready to land.Jan 2 2021, 7:58 AM
In D27886#623242, @mmel wrote:

Good catch. It never occurred to me that we could have a bug in atomics on two architectures at the same time, so I wasn't looking for a problem in my own garden :) I will try to fix arm by myself.

Are you saying 32-bit ARM is also broken? Those are a little weirder, I think it uses sys/sys/_atomic_subword.h.

Tested on real HW, everything OK.

Thanks.

In D27886#623242, @mmel wrote:

Good catch. It never occurred to me that we could have a bug in atomics on two architectures at the same time, so I wasn't looking for a problem in my own garden :) I will try to fix arm by myself.

Are you saying 32-bit ARM is also broken? Those are a little weirder, I think it uses sys/sys/_atomic_subword.h.

Okay, I can see that indeed 32-bit arm's atomic_testand{set,clear} also do not match the atomic(9) API with respect to the bit argument.

In D27886#623242, @mmel wrote:

Good catch. It never occurred to me that we could have a bug in atomics on two architectures at the same time, so I wasn't looking for a problem in my own garden :) I will try to fix arm by myself.

Are you saying 32-bit ARM is also broken? Those are a little weirder, I think it uses sys/sys/_atomic_subword.h.

Yes, it is also broken. ARM uses its own implementation, which does not do modulo for the bit position.
See https://cgit.freebsd.org/src/tree/sys/arm/include/atomic-v6.h#n862.
I will commit proper fix in next hour or so...

In D27886#623424, @mmel wrote:
In D27886#623242, @mmel wrote:

Good catch. It never occurred to me that we could have a bug in atomics on two architectures at the same time, so I wasn't looking for a problem in my own garden :) I will try to fix arm by myself.

Are you saying 32-bit ARM is also broken? Those are a little weirder, I think it uses sys/sys/_atomic_subword.h.

Yes, it is also broken. ARM uses its own implementation, which does not do modulo for the bit position.
See https://cgit.freebsd.org/src/tree/sys/arm/include/atomic-v6.h#n862.
I will commit proper fix in next hour or so...

Yeah, I have seen this too. I have a patch, but no hardware, and no luck so far with getting qemu-system-arm to work.