From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Vivier Subject: Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness Date: Mon, 29 Feb 2016 14:24:02 +0100 Message-ID: <56D44672.2050304@redhat.com> References: <1456506526-10803-1-git-send-email-lvivier@redhat.com> <1456506526-10803-3-git-send-email-lvivier@redhat.com> <20160226184502.35oadt5jw2ck46la@hawk.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, thuth@redhat.com, dgibson@redhat.com, agraf@suse.de, pbonzini@redhat.com To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45055 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751053AbcB2NYH (ORCPT ); Mon, 29 Feb 2016 08:24:07 -0500 In-Reply-To: <20160226184502.35oadt5jw2ck46la@hawk.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> --- >> 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 >> #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 >> +# >> +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 >> +# >> +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 >> -# >> -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 >> +#include >> >> #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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Vivier Date: Mon, 29 Feb 2016 13:24:02 +0000 Subject: Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness Message-Id: <56D44672.2050304@redhat.com> List-Id: References: <1456506526-10803-1-git-send-email-lvivier@redhat.com> <1456506526-10803-3-git-send-email-lvivier@redhat.com> <20160226184502.35oadt5jw2ck46la@hawk.localdomain> In-Reply-To: <20160226184502.35oadt5jw2ck46la@hawk.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Andrew Jones Cc: kvm@vger.kernel.org, kvm-ppc@vger.kernel.org, thuth@redhat.com, dgibson@redhat.com, agraf@suse.de, pbonzini@redhat.com 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 >> --- >> 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 >> #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 >> +# >> +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%6 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 >> +# >> +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 >> -# >> -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 >> +#include >> >> #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 >