On 9/11/23 23:26, Philippe Mathieu-Daudé wrote:
Hi Leo,
First, I can't find your patch in my mailbox, so I'm replying to
Dongxue's review.
On 9/11/23 03:41, Dongxue Zhang wrote:
Reviewed-by: Dongxue Zhang <zhangdongxue@canaan-creative.com>
On Thu, Nov 9, 2023 at 10:22 AM Leo Hou <LeoHou@canaan-creative.com> wrote:
From: Leo Hou <houyingle@canaan-creative.com>
cpu_by_arch_id() uses hartid-base as the index to obtain the corresponding
CPUState structure variable.
qemu_get_cpu() uses cpu_index as the index to obtain the corresponding CPUState
structure variable.
In heterogeneous CPU or multi-socket scenarios, multiple aclint needs to be
instantiated,
and the hartid-base of each cpu bound by aclint can start from 0. If
cpu_by_arch_id() is still used
in this case, all aclint will bind to the earliest initialized hart with
hartid-base 0 and cause conflicts.
So with cpu_index as the index, use qemu_get_cpu() to get the CPUState struct
variable,
and connect the aclint interrupt line to the hart of the CPU indexed with
cpu_index
(the corresponding hartid-base can start at 0). It's more reasonable.
Signed-off-by: Leo Hou <houyingle@canaan-creative.com>
---
hw/intc/riscv_aclint.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/hw/intc/riscv_aclint.c b/hw/intc/riscv_aclint.c
index ab1a0b4b3a..be8f539fcb 100644
--- a/hw/intc/riscv_aclint.c
+++ b/hw/intc/riscv_aclint.c
@@ -130,7 +130,7 @@ static uint64_t riscv_aclint_mtimer_read(void *opaque,
hwaddr addr,
addr < (mtimer->timecmp_base + (mtimer->num_harts << 3))) {
size_t hartid = mtimer->hartid_base +
((addr - mtimer->timecmp_base) >> 3);
- CPUState *cpu = cpu_by_arch_id(hartid);
+ CPUState *cpu = qemu_get_cpu(hartid);
There is some code smell here. qemu_get_cpu() shouldn't be called by
device models, but only by accelerators.
Yes, qemu_get_cpu() is designed to be called by accelerators.
But there is currently no new API to support multi-socket and
heterogeneous processor architectures，and sifive_plic has been
designed with qemu_get_cpu().
Please refer to:
[1]
https://lore.kernel.org/qemu-devel/1519683480-33201-16-git-send-email-mjc@sifive.com/
[2]
https://lore.kernel.org/qemu-devel/20200825184836.1282371-3-alistair.francis@wdc.com/
Maybe the timer should get a link of the hart array it belongs to,
and offset to this array base hartid?
The same problem exists not only with timer, but also with aclint.
There needs to be a general approach to this problem.