Message ID | 20230817152903.694926-1-dbarboza@ventanamicro.com |
---|---|
State | New |
Headers | show |
Series | target/riscv: fix satp_mode_finalize() when satp_mode.supported = 0 | expand |
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
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 > >
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) {
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(-)