| Message ID | 20190210201726.3567-1-lukas.auer@aisec.fraunhofer.de |
|---|---|
| State | New |
| Headers | show |
| Series | hw/riscv/virt: re-add machine-specific compatible string to /soc/ node | expand |
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 > >
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
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
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
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
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
Hi Bin, On Mon, 2019-03-11 at 23:28 +0800, Bin Meng wrote: > 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) That's correct. The problem with the default simple-bus U-Boot driver is that it does not have the DM_FLAG_PRE_RELOC flag set. The /soc and /soc/clint nodes are therefore not available before relocation, meaning that IPIs cannot be sent to relocate the secondary harts. Thanks, Lukas > > > 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
Hi Lukas, On Tue, Mar 12, 2019 at 10:39 PM Auer, Lukas <lukas.auer@aisec.fraunhofer.de> wrote: > > Hi Bin, > > On Mon, 2019-03-11 at 23:28 +0800, Bin Meng wrote: > > 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) > > That's correct. The problem with the default simple-bus U-Boot driver > is that it does not have the DM_FLAG_PRE_RELOC flag set. The /soc and > /soc/clint nodes are therefore not available before relocation, meaning > that IPIs cannot be sent to relocate the secondary harts. > Thanks for the clarifications. Now I see the problem. But I think we should fix U-Boot "simple-bus" driver instead. As seen on FU540 or likely other hardware, QEMU generates the "simple-bus" compatible string for the /soc node, as well as the DT provided by the hardware. Regards, Bin
Hi Bin, On Wed, 2019-03-13 at 09:51 +0800, Bin Meng wrote: > Hi Lukas, > > On Tue, Mar 12, 2019 at 10:39 PM Auer, Lukas > <lukas.auer@aisec.fraunhofer.de> wrote: > > Hi Bin, > > > > On Mon, 2019-03-11 at 23:28 +0800, Bin Meng wrote: > > > 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) > > > > That's correct. The problem with the default simple-bus U-Boot > > driver > > is that it does not have the DM_FLAG_PRE_RELOC flag set. The /soc > > and > > /soc/clint nodes are therefore not available before relocation, > > meaning > > that IPIs cannot be sent to relocate the secondary harts. > > > > Thanks for the clarifications. Now I see the problem. But I think we > should fix U-Boot "simple-bus" driver instead. As seen on FU540 or > likely other hardware, QEMU generates the "simple-bus" compatible > string for the /soc node, as well as the DT provided by the hardware. > That makes sense, I can send a patch to set the DM_FLAG_PRE_RELOC flag in the simple-bus U-Boot driver. I think it's best to send it separately from the SMP patch series, since it could affect other boards and it's a bit late in the release cycle. What do you think? This patch can be dropped then. Thanks, Lukas
Hi Lukas, On Fri, Mar 15, 2019 at 5:01 AM Auer, Lukas <lukas.auer@aisec.fraunhofer.de> wrote: > > Hi Bin, > > On Wed, 2019-03-13 at 09:51 +0800, Bin Meng wrote: > > Hi Lukas, > > > > On Tue, Mar 12, 2019 at 10:39 PM Auer, Lukas > > <lukas.auer@aisec.fraunhofer.de> wrote: > > > Hi Bin, > > > > > > On Mon, 2019-03-11 at 23:28 +0800, Bin Meng wrote: > > > > 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) > > > > > > That's correct. The problem with the default simple-bus U-Boot > > > driver > > > is that it does not have the DM_FLAG_PRE_RELOC flag set. The /soc > > > and > > > /soc/clint nodes are therefore not available before relocation, > > > meaning > > > that IPIs cannot be sent to relocate the secondary harts. > > > > > > > Thanks for the clarifications. Now I see the problem. But I think we > > should fix U-Boot "simple-bus" driver instead. As seen on FU540 or > > likely other hardware, QEMU generates the "simple-bus" compatible > > string for the /soc node, as well as the DT provided by the hardware. > > > > That makes sense, I can send a patch to set the DM_FLAG_PRE_RELOC flag > in the simple-bus U-Boot driver. I think it's best to send it > separately from the SMP patch series, since it could affect other > boards and it's a bit late in the release cycle. What do you think? > Yes, agreed that the "simple-bus" driver changes should be next U-Boot release. We should try to get the U-Boot SMP series in the v2019.04 release. > This patch can be dropped then. > Regards, Bin
Hi Bin, On Fri, 2019-03-15 at 09:54 +0800, Bin Meng wrote: > Hi Lukas, > > On Fri, Mar 15, 2019 at 5:01 AM Auer, Lukas > <lukas.auer@aisec.fraunhofer.de> wrote: > > Hi Bin, > > > > On Wed, 2019-03-13 at 09:51 +0800, Bin Meng wrote: > > > Hi Lukas, > > > > > > On Tue, Mar 12, 2019 at 10:39 PM Auer, Lukas > > > <lukas.auer@aisec.fraunhofer.de> wrote: > > > > Hi Bin, > > > > > > > > On Mon, 2019-03-11 at 23:28 +0800, Bin Meng wrote: > > > > > 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) > > > > > > > > That's correct. The problem with the default simple-bus U-Boot > > > > driver > > > > is that it does not have the DM_FLAG_PRE_RELOC flag set. The > > > > /soc > > > > and > > > > /soc/clint nodes are therefore not available before relocation, > > > > meaning > > > > that IPIs cannot be sent to relocate the secondary harts. > > > > > > > > > > Thanks for the clarifications. Now I see the problem. But I think > > > we > > > should fix U-Boot "simple-bus" driver instead. As seen on FU540 > > > or > > > likely other hardware, QEMU generates the "simple-bus" compatible > > > string for the /soc node, as well as the DT provided by the > > > hardware. > > > > > > > That makes sense, I can send a patch to set the DM_FLAG_PRE_RELOC > > flag > > in the simple-bus U-Boot driver. I think it's best to send it > > separately from the SMP patch series, since it could affect other > > boards and it's a bit late in the release cycle. What do you think? > > > > Yes, agreed that the "simple-bus" driver changes should be next U- > Boot > release. We should try to get the U-Boot SMP series in the v2019.04 > release. > Yes definitely, I have just sent version 3 of the series. I hope it is ready to go into mainline now. Thanks, Lukas
On Sun, 10 Feb 2019 12:17:26 PST (-0800), 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> I still had this at the top of my patch queue, but from looking at the thread it appears the resolution here is to drop this in favor of generic support. LMK if I missed something. > --- > > 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);
On Tue, 2019-03-19 at 05:19 -0700, Palmer Dabbelt wrote: > On Sun, 10 Feb 2019 12:17:26 PST (-0800), > 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> > > I still had this at the top of my patch queue, but from looking at > the thread > it appears the resolution here is to drop this in favor of generic > support. > LMK if I missed something. Yes, you are right. We are going to fix this from the U-Boot side, so this patch can be dropped. Thanks, Lukas > > > --- > > > > 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);
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);
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(-)