[Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Lukas Auer]

Re-add the previous compatible string "riscv-virtio-soc" to the soc
device tree node to allow U-Boot and Linux to bind machine-specific
drivers to it. The current compatible string "simple-bus" is retained.

This is required by U-Boot to bind devices early, as part of the
pre-relocation driver model.

Fixes: 53f54508dae6("hw/riscv/virtio: Set the soc device tree node as a
simple-bus")
Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
---

 hw/riscv/virt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 3e8b19c668..c53bb905ff 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -157,6 +157,7 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     char *nodename;
     uint32_t plic_phandle, phandle = 1;
     int i;
+    const char soc_compat[] = "riscv-virtio-soc\0simple-bus";
 
     fdt = s->fdt = create_device_tree(&s->fdt_size);
     if (!fdt) {
@@ -171,7 +172,7 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
 
     qemu_fdt_add_subnode(fdt, "/soc");
     qemu_fdt_setprop(fdt, "/soc", "ranges", NULL, 0);
-    qemu_fdt_setprop_string(fdt, "/soc", "compatible", "simple-bus");
+    qemu_fdt_setprop(fdt, "/soc", "compatible", soc_compat, sizeof(soc_compat));
     qemu_fdt_setprop_cell(fdt, "/soc", "#size-cells", 0x2);
     qemu_fdt_setprop_cell(fdt, "/soc", "#address-cells", 0x2);
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Alistair Francis]

On Sun, Feb 10, 2019 at 2:12 PM Lukas Auer
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Re-add the previous compatible string "riscv-virtio-soc" to the soc
> device tree node to allow U-Boot and Linux to bind machine-specific
> drivers to it. The current compatible string "simple-bus" is retained.
>
> This is required by U-Boot to bind devices early, as part of the
> pre-relocation driver model.
>
> Fixes: 53f54508dae6("hw/riscv/virtio: Set the soc device tree node as a
> simple-bus")
> Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>

This looks fine to me.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
>  hw/riscv/virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 3e8b19c668..c53bb905ff 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -157,6 +157,7 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>      char *nodename;
>      uint32_t plic_phandle, phandle = 1;
>      int i;
> +    const char soc_compat[] = "riscv-virtio-soc\0simple-bus";
>
>      fdt = s->fdt = create_device_tree(&s->fdt_size);
>      if (!fdt) {
> @@ -171,7 +172,7 @@ static void *create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>
>      qemu_fdt_add_subnode(fdt, "/soc");
>      qemu_fdt_setprop(fdt, "/soc", "ranges", NULL, 0);
> -    qemu_fdt_setprop_string(fdt, "/soc", "compatible", "simple-bus");
> +    qemu_fdt_setprop(fdt, "/soc", "compatible", soc_compat, sizeof(soc_compat));
>      qemu_fdt_setprop_cell(fdt, "/soc", "#size-cells", 0x2);
>      qemu_fdt_setprop_cell(fdt, "/soc", "#address-cells", 0x2);
>
> --
> 2.20.1
>
>

Re: [Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Bin Meng]

Hi Lukas,

On Mon, Feb 11, 2019 at 6:13 AM Lukas Auer
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Re-add the previous compatible string "riscv-virtio-soc" to the soc
> device tree node to allow U-Boot and Linux to bind machine-specific
> drivers to it. The current compatible string "simple-bus" is retained.
>
> This is required by U-Boot to bind devices early, as part of the
> pre-relocation driver model.
>

I see no problem with U-Boot working with current compatible string
"simple-bus". In fact I had planned to remove the compatible string
"riscv-virtio-soc" in U-Boot but did not get time to work on it.

> Fixes: 53f54508dae6("hw/riscv/virtio: Set the soc device tree node as a
> simple-bus")
> Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> ---
>
>  hw/riscv/virt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>

Regards,
Bin

Re: [Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Auer, Lukas]

Hi Bin,

On Sun, 2019-03-10 at 09:07 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Mon, Feb 11, 2019 at 6:13 AM Lukas Auer
> <lukas.auer@aisec.fraunhofer.de> wrote:
> > Re-add the previous compatible string "riscv-virtio-soc" to the soc
> > device tree node to allow U-Boot and Linux to bind machine-specific
> > drivers to it. The current compatible string "simple-bus" is
> > retained.
> > 
> > This is required by U-Boot to bind devices early, as part of the
> > pre-relocation driver model.
> > 
> 
> I see no problem with U-Boot working with current compatible string
> "simple-bus". In fact I had planned to remove the compatible string
> "riscv-virtio-soc" in U-Boot but did not get time to work on it.
> 

It is only required if U-Boot is running in machine-mode. For
relocation it needs to use the CLINT driver to send appropriate IPIs to
the other harts. To be able to probe the driver, the device and its
parent device tree node (soc) must therefore be available in the pre-
relocation device model.
This patch was the easiest way I could think of for achieving this. It
could be that there is a better way of solving this.

Thanks,
Lukas

> > Fixes: 53f54508dae6("hw/riscv/virtio: Set the soc device tree node
> > as a
> > simple-bus")
> > Signed-off-by: Lukas Auer <lukas.auer@aisec.fraunhofer.de>
> > ---
> > 
> >  hw/riscv/virt.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> 
> Regards,
> Bin

Re: [Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Bin Meng]

Hi Lukas,

On Sun, Mar 10, 2019 at 9:44 PM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Sun, 2019-03-10 at 09:07 +0800, Bin Meng wrote:
> > Hi Lukas,
> >
> > On Mon, Feb 11, 2019 at 6:13 AM Lukas Auer
> > <lukas.auer@aisec.fraunhofer.de> wrote:
> > > Re-add the previous compatible string "riscv-virtio-soc" to the soc
> > > device tree node to allow U-Boot and Linux to bind machine-specific
> > > drivers to it. The current compatible string "simple-bus" is
> > > retained.
> > >
> > > This is required by U-Boot to bind devices early, as part of the
> > > pre-relocation driver model.
> > >
> >
> > I see no problem with U-Boot working with current compatible string
> > "simple-bus". In fact I had planned to remove the compatible string
> > "riscv-virtio-soc" in U-Boot but did not get time to work on it.
> >
>
> It is only required if U-Boot is running in machine-mode. For
> relocation it needs to use the CLINT driver to send appropriate IPIs to
> the other harts. To be able to probe the driver, the device and its
> parent device tree node (soc) must therefore be available in the pre-
> relocation device model.
> This patch was the easiest way I could think of for achieving this. It
> could be that there is a better way of solving this.
>

I tested your SMP U-Boot series in both M-mode and S-mode, using a 4
core 'virt' target. Works fine. I am using QEMU 3.1.0 so it is
"simple-bus".

Regards,
Bin

Re: [Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Auer, Lukas]

Hi Bin,

On Sun, 2019-03-10 at 22:57 +0800, Bin Meng wrote:
> Hi Lukas,
> 
> On Sun, Mar 10, 2019 at 9:44 PM Auer, Lukas
> <lukas.auer@aisec.fraunhofer.de> wrote:
> > Hi Bin,
> > 
> > On Sun, 2019-03-10 at 09:07 +0800, Bin Meng wrote:
> > > Hi Lukas,
> > > 
> > > On Mon, Feb 11, 2019 at 6:13 AM Lukas Auer
> > > <lukas.auer@aisec.fraunhofer.de> wrote:
> > > > Re-add the previous compatible string "riscv-virtio-soc" to the
> > > > soc
> > > > device tree node to allow U-Boot and Linux to bind machine-
> > > > specific
> > > > drivers to it. The current compatible string "simple-bus" is
> > > > retained.
> > > > 
> > > > This is required by U-Boot to bind devices early, as part of
> > > > the
> > > > pre-relocation driver model.
> > > > 
> > > 
> > > I see no problem with U-Boot working with current compatible
> > > string
> > > "simple-bus". In fact I had planned to remove the compatible
> > > string
> > > "riscv-virtio-soc" in U-Boot but did not get time to work on it.
> > > 
> > 
> > It is only required if U-Boot is running in machine-mode. For
> > relocation it needs to use the CLINT driver to send appropriate
> > IPIs to
> > the other harts. To be able to probe the driver, the device and its
> > parent device tree node (soc) must therefore be available in the
> > pre-
> > relocation device model.
> > This patch was the easiest way I could think of for achieving this.
> > It
> > could be that there is a better way of solving this.
> > 
> 
> I tested your SMP U-Boot series in both M-mode and S-mode, using a 4
> core 'virt' target. Works fine. I am using QEMU 3.1.0 so it is
> "simple-bus".
> 

That is actually my fault, it should not work.
What is happening is that U-Boot fails to relocate the secondary harts,
because the CLINT driver cannot get the memory address of the CLINT
device. This error is currently silently ignored.
The secondary harts are still waiting to receive IPIs, so booting Linux
works fine, because U-Boot can now send IPIs. This will however break
if U-Boot overwrites the code the secondary harts are running, which
could happen when loading an image.

I will update my SMP U-Boot series to print a warning if sending an IPI
fails.

Thanks,
Lukas

Re: [Qemu-devel] [PATCH] hw/riscv/virt: re-add machine-specific compatible string to /soc/ node

[Bin Meng]

Hi Lukas,

On Mon, Mar 11, 2019 at 2:03 AM Auer, Lukas
<lukas.auer@aisec.fraunhofer.de> wrote:
>
> Hi Bin,
>
> On Sun, 2019-03-10 at 22:57 +0800, Bin Meng wrote:
> > Hi Lukas,
> >
> > On Sun, Mar 10, 2019 at 9:44 PM Auer, Lukas
> > <lukas.auer@aisec.fraunhofer.de> wrote:
> > > Hi Bin,
> > >
> > > On Sun, 2019-03-10 at 09:07 +0800, Bin Meng wrote:
> > > > Hi Lukas,
> > > >
> > > > On Mon, Feb 11, 2019 at 6:13 AM Lukas Auer
> > > > <lukas.auer@aisec.fraunhofer.de> wrote:
> > > > > Re-add the previous compatible string "riscv-virtio-soc" to the
> > > > > soc
> > > > > device tree node to allow U-Boot and Linux to bind machine-
> > > > > specific
> > > > > drivers to it. The current compatible string "simple-bus" is
> > > > > retained.
> > > > >
> > > > > This is required by U-Boot to bind devices early, as part of
> > > > > the
> > > > > pre-relocation driver model.
> > > > >
> > > >
> > > > I see no problem with U-Boot working with current compatible
> > > > string
> > > > "simple-bus". In fact I had planned to remove the compatible
> > > > string
> > > > "riscv-virtio-soc" in U-Boot but did not get time to work on it.
> > > >
> > >
> > > It is only required if U-Boot is running in machine-mode. For
> > > relocation it needs to use the CLINT driver to send appropriate
> > > IPIs to
> > > the other harts. To be able to probe the driver, the device and its
> > > parent device tree node (soc) must therefore be available in the
> > > pre-
> > > relocation device model.
> > > This patch was the easiest way I could think of for achieving this.
> > > It
> > > could be that there is a better way of solving this.
> > >
> >
> > I tested your SMP U-Boot series in both M-mode and S-mode, using a 4
> > core 'virt' target. Works fine. I am using QEMU 3.1.0 so it is
> > "simple-bus".
> >
>
> That is actually my fault, it should not work.
> What is happening is that U-Boot fails to relocate the secondary harts,
> because the CLINT driver cannot get the memory address of the CLINT
> device. This error is currently silently ignored.

I still don't understand. Why does the CLINT driver fail to get the
memory address? U-Boot has been supporting "simpile-bus" for a long
time. It was because QEMU 3.0.0 generated the /soc node with
"riscv-virtio-soc" compatible string, U-Boot was taught to treat such
compatible string as a "simple-bus" too (that was the U-Boot commit
27dc2c130e29)

> The secondary harts are still waiting to receive IPIs, so booting Linux
> works fine, because U-Boot can now send IPIs. This will however break
> if U-Boot overwrites the code the secondary harts are running, which
> could happen when loading an image.
>
> I will update my SMP U-Boot series to print a warning if sending an IPI
> fails.
>

Regards,
Bin