| Message ID | 1448922238-5696-1-git-send-email-Andrew.Baumann@microsoft.com |
|---|---|
| State | New |
| Headers | show |
On 30 November 2015 at 22:23, Andrew Baumann <Andrew.Baumann@microsoft.com> wrote: > Qemu does not generally perform alignment checks. However, the ARM ARM > requires implementation of alignment exceptions for a number of cases > including LDREX, and Windows-on-ARM relies on this. > > This change adds a helper function to raise an alignment exception > (data abort), a framework for implementing alignment checks in > translated instructions, and adds one such check to the translation of > LDREX instruction (for all variants except single-byte loads). > > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> > --- > I realise this will need to wait until after 2.5, but wanted to get > the review feedback started. If needed, I can resend this later. > > arm_regime_using_lpae_format() is a no-op wrapper I added to export > regime_using_lpae_format (which is a static inline). Would it be > preferable to simply export the existing function, and rename it? If > so, is this still the correct name to use for the function? > > CONFIG_ALIGNMENT_EXCEPTIONS shows how the check can be conditionally > enabled, but isn't presently hooked up to any configure mechanism. I > figured that the overhead of an alignment check in LDREX is not high > enough to warrant disabling the feature, but if it gets used more > widely it might be. > > The same change is almost certainly applicable to arm64, but I am not > in a position to test it. TCG supports "this load/store should do an alignment check" using the MO_ALIGN TCGMemOp flag (which results in a call to the CPU's do_unaligned_access hook if the guest address is not aligned). I think we should use this core-code functionality rather than rolling our own equivalent (it is more efficient). There are some examples in a few of the other targets (eg MIPS) of how to do this, but basically you need to arrange that the initial loads in gen_load_exclusive get the MO_ALIGN flag ORed in, and then wire up the do_unaligned_access hook and make it raise a suitable exception. thanks -- PMM
On Thu, Dec 3, 2015 at 3:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 November 2015 at 22:23, Andrew Baumann > <Andrew.Baumann@microsoft.com> wrote: >> Qemu does not generally perform alignment checks. However, the ARM ARM >> requires implementation of alignment exceptions for a number of cases >> including LDREX, and Windows-on-ARM relies on this. >> >> This change adds a helper function to raise an alignment exception >> (data abort), a framework for implementing alignment checks in >> translated instructions, and adds one such check to the translation of >> LDREX instruction (for all variants except single-byte loads). >> >> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> >> --- >> I realise this will need to wait until after 2.5, but wanted to get >> the review feedback started. If needed, I can resend this later. >> >> arm_regime_using_lpae_format() is a no-op wrapper I added to export >> regime_using_lpae_format (which is a static inline). Would it be >> preferable to simply export the existing function, and rename it? If >> so, is this still the correct name to use for the function? >> >> CONFIG_ALIGNMENT_EXCEPTIONS shows how the check can be conditionally >> enabled, but isn't presently hooked up to any configure mechanism. I >> figured that the overhead of an alignment check in LDREX is not high >> enough to warrant disabling the feature, but if it gets used more >> widely it might be. >> >> The same change is almost certainly applicable to arm64, but I am not >> in a position to test it. > > TCG supports "this load/store should do an alignment check" > using the MO_ALIGN TCGMemOp flag (which results in a call to > the CPU's do_unaligned_access hook if the guest address is not > aligned). I think we should use this core-code functionality > rather than rolling our own equivalent (it is more efficient). > There are some examples in a few of the other targets (eg MIPS) > of how to do this, but basically you need to arrange that the > initial loads in gen_load_exclusive get the MO_ALIGN flag > ORed in, and then wire up the do_unaligned_access hook and > make it raise a suitable exception. After quickly looking at the code in softmmu_template.h, I wonder if MO_ALIGN would correcly handle the ldrexd pair case which requires an 8-byte alignment but does 2 4-byte loads (even if the code is tweaked to read 8-byte at once, then checking 16-byte alignment of AArch64 ldxp 64-bit could not be handled correctly). Thanks, Laurent
On 3 December 2015 at 14:58, Laurent Desnogues <laurent.desnogues@gmail.com> wrote: > On Thu, Dec 3, 2015 at 3:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 30 November 2015 at 22:23, Andrew Baumann >> <Andrew.Baumann@microsoft.com> wrote: >>> Qemu does not generally perform alignment checks. However, the ARM ARM >>> requires implementation of alignment exceptions for a number of cases >>> including LDREX, and Windows-on-ARM relies on this. >> TCG supports "this load/store should do an alignment check" >> using the MO_ALIGN TCGMemOp flag (which results in a call to >> the CPU's do_unaligned_access hook if the guest address is not >> aligned). I think we should use this core-code functionality >> rather than rolling our own equivalent (it is more efficient). >> There are some examples in a few of the other targets (eg MIPS) >> of how to do this, but basically you need to arrange that the >> initial loads in gen_load_exclusive get the MO_ALIGN flag >> ORed in, and then wire up the do_unaligned_access hook and >> make it raise a suitable exception. > > After quickly looking at the code in softmmu_template.h, I wonder if > MO_ALIGN would correcly handle the ldrexd pair case which requires an > 8-byte alignment but does 2 4-byte loads (even if the code is tweaked > to read 8-byte at once, then checking 16-byte alignment of AArch64 > ldxp 64-bit could not be handled correctly). You're right, those are not going to be handled correctly. But I think it would be better to enhance the MO_ALIGN handling somehow to deal with "must be more highly aligned than the datasize" cases as well as the "alignment must match datasize" ones. And we can definitely deal with all the AArch32 cases ok, which are the ones Andrew needs. (As you say we'd need to do the ldrexd as a 64-bit access, but we should do that anyway because it's supposed to be single-copy-atomic, architecturally speaking.) thanks -- PMM
On 12/03/2015 07:08 AM, Peter Maydell wrote: > On 3 December 2015 at 14:58, Laurent Desnogues > <laurent.desnogues@gmail.com> wrote: >> On Thu, Dec 3, 2015 at 3:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote: >>> On 30 November 2015 at 22:23, Andrew Baumann >>> <Andrew.Baumann@microsoft.com> wrote: >>>> Qemu does not generally perform alignment checks. However, the ARM ARM >>>> requires implementation of alignment exceptions for a number of cases >>>> including LDREX, and Windows-on-ARM relies on this. > >>> TCG supports "this load/store should do an alignment check" >>> using the MO_ALIGN TCGMemOp flag (which results in a call to >>> the CPU's do_unaligned_access hook if the guest address is not >>> aligned). I think we should use this core-code functionality >>> rather than rolling our own equivalent (it is more efficient). >>> There are some examples in a few of the other targets (eg MIPS) >>> of how to do this, but basically you need to arrange that the >>> initial loads in gen_load_exclusive get the MO_ALIGN flag >>> ORed in, and then wire up the do_unaligned_access hook and >>> make it raise a suitable exception. >> >> After quickly looking at the code in softmmu_template.h, I wonder if >> MO_ALIGN would correcly handle the ldrexd pair case which requires an >> 8-byte alignment but does 2 4-byte loads (even if the code is tweaked >> to read 8-byte at once, then checking 16-byte alignment of AArch64 >> ldxp 64-bit could not be handled correctly). > > You're right, those are not going to be handled correctly. > But I think it would be better to enhance the MO_ALIGN > handling somehow to deal with "must be more highly aligned than > the datasize" cases as well as the "alignment must match datasize" > ones. What's the full set of features that you'd like here? > (As you say we'd need > to do the ldrexd as a 64-bit access, but we should do that > anyway because it's supposed to be single-copy-atomic, > architecturally speaking.) Something to remember for future is that we're not doing single-copy of 64-bit data for 32-bit hosts. I'm not even sure that's generally possible without generating awful code. r~
On 3 December 2015 at 21:21, Richard Henderson <rth@twiddle.net> wrote: > On 12/03/2015 07:08 AM, Peter Maydell wrote: >> On 3 December 2015 at 14:58, Laurent Desnogues >> <laurent.desnogues@gmail.com> wrote: >>> After quickly looking at the code in softmmu_template.h, I wonder if >>> MO_ALIGN would correcly handle the ldrexd pair case which requires an >>> 8-byte alignment but does 2 4-byte loads (even if the code is tweaked >>> to read 8-byte at once, then checking 16-byte alignment of AArch64 >>> ldxp 64-bit could not be handled correctly). >> >> You're right, those are not going to be handled correctly. >> But I think it would be better to enhance the MO_ALIGN >> handling somehow to deal with "must be more highly aligned than >> the datasize" cases as well as the "alignment must match datasize" >> ones. > > What's the full set of features that you'd like here? As Laurent says, for the "load-pair" instructions we need to be able to load data into two registers with an alignment corresponding to the whole thing. We can obviously do 'load 2x 32 bit regs' with a 64-bit load and then split the result into the two values, but the 'load 2x 64 bit regs' would need either a 128 bit load or a 'load 64 bit reg with 128 bit aligment'. The other instructions with wider-than-the-type alignment are the SIMD ones, which can ask for an alignment of up to 256 bits (for instance VLD1 multiple-single-elements). You could reasonably argue that there we should just emit code to do the check, since we're already pre-calculating the address and then emitting a lot of load or store TCG ops. >> (As you say we'd need >> to do the ldrexd as a 64-bit access, but we should do that >> anyway because it's supposed to be single-copy-atomic, >> architecturally speaking.) > > Something to remember for future is that we're not doing single-copy > of 64-bit data for 32-bit hosts. I'm not even sure that's generally > possible without generating awful code. Since this is in LDREXD we're going to be doing something fairly heavy-overhead anyway for the exclusive handling, but yes, worth remembering when we have multithreaded TCG. (Guests do rely on the single-copy-atomicity of (non-exclusive) 64 bit accesses for updating page table entries for LPAE CPUs.) thanks -- PMM
diff --git a/target-arm/helper.c b/target-arm/helper.c index afc4163..59d5a41 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -5996,6 +5996,14 @@ static inline bool regime_using_lpae_format(CPUARMState *env, return false; } +/* Returns true if the translation regime is using LPAE format page tables. + * Used when raising alignment exceptions, whose FSR changes depending on + * whether the long or short descriptor format is in use. */ +bool arm_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx) +{ + return regime_using_lpae_format(env, mmu_idx); +} + static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx) { switch (mmu_idx) { diff --git a/target-arm/helper.h b/target-arm/helper.h index c2a85c7..16d0137 100644 --- a/target-arm/helper.h +++ b/target-arm/helper.h @@ -48,6 +48,7 @@ DEF_HELPER_FLAGS_3(sel_flags, TCG_CALL_NO_RWG_SE, i32, i32, i32, i32) DEF_HELPER_2(exception_internal, void, env, i32) DEF_HELPER_4(exception_with_syndrome, void, env, i32, i32, i32) +DEF_HELPER_2(alignment_exception, void, env, tl) DEF_HELPER_1(wfi, void, env) DEF_HELPER_1(wfe, void, env) DEF_HELPER_1(yield, void, env) diff --git a/target-arm/internals.h b/target-arm/internals.h index 347998c..17afe2a 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -441,4 +441,7 @@ struct ARMMMUFaultInfo { bool arm_tlb_fill(CPUState *cpu, vaddr address, int rw, int mmu_idx, uint32_t *fsr, ARMMMUFaultInfo *fi); +/* Return true if the translation regime is using LPAE format page tables */ +bool arm_regime_using_lpae_format(CPUARMState *env, ARMMMUIdx mmu_idx); + #endif diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c index 6cd54c8..5c46210 100644 --- a/target-arm/op_helper.c +++ b/target-arm/op_helper.c @@ -376,6 +376,27 @@ void HELPER(exception_with_syndrome)(CPUARMState *env, uint32_t excp, raise_exception(env, excp, syndrome, target_el); } +/* Raise a data fault alignment exception for the specified virtual address */ +void HELPER(alignment_exception)(CPUARMState *env, target_ulong vaddr) +{ + int target_el = exception_target_el(env); + bool same_el = (arm_current_el(env) != target_el); + + env->exception.vaddress = vaddr; + + /* the DFSR for an alignment fault depends on whether we're using + * the LPAE long descriptor format, or the short descriptor format */ + if (arm_regime_using_lpae_format(env, cpu_mmu_index(env, false))) { + env->exception.fsr = 0x21; + } else { + env->exception.fsr = 0x1; + } + + raise_exception(env, EXCP_DATA_ABORT, + syn_data_abort(same_el, 0, 0, 0, 0, 0x21), + target_el); +} + uint32_t HELPER(cpsr_read)(CPUARMState *env) { return cpsr_read(env) & ~(CPSR_EXEC | CPSR_RESERVED); diff --git a/target-arm/translate.c b/target-arm/translate.c index 5d22879..c05ea1f 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -37,6 +37,7 @@ #include "trace-tcg.h" +#define CONFIG_ALIGNMENT_EXCEPTIONS 1 #define ENABLE_ARCH_4T arm_dc_feature(s, ARM_FEATURE_V4T) #define ENABLE_ARCH_5 arm_dc_feature(s, ARM_FEATURE_V5) @@ -1058,6 +1059,28 @@ static void gen_exception_insn(DisasContext *s, int offset, int excp, s->is_jmp = DISAS_JUMP; } +/* Emit an inline alignment check, which raises an exception if the given + * address is not aligned according to "size" (which must be a power of 2). */ +static void gen_alignment_check(DisasContext *s, int pc_offset, + target_ulong size, TCGv addr) +{ +#ifdef CONFIG_ALIGNMENT_EXCEPTIONS + TCGLabel *alignok_label = gen_new_label(); + TCGv tmp = tcg_temp_new(); + + /* check alignment, branch to alignok_label if aligned */ + tcg_gen_andi_tl(tmp, addr, size - 1); + tcg_gen_brcondi_tl(TCG_COND_EQ, tmp, 0, alignok_label); + + /* emit alignment exception */ + gen_set_pc_im(s, s->pc - pc_offset); + gen_helper_alignment_exception(cpu_env, addr); + + gen_set_label(alignok_label); + tcg_temp_free(tmp); +#endif +} + /* Force a TB lookup after an instruction that changes the CPU state. */ static inline void gen_lookup_tb(DisasContext *s) { @@ -7430,6 +7453,12 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2, s->is_ldex = true; + /* emit alignment check if needed */ + if (size != 0) { + /* NB: all LDREX variants (incl. thumb) occupy 4 bytes */ + gen_alignment_check(s, 4, (target_ulong)1 << size, addr); + } + switch (size) { case 0: gen_aa32_ld8u(tmp, addr, get_mem_index(s));
Qemu does not generally perform alignment checks. However, the ARM ARM requires implementation of alignment exceptions for a number of cases including LDREX, and Windows-on-ARM relies on this. This change adds a helper function to raise an alignment exception (data abort), a framework for implementing alignment checks in translated instructions, and adds one such check to the translation of LDREX instruction (for all variants except single-byte loads). Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com> --- I realise this will need to wait until after 2.5, but wanted to get the review feedback started. If needed, I can resend this later. arm_regime_using_lpae_format() is a no-op wrapper I added to export regime_using_lpae_format (which is a static inline). Would it be preferable to simply export the existing function, and rename it? If so, is this still the correct name to use for the function? CONFIG_ALIGNMENT_EXCEPTIONS shows how the check can be conditionally enabled, but isn't presently hooked up to any configure mechanism. I figured that the overhead of an alignment check in LDREX is not high enough to warrant disabling the feature, but if it gets used more widely it might be. The same change is almost certainly applicable to arm64, but I am not in a position to test it. target-arm/helper.c | 8 ++++++++ target-arm/helper.h | 1 + target-arm/internals.h | 3 +++ target-arm/op_helper.c | 21 +++++++++++++++++++++ target-arm/translate.c | 29 +++++++++++++++++++++++++++++ 5 files changed, 62 insertions(+)