| Message ID | 20211208231154.392029-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement LVA, LPA, LPA2 features | expand |
Richard Henderson <richard.henderson@linaro.org> writes: > Without FEAT_LVA, the behaviour of programming an invalid value > is IMPLEMENTATION DEFINED. With FEAT_LVA, programming an invalid > minimum value requires a Translation fault. > > It is most self-consistent to choose to generate the fault always. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
On Wed, 8 Dec 2021 at 23:16, Richard Henderson <richard.henderson@linaro.org> wrote: > > Without FEAT_LVA, the behaviour of programming an invalid value > is IMPLEMENTATION DEFINED. With FEAT_LVA, programming an invalid > minimum value requires a Translation fault. > > It is most self-consistent to choose to generate the fault always. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 9b317899a6..575723d62c 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -11129,7 +11129,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va, > { > uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr; > bool epd, hpd, using16k, using64k; > - int select, tsz, tbi, max_tsz; > + int select, tsz, tbi; > > if (!regime_has_2_ranges(mmu_idx)) { > select = 0; > @@ -11165,15 +11165,6 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va, > } > } > > - if (cpu_isar_feature(aa64_st, env_archcpu(env))) { > - max_tsz = 48 - using64k; > - } else { > - max_tsz = 39; > - } > - > - tsz = MIN(tsz, max_tsz); > - tsz = MAX(tsz, 16); /* TODO: ARMv8.2-LVA */ > - These changes are OK in themselves, but we also use the aa64_va_parameters() calculated tsz value in the pointer-auth code to work out the bottom bit of the pointer auth field: bot_bit = 64 - param.tsz; top_bit = 64 - 8 * param.tbi; Without the clamping of param.tsz to the valid range, the guest can now program it to a value that will cause us to have bot_bit > top_bit (eg tsz = 0). We don't guard against that and as a result code like extract64(test, bot_bit, top_bit - bot_bit) will assert on the bogus length value. (Section D5.1.5 says what the pauth code is allowed to do if the TnSZ field is out-of-limits: it can use the value as-is, or it can clamp it to the limit.) -- PMM
On Thu, 6 Jan 2022 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 8 Dec 2021 at 23:16, Richard Henderson > <richard.henderson@linaro.org> wrote: > > > > Without FEAT_LVA, the behaviour of programming an invalid value > > is IMPLEMENTATION DEFINED. With FEAT_LVA, programming an invalid > > minimum value requires a Translation fault. > > > > It is most self-consistent to choose to generate the fault always. > > - if (cpu_isar_feature(aa64_st, env_archcpu(env))) { > > - max_tsz = 48 - using64k; > > - } else { > > - max_tsz = 39; > > - } > > - > > - tsz = MIN(tsz, max_tsz); > > - tsz = MAX(tsz, 16); /* TODO: ARMv8.2-LVA */ > > - > > These changes are OK in themselves, but we also use the > aa64_va_parameters() calculated tsz value in the > pointer-auth code to work out the bottom bit of the > pointer auth field: > > bot_bit = 64 - param.tsz; > top_bit = 64 - 8 * param.tbi; ...and in particular, for linux-user mode as far as I can tell we aren't initializing TCR_EL1 to anything particularly sensible (we set TBI0 and leave the rest to 0) so we are effectively relying on the clamping there at the moment. We should probably set TCR_EL1 properly. (cf the user report in qemu-discuss just now.) -- PMM
diff --git a/target/arm/helper.c b/target/arm/helper.c index 9b317899a6..575723d62c 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -11129,7 +11129,7 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va, { uint64_t tcr = regime_tcr(env, mmu_idx)->raw_tcr; bool epd, hpd, using16k, using64k; - int select, tsz, tbi, max_tsz; + int select, tsz, tbi; if (!regime_has_2_ranges(mmu_idx)) { select = 0; @@ -11165,15 +11165,6 @@ ARMVAParameters aa64_va_parameters(CPUARMState *env, uint64_t va, } } - if (cpu_isar_feature(aa64_st, env_archcpu(env))) { - max_tsz = 48 - using64k; - } else { - max_tsz = 39; - } - - tsz = MIN(tsz, max_tsz); - tsz = MAX(tsz, 16); /* TODO: ARMv8.2-LVA */ - /* Present TBI as a composite with TBID. */ tbi = aa64_va_parameter_tbi(tcr, mmu_idx); if (!data) { @@ -11309,9 +11300,30 @@ static bool get_phys_addr_lpae(CPUARMState *env, uint64_t address, /* TODO: This code does not support shareability levels. */ if (aarch64) { + int min_tsz = 16, max_tsz = 39; /* TODO: ARMv8.2-LVA */ + param = aa64_va_parameters(env, address, mmu_idx, access_type != MMU_INST_FETCH); level = 0; + + if (cpu_isar_feature(aa64_st, env_archcpu(env))) { + max_tsz = 48 - param.using64k; + } + + /* + * If TxSZ is programmed to a value larger than the maximum, + * or smaller than the effective minimum, it is IMPLEMENTATION + * DEFINED whether we behave as if the field were programmed + * within bounds, or if a level 0 Translation fault is generated. + * + * With FEAT_LVA, fault on less than minimum becomes required, + * so our choice is to always raise the fault. + */ + if (param.tsz < min_tsz || param.tsz > max_tsz) { + fault_type = ARMFault_Translation; + goto do_fault; + } + addrsize = 64 - 8 * param.tbi; inputsize = 64 - param.tsz; } else {
Without FEAT_LVA, the behaviour of programming an invalid value is IMPLEMENTATION DEFINED. With FEAT_LVA, programming an invalid minimum value requires a Translation fault. It is most self-consistent to choose to generate the fault always. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.c | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-)