* [PATCH] target/riscv: Avoid leaking "no translation" TLB entries
@ 2022-03-30 16:59 Palmer Dabbelt
2022-03-31 5:01 ` Alistair Francis
2022-03-31 22:12 ` Alistair Francis
0 siblings, 2 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2022-03-30 16:59 UTC (permalink / raw)
To: Alistair Francis, Atish Patra; +Cc: qemu-riscv, qemu-devel, Palmer Dabbelt
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(-)
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/riscv: Avoid leaking "no translation" TLB entries
2022-03-30 16:59 [PATCH] target/riscv: Avoid leaking "no translation" TLB entries Palmer Dabbelt
@ 2022-03-31 5:01 ` Alistair Francis
2022-03-31 22:12 ` Alistair Francis
1 sibling, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2022-03-31 5:01 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Atish Patra, Alistair Francis, open list:RISC-V,
qemu-devel@nongnu.org Developers
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/riscv: Avoid leaking "no translation" TLB entries
@ 2022-03-31 5:01 ` Alistair Francis
0 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2022-03-31 5:01 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, Atish Patra, open list:RISC-V,
qemu-devel@nongnu.org Developers
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/riscv: Avoid leaking "no translation" TLB entries
2022-03-30 16:59 [PATCH] target/riscv: Avoid leaking "no translation" TLB entries Palmer Dabbelt
@ 2022-03-31 22:12 ` Alistair Francis
2022-03-31 22:12 ` Alistair Francis
1 sibling, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2022-03-31 22:12 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Atish Patra, Alistair Francis, open list:RISC-V,
qemu-devel@nongnu.org Developers
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] target/riscv: Avoid leaking "no translation" TLB entries
@ 2022-03-31 22:12 ` Alistair Francis
0 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2022-03-31 22:12 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, Atish Patra, open list:RISC-V,
qemu-devel@nongnu.org Developers
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-31 22:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-30 16:59 [PATCH] target/riscv: Avoid leaking "no translation" TLB entries Palmer Dabbelt
2022-03-31 5:01 ` Alistair Francis
2022-03-31 5:01 ` Alistair Francis
2022-03-31 22:12 ` Alistair Francis
2022-03-31 22:12 ` Alistair Francis
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.