* 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).