On 26/02/2016 19:45, Andrew Jones wrote:
> On Fri, Feb 26, 2016 at 06:08:46PM +0100, Laurent Vivier wrote:
>> This patch allows to build tests for ppc64 little endian target
>> (ppc64le) on big and little endian hosts.
>>
>> We add a new parameter to configure to select the endianness of the
>> tests (--endian=little or --endian=big).
>>
>> I have built and tested big and little endian tests on a little
>> endian host, a big endian host, with kvm_hv and kvm_pr, and on
>> x86_64 with ppc64 as a TCG target.
>>
>> Signed-off-by: Laurent Vivier <lvivier@xxxxxxxxxx>
>> ---
>> v2: replace FIXUP_ENDIAN from linux by a home made version
>> (B_BE and RETURN_FROM_BE)
>>
>> Makefile | 11 ++++++-----
>> configure | 6 ++++++
>> lib/powerpc/asm/ppc_asm.h | 36 ++++++++++++++++++++++++++++++++++++
>> lib/ppc64/asm/io.h | 8 ++++++++
>> lib/ppc64/asm/ppc_asm.h | 1 +
>> powerpc/Makefile | 2 +-
>> powerpc/Makefile.big | 21 +++++++++++++++++++++
>> powerpc/Makefile.common | 18 ++++++++++--------
>> powerpc/Makefile.little | 21 +++++++++++++++++++++
>> powerpc/Makefile.ppc64 | 22 ----------------------
>> powerpc/cstart64.S | 14 ++++++++++----
>
> By renaming Makefile.ppc64 to Makefile.{little,big} we lose the
> 'ppc64' information. Since you've defined the ENDIAN config
> variable, then how about just doing
>
> ifeq ($(ENDIAN),little)
> CFLAGS = -mlittle-endian
> LDFLAGS = -EL
> else
> CFLAGS = -mbig-endian
> LDFLAGS = -EB
> endif
>
> Or even just substitute $(ENDIAN) when you want "big"/"little",
> such as in the CFLAGS.
I need the LDFLAGS too, so I will rename the file to Makefile.ppc64 and
use the "ifeq".
>
>> 11 files changed, 120 insertions(+), 40 deletions(-)
>> create mode 100644 lib/powerpc/asm/ppc_asm.h
>> create mode 100644 lib/ppc64/asm/ppc_asm.h
>> create mode 100644 powerpc/Makefile.big
>> create mode 100644 powerpc/Makefile.little
>> delete mode 100644 powerpc/Makefile.ppc64
>>
>> diff --git a/Makefile b/Makefile
>> index ddba941..8ed244a 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -40,12 +40,13 @@ include $(TEST_DIR)/Makefile
>> cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \
>> > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;)
>>
>> -CFLAGS += -g
>> -CFLAGS += $(autodepend-flags) -Wall
>> -CFLAGS += $(call cc-option, -fomit-frame-pointer, "")
>> -CFLAGS += $(call cc-option, -fno-stack-protector, "")
>> -CFLAGS += $(call cc-option, -fno-stack-protector-all, "")
>> +main_CFLAGS = -g
>> +main_CFLAGS += $(autodepend-flags) -Wall
>> +main_CFLAGS += $(call cc-option, -fomit-frame-pointer, "")
>> +main_CFLAGS += $(call cc-option, -fno-stack-protector, "")
>> +main_CFLAGS += $(call cc-option, -fno-stack-protector-all, "")
>
> I guess this is because cc-option doesn't work well with cross-endian
> compiling? Can we fix the function instead? In any case, do we have
> drop '-g -Wall and -MMD -MF $(dir $*).$(notdir $*).d' too?
In fact, my problem here is to always compile the boot_rom in gin endian
mode and the other files using the provided endianness.
So I have to extract all the common flags and add the good endianness
according the file to build.
But I agree, I did a minimalist change (renaming): I can try to extract
flags needed to build boot_rom to not change the common CFLAGS.
>>
>> +CFLAGS += $(main_CFLAGS)
>> CXXFLAGS += $(CFLAGS)
>>
>> autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
>> diff --git a/configure b/configure
>> index a685cca..0043ee9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -10,6 +10,7 @@ ar=ar
>> arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
>> host=$arch
>> cross_prefix=
>> +endian=big # default for ppc64, the only user
>
> If we default to 'little', then we don't have to worry about the current
> architectures using it. Is big the better default for some reason? Also,
> as both big and little work on both big and little, then maybe we should
> require it to be explicitly added (to make sure the user knows which
> they're using).
This is not true: on old PowerPC machines, using ppc970 (like my
PowerMac G5), the little endian mode doesn't work. It's why I set the
default to big endian.
But I agree: it's a good idea to have it added explicitly to know what
we are using.
>>
>> usage() {
>> cat <<-EOF
>> @@ -23,6 +24,7 @@ usage() {
>> --ld=LD ld linker to use ($ld)
>> --prefix=PREFIX where to install things ($prefix)
>> --kerneldir=DIR kernel build directory for kvm.h ($kerneldir)
>> + --endian=ENDIAN endianness to compile for (little or big)
>> EOF
>> exit 1
>> }
>> @@ -50,6 +52,9 @@ while [[ "$1" = -* ]]; do
>> --cross-prefix)
>> cross_prefix="$arg"
>> ;;
>> + --endian)
>> + endian="$arg"
>> + ;;
>
> Looks like a inconsistent whitespace here (should use tabs).
>
>> --cc)
>> cc="$arg"
>> ;;
>> @@ -139,4 +144,5 @@ AR=$cross_prefix$ar
>> API=$api
>> TEST_DIR=$testdir
>> FIRMWARE=$firmware
>> +ENDIAN=$endian
>> EOF
>> diff --git a/lib/powerpc/asm/ppc_asm.h b/lib/powerpc/asm/ppc_asm.h
>> new file mode 100644
>> index 0000000..f0ab241
>> --- /dev/null
>> +++ b/lib/powerpc/asm/ppc_asm.h
>> @@ -0,0 +1,36 @@
>> +#ifndef _ASMPOWERPC_PPC_ASM_H
>> +#define _ASMPOWERPC_PPC_ASM_H
>
> Adding this file is a good idea, we should probably move
> LOAD_REG_IMMEDIATE and LOAD_REG_ADDR here too.
I can add them in the series.
>> +
>> +#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> +
>> +#define B_BE(addr) \
>> + mtctr addr; \
>> + nop; \
>> + bctr
>> +
>> +#define RETURN_FROM_BE
>> +
>> +#elif __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> +
>> +#define B_BE(addr) \
>> + mfmsr r11; \
>> + xori r11,r11,1; \
>> + mtsrr0 addr; \
>> + mtsrr1 r11; \
>> + rfid; \
>> + b .
>> +
>> +#define RETURN_FROM_BE \
>> + .long 0x05000048; /* bl . + 4 */ \
>> + .long 0xa602487d; /* mflr r10 */ \
>> + .long 0x20004a39; /* addi r10,r10,32 */ \
>> + .long 0xa600607d; /* mfmsr r11 */ \
>> + .long 0x01006b69; /* xori r11,r11,1 */ \
>> + .long 0xa6035a7d; /* mtsrr0 r10 */ \
>> + .long 0xa6037b7d; /* mtsrr1 r11 */ \
>> + .long 0x2400004c; /* rfid */ \
>> + .long 0x00000048; /* b . */ \
>
> I think we only need RETURN_FROM_BE. B_BE is currently only used
> in enter_rtas, where it's fine now, but based on some of David's
> comments, I think we may eventually want to change even more
> state before the rtas call, which means the BE version will need
> the rfid anyway. Thus we can just drop the LE version straight into
> enter_rtas, replacing the xor with explicitly setting BE mode.
OK
>> +
>> +#endif /* __BYTE_ORDER__ */
>> +
>> +#endif /* _ASMPOWERPC_PPC_ASM_H */
>> diff --git a/lib/ppc64/asm/io.h b/lib/ppc64/asm/io.h
>> index c0801d4..4f2c31b 100644
>> --- a/lib/ppc64/asm/io.h
>> +++ b/lib/ppc64/asm/io.h
>> @@ -1,5 +1,13 @@
>> #ifndef _ASMPPC64_IO_H_
>> #define _ASMPPC64_IO_H_
>> +
>> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
>> +#define __cpu_is_be() (0)
>> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
>> #define __cpu_is_be() (1)
>> +#else
>> +#error Undefined byte order
>> +#endif
>> +
>> #include <asm-generic/io.h>
>> #endif
>> diff --git a/lib/ppc64/asm/ppc_asm.h b/lib/ppc64/asm/ppc_asm.h
>> new file mode 100644
>> index 0000000..e3929ee
>> --- /dev/null
>> +++ b/lib/ppc64/asm/ppc_asm.h
>> @@ -0,0 +1 @@
>> +#include "../../powerpc/asm/ppc_asm.h"
>> diff --git a/powerpc/Makefile b/powerpc/Makefile
>> index 369a38b..cc39a09 100644
>> --- a/powerpc/Makefile
>> +++ b/powerpc/Makefile
>> @@ -1 +1 @@
>> -include $(TEST_DIR)/Makefile.$(ARCH)
>> +include $(TEST_DIR)/Makefile.$(ENDIAN)
>> diff --git a/powerpc/Makefile.big b/powerpc/Makefile.big
>> new file mode 100644
>> index 0000000..d81c52d
>> --- /dev/null
>> +++ b/powerpc/Makefile.big
>> @@ -0,0 +1,21 @@
>> +#
>> +# ppc64 big-endian makefile
>> +#
>> +# Authors: Andrew Jones <drjones@xxxxxxxxxx>
>> +#
>> +bits = 64
>> +
>> +arch_CFLAGS = -mbig-endian
>> +arch_LDFLAGS = -EB
>> +
>> +cstart.o = $(TEST_DIR)/cstart64.o
>> +reloc.o = $(TEST_DIR)/reloc64.o
>> +cflatobjs += lib/ppc64/spinlock.o
>> +
>> +# ppc64 specific tests
>> +tests =
>> +
>> +include $(TEST_DIR)/Makefile.common
>> +
>> +arch_clean: powerpc_clean
>> + $(RM) lib/ppc64/.*.d
>> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
>> index cc27ac8..b088af6 100644
>> --- a/powerpc/Makefile.common
>> +++ b/powerpc/Makefile.common
>> @@ -11,14 +11,15 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
>>
>> ##################################################################
>>
>> -CFLAGS += $(arch_CFLAGS)
>> -CFLAGS += -std=gnu99
>> -CFLAGS += -ffreestanding
>> -CFLAGS += -Wextra
>> -CFLAGS += -O2
>> -CFLAGS += -I lib -I lib/libfdt
>> -CFLAGS += -Wa,-mregnames
>> -CFLAGS += -fpie
>> +common_CFLAGS = -std=gnu99
>> +common_CFLAGS += -ffreestanding
>> +common_CFLAGS += -Wextra
>> +common_CFLAGS += -O2
>> +common_CFLAGS += -I lib -I lib/libfdt
>> +common_CFLAGS += -Wa,-mregnames
>> +common_CFLAGS += -fpie
>> +
>> +CFLAGS += $(arch_CFLAGS) $(common_CFLAGS)
>
> I'm not sure what we gain by renaming to common_CFLAGS. Doesn't just
>
> %.elf: CFLAGS += $(arch_CFLAGS)
>
> work?
>
>>
>> asm-offsets = lib/$(ARCH)/asm-offsets.h
>> include scripts/asm-offsets.mak
>> @@ -48,6 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
>> dd if=/dev/zero of=$@ bs=256 count=1
>> $(OBJCOPY) -O binary $^ >(cat - >>$@)
>>
>> +$(TEST_DIR)/boot_rom.elf: CFLAGS = -mbig-endian $(common_CFLAGS) $(main_CFLAGS)
>
> And just
> $(TEST_DIR)/boot_rom.elf: CFLAGS += -mbig-endian
I can try.
>
>> $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
>> $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>>
>> diff --git a/powerpc/Makefile.little b/powerpc/Makefile.little
>> new file mode 100644
>> index 0000000..c37d2fc
>> --- /dev/null
>> +++ b/powerpc/Makefile.little
>> @@ -0,0 +1,21 @@
>> +#
>> +# ppc64 little-endian makefile
>> +#
>> +# Authors: Andrew Jones <drjones@xxxxxxxxxx>
>> +#
>> +bits = 64
>> +
>> +arch_CFLAGS = -mlittle-endian
>> +arch_LDFLAGS = -EL
>> +
>> +cstart.o = $(TEST_DIR)/cstart64.o
>> +reloc.o = $(TEST_DIR)/reloc64.o
>> +cflatobjs += lib/ppc64/spinlock.o
>> +
>> +# ppc64 specific tests
>> +tests =
>> +
>> +include $(TEST_DIR)/Makefile.common
>> +
>> +arch_clean: powerpc_clean
>> + $(RM) lib/ppc64/.*.d
>> diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
>> deleted file mode 100644
>> index 1cf277e..0000000
>> --- a/powerpc/Makefile.ppc64
>> +++ /dev/null
>> @@ -1,22 +0,0 @@
>> -#
>> -# ppc64 makefile
>> -#
>> -# Authors: Andrew Jones <drjones@xxxxxxxxxx>
>> -#
>> -bits = 64
>> -ldarch = elf64-powerpc
>> -
>> -arch_CFLAGS = -mbig-endian
>> -arch_LDFLAGS = -EB
>> -
>> -cstart.o = $(TEST_DIR)/cstart64.o
>> -reloc.o = $(TEST_DIR)/reloc64.o
>> -cflatobjs += lib/ppc64/spinlock.o
>> -
>> -# ppc64 specific tests
>> -tests =
>> -
>> -include $(TEST_DIR)/Makefile.common
>> -
>> -arch_clean: powerpc_clean
>> - $(RM) lib/ppc64/.*.d
>> diff --git a/powerpc/cstart64.S b/powerpc/cstart64.S
>> index f5530fb..60557a9 100644
>> --- a/powerpc/cstart64.S
>> +++ b/powerpc/cstart64.S
>> @@ -7,6 +7,7 @@
>> */
>> #define __ASSEMBLY__
>> #include <asm/hcall.h>
>> +#include <asm/ppc_asm.h>
>>
>> #define LOAD_REG_IMMEDIATE(reg,expr) \
>> lis reg,(expr)@highest; \
>> @@ -25,6 +26,7 @@
>> */
>> .globl start
>> start:
>> + RETURN_FROM_BE
>> /*
>> * We were loaded at QEMU's kernel load address, but we're not
>> * allowed to link there due to how QEMU deals with linker VMAs,
>> @@ -93,11 +95,15 @@ halt:
>> enter_rtas:
>> mflr r0
>> std r0, 16(r1)
>> +
>> + LOAD_REG_ADDR(r10,rtas_return_loc)
>> + mtlr r10
>> +
>> LOAD_REG_ADDR(r10, rtas_blob)
>> -//FIXME: change this bctrl to an rtas-prep, rfid, rtas-return sequence
>> - mtctr r10
>> - nop
>> - bctrl
>> + B_BE(r10)
>> +
>> +rtas_return_loc:
>> + RETURN_FROM_BE
>> ld r0, 16(r1)
>> mtlr r0
>> blr
>
> Besides my earlier comment about always needing rfid to prep for the
> rtas call (which my FIXUP comment here poorly indicated), then this
> looks good to me. Although I'm not sure I like the name RETURN_FROM_BE,
> as sometimes we're returning to LE, and sometimes we're not. In the
> not case, it's a bit confusing.
>
> Thanks,
> drew
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
