Closed michaeljclark closed 6 years ago
@michaeljclark the atomic_reads are there to avoid C-level undefined behavior caused by compiler optimizations (rematerialization), not CPU behavior, so I don't think this is the right approach. Let's make mip a uint32_t instead of a target_ulong and revisit this if we ever need to support hardware with more than 16 local interrupts, sound good?
Sure. mip is defined as XLEN however the upper bits are WIRI, so it will just drop writes to them.
That said, I don't know of any platforms where atomic_read on naturally aligned words is not just a regular load (a relaxed atomic). I guess it makes sure we get a compile failure if it is not atomic. Writes to the significant bits will certainly be atomic.
We need to do atomic_load_explicit(&env->mip, memory_order_acquire) if we want a fence.
It's not much of a muchness. I'll update mip
to uint32_t on PR https://github.com/riscv/riscv-qemu/pull/105 as that's the branch i'm compile testing on i386.
To be consistent, we'd have to update mip
, mie
and mideleg
otherwise it's very asymmetric.
uint64_t for mip isn't a complete fix anyway, because implementations can have more than 64 local interrupts. Rocket currently generates local interrupt 128 for bus errors. So in the long run we'd need uint32_t *local_interrupts; and then do a loop over them (local interrupts higher than XLEN are not visible in mip and cannot be disabled or delegated, but otherwise work normally). I don't think that should be a priority.
@sorear This patch fixes qemu-system-riscv64 on i386 (32-bit). See compile error below.
The code is theoretically racy (see two 32-bit atomic operations on 32-bit hosts), but given the significant
mip
interrupt bits are in the lower 32-bits of the word, it's only a theoretical issue, as reads and writes of the lower 32-bits ofmip
will be atomic, as are normal aligned loads and stores, at least on i386 (misaligned loads and stores spanning cache lines however are not, but luckily structure alignment rules prevent this from happening except in contrived examples). i.e. it is still practically atomic after this change. This wouldn't be the case if more than 32-bits were used (where we'd see tears on 32-bit boundaries), however, all currently used interrupt bits are in the low 12-bits of the word so there is no impact. If we had >32 interrupt bits, then we can worry about adding locking.Boot tested riscv64 linux: