The absence of a satp mode in riscv_host_cpu_init() is causing the
following error:

$ ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
    -m 2G -smp 1  -nographic -snapshot \
    -kernel ./guest_imgs/Image \
    -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
    -append "earlycon=sbi root=/dev/ram rw" \
    -cpu host
**
ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
reached
Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
not be reached
Aborted

The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.

For this KVM cpu we would need to inherit the satp supported modes
from the RISC-V host. At this moment this is not possible because the
KVM driver does not support it. And even when it does we can't just let
this broken for every other older kernel.

Since mmu-type is not a required node, according to [1], skip the
'mmu-type' FDT node if there's no satp_mode set. We'll revisit this
logic when we can get satp information from KVM.

[1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml

Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/riscv/virt.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 8ff4b5fd71..ee77b005ef 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -244,13 +244,13 @@ static void create_fdt_socket_cpus(RISCVVirtState *s, int socket,
             s->soc[socket].hartid_base + cpu);
         qemu_fdt_add_subnode(ms->fdt, cpu_name);
 
-        satp_mode_max = satp_mode_max_from_map(
-            s->soc[socket].harts[cpu].cfg.satp_mode.map);
-        sv_name = g_strdup_printf("riscv,%s",
-                                  satp_mode_str(satp_mode_max, is_32_bit));
-        qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
-        g_free(sv_name);
-
+        if (cpu_ptr->cfg.satp_mode.supported != 0) {
+            satp_mode_max = satp_mode_max_from_map(cpu_ptr->cfg.satp_mode.map);
+            sv_name = g_strdup_printf("riscv,%s",
+                                      satp_mode_str(satp_mode_max, is_32_bit));
+            qemu_fdt_setprop_string(ms->fdt, cpu_name, "mmu-type", sv_name);
+            g_free(sv_name);
+        }
 
         name = riscv_isa_string(cpu_ptr);
         qemu_fdt_setprop_string(ms->fdt, cpu_name, "riscv,isa", name);
-- 
2.41.0

On Wed, Jul 05, 2023 at 06:39:37PM -0300, Daniel Henrique Barboza wrote:
> The absence of a satp mode in riscv_host_cpu_init() is causing the
> following error:
> 
> $ ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
>     -m 2G -smp 1  -nographic -snapshot \
>     -kernel ./guest_imgs/Image \
>     -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
>     -append "earlycon=sbi root=/dev/ram rw" \
>     -cpu host
> **
> ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
> reached
> Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
> not be reached
> Aborted
> 
> The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
> It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.
> 
> For this KVM cpu we would need to inherit the satp supported modes
> from the RISC-V host. At this moment this is not possible because the
> KVM driver does not support it. And even when it does we can't just let
> this broken for every other older kernel.
> 
> Since mmu-type is not a required node, according to [1], skip the
> 'mmu-type' FDT node if there's no satp_mode set. We'll revisit this
> logic when we can get satp information from KVM.
> 
> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml

I don't think this is the correct link to reference as backup, as the
generic binding sets out no requirements. I think you would want to link
to the RISC-V specific cpus binding.

That said, things like FreeBSD and U-Boot appear to require mmu-type
https://lore.kernel.org/all/20230705-fondue-bagginess-66c25f1a4135@spud/
so I am wondering if we should in fact make the mmu-type a required
property in the RISC-V specific binding.

Since nommu is covered by an mmu type of "riscv,none", I am kinda
struggling to think of a case where it should be left out (while
describing real hardware at least).

Cheers,
Conor.

On 7/5/23 18:49, Conor Dooley wrote:
> On Wed, Jul 05, 2023 at 06:39:37PM -0300, Daniel Henrique Barboza wrote:
>> The absence of a satp mode in riscv_host_cpu_init() is causing the
>> following error:
>>
>> $ ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
>>      -m 2G -smp 1  -nographic -snapshot \
>>      -kernel ./guest_imgs/Image \
>>      -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
>>      -append "earlycon=sbi root=/dev/ram rw" \
>>      -cpu host
>> **
>> ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
>> reached
>> Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
>> not be reached
>> Aborted
>>
>> The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
>> It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.
>>
>> For this KVM cpu we would need to inherit the satp supported modes
>> from the RISC-V host. At this moment this is not possible because the
>> KVM driver does not support it. And even when it does we can't just let
>> this broken for every other older kernel.
>>
>> Since mmu-type is not a required node, according to [1], skip the
>> 'mmu-type' FDT node if there's no satp_mode set. We'll revisit this
>> logic when we can get satp information from KVM.
>>
>> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml
> 
> I don't think this is the correct link to reference as backup, as the
> generic binding sets out no requirements. I think you would want to link
> to the RISC-V specific cpus binding.

You mean this link?

https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/riscv/cpus.yaml


> 
> That said, things like FreeBSD and U-Boot appear to require mmu-type
> https://lore.kernel.org/all/20230705-fondue-bagginess-66c25f1a4135@spud/
> so I am wondering if we should in fact make the mmu-type a required
> property in the RISC-V specific binding.


To make it required, as far as QEMU is concerned, we'll need to assume a
default value for the 'host' CPU type (e.g. sv57). In the future we can read the
satp host value directly when/if KVM provides satp_mode via get_one_reg().


Thanks,

Daniel

> 
> Since nommu is covered by an mmu type of "riscv,none", I am kinda
> struggling to think of a case where it should be left out (while
> describing real hardware at least).
> 
> Cheers,
> Conor.

On Wed, Jul 05, 2023 at 07:00:52PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 7/5/23 18:49, Conor Dooley wrote:
> > On Wed, Jul 05, 2023 at 06:39:37PM -0300, Daniel Henrique Barboza wrote:
> > > The absence of a satp mode in riscv_host_cpu_init() is causing the
> > > following error:
> > > 
> > > $ ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
> > >      -m 2G -smp 1  -nographic -snapshot \
> > >      -kernel ./guest_imgs/Image \
> > >      -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
> > >      -append "earlycon=sbi root=/dev/ram rw" \
> > >      -cpu host
> > > **
> > > ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
> > > reached
> > > Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
> > > not be reached
> > > Aborted
> > > 
> > > The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
> > > It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.
> > > 
> > > For this KVM cpu we would need to inherit the satp supported modes
> > > from the RISC-V host. At this moment this is not possible because the
> > > KVM driver does not support it. And even when it does we can't just let
> > > this broken for every other older kernel.
> > > 
> > > Since mmu-type is not a required node, according to [1], skip the
> > > 'mmu-type' FDT node if there's no satp_mode set. We'll revisit this
> > > logic when we can get satp information from KVM.
> > > 
> > > [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml
> > 
> > I don't think this is the correct link to reference as backup, as the
> > generic binding sets out no requirements. I think you would want to link
> > to the RISC-V specific cpus binding.
> 
> You mean this link?
> 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/riscv/cpus.yaml

Yeah, that's the correct file. Should probably have linked it, sorry
about that. And in case it was not clear, not suggesting that this would
require a resend, since the reasoning is correct.

> > That said, things like FreeBSD and U-Boot appear to require mmu-type
> > https://lore.kernel.org/all/20230705-fondue-bagginess-66c25f1a4135@spud/
> > so I am wondering if we should in fact make the mmu-type a required
> > property in the RISC-V specific binding.
> 
> 
> To make it required, as far as QEMU is concerned, we'll need to assume a
> default value for the 'host' CPU type (e.g. sv57). In the future we can read the
> satp host value directly when/if KVM provides satp_mode via get_one_reg().

I dunno if assuming is the right thing to do, since it could be actively
wrong. Leaving it out, as you are doing here, is, IMO, nicer to those
guests. Once there's an API for it, I think it could then be added and
then the additional guests would be supported.

Thanks,
Conor.

On 7/5/23 19:12, Conor Dooley wrote:
> On Wed, Jul 05, 2023 at 07:00:52PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 7/5/23 18:49, Conor Dooley wrote:
>>> On Wed, Jul 05, 2023 at 06:39:37PM -0300, Daniel Henrique Barboza wrote:
>>>> The absence of a satp mode in riscv_host_cpu_init() is causing the
>>>> following error:
>>>>
>>>> $ ./qemu/build/qemu-system-riscv64  -machine virt,accel=kvm \
>>>>       -m 2G -smp 1  -nographic -snapshot \
>>>>       -kernel ./guest_imgs/Image \
>>>>       -initrd ./guest_imgs/rootfs_kvm_riscv64.img \
>>>>       -append "earlycon=sbi root=/dev/ram rw" \
>>>>       -cpu host
>>>> **
>>>> ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should not be
>>>> reached
>>>> Bail out! ERROR:../target/riscv/cpu.c:320:satp_mode_str: code should
>>>> not be reached
>>>> Aborted
>>>>
>>>> The error is triggered from create_fdt_socket_cpus() in hw/riscv/virt.c.
>>>> It's trying to get satp_mode_str for a NULL cpu->cfg.satp_mode.map.
>>>>
>>>> For this KVM cpu we would need to inherit the satp supported modes
>>>> from the RISC-V host. At this moment this is not possible because the
>>>> KVM driver does not support it. And even when it does we can't just let
>>>> this broken for every other older kernel.
>>>>
>>>> Since mmu-type is not a required node, according to [1], skip the
>>>> 'mmu-type' FDT node if there's no satp_mode set. We'll revisit this
>>>> logic when we can get satp information from KVM.
>>>>
>>>> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/cpu.yaml
>>>
>>> I don't think this is the correct link to reference as backup, as the
>>> generic binding sets out no requirements. I think you would want to link
>>> to the RISC-V specific cpus binding.
>>
>> You mean this link?
>>
>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/riscv/cpus.yaml
> 
> Yeah, that's the correct file. Should probably have linked it, sorry
> about that. And in case it was not clear, not suggesting that this would
> require a resend, since the reasoning is correct.

I don't mind amending this in case we need another version for any other reason.
Otherwise we'll hope that Alistair will be a true, real gentlemann and amend the
commit msg for us :D

> 
>>> That said, things like FreeBSD and U-Boot appear to require mmu-type
>>> https://lore.kernel.org/all/20230705-fondue-bagginess-66c25f1a4135@spud/
>>> so I am wondering if we should in fact make the mmu-type a required
>>> property in the RISC-V specific binding.
>>
>>
>> To make it required, as far as QEMU is concerned, we'll need to assume a
>> default value for the 'host' CPU type (e.g. sv57). In the future we can read the
>> satp host value directly when/if KVM provides satp_mode via get_one_reg().
> 
> I dunno if assuming is the right thing to do, since it could be actively
> wrong. Leaving it out, as you are doing here, is, IMO, nicer to those
> guests. Once there's an API for it, I think it could then be added and
> then the additional guests would be supported.

Makes sense. We'll revisit this piece of code when that API I sent today find
its way upstream. Thanks,


Daniel

> 
> Thanks,
> Conor.