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

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

Details

Reviewers

andrew
markj
manu
mmel

Commits

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

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 created this revision.Jan 2 2021, 5:09 AM

Herald added a reviewer: manu. · View Herald TranscriptJan 2 2021, 5:09 AM

Herald added subscribers: emaste, imp. · View Herald Transcript

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

Harbormaster completed remote builds in B35850: Diff 81493.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

mmel mentioned this in D27891: bitset: Fix BIT_TEST_SET_ATOMIC and BIT_TEST_CLR_ATOMIC..Jan 2 2021, 12:06 PM

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#623414, @rlibby 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.

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#623414, @rlibby 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...

In D27886#623424, @mmel wrote:

In D27886#623414, @rlibby 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...