target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0

[PATCH] target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0

Posted by Daniel Henrique Barboza 9 months ago

In the same emulated RISC-V host, the 'host' KVM CPU takes 4 times
longer to boot than the 'rv64' KVM CPU.

The reason is an unintended behavior of riscv_cpu_satp_mode_finalize()
when satp_mode.supported = 0, i.e. when cpu_init() does not set
satp_mode_max_supported(). satp_mode_max_from_map(map) does:

31 - __builtin_clz(map)

This means that, if satp_mode.supported = 0, satp_mode_supported_max
wil be '31 - 32'. But this is C, so satp_mode_supported_max will gladly
set it to UINT_MAX (4294967295). After that, if the user didn't set a
satp_mode, set_satp_mode_default_map(cpu) will make

cfg.satp_mode.map = cfg.satp_mode.supported

So satp_mode.map = 0. And then satp_mode_map_max will be set to
satp_mode_max_from_map(cpu->cfg.satp_mode.map), i.e. also UINT_MAX. The
guard "satp_mode_map_max > satp_mode_supported_max" doesn't protect us
here since both are UINT_MAX.

And finally we have 2 loops:

        for (int i = satp_mode_map_max - 1; i >= 0; --i) {

Which are, in fact, 2 loops from UINT_MAX -1 to -1. This is where the
extra delay when booting the 'host' CPU is coming from.

Commit 43d1de32f8 already set a precedence for satp_mode.supported = 0
in a different manner. We're doing the same here. If supported == 0,
interpret as 'the CPU wants the OS to handle satp mode alone' and skip
satp_mode_finalize().

We'll also put a guard in satp_mode_max_from_map() to assert out if map
is 0 since the function is not ready to deal with it.

Cc: Alexandre Ghiti <alexghiti@rivosinc.com>
Fixes: 6f23aaeb9b ("riscv: Allow user to set the satp mode")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
 target/riscv/cpu.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d608026a28..86da93c7bc 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -349,6 +349,17 @@ static uint8_t satp_mode_from_str(const char *satp_mode_str)
 
 uint8_t satp_mode_max_from_map(uint32_t map)
 {
+    /*
+     * 'map = 0' will make us return (31 - 32), which C will
+     * happily overflow to UINT_MAX. There's no good result to
+     * return if 'map = 0' (e.g. returning 0 will be ambiguous
+     * with the result for 'map = 1').
+     *
+     * Assert out if map = 0. Callers will have to deal with
+     * it outside of this function.
+     */
+    g_assert(map > 0);
+
     /* map here has at least one bit set, so no problem with clz */
     return 31 - __builtin_clz(map);
 }
@@ -1387,9 +1398,15 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
 static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
 {
     bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
-    uint8_t satp_mode_map_max;
-    uint8_t satp_mode_supported_max =
-                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
+    uint8_t satp_mode_map_max, satp_mode_supported_max;
+
+    /* The CPU wants the OS to decide which satp mode to use */
+    if (cpu->cfg.satp_mode.supported == 0) {
+        return;
+    }
+
+    satp_mode_supported_max =
+                    satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
 
     if (cpu->cfg.satp_mode.map == 0) {
         if (cpu->cfg.satp_mode.init == 0) {
-- 
2.41.0

Re: [PATCH] target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0

Posted by Alistair Francis 8 months, 4 weeks ago

On Thu, Aug 17, 2023 at 11:29 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> In the same emulated RISC-V host, the 'host' KVM CPU takes 4 times
> longer to boot than the 'rv64' KVM CPU.
>
> The reason is an unintended behavior of riscv_cpu_satp_mode_finalize()
> when satp_mode.supported = 0, i.e. when cpu_init() does not set
> satp_mode_max_supported(). satp_mode_max_from_map(map) does:
>
> 31 - __builtin_clz(map)
>
> This means that, if satp_mode.supported = 0, satp_mode_supported_max
> wil be '31 - 32'. But this is C, so satp_mode_supported_max will gladly
> set it to UINT_MAX (4294967295). After that, if the user didn't set a
> satp_mode, set_satp_mode_default_map(cpu) will make
>
> cfg.satp_mode.map = cfg.satp_mode.supported
>
> So satp_mode.map = 0. And then satp_mode_map_max will be set to
> satp_mode_max_from_map(cpu->cfg.satp_mode.map), i.e. also UINT_MAX. The
> guard "satp_mode_map_max > satp_mode_supported_max" doesn't protect us
> here since both are UINT_MAX.
>
> And finally we have 2 loops:
>
>         for (int i = satp_mode_map_max - 1; i >= 0; --i) {
>
> Which are, in fact, 2 loops from UINT_MAX -1 to -1. This is where the
> extra delay when booting the 'host' CPU is coming from.
>
> Commit 43d1de32f8 already set a precedence for satp_mode.supported = 0
> in a different manner. We're doing the same here. If supported == 0,
> interpret as 'the CPU wants the OS to handle satp mode alone' and skip
> satp_mode_finalize().
>
> We'll also put a guard in satp_mode_max_from_map() to assert out if map
> is 0 since the function is not ready to deal with it.
>
> Cc: Alexandre Ghiti <alexghiti@rivosinc.com>
> Fixes: 6f23aaeb9b ("riscv: Allow user to set the satp mode")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/cpu.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d608026a28..86da93c7bc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -349,6 +349,17 @@ static uint8_t satp_mode_from_str(const char *satp_mode_str)
>
>  uint8_t satp_mode_max_from_map(uint32_t map)
>  {
> +    /*
> +     * 'map = 0' will make us return (31 - 32), which C will
> +     * happily overflow to UINT_MAX. There's no good result to
> +     * return if 'map = 0' (e.g. returning 0 will be ambiguous
> +     * with the result for 'map = 1').
> +     *
> +     * Assert out if map = 0. Callers will have to deal with
> +     * it outside of this function.
> +     */
> +    g_assert(map > 0);
> +
>      /* map here has at least one bit set, so no problem with clz */
>      return 31 - __builtin_clz(map);
>  }
> @@ -1387,9 +1398,15 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>  static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>  {
>      bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> -    uint8_t satp_mode_map_max;
> -    uint8_t satp_mode_supported_max =
> -                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> +    uint8_t satp_mode_map_max, satp_mode_supported_max;
> +
> +    /* The CPU wants the OS to decide which satp mode to use */
> +    if (cpu->cfg.satp_mode.supported == 0) {
> +        return;
> +    }
> +
> +    satp_mode_supported_max =
> +                    satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
>
>      if (cpu->cfg.satp_mode.map == 0) {
>          if (cpu->cfg.satp_mode.init == 0) {
> --
> 2.41.0
>
>

Re: [PATCH] target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0

Posted by Andrew Jones 9 months ago

On Thu, Aug 17, 2023 at 12:29:03PM -0300, Daniel Henrique Barboza wrote:
> In the same emulated RISC-V host, the 'host' KVM CPU takes 4 times
> longer to boot than the 'rv64' KVM CPU.
> 
> The reason is an unintended behavior of riscv_cpu_satp_mode_finalize()
> when satp_mode.supported = 0, i.e. when cpu_init() does not set
> satp_mode_max_supported(). satp_mode_max_from_map(map) does:
> 
> 31 - __builtin_clz(map)
> 
> This means that, if satp_mode.supported = 0, satp_mode_supported_max
> wil be '31 - 32'. But this is C, so satp_mode_supported_max will gladly
> set it to UINT_MAX (4294967295). After that, if the user didn't set a
> satp_mode, set_satp_mode_default_map(cpu) will make
> 
> cfg.satp_mode.map = cfg.satp_mode.supported
> 
> So satp_mode.map = 0. And then satp_mode_map_max will be set to
> satp_mode_max_from_map(cpu->cfg.satp_mode.map), i.e. also UINT_MAX. The
> guard "satp_mode_map_max > satp_mode_supported_max" doesn't protect us
> here since both are UINT_MAX.
> 
> And finally we have 2 loops:
> 
>         for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> 
> Which are, in fact, 2 loops from UINT_MAX -1 to -1. This is where the
> extra delay when booting the 'host' CPU is coming from.
> 
> Commit 43d1de32f8 already set a precedence for satp_mode.supported = 0
> in a different manner. We're doing the same here. If supported == 0,
> interpret as 'the CPU wants the OS to handle satp mode alone' and skip
> satp_mode_finalize().
> 
> We'll also put a guard in satp_mode_max_from_map() to assert out if map
> is 0 since the function is not ready to deal with it.
> 
> Cc: Alexandre Ghiti <alexghiti@rivosinc.com>
> Fixes: 6f23aaeb9b ("riscv: Allow user to set the satp mode")
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
>  target/riscv/cpu.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index d608026a28..86da93c7bc 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -349,6 +349,17 @@ static uint8_t satp_mode_from_str(const char *satp_mode_str)
>  
>  uint8_t satp_mode_max_from_map(uint32_t map)
>  {
> +    /*
> +     * 'map = 0' will make us return (31 - 32), which C will
> +     * happily overflow to UINT_MAX. There's no good result to
> +     * return if 'map = 0' (e.g. returning 0 will be ambiguous
> +     * with the result for 'map = 1').
> +     *
> +     * Assert out if map = 0. Callers will have to deal with
> +     * it outside of this function.
> +     */
> +    g_assert(map > 0);
> +
>      /* map here has at least one bit set, so no problem with clz */
>      return 31 - __builtin_clz(map);
>  }
> @@ -1387,9 +1398,15 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>  static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
>  {
>      bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> -    uint8_t satp_mode_map_max;
> -    uint8_t satp_mode_supported_max =
> -                        satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> +    uint8_t satp_mode_map_max, satp_mode_supported_max;
> +
> +    /* The CPU wants the OS to decide which satp mode to use */
> +    if (cpu->cfg.satp_mode.supported == 0) {
> +        return;
> +    }
> +
> +    satp_mode_supported_max =
> +                    satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
>  
>      if (cpu->cfg.satp_mode.map == 0) {
>          if (cpu->cfg.satp_mode.init == 0) {
> -- 
> 2.41.0
> 
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew