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)
Dec 20 2023, 8:37 AM
Unknown Object (File)
Dec 12 2023, 1:08 PM
Unknown Object (File)
Dec 1 2023, 1:59 PM
Unknown Object (File)
Nov 4 2023, 4:39 AM
Unknown Object (File)
Aug 31 2023, 9:23 PM
Unknown Object (File)
Aug 31 2023, 9:22 PM
Unknown Object (File)
Aug 31 2023, 9:19 PM
Unknown Object (File)
Aug 31 2023, 9:12 PM
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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35850
Build 32739: arc lint + arc unit

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.