| Message ID | 1564017946033.57669@bt.com |
|---|---|
| State | New |
| Headers | show |
| Series | configure: Define target access alignment in configure | expand |
On Thu, Jul 25, 2019 at 3:25 AM <tony.nguyen@bt.com> wrote: > Rename ALIGNED_ONLY to TARGET_ALIGNED_ONLY for clarity and move > defines out of target/foo/cpu.h into configure, as we do with > TARGET_WORDS_BIGENDIAN, so that it is always defined early. > > Also, poison the symbol in include/exec/poison.h to prevent use in > common code. > > Hi, Tony. Thanks for this new version. When one mentions "also" in the commit message, this is a kind of strong indication that the patch should be split into two patches. So, could you please consider moving "poison" part into a separate patch? Please don't misunderstand me that I am somehow preventing you from integrating this change. I do it for the sake of you becoming a better contributor, and everybody is welcoming you into QEMU community. Try to follow out guidelines for contributing and also good examples of existing commit messages. Yours, Aleksandar > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Reviewed-by: Richard Henderson <rth@twiddle.net> > Signed-off-by: Tony Nguyen <tony.nguyen@bt.com> > --- > configure | 10 +++++++++- > include/exec/poison.h | 1 + > include/qom/cpu.h | 2 +- > target/alpha/cpu.h | 2 -- > target/hppa/cpu.h | 1 - > target/mips/cpu.h | 2 -- > target/sh4/cpu.h | 2 -- > target/sparc/cpu.h | 2 -- > target/xtensa/cpu.h | 2 -- > tcg/tcg.c | 2 +- > tcg/tcg.h | 8 +++++--- > 11 files changed, 17 insertions(+), 17 deletions(-) > > diff --git a/configure b/configure > index 714e7fb..482ba0b 100755 > --- a/configure > +++ b/configure > @@ -7431,8 +7431,13 @@ for target in $target_list; do > target_dir="$target" > config_target_mak=$target_dir/config-target.mak > target_name=$(echo $target | cut -d '-' -f 1) > +target_aligned_only="no" > +case "$target_name" in > + > alpha|hppa|mips64el|mips64|mipsel|mips|mipsn32|mipsn32el|sh4|sh4eb|sparc|sparc64|sparc32plus|xtensa|xtensaeb) > + target_aligned_only="yes" > + ;; > +esac > target_bigendian="no" > - > case "$target_name" in > > armeb|aarch64_be|hppa|lm32|m68k|microblaze|mips|mipsn32|mips64|moxie|or1k|ppc|ppc64|ppc64abi32|s390x|sh4eb|sparc|sparc64|sparc32plus|xtensaeb) > target_bigendian=yes > @@ -7717,6 +7722,9 @@ fi > if supported_whpx_target $target; then > echo "CONFIG_WHPX=y" >> $config_target_mak > fi > +if test "$target_aligned_only" = "yes" ; then > + echo "TARGET_ALIGNED_ONLY=y" >> $config_target_mak > +fi > if test "$target_bigendian" = "yes" ; then > echo "TARGET_WORDS_BIGENDIAN=y" >> $config_target_mak > fi > diff --git a/include/exec/poison.h b/include/exec/poison.h > index b862320..955eb86 100644 > --- a/include/exec/poison.h > +++ b/include/exec/poison.h > @@ -35,6 +35,7 @@ > #pragma GCC poison TARGET_UNICORE32 > #pragma GCC poison TARGET_XTENSA > > +#pragma GCC poison TARGET_ALIGNED_ONLY > #pragma GCC poison TARGET_HAS_BFLT > #pragma GCC poison TARGET_NAME > #pragma GCC poison TARGET_SUPPORTS_MTTCG > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 5ee0046..9b50b73 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -89,7 +89,7 @@ struct TranslationBlock; > * @do_unassigned_access: Callback for unassigned access handling. > * (this is deprecated: new targets should use do_transaction_failed > instead) > * @do_unaligned_access: Callback for unaligned access handling, if > - * the target defines #ALIGNED_ONLY. > + * the target defines #TARGET_ALIGNED_ONLY. > * @do_transaction_failed: Callback for handling failed memory > transactions > * (ie bus faults or external aborts; not MMU faults) > * @virtio_is_big_endian: Callback to return %true if a CPU which supports > diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h > index b3e8a82..16eb804 100644 > --- a/target/alpha/cpu.h > +++ b/target/alpha/cpu.h > @@ -23,8 +23,6 @@ > #include "cpu-qom.h" > #include "exec/cpu-defs.h" > > -#define ALIGNED_ONLY > - > /* Alpha processors have a weak memory model */ > #define TCG_GUEST_DEFAULT_MO (0) > > diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h > index aab251b..2be67c2 100644 > --- a/target/hppa/cpu.h > +++ b/target/hppa/cpu.h > @@ -30,7 +30,6 @@ > basis. It's probably easier to fall back to a strong memory model. */ > #define TCG_GUEST_DEFAULT_MO TCG_MO_ALL > > -#define ALIGNED_ONLY > #define MMU_KERNEL_IDX 0 > #define MMU_USER_IDX 3 > #define MMU_PHYS_IDX 4 > diff --git a/target/mips/cpu.h b/target/mips/cpu.h > index 21c0615..c13cd4e 100644 > --- a/target/mips/cpu.h > +++ b/target/mips/cpu.h > @@ -1,8 +1,6 @@ > #ifndef MIPS_CPU_H > #define MIPS_CPU_H > > -#define ALIGNED_ONLY > - > #include "cpu-qom.h" > #include "exec/cpu-defs.h" > #include "fpu/softfloat.h" > diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h > index aee733e..ecaa7a1 100644 > --- a/target/sh4/cpu.h > +++ b/target/sh4/cpu.h > @@ -23,8 +23,6 @@ > #include "cpu-qom.h" > #include "exec/cpu-defs.h" > > -#define ALIGNED_ONLY > - > /* CPU Subtypes */ > #define SH_CPU_SH7750 (1 << 0) > #define SH_CPU_SH7750S (1 << 1) > diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h > index 8ed2250..1406f0b 100644 > --- a/target/sparc/cpu.h > +++ b/target/sparc/cpu.h > @@ -5,8 +5,6 @@ > #include "cpu-qom.h" > #include "exec/cpu-defs.h" > > -#define ALIGNED_ONLY > - > #if !defined(TARGET_SPARC64) > #define TARGET_DPREGS 16 > #else > diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h > index 2c27713..0459243 100644 > --- a/target/xtensa/cpu.h > +++ b/target/xtensa/cpu.h > @@ -32,8 +32,6 @@ > #include "exec/cpu-defs.h" > #include "xtensa-isa.h" > > -#define ALIGNED_ONLY > - > /* Xtensa processors have a weak memory model */ > #define TCG_GUEST_DEFAULT_MO (0) > > diff --git a/tcg/tcg.c b/tcg/tcg.c > index be2c33c..8d23fb0 100644 > --- a/tcg/tcg.c > +++ b/tcg/tcg.c > @@ -1926,7 +1926,7 @@ static const char * const ldst_name[] = > }; > > static const char * const alignment_name[(MO_AMASK >> MO_ASHIFT) + 1] = { > -#ifdef ALIGNED_ONLY > +#ifdef TARGET_ALIGNED_ONLY > [MO_UNALN >> MO_ASHIFT] = "un+", > [MO_ALIGN >> MO_ASHIFT] = "", > #else > diff --git a/tcg/tcg.h b/tcg/tcg.h > index b411e17..529acb2 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -333,10 +333,12 @@ typedef enum TCGMemOp { > MO_TE = MO_LE, > #endif > > - /* MO_UNALN accesses are never checked for alignment. > + /* > + * MO_UNALN accesses are never checked for alignment. > * MO_ALIGN accesses will result in a call to the CPU's > * do_unaligned_access hook if the guest address is not aligned. > - * The default depends on whether the target CPU defines ALIGNED_ONLY. > + * The default depends on whether the target CPU defines > + * TARGET_ALIGNED_ONLY. > * > * Some architectures (e.g. ARMv8) need the address which is aligned > * to a size more than the size of the memory access. > @@ -353,7 +355,7 @@ typedef enum TCGMemOp { > */ > MO_ASHIFT = 4, > MO_AMASK = 7 << MO_ASHIFT, > -#ifdef ALIGNED_ONLY > +#ifdef TARGET_ALIGNED_ONLY > MO_ALIGN = 0, > MO_UNALN = MO_AMASK, > #else > -- > 1.8.3.1 > >
On Tue, 30 Jul 2019 at 21:00, Aleksandar Markovic <aleksandar.m.mail@gmail.com> wrote: > > On Thu, Jul 25, 2019 at 3:25 AM <tony.nguyen@bt.com> wrote: > > > Rename ALIGNED_ONLY to TARGET_ALIGNED_ONLY for clarity and move > > defines out of target/foo/cpu.h into configure, as we do with > > TARGET_WORDS_BIGENDIAN, so that it is always defined early. > > > > Also, poison the symbol in include/exec/poison.h to prevent use in > > common code. > > > > > Hi, Tony. > > Thanks for this new version. > > When one mentions "also" in the commit message, this is a kind of strong > indication that the patch should be split into two patches. > > So, could you please consider moving "poison" part into a separate patch? I think in this case the issue is more in the phrasing of the commit message rather than the patch composition. The patch is introducing TARGET_ALIGNED_ONLY as something defined by configure, and the correct status for that macro is "needs to be in poison.h"; having an intermediate state where the macro exists but isn't poisoned isn't really a logical split of the work. (The previous ALIGNED_ONLY didn't have so much need to be poisoned because it was defined in cpu.h and so the only way to expect to get it was by pulling in cpu.h.) thanks -- PMM
On Tue, Jul 30, 2019 at 10:06 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Tue, 30 Jul 2019 at 21:00, Aleksandar Markovic > <aleksandar.m.mail@gmail.com> wrote: > > > > On Thu, Jul 25, 2019 at 3:25 AM <tony.nguyen@bt.com> wrote: > > > > > Rename ALIGNED_ONLY to TARGET_ALIGNED_ONLY for clarity and move > > > defines out of target/foo/cpu.h into configure, as we do with > > > TARGET_WORDS_BIGENDIAN, so that it is always defined early. > > > > > > Also, poison the symbol in include/exec/poison.h to prevent use in > > > common code. > > > > > > > > Hi, Tony. > > > > Thanks for this new version. > > > > When one mentions "also" in the commit message, this is a kind of strong > > indication that the patch should be split into two patches. > > > > So, could you please consider moving "poison" part into a separate patch? > > I think in this case the issue is more in the phrasing of the commit > message rather than the patch composition. The patch is > introducing TARGET_ALIGNED_ONLY as something defined > by configure, and the correct status for that macro is "needs to > be in poison.h"; having an intermediate state where the macro > exists but isn't poisoned isn't really a logical split of the work. > (The previous ALIGNED_ONLY didn't have so much need to be > poisoned because it was defined in cpu.h and so the only way to > expect to get it was by pulling in cpu.h.) > > Yes, I would say the same now that I am looking at the change more carefully. But then, repeating what you said, something like "poisoning is needed" (together with an explanation "why") should be in the commit message. "Also" doesn't not fit well here. Aleksandar > thanks > -- PMM >
diff --git a/configure b/configure index 714e7fb..482ba0b 100755 --- a/configure +++ b/configure @@ -7431,8 +7431,13 @@ for target in $target_list; do target_dir="$target" config_target_mak=$target_dir/config-target.mak target_name=$(echo $target | cut -d '-' -f 1) +target_aligned_only="no" +case "$target_name" in + alpha|hppa|mips64el|mips64|mipsel|mips|mipsn32|mipsn32el|sh4|sh4eb|sparc|sparc64|sparc32plus|xtensa|xtensaeb) + target_aligned_only="yes" + ;; +esac target_bigendian="no" - case "$target_name" in armeb|aarch64_be|hppa|lm32|m68k|microblaze|mips|mipsn32|mips64|moxie|or1k|ppc|ppc64|ppc64abi32|s390x|sh4eb|sparc|sparc64|sparc32plus|xtensaeb) target_bigendian=yes @@ -7717,6 +7722,9 @@ fi if supported_whpx_target $target; then echo "CONFIG_WHPX=y" >> $config_target_mak fi +if test "$target_aligned_only" = "yes" ; then + echo "TARGET_ALIGNED_ONLY=y" >> $config_target_mak +fi if test "$target_bigendian" = "yes" ; then echo "TARGET_WORDS_BIGENDIAN=y" >> $config_target_mak fi diff --git a/include/exec/poison.h b/include/exec/poison.h index b862320..955eb86 100644 --- a/include/exec/poison.h +++ b/include/exec/poison.h @@ -35,6 +35,7 @@ #pragma GCC poison TARGET_UNICORE32 #pragma GCC poison TARGET_XTENSA +#pragma GCC poison TARGET_ALIGNED_ONLY #pragma GCC poison TARGET_HAS_BFLT #pragma GCC poison TARGET_NAME #pragma GCC poison TARGET_SUPPORTS_MTTCG diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 5ee0046..9b50b73 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -89,7 +89,7 @@ struct TranslationBlock; * @do_unassigned_access: Callback for unassigned access handling. * (this is deprecated: new targets should use do_transaction_failed instead) * @do_unaligned_access: Callback for unaligned access handling, if - * the target defines #ALIGNED_ONLY. + * the target defines #TARGET_ALIGNED_ONLY. * @do_transaction_failed: Callback for handling failed memory transactions * (ie bus faults or external aborts; not MMU faults) * @virtio_is_big_endian: Callback to return %true if a CPU which supports diff --git a/target/alpha/cpu.h b/target/alpha/cpu.h index b3e8a82..16eb804 100644 --- a/target/alpha/cpu.h +++ b/target/alpha/cpu.h @@ -23,8 +23,6 @@ #include "cpu-qom.h" #include "exec/cpu-defs.h" -#define ALIGNED_ONLY - /* Alpha processors have a weak memory model */ #define TCG_GUEST_DEFAULT_MO (0) diff --git a/target/hppa/cpu.h b/target/hppa/cpu.h index aab251b..2be67c2 100644 --- a/target/hppa/cpu.h +++ b/target/hppa/cpu.h @@ -30,7 +30,6 @@ basis. It's probably easier to fall back to a strong memory model. */ #define TCG_GUEST_DEFAULT_MO TCG_MO_ALL -#define ALIGNED_ONLY #define MMU_KERNEL_IDX 0 #define MMU_USER_IDX 3 #define MMU_PHYS_IDX 4 diff --git a/target/mips/cpu.h b/target/mips/cpu.h index 21c0615..c13cd4e 100644 --- a/target/mips/cpu.h +++ b/target/mips/cpu.h @@ -1,8 +1,6 @@ #ifndef MIPS_CPU_H #define MIPS_CPU_H -#define ALIGNED_ONLY - #include "cpu-qom.h" #include "exec/cpu-defs.h" #include "fpu/softfloat.h" diff --git a/target/sh4/cpu.h b/target/sh4/cpu.h index aee733e..ecaa7a1 100644 --- a/target/sh4/cpu.h +++ b/target/sh4/cpu.h @@ -23,8 +23,6 @@ #include "cpu-qom.h" #include "exec/cpu-defs.h" -#define ALIGNED_ONLY - /* CPU Subtypes */ #define SH_CPU_SH7750 (1 << 0) #define SH_CPU_SH7750S (1 << 1) diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h index 8ed2250..1406f0b 100644 --- a/target/sparc/cpu.h +++ b/target/sparc/cpu.h @@ -5,8 +5,6 @@ #include "cpu-qom.h" #include "exec/cpu-defs.h" -#define ALIGNED_ONLY - #if !defined(TARGET_SPARC64) #define TARGET_DPREGS 16 #else diff --git a/target/xtensa/cpu.h b/target/xtensa/cpu.h index 2c27713..0459243 100644 --- a/target/xtensa/cpu.h +++ b/target/xtensa/cpu.h @@ -32,8 +32,6 @@ #include "exec/cpu-defs.h" #include "xtensa-isa.h" -#define ALIGNED_ONLY - /* Xtensa processors have a weak memory model */ #define TCG_GUEST_DEFAULT_MO (0) diff --git a/tcg/tcg.c b/tcg/tcg.c index be2c33c..8d23fb0 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -1926,7 +1926,7 @@ static const char * const ldst_name[] = }; static const char * const alignment_name[(MO_AMASK >> MO_ASHIFT) + 1] = { -#ifdef ALIGNED_ONLY +#ifdef TARGET_ALIGNED_ONLY [MO_UNALN >> MO_ASHIFT] = "un+", [MO_ALIGN >> MO_ASHIFT] = "", #else diff --git a/tcg/tcg.h b/tcg/tcg.h index b411e17..529acb2 100644 --- a/tcg/tcg.h +++ b/tcg/tcg.h @@ -333,10 +333,12 @@ typedef enum TCGMemOp { MO_TE = MO_LE, #endif - /* MO_UNALN accesses are never checked for alignment. + /* + * MO_UNALN accesses are never checked for alignment. * MO_ALIGN accesses will result in a call to the CPU's * do_unaligned_access hook if the guest address is not aligned. - * The default depends on whether the target CPU defines ALIGNED_ONLY. + * The default depends on whether the target CPU defines + * TARGET_ALIGNED_ONLY. * * Some architectures (e.g. ARMv8) need the address which is aligned * to a size more than the size of the memory access. @@ -353,7 +355,7 @@ typedef enum TCGMemOp { */ MO_ASHIFT = 4, MO_AMASK = 7 << MO_ASHIFT, -#ifdef ALIGNED_ONLY +#ifdef TARGET_ALIGNED_ONLY MO_ALIGN = 0, MO_UNALN = MO_AMASK, #else