Message ID | rq81o86n-17ps-92no-p65o-79o88476266@syhkavp.arg |
---|---|
State | New |
Headers | show |
Series | target/riscv/pmp: fix NAPOT range computation overflow | expand |
On Sat, Apr 9, 2022 at 2:25 AM Nicolas Pitre <nico@fluxnic.net> wrote: > > There is an overflow with the current code where a pmpaddr value of > 0x1fffffff is decoded as sa=0 and ea=0 whereas it should be sa=0 and > ea=0xffffffff. > > Fix that by simplifying the computation. There is in fact no need for > ctz64() nor special case for -1 to achieve proper results. > > Signed-off-by: Nicolas Pitre <nico@fluxnic.net> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > > This is in fact the same patch I posted yesterday but turns out its > scope is far more important than I initially thought. > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 81b61bb65c..151da3fa08 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -141,17 +141,9 @@ static void pmp_decode_napot(target_ulong a, target_ulong *sa, target_ulong *ea) > 0111...1111 2^(XLEN+2)-byte NAPOT range > 1111...1111 Reserved > */ > - if (a == -1) { > - *sa = 0u; > - *ea = -1; > - return; > - } else { > - target_ulong t1 = ctz64(~a); > - target_ulong base = (a & ~(((target_ulong)1 << t1) - 1)) << 2; > - target_ulong range = ((target_ulong)1 << (t1 + 3)) - 1; > - *sa = base; > - *ea = base + range; > - } > + a = (a << 2) | 0x3; > + *sa = a & (a + 1); > + *ea = a | (a + 1); > } > > void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index) >
On Sat, Apr 9, 2022 at 2:25 AM Nicolas Pitre <nico@fluxnic.net> wrote: > > There is an overflow with the current code where a pmpaddr value of > 0x1fffffff is decoded as sa=0 and ea=0 whereas it should be sa=0 and > ea=0xffffffff. > > Fix that by simplifying the computation. There is in fact no need for > ctz64() nor special case for -1 to achieve proper results. > > Signed-off-by: Nicolas Pitre <nico@fluxnic.net> Thanks! Applied to riscv-to-apply.next Alistair > --- > > This is in fact the same patch I posted yesterday but turns out its > scope is far more important than I initially thought. > > diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c > index 81b61bb65c..151da3fa08 100644 > --- a/target/riscv/pmp.c > +++ b/target/riscv/pmp.c > @@ -141,17 +141,9 @@ static void pmp_decode_napot(target_ulong a, target_ulong *sa, target_ulong *ea) > 0111...1111 2^(XLEN+2)-byte NAPOT range > 1111...1111 Reserved > */ > - if (a == -1) { > - *sa = 0u; > - *ea = -1; > - return; > - } else { > - target_ulong t1 = ctz64(~a); > - target_ulong base = (a & ~(((target_ulong)1 << t1) - 1)) << 2; > - target_ulong range = ((target_ulong)1 << (t1 + 3)) - 1; > - *sa = base; > - *ea = base + range; > - } > + a = (a << 2) | 0x3; > + *sa = a & (a + 1); > + *ea = a | (a + 1); > } > > void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index) >
diff --git a/target/riscv/pmp.c b/target/riscv/pmp.c index 81b61bb65c..151da3fa08 100644 --- a/target/riscv/pmp.c +++ b/target/riscv/pmp.c @@ -141,17 +141,9 @@ static void pmp_decode_napot(target_ulong a, target_ulong *sa, target_ulong *ea) 0111...1111 2^(XLEN+2)-byte NAPOT range 1111...1111 Reserved */ - if (a == -1) { - *sa = 0u; - *ea = -1; - return; - } else { - target_ulong t1 = ctz64(~a); - target_ulong base = (a & ~(((target_ulong)1 << t1) - 1)) << 2; - target_ulong range = ((target_ulong)1 << (t1 + 3)) - 1; - *sa = base; - *ea = base + range; - } + a = (a << 2) | 0x3; + *sa = a & (a + 1); + *ea = a | (a + 1); } void pmp_update_rule_addr(CPURISCVState *env, uint32_t pmp_index)
There is an overflow with the current code where a pmpaddr value of 0x1fffffff is decoded as sa=0 and ea=0 whereas it should be sa=0 and ea=0xffffffff. Fix that by simplifying the computation. There is in fact no need for ctz64() nor special case for -1 to achieve proper results. Signed-off-by: Nicolas Pitre <nico@fluxnic.net> --- This is in fact the same patch I posted yesterday but turns out its scope is far more important than I initially thought.