From: Alistair Francis <alistair23@gmail.com>
To: Alexandre Ghiti <alexghiti@rivosinc.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>,
Alistair Francis <alistair.francis@wdc.com>,
Bin Meng <bin.meng@windriver.com>,
Andrew Jones <ajones@ventanamicro.com>,
Frank Chang <frank.chang@sifive.com>,
qemu-riscv@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities
Date: Tue, 24 Jan 2023 10:41:05 +1000 [thread overview]
Message-ID: <CAKmqyKPrGGt1RpKbLr0zmjNz_D4B7xkELYyGA+LAhtDTqSRd2w@mail.gmail.com> (raw)
In-Reply-To: <20230123090324.732681-6-alexghiti@rivosinc.com>
On Mon, Jan 23, 2023 at 7:09 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Currently, the max satp mode is set with the only constraint that it must be
> implemented in qemu, i.e. set in valid_vm_1_10_[32|64].
>
> But we actually need to add another level of constraint: what the hw is
> actually capable of, because currently, a linux booting on a sifive-u54
> boots in sv57 mode which is incompatible with the cpu's sv39 max
> capability.
>
> So add a new bitmap to RISCVSATPMap which contains this capability and
> initialize it in every XXX_cpu_init.
>
> Finally, we have the following chain of constraints:
>
> Qemu capability > HW capability > User choice > Software capability
>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> ---
> target/riscv/cpu.c | 78 +++++++++++++++++++++++++++++++---------------
> target/riscv/cpu.h | 8 +++--
> 2 files changed, 59 insertions(+), 27 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index e409e6ab64..19a37fee2b 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -292,24 +292,39 @@ const char *satp_mode_str(uint8_t satp_mode, bool is_32_bit)
> g_assert_not_reached();
> }
>
> -/* Sets the satp mode to the max supported */
> -static void set_satp_mode_default(RISCVCPU *cpu, bool is_32_bit)
> +static void set_satp_mode_max_supported(RISCVCPU *cpu,
> + const char *satp_mode_str,
> + bool is_32_bit)
> {
> - if (riscv_feature(&cpu->env, RISCV_FEATURE_MMU)) {
> - cpu->cfg.satp_mode.map |=
> - (1 << satp_mode_from_str(is_32_bit ? "sv32" : "sv57"));
> - } else {
> - cpu->cfg.satp_mode.map |= (1 << satp_mode_from_str("mbare"));
> + uint8_t satp_mode = satp_mode_from_str(satp_mode_str);
> + const bool *valid_vm = is_32_bit ? valid_vm_1_10_32 : valid_vm_1_10_64;
> +
> + for (int i = 0; i <= satp_mode; ++i) {
> + if (valid_vm[i]) {
> + cpu->cfg.satp_mode.supported |= (1 << i);
> + }
> }
> }
>
> +/* Sets the satp mode to the max supported */
> +static void set_satp_mode_default(RISCVCPU *cpu)
> +{
> + uint8_t satp_mode = satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
> +
> + cpu->cfg.satp_mode.map |= (1 << satp_mode);
> +}
> +
> static void riscv_any_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> + RISCVCPU *cpu = RISCV_CPU(obj);
> +
> #if defined(TARGET_RISCV32)
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> + set_satp_mode_max_supported(cpu, "sv32", true);
> #elif defined(TARGET_RISCV64)
> set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> + set_satp_mode_max_supported(cpu, "sv57", false);
> #endif
> set_priv_version(env, PRIV_VERSION_1_12_0);
> register_cpu_props(obj);
> @@ -319,18 +334,24 @@ static void riscv_any_cpu_init(Object *obj)
> static void rv64_base_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> + RISCVCPU *cpu = RISCV_CPU(obj);
> +
> /* We set this in the realise function */
> set_misa(env, MXL_RV64, 0);
> register_cpu_props(obj);
> /* Set latest version of privileged specification */
> set_priv_version(env, PRIV_VERSION_1_12_0);
> + set_satp_mode_max_supported(cpu, "sv57", false);
> }
>
> static void rv64_sifive_u_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> + RISCVCPU *cpu = RISCV_CPU(obj);
> +
> set_misa(env, MXL_RV64, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> set_priv_version(env, PRIV_VERSION_1_10_0);
> + set_satp_mode_max_supported(cpu, "sv39", false);
Can we just not expose the properties on these vendor CPUs and then
not worry about setting maximums?
Alistair
> }
>
> static void rv64_sifive_e_cpu_init(Object *obj)
> @@ -341,6 +362,7 @@ static void rv64_sifive_e_cpu_init(Object *obj)
> set_misa(env, MXL_RV64, RVI | RVM | RVA | RVC | RVU);
> set_priv_version(env, PRIV_VERSION_1_10_0);
> cpu->cfg.mmu = false;
> + set_satp_mode_max_supported(cpu, "mbare", false);
> }
>
> static void rv128_base_cpu_init(Object *obj)
> @@ -352,11 +374,13 @@ static void rv128_base_cpu_init(Object *obj)
> exit(EXIT_FAILURE);
> }
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> + RISCVCPU *cpu = RISCV_CPU(obj);
> /* We set this in the realise function */
> set_misa(env, MXL_RV128, 0);
> register_cpu_props(obj);
> /* Set latest version of privileged specification */
> set_priv_version(env, PRIV_VERSION_1_12_0);
> + set_satp_mode_max_supported(cpu, "sv57", false);
> }
> #else
> static void rv32_base_cpu_init(Object *obj)
> @@ -367,13 +391,17 @@ static void rv32_base_cpu_init(Object *obj)
> register_cpu_props(obj);
> /* Set latest version of privileged specification */
> set_priv_version(env, PRIV_VERSION_1_12_0);
> + set_satp_mode_max_supported(cpu, "sv32", true);
> }
>
> static void rv32_sifive_u_cpu_init(Object *obj)
> {
> CPURISCVState *env = &RISCV_CPU(obj)->env;
> + RISCVCPU *cpu = RISCV_CPU(obj);
> +
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVD | RVC | RVS | RVU);
> set_priv_version(env, PRIV_VERSION_1_10_0);
> + set_satp_mode_max_supported(cpu, "sv32", true);
> }
>
> static void rv32_sifive_e_cpu_init(Object *obj)
> @@ -384,6 +412,7 @@ static void rv32_sifive_e_cpu_init(Object *obj)
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVC | RVU);
> set_priv_version(env, PRIV_VERSION_1_10_0);
> cpu->cfg.mmu = false;
> + set_satp_mode_max_supported(cpu, "mbare", true);
> }
>
> static void rv32_ibex_cpu_init(Object *obj)
> @@ -394,6 +423,7 @@ static void rv32_ibex_cpu_init(Object *obj)
> set_misa(env, MXL_RV32, RVI | RVM | RVC | RVU);
> set_priv_version(env, PRIV_VERSION_1_11_0);
> cpu->cfg.mmu = false;
> + set_satp_mode_max_supported(cpu, "mbare", true);
> cpu->cfg.epmp = true;
> }
>
> @@ -405,6 +435,7 @@ static void rv32_imafcu_nommu_cpu_init(Object *obj)
> set_misa(env, MXL_RV32, RVI | RVM | RVA | RVF | RVC | RVU);
> set_priv_version(env, PRIV_VERSION_1_10_0);
> cpu->cfg.mmu = false;
> + set_satp_mode_max_supported(cpu, "mbare", true);
> }
> #endif
>
> @@ -696,7 +727,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> {
> bool rv32 = riscv_cpu_mxl(&cpu->env) == MXL_RV32;
> - const bool *valid_vm = rv32 ? valid_vm_1_10_32 : valid_vm_1_10_64;
> + uint8_t satp_mode_map_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> + uint8_t satp_mode_supported_max =
> + satp_mode_max_from_map(cpu->cfg.satp_mode.supported);
>
> if (cpu->cfg.satp_mode.map == 0) {
> /*
> @@ -704,7 +737,7 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> * satp mode.
> */
> if (cpu->cfg.satp_mode.init == 0) {
> - set_satp_mode_default(cpu, rv32);
> + set_satp_mode_default(cpu);
> } else {
> /*
> * Find the lowest level that was disabled and then enable the
> @@ -714,9 +747,9 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> for (int i = 1; i < 16; ++i) {
> if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> (cpu->cfg.satp_mode.init & (1 << i)) &&
> - valid_vm[i]) {
> + (cpu->cfg.satp_mode.supported & (1 << i))) {
> for (int j = i - 1; j >= 0; --j) {
> - if (valid_vm[j]) {
> + if (cpu->cfg.satp_mode.supported & (1 << j)) {
> cpu->cfg.satp_mode.map |= (1 << j);
> break;
> }
> @@ -727,13 +760,12 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> }
> }
>
> - /* Make sure the configuration asked is supported by qemu */
> - for (int i = 0; i < 16; ++i) {
> - if ((cpu->cfg.satp_mode.map & (1 << i)) && !valid_vm[i]) {
> - error_setg(errp, "satp_mode %s is not valid",
> - satp_mode_str(i, rv32));
> - return;
> - }
> + /* Make sure the user asked for a supported configuration (HW and qemu) */
> + if (satp_mode_map_max > satp_mode_supported_max) {
> + error_setg(errp, "satp_mode %s is higher than hw max capability %s",
> + satp_mode_str(satp_mode_map_max, rv32),
> + satp_mode_str(satp_mode_supported_max, rv32));
> + return;
> }
>
> /*
> @@ -741,17 +773,13 @@ static void riscv_cpu_satp_mode_finalize(RISCVCPU *cpu, Error **errp)
> * the specification.
> */
> if (!rv32) {
> - uint8_t satp_mode_max;
> -
> - satp_mode_max = satp_mode_max_from_map(cpu->cfg.satp_mode.map);
> -
> - for (int i = satp_mode_max - 1; i >= 0; --i) {
> + for (int i = satp_mode_map_max - 1; i >= 0; --i) {
> if (!(cpu->cfg.satp_mode.map & (1 << i)) &&
> (cpu->cfg.satp_mode.init & (1 << i)) &&
> - valid_vm[i]) {
> + (cpu->cfg.satp_mode.supported & (1 << i))) {
> error_setg(errp, "cannot disable %s satp mode if %s "
> "is enabled", satp_mode_str(i, false),
> - satp_mode_str(satp_mode_max, false));
> + satp_mode_str(satp_mode_map_max, false));
> return;
> }
> }
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index e37177db5c..b591122099 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -416,13 +416,17 @@ struct RISCVCPUClass {
>
> /*
> * map is a 16-bit bitmap: the most significant set bit in map is the maximum
> - * satp mode that is supported.
> + * satp mode that is supported. It may be chosen by the user and must respect
> + * what qemu implements (valid_1_10_32/64) and what the hw is capable of
> + * (supported bitmap below).
> *
> * init is a 16-bit bitmap used to make sure the user selected a correct
> * configuration as per the specification.
> + *
> + * supported is a 16-bit bitmap used to reflect the hw capabilities.
> */
> typedef struct {
> - uint16_t map, init;
> + uint16_t map, init, supported;
> } RISCVSATPMap;
>
> struct RISCVCPUConfig {
> --
> 2.37.2
>
>
next prev parent reply other threads:[~2023-01-24 0:42 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-23 9:03 [PATCH v6 0/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-23 9:03 ` [PATCH v6 1/5] riscv: Pass Object to register_cpu_props instead of DeviceState Alexandre Ghiti
2023-01-23 9:03 ` [PATCH v6 2/5] riscv: Change type of valid_vm_1_10_[32|64] to bool Alexandre Ghiti
2023-01-23 9:48 ` Andrew Jones
2023-01-24 0:02 ` Alistair Francis
2023-01-23 9:03 ` [PATCH v6 3/5] riscv: Allow user to set the satp mode Alexandre Ghiti
2023-01-23 10:11 ` Andrew Jones
2023-01-24 9:56 ` Alexandre Ghiti
2023-01-23 10:14 ` Andrew Jones
2023-01-24 9:56 ` Alexandre Ghiti
2023-01-23 10:29 ` Andrew Jones
2023-01-24 10:00 ` Alexandre Ghiti
2023-01-23 9:03 ` [PATCH v6 4/5] riscv: Correctly set the device-tree entry 'mmu-type' Alexandre Ghiti
2023-01-23 10:12 ` Andrew Jones
2023-01-24 0:35 ` Alistair Francis
2023-01-23 9:03 ` [PATCH v6 5/5] riscv: Introduce satp mode hw capabilities Alexandre Ghiti
2023-01-23 10:51 ` Andrew Jones
2023-01-23 11:15 ` Alexandre Ghiti
2023-01-23 13:31 ` Andrew Jones
2023-01-24 13:13 ` Alexandre Ghiti
2023-01-24 10:07 ` Alexandre Ghiti
2023-01-24 15:31 ` Andrew Jones
2023-01-23 13:51 ` Andrew Jones
2023-01-24 13:24 ` Alexandre Ghiti
2023-01-24 0:41 ` Alistair Francis [this message]
2023-01-24 9:13 ` Alexandre Ghiti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKmqyKPrGGt1RpKbLr0zmjNz_D4B7xkELYyGA+LAhtDTqSRd2w@mail.gmail.com \
--to=alistair23@gmail.com \
--cc=ajones@ventanamicro.com \
--cc=alexghiti@rivosinc.com \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=frank.chang@sifive.com \
--cc=palmer@dabbelt.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.