| Message ID | 1558108285-19571-2-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, 2019-05-17 at 08:51 -0700, Bin Meng wrote: > At present the PLIC is instantiated to support only one hart, while > the machine allows at most 4 harts to be created. When more than 1 > hart is configured, PLIC needs to instantiated to support multicore, > otherwise an SMP OS does not work. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > > hw/riscv/sifive_u.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index e2120ac..a416d5d 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -344,6 +344,8 @@ static void > riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > MemoryRegion *system_memory = get_system_memory(); > MemoryRegion *mask_rom = g_new(MemoryRegion, 1); > qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; > + char *plic_hart_config; > + size_t plic_hart_config_len; > int i; > Error *err = NULL; > NICInfo *nd = &nd_table[0]; > @@ -357,9 +359,21 @@ static void > riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) > memory_region_add_subregion(system_memory, > memmap[SIFIVE_U_MROM].base, > mask_rom); > > + /* create PLIC hart topology configuration string */ > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * > smp_cpus; > + plic_hart_config = g_malloc0(plic_hart_config_len); > + for (i = 0; i < smp_cpus; i++) { > + if (i != 0) { > + strncat(plic_hart_config, ",", plic_hart_config_len); > + } > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > + plic_hart_config_len); > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + > 1); > + } > + > /* MMIO */ > s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, > - (char *)SIFIVE_U_PLIC_HART_CONFIG, > + plic_hart_config, > SIFIVE_U_PLIC_NUM_SOURCES, > SIFIVE_U_PLIC_NUM_PRIORITIES, > SIFIVE_U_PLIC_PRIORITY_BASE,
On Fri, 17 May 2019 14:35:56 PDT (-0700), Alistair Francis wrote: > On Fri, 2019-05-17 at 08:51 -0700, Bin Meng wrote: >> At present the PLIC is instantiated to support only one hart, while >> the machine allows at most 4 harts to be created. When more than 1 >> hart is configured, PLIC needs to instantiated to support multicore, >> otherwise an SMP OS does not work. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> I got this one as well. > > Alistair > >> --- >> >> hw/riscv/sifive_u.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c >> index e2120ac..a416d5d 100644 >> --- a/hw/riscv/sifive_u.c >> +++ b/hw/riscv/sifive_u.c >> @@ -344,6 +344,8 @@ static void >> riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) >> MemoryRegion *system_memory = get_system_memory(); >> MemoryRegion *mask_rom = g_new(MemoryRegion, 1); >> qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; >> + char *plic_hart_config; >> + size_t plic_hart_config_len; >> int i; >> Error *err = NULL; >> NICInfo *nd = &nd_table[0]; >> @@ -357,9 +359,21 @@ static void >> riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) >> memory_region_add_subregion(system_memory, >> memmap[SIFIVE_U_MROM].base, >> mask_rom); >> >> + /* create PLIC hart topology configuration string */ >> + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * >> smp_cpus; >> + plic_hart_config = g_malloc0(plic_hart_config_len); >> + for (i = 0; i < smp_cpus; i++) { >> + if (i != 0) { >> + strncat(plic_hart_config, ",", plic_hart_config_len); >> + } >> + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, >> + plic_hart_config_len); >> + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + >> 1); >> + } >> + >> /* MMIO */ >> s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, >> - (char *)SIFIVE_U_PLIC_HART_CONFIG, >> + plic_hart_config, >> SIFIVE_U_PLIC_NUM_SOURCES, >> SIFIVE_U_PLIC_NUM_PRIORITIES, >> SIFIVE_U_PLIC_PRIORITY_BASE,
Hi Bin, Thanks for this patch. I know I am very late to the game but I have a comment here. On 17/05/2019 17:51, Bin Meng wrote: > + /* create PLIC hart topology configuration string */ > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; > + plic_hart_config = g_malloc0(plic_hart_config_len); > + for (i = 0; i < smp_cpus; i++) { > + if (i != 0) { > + strncat(plic_hart_config, ",", plic_hart_config_len); > + } > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > + plic_hart_config_len); > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > + } > + This will create up to 4 MS PLIC devices. However on the Unleashed FU540 the PLICs are M,MS,MS,MS,MS because of the monitor hart #0. This means a different memory layout than the real hardware. For instance address 0x0C00_2080 will be hart #0 S-Mode interrupt enables in QEMU, instead of #1 M-Mode interrupt enables for the real hardware. To fix this I suggest to change this loop to: for (i = 0; i < smp_cpus; i++) { if (i != 0) { strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, plic_hart_config_len); } else { strncat(plic_hart_config, "M", plic_hart_config_len); } plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); } This will make hart #0 PLIC in M mode and the others in MS. What do you think? Best regards,
On Mon, Jul 8, 2019 at 9:32 AM Fabien Chouteau <chouteau@adacore.com> wrote: > > Hi Bin, > > Thanks for this patch. > > I know I am very late to the game but I have a comment here. > > On 17/05/2019 17:51, Bin Meng wrote: > > + /* create PLIC hart topology configuration string */ > > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; > > + plic_hart_config = g_malloc0(plic_hart_config_len); > > + for (i = 0; i < smp_cpus; i++) { > > + if (i != 0) { > > + strncat(plic_hart_config, ",", plic_hart_config_len); > > + } > > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > > + plic_hart_config_len); > > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > > + } > > + > > This will create up to 4 MS PLIC devices. However on the Unleashed FU540 the PLICs are M,MS,MS,MS,MS because of the monitor hart #0. > > This means a different memory layout than the real hardware. > > For instance address 0x0C00_2080 will be hart #0 S-Mode interrupt enables in QEMU, instead of #1 M-Mode interrupt enables for the real hardware. > > To fix this I suggest to change this loop to: > > for (i = 0; i < smp_cpus; i++) { > if (i != 0) { > strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, > plic_hart_config_len); > } else { > strncat(plic_hart_config, "M", plic_hart_config_len); > } > plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > } > > This will make hart #0 PLIC in M mode and the others in MS. > > What do you think? I think I understand what you mean, in which case I also think you are correct. Do you want to send a patch and we can discuss there? Alistair > > Best regards, >
Hi Fabien, On Tue, Jul 9, 2019 at 12:31 AM Fabien Chouteau <chouteau@adacore.com> wrote: > > Hi Bin, > > Thanks for this patch. > > I know I am very late to the game but I have a comment here. > > On 17/05/2019 17:51, Bin Meng wrote: > > + /* create PLIC hart topology configuration string */ > > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; > > + plic_hart_config = g_malloc0(plic_hart_config_len); > > + for (i = 0; i < smp_cpus; i++) { > > + if (i != 0) { > > + strncat(plic_hart_config, ",", plic_hart_config_len); > > + } > > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > > + plic_hart_config_len); > > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > > + } > > + > > This will create up to 4 MS PLIC devices. However on the Unleashed FU540 the PLICs are M,MS,MS,MS,MS because of the monitor hart #0. > > This means a different memory layout than the real hardware. > > For instance address 0x0C00_2080 will be hart #0 S-Mode interrupt enables in QEMU, instead of #1 M-Mode interrupt enables for the real hardware. Thanks for the notes! I agree to better match the real hardware, it should be modeled like that. However I am not sure what the original intention was when creating the "sifive_u" machine. Both OpenSBI and U-Boot list sifive_u as a special target, instead of the real Unleashed board hence I assume this is a hypothetical target too, like the "virt", but was created to best match the real Unleashed board though. > > To fix this I suggest to change this loop to: > > for (i = 0; i < smp_cpus; i++) { > if (i != 0) { > strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, > plic_hart_config_len); > } else { > strncat(plic_hart_config, "M", plic_hart_config_len); > } > plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > } > > This will make hart #0 PLIC in M mode and the others in MS. > > What do you think? Regards, Bin
On Sat, Jul 13, 2019 at 8:23 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Fabien, > > On Tue, Jul 9, 2019 at 12:31 AM Fabien Chouteau <chouteau@adacore.com> wrote: > > > > Hi Bin, > > > > Thanks for this patch. > > > > I know I am very late to the game but I have a comment here. > > > > On 17/05/2019 17:51, Bin Meng wrote: > > > + /* create PLIC hart topology configuration string */ > > > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; > > > + plic_hart_config = g_malloc0(plic_hart_config_len); > > > + for (i = 0; i < smp_cpus; i++) { > > > + if (i != 0) { > > > + strncat(plic_hart_config, ",", plic_hart_config_len); > > > + } > > > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > > > + plic_hart_config_len); > > > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > > > + } > > > + > > > > This will create up to 4 MS PLIC devices. However on the Unleashed FU540 the PLICs are M,MS,MS,MS,MS because of the monitor hart #0. > > > > This means a different memory layout than the real hardware. > > > > For instance address 0x0C00_2080 will be hart #0 S-Mode interrupt enables in QEMU, instead of #1 M-Mode interrupt enables for the real hardware. > > Thanks for the notes! I agree to better match the real hardware, it > should be modeled like that. However I am not sure what the original > intention was when creating the "sifive_u" machine. Both OpenSBI and > U-Boot list sifive_u as a special target, instead of the real > Unleashed board hence I assume this is a hypothetical target too, like > the "virt", but was created to best match the real Unleashed board > though. I thought (Palmer correct me if I'm wrong) that the sifive_u machine *should* match the hardware. The problem is that QEMU doesn't support everything that the HW supports which is why U-Boot and OpenSBI have their own targets. The goal is to not require special QEMU targets, so this is a step in the right direction. Alistair > > > > > To fix this I suggest to change this loop to: > > > > for (i = 0; i < smp_cpus; i++) { > > if (i != 0) { > > strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, > > plic_hart_config_len); > > } else { > > strncat(plic_hart_config, "M", plic_hart_config_len); > > } > > plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > > } > > > > This will make hart #0 PLIC in M mode and the others in MS. > > > > What do you think? > > > Regards, > Bin >
Hi Alistair, On Tue, Jul 16, 2019 at 5:33 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Sat, Jul 13, 2019 at 8:23 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > Hi Fabien, > > > > On Tue, Jul 9, 2019 at 12:31 AM Fabien Chouteau <chouteau@adacore.com> wrote: > > > > > > Hi Bin, > > > > > > Thanks for this patch. > > > > > > I know I am very late to the game but I have a comment here. > > > > > > On 17/05/2019 17:51, Bin Meng wrote: > > > > + /* create PLIC hart topology configuration string */ > > > > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; > > > > + plic_hart_config = g_malloc0(plic_hart_config_len); > > > > + for (i = 0; i < smp_cpus; i++) { > > > > + if (i != 0) { > > > > + strncat(plic_hart_config, ",", plic_hart_config_len); > > > > + } > > > > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > > > > + plic_hart_config_len); > > > > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > > > > + } > > > > + > > > > > > This will create up to 4 MS PLIC devices. However on the Unleashed FU540 the PLICs are M,MS,MS,MS,MS because of the monitor hart #0. > > > > > > This means a different memory layout than the real hardware. > > > > > > For instance address 0x0C00_2080 will be hart #0 S-Mode interrupt enables in QEMU, instead of #1 M-Mode interrupt enables for the real hardware. > > > > Thanks for the notes! I agree to better match the real hardware, it > > should be modeled like that. However I am not sure what the original > > intention was when creating the "sifive_u" machine. Both OpenSBI and > > U-Boot list sifive_u as a special target, instead of the real > > Unleashed board hence I assume this is a hypothetical target too, like > > the "virt", but was created to best match the real Unleashed board > > though. > > I thought (Palmer correct me if I'm wrong) that the sifive_u machine > *should* match the hardware. The problem is that QEMU doesn't support > everything that the HW supports which is why U-Boot and OpenSBI have > their own targets. The goal is to not require special QEMU targets, so > this is a step in the right direction. > I've sent a series that improves the emulation fidelity of sifive_u machine, so that the upstream OpenSBI, U-Boot and kernel images built for the SiFive HiFive Unleashed board can be used out of the box without any special hack. Please have a look. http://patchwork.ozlabs.org/project/qemu-devel/list/?series=123386 Regards, Bin
Hi Fabien, On Tue, Jul 9, 2019 at 12:31 AM Fabien Chouteau <chouteau@adacore.com> wrote: > > Hi Bin, > > Thanks for this patch. > > I know I am very late to the game but I have a comment here. > > On 17/05/2019 17:51, Bin Meng wrote: > > + /* create PLIC hart topology configuration string */ > > + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; > > + plic_hart_config = g_malloc0(plic_hart_config_len); > > + for (i = 0; i < smp_cpus; i++) { > > + if (i != 0) { > > + strncat(plic_hart_config, ",", plic_hart_config_len); > > + } > > + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, > > + plic_hart_config_len); > > + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > > + } > > + > > This will create up to 4 MS PLIC devices. However on the Unleashed FU540 the PLICs are M,MS,MS,MS,MS because of the monitor hart #0. > > This means a different memory layout than the real hardware. > > For instance address 0x0C00_2080 will be hart #0 S-Mode interrupt enables in QEMU, instead of #1 M-Mode interrupt enables for the real hardware. > > To fix this I suggest to change this loop to: > > for (i = 0; i < smp_cpus; i++) { > if (i != 0) { > strncat(plic_hart_config, "," SIFIVE_U_PLIC_HART_CONFIG, > plic_hart_config_len); > } else { > strncat(plic_hart_config, "M", plic_hart_config_len); > } > plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); > } > > This will make hart #0 PLIC in M mode and the others in MS. > > What do you think? Thank you for the suggestion. A patch was created for this: http://patchwork.ozlabs.org/patch/1142282/ Regards, Bin
On 05/08/2019 18:10, Bin Meng wrote: > Thank you for the suggestion. A patch was created for this: > http://patchwork.ozlabs.org/patch/1142282/ Awesome, thank you Bin!
diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c index e2120ac..a416d5d 100644 --- a/hw/riscv/sifive_u.c +++ b/hw/riscv/sifive_u.c @@ -344,6 +344,8 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) MemoryRegion *system_memory = get_system_memory(); MemoryRegion *mask_rom = g_new(MemoryRegion, 1); qemu_irq plic_gpios[SIFIVE_U_PLIC_NUM_SOURCES]; + char *plic_hart_config; + size_t plic_hart_config_len; int i; Error *err = NULL; NICInfo *nd = &nd_table[0]; @@ -357,9 +359,21 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, Error **errp) memory_region_add_subregion(system_memory, memmap[SIFIVE_U_MROM].base, mask_rom); + /* create PLIC hart topology configuration string */ + plic_hart_config_len = (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1) * smp_cpus; + plic_hart_config = g_malloc0(plic_hart_config_len); + for (i = 0; i < smp_cpus; i++) { + if (i != 0) { + strncat(plic_hart_config, ",", plic_hart_config_len); + } + strncat(plic_hart_config, SIFIVE_U_PLIC_HART_CONFIG, + plic_hart_config_len); + plic_hart_config_len -= (strlen(SIFIVE_U_PLIC_HART_CONFIG) + 1); + } + /* MMIO */ s->plic = sifive_plic_create(memmap[SIFIVE_U_PLIC].base, - (char *)SIFIVE_U_PLIC_HART_CONFIG, + plic_hart_config, SIFIVE_U_PLIC_NUM_SOURCES, SIFIVE_U_PLIC_NUM_PRIORITIES, SIFIVE_U_PLIC_PRIORITY_BASE,
At present the PLIC is instantiated to support only one hart, while the machine allows at most 4 harts to be created. When more than 1 hart is configured, PLIC needs to instantiated to support multicore, otherwise an SMP OS does not work. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- hw/riscv/sifive_u.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)