Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness

[Laurent Vivier <lvivier@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 <lvivier@redhat.com>
>> ---
>> 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@redhat.com>
>> +#
>> +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@redhat.com>
>> +#
>> +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@redhat.com>
>> -#
>> -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
>