On Thu, Mar 31, 2022 at 3:11 AM Palmer Dabbelt <pal...@rivosinc.com> wrote:
>
> The ISA doesn't allow bare mappings to be cached, as the caches are
> translations and bare mppings are not translated. We cache these
> translations in QEMU in order to utilize the TLB code, but that leaks
> out to the guest.
>
> Suggested-by: phan...@zju.edu.cn # no name in the From field
> Fixes: 1e0d985fa9 ("target/riscv: Only flush TLB if SATP.ASID changes")
> Signed-off-by: Palmer Dabbelt <pal...@rivosinc.com>
Reviewed-by: Alistair Francis <alistair.fran...@wdc.com>
Alistair
>
> ---
>
> Another way to fix this would be to utilize a MMU index that cooresponds
> to no ASID to hold these direct mappings, but given that we're not
> currently taking advantage of ASIDs for translation performance that
> would be a larger chunk of work. This causes a Linux boot regression,
> so the band-aid seems appropriate.
>
> I think the original version of this was also more broadly broken, in
> that changing to ASID 0 would allow old mappings, but I might be missing
> something there. I seem to remember ASID 0 as having been special at
> some point, but it's not in the ISA as it stands so maybe I'm just
> crazy.
>
> This, when applied on top of Alistair's riscv-to-apply.next, boots my
> for-next (which is very close to Linus' master).
> ---
> target/riscv/csr.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 0606cd0ea8..cabef5a20b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1844,7 +1844,7 @@ static RISCVException read_satp(CPURISCVState *env, int
> csrno,
> static RISCVException write_satp(CPURISCVState *env, int csrno,
> target_ulong val)
> {
> - target_ulong vm, mask, asid;
> + target_ulong vm, mask;
>
> if (!riscv_feature(env, RISCV_FEATURE_MMU)) {
> return RISCV_EXCP_NONE;
> @@ -1853,20 +1853,22 @@ static RISCVException write_satp(CPURISCVState *env,
> int csrno,
> if (riscv_cpu_mxl(env) == MXL_RV32) {
> vm = validate_vm(env, get_field(val, SATP32_MODE));
> mask = (val ^ env->satp) & (SATP32_MODE | SATP32_ASID | SATP32_PPN);
> - asid = (val ^ env->satp) & SATP32_ASID;
> } else {
> vm = validate_vm(env, get_field(val, SATP64_MODE));
> mask = (val ^ env->satp) & (SATP64_MODE | SATP64_ASID | SATP64_PPN);
> - asid = (val ^ env->satp) & SATP64_ASID;
> }
>
> if (vm && mask) {
> if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> return RISCV_EXCP_ILLEGAL_INST;
> } else {
> - if (asid) {
> - tlb_flush(env_cpu(env));
> - }
> + /*
> + * The ISA defines SATP.MODE=Bare as "no translation", but we
> still
> + * pass these through QEMU's TLB emulation as it improves
> + * performance. Flushing the TLB on SATP writes with paging
> + * enabled avoids leaking those invalid cached mappings.
> + */
> + tlb_flush(env_cpu(env));
> env->satp = val;
> }
> }
> --
> 2.34.1
>
>
