RISC-V - atomic_read breaks RV64 build on i386

[michaeljclark]

@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 of mip 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.

In file included from /home/mclark/src/riscv-qemu/include/qemu/osdep.h:36:0,
                 from /home/mclark/src/riscv-qemu/target/riscv/helper.c:21:
/home/mclark/src/riscv-qemu/target/riscv/helper.c: In function ‘riscv_cpu_hw_interrupts_pending’:
/home/mclark/src/riscv-qemu/include/qemu/compiler.h:86:30: error: static assertion failed: "not expecting: sizeof(*&env->mip) > ATOMIC_REG_SIZE"
 #define QEMU_BUILD_BUG_ON(x) _Static_assert(!(x), "not expecting: " #x)
                              ^
/home/mclark/src/riscv-qemu/include/qemu/atomic.h:125:5: note: in expansion of macro ‘QEMU_BUILD_BUG_ON’
     QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
     ^
/home/mclark/src/riscv-qemu/target/riscv/helper.c:46:39: note: in expansion of macro ‘atomic_read’
     target_ulong pending_interrupts = atomic_read(&env->mip) & env->mie;
                                       ^
make[1]: *** [target/riscv/helper.o] Error 1
make: *** [subdir-riscv64-softmmu] Error 2

Boot tested riscv64 linux:

mclark@ubuntu-32:~/src/riscv-qemu$ ./riscv64-softmmu/qemu-system-riscv64 -nographic -machine spike_v1.10 -kernel qemu-images/bbl-1.10
[    0.000000] OF: fdt: Ignoring memory range 0x80000000 - 0x80200000
[    0.000000] Linux version 4.14.0-00030-gc2d852cb2f3d (mclark@minty) (gcc version 7.1.1 20170509 (GCC)) #93 Mon Feb 5 19:52:16 NZDT 2018
[    0.000000] bootconsole [early0] enabled
[    0.000000] Initial ramdisk at: 0xffffffe000018e60 (1122644 bytes)
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x0000000080200000-0x0000000087ffffff]
[    0.000000]   Normal   empty
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000080200000-0x0000000087ffffff]
[    0.000000] Initmem setup node 0 [mem 0x0000000080200000-0x0000000087ffffff]
[    0.000000] elf_hwcap is 0x112d
[    0.000000] Built 1 zonelists, mobility grouping on.  Total pages: 31815
[    0.000000] Kernel command line: rdinit=/sbin/init rdinit=/sbin/init
[    0.000000] PID hash table entries: 512 (order: 0, 4096 bytes)
[    0.000000] Dentry cache hash table entries: 16384 (order: 5, 131072 bytes)
[    0.000000] Inode-cache hash table entries: 8192 (order: 4, 65536 bytes)
[    0.000000] Sorting __ex_table...
[    0.000000] Memory: 122164K/129024K available (2070K kernel code, 168K rwdata, 547K rodata, 1200K init, 763K bss, 6860K reserved, 0K cma-reserved)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
[    0.000000] NR_IRQS: 0, nr_irqs: 0, preallocated irqs: 0
[    0.000000] riscv,cpu_intc,0: 64 local interrupts mapped
[    0.000000] clocksource: riscv_clocksource: mask: 0xffffffffffffffff max_cycles: 0x24e6a1710, max_idle_ns: 440795202120 ns
[    0.000000] console [hvc0] enabled
[    0.000000] console [hvc0] enabled
[    0.000000] bootconsole [early0] disabled
[    0.000000] bootconsole [early0] disabled
[    0.000000] Calibrating delay loop (skipped), value calculated using timer frequency.. 20.00 BogoMIPS (lpj=100000)
[    0.010000] pid_max: default: 32768 minimum: 301
[    0.010000] Mount-cache hash table entries: 512 (order: 0, 4096 bytes)
[    0.010000] Mountpoint-cache hash table entries: 512 (order: 0, 4096 bytes)
[    0.070000] devtmpfs: initialized
[    0.080000] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
[    0.090000] futex hash table entries: 256 (order: 0, 6144 bytes)
[    0.090000] random: get_random_u32 called from bucket_table_alloc+0x80/0x1f4 with crng_init=0
[    0.090000] NET: Registered protocol family 16
[    0.110000] vgaarb: loaded
[    0.120000] clocksource: Switched to clocksource riscv_clocksource
[    0.120000] NET: Registered protocol family 2
[    0.130000] TCP established hash table entries: 1024 (order: 1, 8192 bytes)
[    0.130000] TCP bind hash table entries: 1024 (order: 1, 8192 bytes)
[    0.130000] TCP: Hash tables configured (established 1024 bind 1024)
[    0.140000] UDP hash table entries: 256 (order: 1, 8192 bytes)
[    0.140000] UDP-Lite hash table entries: 256 (order: 1, 8192 bytes)
[    0.140000] NET: Registered protocol family 1
[    0.220000] Unpacking initramfs...
[    0.290000] workingset: timestamp_bits=62 max_order=15 bucket_order=0
[    0.330000] random: fast init done
[    0.360000] io scheduler noop registered
[    0.370000] io scheduler cfq registered (default)
[    0.370000] io scheduler mq-deadline registered
[    0.370000] io scheduler kyber registered
[    0.480000] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.500000] Freeing unused kernel memory: 1200K
[    0.500000] This architecture does not have kernel memory protection.
/ # 
/ # 
/ # uname -a
Linux ucbvax 4.14.0-00030-gc2d852cb2f3d #93 Mon Feb 5 19:52:16 NZDT 2018 riscv64 GNU/Linux
/ # qemu-system-riscv64: terminating on signal 15 from pid 25076 ()
mclark@ubuntu-32:~/src/riscv-qemu$ uname -a
Linux ubuntu-32 4.4.0-31-generic #50~14.04.1-Ubuntu SMP Wed Jul 13 01:06:37 UTC 2016 i686 i686 i686 GNU/Linux

[sorear]

@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?

[michaeljclark]

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 #105 as that's the branch i'm compile testing on i386.

[michaeljclark]

To be consistent, we'd have to update mip, mie and mideleg otherwise it's very asymmetric.

[sorear]

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.