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 <[email protected]>
On Thu, Nov 9, 2023 at 10:22 AM Leo Hou <[email protected]> wrote:
From: Leo Hou <[email protected]>
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 <[email protected]>
---
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.
Maybe the timer should get a link of the hart array it belongs to,
and offset to this array base hartid?
I'm going to
NACK
this patch until further review / clarifications.
Regards,
Phil.
