| Message ID | 20220330165913.8836-1-palmer@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Avoid leaking "no translation" TLB entries | expand |
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> Reviewed-by: Alistair Francis <alistair.francis@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 > >
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 > >
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; } }
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> --- 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(-)