Page MenuHomeFreeBSD

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

Authored by rlibby on Jan 2 2021, 5:09 AM.

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
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.