* Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index
[not found] ` <6d1d8cc8c2b37b145e4a826095619097fa4a34d5.1699496263.git.houyingle@canaan-creative.com>
@ 2023-11-09 2:41 ` Dongxue Zhang
2023-11-09 15:26 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 6+ messages in thread
From: Dongxue Zhang @ 2023-11-09 2:41 UTC (permalink / raw)
To: Leo Hou
Cc: Alistair Francis, Anup Patel, Bin Meng, Daniel Henrique Barboza,
Leo Hou, Liu Zhiwei, Mayuresh Chitale, Palmer Dabbelt,
Palmer Dabbelt, Weiwei Li, QEMU Developers, qemu-riscv, Elta Era
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);
> CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> if (!env) {
> qemu_log_mask(LOG_GUEST_ERROR,
> @@ -173,7 +173,7 @@ static void riscv_aclint_mtimer_write(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);
> CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> if (!env) {
> qemu_log_mask(LOG_GUEST_ERROR,
> @@ -232,7 +232,7 @@ static void riscv_aclint_mtimer_write(void *opaque, hwaddr addr,
>
> /* Check if timer interrupt is triggered for each hart. */
> for (i = 0; i < mtimer->num_harts; i++) {
> - CPUState *cpu = cpu_by_arch_id(mtimer->hartid_base + i);
> + CPUState *cpu = qemu_get_cpu(mtimer->hartid_base + i);
> CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> if (!env) {
> continue;
> @@ -293,7 +293,7 @@ static void riscv_aclint_mtimer_realize(DeviceState *dev, Error **errp)
> 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(cpu_by_arch_id(s->hartid_base + i));
> + RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(s->hartid_base + i));
> if (riscv_cpu_claim_interrupts(cpu, MIP_MTIP) < 0) {
> error_report("MTIP already claimed");
> exit(1);
> @@ -373,7 +373,7 @@ DeviceState *riscv_aclint_mtimer_create(hwaddr addr, hwaddr size,
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
>
> for (i = 0; i < num_harts; i++) {
> - CPUState *cpu = cpu_by_arch_id(hartid_base + i);
> + CPUState *cpu = qemu_get_cpu(hartid_base + i);
> RISCVCPU *rvcpu = RISCV_CPU(cpu);
> CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> riscv_aclint_mtimer_callback *cb =
> @@ -408,7 +408,7 @@ static uint64_t riscv_aclint_swi_read(void *opaque, hwaddr addr,
>
> if (addr < (swi->num_harts << 2)) {
> size_t hartid = swi->hartid_base + (addr >> 2);
> - CPUState *cpu = cpu_by_arch_id(hartid);
> + CPUState *cpu = qemu_get_cpu(hartid);
> CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> if (!env) {
> qemu_log_mask(LOG_GUEST_ERROR,
> @@ -431,7 +431,7 @@ static void riscv_aclint_swi_write(void *opaque, hwaddr addr, uint64_t value,
>
> if (addr < (swi->num_harts << 2)) {
> size_t hartid = swi->hartid_base + (addr >> 2);
> - CPUState *cpu = cpu_by_arch_id(hartid);
> + CPUState *cpu = qemu_get_cpu(hartid);
> CPURISCVState *env = cpu ? cpu_env(cpu) : NULL;
> if (!env) {
> qemu_log_mask(LOG_GUEST_ERROR,
> @@ -546,7 +546,7 @@ DeviceState *riscv_aclint_swi_create(hwaddr addr, uint32_t hartid_base,
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
>
> for (i = 0; i < num_harts; i++) {
> - CPUState *cpu = cpu_by_arch_id(hartid_base + i);
> + CPUState *cpu = qemu_get_cpu(hartid_base + i);
> RISCVCPU *rvcpu = RISCV_CPU(cpu);
>
> qdev_connect_gpio_out(dev, i,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index
2023-11-09 2:41 ` [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index Dongxue Zhang
@ 2023-11-09 15:26 ` Philippe Mathieu-Daudé
2023-11-13 4:25 ` LeoHou
0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-09 15:26 UTC (permalink / raw)
To: Dongxue Zhang, Leo Hou
Cc: Alistair Francis, Anup Patel, Bin Meng, Daniel Henrique Barboza,
Leo Hou, Liu Zhiwei, Mayuresh Chitale, Palmer Dabbelt,
Palmer Dabbelt, Weiwei Li, QEMU Developers, qemu-riscv
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.
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index
2023-11-09 15:26 ` Philippe Mathieu-Daudé
@ 2023-11-13 4:25 ` LeoHou
2023-11-20 12:16 ` Philippe Mathieu-Daudé
2023-11-24 14:47 ` Igor Mammedov
0 siblings, 2 replies; 6+ messages in thread
From: LeoHou @ 2023-11-13 4:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, Dongxue Zhang
Cc: Alistair Francis, Anup Patel, Bin Meng, Daniel Henrique Barboza,
侯英乐, Liu Zhiwei, Mayuresh Chitale,
Palmer Dabbelt, Palmer Dabbelt, Weiwei Li, QEMU Developers,
qemu-riscv, 张东雪, leohou1402@gmail.com
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.
>I'm going to
>NACK
>this patch until further review / clarifications.
Regards,
Leo Hou.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index
2023-11-13 4:25 ` LeoHou
@ 2023-11-20 12:16 ` Philippe Mathieu-Daudé
2023-11-21 4:25 ` Alistair Francis
2023-11-24 14:47 ` Igor Mammedov
1 sibling, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-11-20 12:16 UTC (permalink / raw)
To: LeoHou, Dongxue Zhang
Cc: Alistair Francis, Anup Patel, Bin Meng, Daniel Henrique Barboza,
侯英乐, Liu Zhiwei, Mayuresh Chitale,
Palmer Dabbelt, Palmer Dabbelt, Weiwei Li, QEMU Developers,
qemu-riscv, 张东雪, leohou1402@gmail.com
On 13/11/23 05:25, LeoHou wrote:
> 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.
Right. However since there is no heterogeneous support in QEMU
at present, we don't need this patch in the next release.
So I'd rather wait and work on a correct fix. Up to the maintainer.
Regards,
Phil.
>> I'm going to
>> NACK
>> this patch until further review / clarifications.
>
> Regards,
>
> Leo Hou.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index
2023-11-20 12:16 ` Philippe Mathieu-Daudé
@ 2023-11-21 4:25 ` Alistair Francis
0 siblings, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2023-11-21 4:25 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: LeoHou, Dongxue Zhang, Alistair Francis, Anup Patel, Bin Meng,
Daniel Henrique Barboza, 侯英乐, Liu Zhiwei,
Mayuresh Chitale, Palmer Dabbelt, Palmer Dabbelt, Weiwei Li,
QEMU Developers, qemu-riscv, 张东雪,
leohou1402@gmail.com
On Mon, Nov 20, 2023 at 10:17 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 13/11/23 05:25, LeoHou wrote:
> > 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.
I also can't find the original patch
> >>
> >> 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
This doesn't sound right
cpu_by_arch_id() calls riscv_get_arch_id() to compare against the id.
riscv_get_arch_id() just returns the mhartid.
From the RISC-V priv spec on mhartid:
Hart IDs might not necessarily be numbered contiguously in a
multiprocessor system, but at least one hart must have a hart ID of
zero. Hart IDs must be unique within the execution environment.
So no two harts should have the same ID. So only a single aclint
should have a hartid-base of 0.
From a quick look I couldn't see where we set mhartid though, so it's
possible we just don't set it or set it correctly. In which case that
is the bug to be fixed.
Alistair
> >>>> 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.
>
> Right. However since there is no heterogeneous support in QEMU
> at present, we don't need this patch in the next release.
>
> So I'd rather wait and work on a correct fix. Up to the maintainer.
>
> Regards,
>
> Phil.
>
> >> I'm going to
> >> NACK
> >> this patch until further review / clarifications.
> >
> > Regards,
> >
> > Leo Hou.
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index
2023-11-13 4:25 ` LeoHou
2023-11-20 12:16 ` Philippe Mathieu-Daudé
@ 2023-11-24 14:47 ` Igor Mammedov
1 sibling, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2023-11-24 14:47 UTC (permalink / raw)
To: LeoHou
Cc: Philippe Mathieu-Daudé, Dongxue Zhang, Alistair Francis,
Anup Patel, Bin Meng, Daniel Henrique Barboza,
侯英乐, Liu Zhiwei, Mayuresh Chitale,
Palmer Dabbelt, Palmer Dabbelt, Weiwei Li, QEMU Developers,
qemu-riscv, 张东雪, leohou1402@gmail.com
On Mon, 13 Nov 2023 04:25:43 +0000
LeoHou <LeoHou@canaan-creative.com> wrote:
> 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.
cpu_index might match arch_id in simple cases but might be not the same
when architecture/topo gets more complex, and just breaks in case of
sparse or out of order created CPUs/intc (hotplug case).
So using it is like a ticking bomb.
> 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.
>
>
> >I'm going to
> >NACK
> >this patch until further review / clarifications.
>
> Regards,
>
> Leo Hou.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-24 14:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <cover.1699496263.git.houyingle@canaan-creative.com>
[not found] ` <6d1d8cc8c2b37b145e4a826095619097fa4a34d5.1699496263.git.houyingle@canaan-creative.com>
2023-11-09 2:41 ` [PATCH 1/1] hw/intc/riscv_aclint:Change the way to get CPUState from hard-base to pu_index Dongxue Zhang
2023-11-09 15:26 ` Philippe Mathieu-Daudé
2023-11-13 4:25 ` LeoHou
2023-11-20 12:16 ` Philippe Mathieu-Daudé
2023-11-21 4:25 ` Alistair Francis
2023-11-24 14:47 ` Igor Mammedov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).