qemu-riscv
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] target/riscv: Avoid leaking "no translation" TLB entries


From: Alistair Francis
Subject: Re: [PATCH] target/riscv: Avoid leaking "no translation" TLB entries
Date: Fri, 1 Apr 2022 08:12:35 +1000

On Thu, Mar 31, 2022 at 3:11 AM Palmer Dabbelt <palmer@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: phantom@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 <palmer@rivosinc.com>

Thanks!

Applied to riscv-to-apply.next

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
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]