diff mbox series

[2/2] riscv: sifive_u: Update the plic hart config to support multicore

Message ID 1558108285-19571-2-git-send-email-bmeng.cn@gmail.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Bin Meng May 17, 2019, 3:51 p.m. UTC
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(-)

Comments

Alistair Francis May 17, 2019, 9:35 p.m. UTC | #1
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,
Palmer Dabbelt June 26, 2019, 7:59 a.m. UTC | #2
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,
Fabien Chouteau July 8, 2019, 4:31 p.m. UTC | #3
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,
Alistair Francis July 10, 2019, 11:40 p.m. UTC | #4
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,
>
Bin Meng July 14, 2019, 3:22 a.m. UTC | #5
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
Alistair Francis July 15, 2019, 9:30 p.m. UTC | #6
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
>
Bin Meng Aug. 5, 2019, 4:09 p.m. UTC | #7
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
Bin Meng Aug. 5, 2019, 4:10 p.m. UTC | #8
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
Fabien Chouteau Aug. 5, 2019, 4:19 p.m. UTC | #9
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 mbox series

Patch

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,