Re: [Qemu-devel] [Qemu-riscv] [PATCH 2/2] riscv: sifive_u: Update the plic hart config to support multicore

[Alistair Francis]

On Sat, Jul 13, 2019 at 8:23 PM Bin Meng <address@hidden> wrote:
>
> Hi Fabien,
>
> On Tue, Jul 9, 2019 at 12:31 AM Fabien Chouteau <address@hidden> 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
>