On Thu, Aug 11, 2022 at 4:57 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> Historically, The mtime/mtimecmp has been part of the CPU because
> they are per hart entities. However, they actually belong to aclint
> which is a MMIO device.
>
> Move them to the ACLINT device. This also emulates the real hardware
> more closely.
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
This patch breaks my multi-socket boot.
When using OpenSBI 1.1 and Linux 5.19-rc7
qemu-system-riscv64 \
-machine virt \
-serial mon:stdio -serial null -nographic \
-append "root=/dev/vda rw highres=off console=ttyS0 ip=dhcp earlycon=sbi" \
-device virtio-net-device,netdev=net0,mac=52:54:00:12:34:02 \
-netdev user,id=net0 \
-object rng-random,filename=/dev/urandom,id=rng0 \
-device virtio-rng-device,rng=rng0 \
-smp 4 \
-d guest_errors \
-m 2G \
-object memory-backend-ram,size=1G,policy=bind,host-nodes=0,id=ram-node0 \
-numa node,memdev=ram-node0 \
-object memory-backend-ram,size=1G,policy=bind,host-nodes=0,id=ram-node1 \
-numa node,memdev=ram-node1 \
-numa cpu,node-id=0,core-id=0 \
-numa cpu,node-id=0,core-id=1 \
-numa cpu,node-id=1,core-id=2 \
-numa cpu,node-id=1,core-id=3 \
-kernel ./images/qemuriscv64/Image
-bios default
It looks like OpenSBI hangs when booting after applying this patch
Argh. It was due to relative hartid vs absolute hartid per socket. This fixes the issue for me.
Sorry for the breakage!
diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index a125c73d535c..eee04643cb19 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -66,18 +66,21 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
+ /* Compute the relative hartid w.r.t the socket */
+ hartid = hartid - mtimer->hartid_base;
+
mtimer->timecmp[hartid] = value;
if (mtimer->timecmp[hartid] <= rtc_r) {
/*
* If we're setting an MTIMECMP value in the "past",
* immediately raise the timer interrupt
*/
- qemu_irq_raise(mtimer->timer_irqs[hartid - mtimer->hartid_base]);
+ qemu_irq_raise(mtimer->timer_irqs[hartid]);
return;
}
/* otherwise, set up the future timer interrupt */
- qemu_irq_lower(mtimer->timer_irqs[hartid - mtimer->hartid_base]);
+ qemu_irq_lower(mtimer->timer_irqs[hartid]);
diff = mtimer->timecmp[hartid] - rtc_r;
/* back to ns (note args switched in muldiv64) */
uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
Alistair
> ---
> hw/intc/riscv_aclint.c | 41 ++++++++++++++++++++++++----------
> hw/timer/ibex_timer.c | 18 ++++++---------
> include/hw/intc/riscv_aclint.h | 2 ++
> include/hw/timer/ibex_timer.h | 2 ++
> target/riscv/cpu.h | 2 --
> target/riscv/machine.c | 5 ++---
> 6 files changed, 42 insertions(+), 28 deletions(-)
>
> diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
> index e7942c4e5a32..a125c73d535c 100644
> --- a/hw/intc/riscv_aclint.c
> +++ b/hw/intc/riscv_aclint.c
> @@ -32,6 +32,7 @@
> #include "hw/intc/riscv_aclint.h"
> #include "qemu/timer.h"
> #include "hw/irq.h"
> +#include "migration/vmstate.h"
>
> typedef struct riscv_aclint_mtimer_callback {
> RISCVAclintMTimerState *s;
> @@ -65,8 +66,8 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>
> uint64_t rtc_r = cpu_riscv_read_rtc(mtimer);
>
> - cpu->env.timecmp = value;
> - if (cpu->env.timecmp <= rtc_r) {
> + mtimer->timecmp[hartid] = value;
> + if (mtimer->timecmp[hartid] <= rtc_r) {
> /*
> * If we're setting an MTIMECMP value in the "past",
> * immediately raise the timer interrupt
> @@ -77,7 +78,7 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
>
> /* otherwise, set up the future timer interrupt */
> qemu_irq_lower(mtimer->timer_irqs[hartid - mtimer->hartid_base]);
> - diff = cpu->env.timecmp - rtc_r;
> + diff = mtimer->timecmp[hartid] - rtc_r;
> /* back to ns (note args switched in muldiv64) */
> uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
>
> @@ -102,7 +103,7 @@ static void riscv_aclint_mtimer_write_timecmp(RISCVAclintMTimerState *mtimer,
> next = MIN(next, INT64_MAX);
> }
>
> - timer_mod(cpu->env.timer, next);
> + timer_mod(mtimer->timers[hartid], next);
> }
>
> /*
> @@ -133,11 +134,11 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque, hwaddr addr,
> "aclint-mtimer: invalid hartid: %zu", hartid);
> } else if ((addr & 0x7) == 0) {
> /* timecmp_lo for RV32/RV64 or timecmp for RV64 */
> - uint64_t timecmp = env->timecmp;
> + uint64_t timecmp = mtimer->timecmp[hartid];
> return (size == 4) ? (timecmp & 0xFFFFFFFF) : timecmp;
> } else if ((addr & 0x7) == 4) {
> /* timecmp_hi */
> - uint64_t timecmp = env->timecmp;
> + uint64_t timecmp = mtimer->timecmp[hartid];
> return (timecmp >> 32) & 0xFFFFFFFF;
> } else {
> qemu_log_mask(LOG_UNIMP,
> @@ -177,7 +178,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
> } else if ((addr & 0x7) == 0) {
> if (size == 4) {
> /* timecmp_lo for RV32/RV64 */
> - uint64_t timecmp_hi = env->timecmp >> 32;
> + uint64_t timecmp_hi = mtimer->timecmp[hartid] >> 32;
> riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> timecmp_hi << 32 | (value & 0xFFFFFFFF));
> } else {
> @@ -188,7 +189,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
> } else if ((addr & 0x7) == 4) {
> if (size == 4) {
> /* timecmp_hi for RV32/RV64 */
> - uint64_t timecmp_lo = env->timecmp;
> + uint64_t timecmp_lo = mtimer->timecmp[hartid];
> riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu), hartid,
> value << 32 | (timecmp_lo & 0xFFFFFFFF));
> } else {
> @@ -234,7 +235,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
> }
> riscv_aclint_mtimer_write_timecmp(mtimer, RISCV_CPU(cpu),
> mtimer->hartid_base + i,
> - env->timecmp);
> + mtimer->timecmp[i]);
> }
> return;
> }
> @@ -284,6 +285,8 @@ static void riscv_aclint_mtimer_realize(DeviceState *dev, Error **errp)
> s->timer_irqs = g_new(qemu_irq, s->num_harts);
> qdev_init_gpio_out(dev, s->timer_irqs, s->num_harts);
>
> + s->timers = g_new0(QEMUTimer *, s->num_harts);
> + s->timecmp = g_new0(uint64_t, s->num_harts);
> /* Claim timer interrupt bits */
> for (i = 0; i < s->num_harts; i++) {
> RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(s->hartid_base + i));
> @@ -310,6 +313,18 @@ static void riscv_aclint_mtimer_reset_enter(Object *obj, ResetType type)
> riscv_aclint_mtimer_write(mtimer, mtimer->time_base, 0, 8);
> }
>
> +static const VMStateDescription vmstate_riscv_mtimer = {
> + .name = "riscv_mtimer",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_VARRAY_UINT32(timecmp, RISCVAclintMTimerState,
> + num_harts, 0,
> + vmstate_info_uint64, uint64_t),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> static void riscv_aclint_mtimer_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -317,6 +332,7 @@ static void riscv_aclint_mtimer_class_init(ObjectClass *klass, void *data)
> device_class_set_props(dc, riscv_aclint_mtimer_properties);
> ResettableClass *rc = RESETTABLE_CLASS(klass);
> rc->phases.enter = riscv_aclint_mtimer_reset_enter;
> + dc->vmsd = &vmstate_riscv_mtimer;
> }
>
> static const TypeInfo riscv_aclint_mtimer_info = {
> @@ -336,6 +352,7 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr, hwaddr size,
> {
> int i;
> DeviceState *dev = qdev_new(TYPE_RISCV_ACLINT_MTIMER);
> + RISCVAclintMTimerState *s = RISCV_ACLINT_MTIMER(dev);
>
> assert(num_harts <= RISCV_ACLINT_MAX_HARTS);
> assert(!(addr & 0x7));
> @@ -366,11 +383,11 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr, hwaddr size,
> riscv_cpu_set_rdtime_fn(env, cpu_riscv_read_rtc, dev);
> }
>
> - cb->s = RISCV_ACLINT_MTIMER(dev);
> + cb->s = s;
> cb->num = i;
> - env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> + s->timers[i] = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> &riscv_aclint_mtimer_cb, cb);
> - env->timecmp = 0;
> + s->timecmp[i] = 0;
>
> qdev_connect_gpio_out(dev, i,
> qdev_get_gpio_in(DEVICE(rvcpu), IRQ_M_TIMER));
> diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c
> index 8c2ca364daab..d8b8e4e1f602 100644
> --- a/hw/timer/ibex_timer.c
> +++ b/hw/timer/ibex_timer.c
> @@ -60,8 +60,6 @@ static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
>
> static void ibex_timer_update_irqs(IbexTimerState *s)
> {
> - CPUState *cs = qemu_get_cpu(0);
> - RISCVCPU *cpu = RISCV_CPU(cs);
> uint64_t value = s->timer_compare_lower0 |
> ((uint64_t)s->timer_compare_upper0 << 32);
> uint64_t next, diff;
> @@ -73,9 +71,9 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> }
>
> /* Update the CPUs mtimecmp */
> - cpu->env.timecmp = value;
> + s->mtimecmp = value;
>
> - if (cpu->env.timecmp <= now) {
> + if (s->mtimecmp <= now) {
> /*
> * If the mtimecmp was in the past raise the interrupt now.
> */
> @@ -91,7 +89,7 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
> qemu_irq_lower(s->m_timer_irq);
> qemu_set_irq(s->irq, false);
>
> - diff = cpu->env.timecmp - now;
> + diff = s->mtimecmp - now;
> next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> muldiv64(diff,
> NANOSECONDS_PER_SECOND,
> @@ -99,9 +97,9 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
>
> if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> /* We overflowed the timer, just set it as large as we can */
> - timer_mod(cpu->env.timer, 0x7FFFFFFFFFFFFFFF);
> + timer_mod(s->mtimer, 0x7FFFFFFFFFFFFFFF);
> } else {
> - timer_mod(cpu->env.timer, next);
> + timer_mod(s->mtimer, next);
> }
> }
>
> @@ -120,11 +118,9 @@ static void ibex_timer_reset(DeviceState *dev)
> {
> IbexTimerState *s = IBEX_TIMER(dev);
>
> - CPUState *cpu = qemu_get_cpu(0);
> - CPURISCVState *env = cpu->env_ptr;
> - env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> + s->mtimer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> &ibex_timer_cb, s);
> - env->timecmp = 0;
> + s->mtimecmp = 0;
>
> s->timer_ctrl = 0x00000000;
> s->timer_cfg0 = 0x00010000;
> diff --git a/include/hw/intc/riscv_aclint.h b/include/hw/intc/riscv_aclint.h
> index 26d4048687fb..693415eb6def 100644
> --- a/include/hw/intc/riscv_aclint.h
> +++ b/include/hw/intc/riscv_aclint.h
> @@ -32,6 +32,8 @@ typedef struct RISCVAclintMTimerState {
> /*< private >*/
> SysBusDevice parent_obj;
> uint64_t time_delta;
> + uint64_t *timecmp;
> + QEMUTimer **timers;
>
> /*< public >*/
> MemoryRegion mmio;
> diff --git a/include/hw/timer/ibex_timer.h b/include/hw/timer/ibex_timer.h
> index 1a0a28d5fab5..41f5c82a920b 100644
> --- a/include/hw/timer/ibex_timer.h
> +++ b/include/hw/timer/ibex_timer.h
> @@ -33,6 +33,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(IbexTimerState, IBEX_TIMER)
> struct IbexTimerState {
> /* <private> */
> SysBusDevice parent_obj;
> + uint64_t mtimecmp;
> + QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
>
> /* <public> */
> MemoryRegion mmio;
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 4be4b82a837d..0fae1569945c 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -308,7 +308,6 @@ struct CPUArchState {
> /* temporary htif regs */
> uint64_t mfromhost;
> uint64_t mtohost;
> - uint64_t timecmp;
>
> /* physical memory protection */
> pmp_table_t pmp_state;
> @@ -363,7 +362,6 @@ struct CPUArchState {
> float_status fp_status;
>
> /* Fields from here on are preserved across CPU reset. */
> - QEMUTimer *timer; /* Internal timer */
>
> hwaddr kernel_addr;
> hwaddr fdt_addr;
> diff --git a/target/riscv/machine.c b/target/riscv/machine.c
> index dc182ca81119..b508b042cb73 100644
> --- a/target/riscv/machine.c
> +++ b/target/riscv/machine.c
> @@ -307,8 +307,8 @@ static const VMStateDescription vmstate_pmu_ctr_state = {
>
> const VMStateDescription vmstate_riscv_cpu = {
> .name = "cpu",
> - .version_id = 3,
> - .minimum_version_id = 3,
> + .version_id = 4,
> + .minimum_version_id = 4,
> .post_load = riscv_cpu_post_load,
> .fields = (VMStateField[]) {
> VMSTATE_UINTTL_ARRAY(env.gpr, RISCVCPU, 32),
> @@ -359,7 +359,6 @@ const VMStateDescription vmstate_riscv_cpu = {
> VMSTATE_UINTTL(env.mscratch, RISCVCPU),
> VMSTATE_UINT64(env.mfromhost, RISCVCPU),
> VMSTATE_UINT64(env.mtohost, RISCVCPU),
> - VMSTATE_UINT64(env.timecmp, RISCVCPU),
>
> VMSTATE_END_OF_LIST()
> },
> --
> 2.25.1
>
>