* [kvm-unit-tests PATCH v2 0/2] powerpc: add little-endian support
@ 2016-02-26 17:08 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-26 17:08 UTC (permalink / raw)
To: kvm, kvm-ppc; +Cc: drjones, thuth, dgibson, agraf, pbonzini, Laurent Vivier
This series must be applied on top of Drew's series (v7):
ppc64: initial drop
It allows to build and use big-endian kvm-unit-tests on little-
endian host (Patch #1), and to build and use little-endian
kvm-unit-tests (Patch #2) on big and little endian hosts.
The series is also available from:
https://github.com/vivier/kvm-unit-tests/commits/ppc64/endianness-v2
v2: replace FIXUP_ENDIAN from linux by a home made version
(B_BE and RETURN_FROM_BE)
Laurent Vivier (2):
powerpc: allow to build big-endian binaries on little-endian host
powerpc: select endianness
Makefile | 11 ++++++-----
configure | 8 +++++++-
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 | 29 +++++++++++++++--------------
powerpc/Makefile.little | 21 +++++++++++++++++++++
powerpc/Makefile.ppc64 | 19 -------------------
powerpc/cstart64.S | 14 ++++++++++----
11 files changed, 126 insertions(+), 44 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
--
2.5.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 0/2] powerpc: add little-endian support
@ 2016-02-26 17:08 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-26 17:08 UTC (permalink / raw)
To: kvm, kvm-ppc; +Cc: drjones, thuth, dgibson, agraf, pbonzini, Laurent Vivier
This series must be applied on top of Drew's series (v7):
ppc64: initial drop
It allows to build and use big-endian kvm-unit-tests on little-
endian host (Patch #1), and to build and use little-endian
kvm-unit-tests (Patch #2) on big and little endian hosts.
The series is also available from:
https://github.com/vivier/kvm-unit-tests/commits/ppc64/endianness-v2
v2: replace FIXUP_ENDIAN from linux by a home made version
(B_BE and RETURN_FROM_BE)
Laurent Vivier (2):
powerpc: allow to build big-endian binaries on little-endian host
powerpc: select endianness
Makefile | 11 ++++++-----
configure | 8 +++++++-
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 | 29 +++++++++++++++--------------
powerpc/Makefile.little | 21 +++++++++++++++++++++
powerpc/Makefile.ppc64 | 19 -------------------
powerpc/cstart64.S | 14 ++++++++++----
11 files changed, 126 insertions(+), 44 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
--
2.5.0
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host
2016-02-26 17:08 ` Laurent Vivier
@ 2016-02-26 17:08 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-26 17:08 UTC (permalink / raw)
To: kvm, kvm-ppc; +Cc: drjones, thuth, dgibson, agraf, pbonzini, Laurent Vivier
boot_rom will be always big endian (like SLOF is).
Make the endianness of the test files configurable with
arch_CFLAGS and arch_LDFLAGS.
For the moment, this only works with big endian.
Of course, once build on a little endian host, these binaries
can be used on the little endian host to test the
big endian mode of KVM.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
configure | 2 +-
powerpc/Makefile.common | 13 ++++++-------
powerpc/Makefile.ppc64 | 3 +++
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/configure b/configure
index 7d5702e..a685cca 100755
--- a/configure
+++ b/configure
@@ -7,7 +7,7 @@ ld=ld
objcopy=objcopy
objdump=objdump
ar=ar
-arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
+arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
host=$arch
cross_prefix=
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index cad728e..cc27ac8 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -11,6 +11,7 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
##################################################################
+CFLAGS += $(arch_CFLAGS)
CFLAGS += -std=gnu99
CFLAGS += -ffreestanding
CFLAGS += -Wextra
@@ -30,13 +31,11 @@ cflatobjs += lib/powerpc/hcall.o
cflatobjs += lib/powerpc/setup.o
cflatobjs += lib/powerpc/rtas.o
-libgcc := $(shell $(CC) $(machine) --print-libgcc-file-name)
-
-FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc)
-%.elf: LDFLAGS = $(CFLAGS) -nostdlib -pie
+FLATLIBS = $(libcflat) $(LIBFDT_archive)
+%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie
%.elf: %.o $(FLATLIBS) powerpc/flat.lds
- $(CC) $(LDFLAGS) -o $@ \
- -Wl,-T,powerpc/flat.lds,--build-id=none \
+ $(LD) $(LDFLAGS) -o $@ \
+ -T powerpc/flat.lds --build-id=none \
$(filter %.o, $^) $(FLATLIBS)
@echo -n Checking $@ for unsupported reloc types...
@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then \
@@ -50,7 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
$(OBJCOPY) -O binary $^ >(cat - >>$@)
$(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
- $(LD) -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
+ $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
powerpc_clean: libfdt_clean asm_offsets_clean
$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
index 899dd5e..1cf277e 100644
--- a/powerpc/Makefile.ppc64
+++ b/powerpc/Makefile.ppc64
@@ -6,6 +6,9 @@
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
--
2.5.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host
@ 2016-02-26 17:08 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-26 17:08 UTC (permalink / raw)
To: kvm, kvm-ppc; +Cc: drjones, thuth, dgibson, agraf, pbonzini, Laurent Vivier
boot_rom will be always big endian (like SLOF is).
Make the endianness of the test files configurable with
arch_CFLAGS and arch_LDFLAGS.
For the moment, this only works with big endian.
Of course, once build on a little endian host, these binaries
can be used on the little endian host to test the
big endian mode of KVM.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
configure | 2 +-
powerpc/Makefile.common | 13 ++++++-------
powerpc/Makefile.ppc64 | 3 +++
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/configure b/configure
index 7d5702e..a685cca 100755
--- a/configure
+++ b/configure
@@ -7,7 +7,7 @@ ld=ld
objcopy=objcopy
objdump=objdump
ar=ar
-arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
+arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
host=$arch
cross_prefix
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index cad728e..cc27ac8 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -11,6 +11,7 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
##################################################################
+CFLAGS += $(arch_CFLAGS)
CFLAGS += -std=gnu99
CFLAGS += -ffreestanding
CFLAGS += -Wextra
@@ -30,13 +31,11 @@ cflatobjs += lib/powerpc/hcall.o
cflatobjs += lib/powerpc/setup.o
cflatobjs += lib/powerpc/rtas.o
-libgcc := $(shell $(CC) $(machine) --print-libgcc-file-name)
-
-FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc)
-%.elf: LDFLAGS = $(CFLAGS) -nostdlib -pie
+FLATLIBS = $(libcflat) $(LIBFDT_archive)
+%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie
%.elf: %.o $(FLATLIBS) powerpc/flat.lds
- $(CC) $(LDFLAGS) -o $@ \
- -Wl,-T,powerpc/flat.lds,--build-id=none \
+ $(LD) $(LDFLAGS) -o $@ \
+ -T powerpc/flat.lds --build-id=none \
$(filter %.o, $^) $(FLATLIBS)
@echo -n Checking $@ for unsupported reloc types...
@if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then \
@@ -50,7 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
$(OBJCOPY) -O binary $^ >(cat - >>$@)
$(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
- $(LD) -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
+ $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
powerpc_clean: libfdt_clean asm_offsets_clean
$(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
index 899dd5e..1cf277e 100644
--- a/powerpc/Makefile.ppc64
+++ b/powerpc/Makefile.ppc64
@@ -6,6 +6,9 @@
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
--
2.5.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
2016-02-26 17:08 ` Laurent Vivier
@ 2016-02-26 17:08 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-26 17:08 UTC (permalink / raw)
To: kvm, kvm-ppc; +Cc: drjones, thuth, dgibson, agraf, pbonzini, Laurent Vivier
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 ++++++++++----
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, "")
+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
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"
+ ;;
--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
+
+#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 . */ \
+
+#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)
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)
$(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
--
2.5.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
@ 2016-02-26 17:08 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-26 17:08 UTC (permalink / raw)
To: kvm, kvm-ppc; +Cc: drjones, thuth, dgibson, agraf, pbonzini, Laurent Vivier
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 ++++++++++----
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, "")
+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
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"
+ ;;
--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
+
+#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 . */ \
+
+#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)
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)
$(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
--
2.5.0
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host
2016-02-26 17:08 ` Laurent Vivier
@ 2016-02-26 17:41 ` Andrew Jones
-1 siblings, 0 replies; 34+ messages in thread
From: Andrew Jones @ 2016-02-26 17:41 UTC (permalink / raw)
To: Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
On Fri, Feb 26, 2016 at 06:08:45PM +0100, Laurent Vivier wrote:
> boot_rom will be always big endian (like SLOF is).
> Make the endianness of the test files configurable with
> arch_CFLAGS and arch_LDFLAGS.
>
> For the moment, this only works with big endian.
>
> Of course, once build on a little endian host, these binaries
> can be used on the little endian host to test the
> big endian mode of KVM.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
> configure | 2 +-
> powerpc/Makefile.common | 13 ++++++-------
> powerpc/Makefile.ppc64 | 3 +++
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/configure b/configure
> index 7d5702e..a685cca 100755
> --- a/configure
> +++ b/configure
> @@ -7,7 +7,7 @@ ld=ld
> objcopy=objcopy
> objdump=objdump
> ar=ar
> -arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
> +arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
> host=$arch
> cross_prefix=
>
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index cad728e..cc27ac8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -11,6 +11,7 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
>
> ##################################################################
>
> +CFLAGS += $(arch_CFLAGS)
> CFLAGS += -std=gnu99
> CFLAGS += -ffreestanding
> CFLAGS += -Wextra
> @@ -30,13 +31,11 @@ cflatobjs += lib/powerpc/hcall.o
> cflatobjs += lib/powerpc/setup.o
> cflatobjs += lib/powerpc/rtas.o
>
> -libgcc := $(shell $(CC) $(machine) --print-libgcc-file-name)
> -
> -FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc)
> -%.elf: LDFLAGS = $(CFLAGS) -nostdlib -pie
> +FLATLIBS = $(libcflat) $(LIBFDT_archive)
> +%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie
> %.elf: %.o $(FLATLIBS) powerpc/flat.lds
> - $(CC) $(LDFLAGS) -o $@ \
> - -Wl,-T,powerpc/flat.lds,--build-id=none \
> + $(LD) $(LDFLAGS) -o $@ \
> + -T powerpc/flat.lds --build-id=none \
> $(filter %.o, $^) $(FLATLIBS)
> @echo -n Checking $@ for unsupported reloc types...
> @if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then \
> @@ -50,7 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
> $(OBJCOPY) -O binary $^ >(cat - >>$@)
>
> $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
> - $(LD) -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> + $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>
> powerpc_clean: libfdt_clean asm_offsets_clean
> $(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
> index 899dd5e..1cf277e 100644
> --- a/powerpc/Makefile.ppc64
> +++ b/powerpc/Makefile.ppc64
> @@ -6,6 +6,9 @@
> 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
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host
@ 2016-02-26 17:41 ` Andrew Jones
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Jones @ 2016-02-26 17:41 UTC (permalink / raw)
To: Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
On Fri, Feb 26, 2016 at 06:08:45PM +0100, Laurent Vivier wrote:
> boot_rom will be always big endian (like SLOF is).
> Make the endianness of the test files configurable with
> arch_CFLAGS and arch_LDFLAGS.
>
> For the moment, this only works with big endian.
>
> Of course, once build on a little endian host, these binaries
> can be used on the little endian host to test the
> big endian mode of KVM.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
> configure | 2 +-
> powerpc/Makefile.common | 13 ++++++-------
> powerpc/Makefile.ppc64 | 3 +++
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/configure b/configure
> index 7d5702e..a685cca 100755
> --- a/configure
> +++ b/configure
> @@ -7,7 +7,7 @@ ld=ld
> objcopy=objcopy
> objdump=objdump
> ar=ar
> -arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
> +arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
> host=$arch
> cross_prefix>
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index cad728e..cc27ac8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -11,6 +11,7 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
>
> ##################################################################
>
> +CFLAGS += $(arch_CFLAGS)
> CFLAGS += -std=gnu99
> CFLAGS += -ffreestanding
> CFLAGS += -Wextra
> @@ -30,13 +31,11 @@ cflatobjs += lib/powerpc/hcall.o
> cflatobjs += lib/powerpc/setup.o
> cflatobjs += lib/powerpc/rtas.o
>
> -libgcc := $(shell $(CC) $(machine) --print-libgcc-file-name)
> -
> -FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc)
> -%.elf: LDFLAGS = $(CFLAGS) -nostdlib -pie
> +FLATLIBS = $(libcflat) $(LIBFDT_archive)
> +%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie
> %.elf: %.o $(FLATLIBS) powerpc/flat.lds
> - $(CC) $(LDFLAGS) -o $@ \
> - -Wl,-T,powerpc/flat.lds,--build-id=none \
> + $(LD) $(LDFLAGS) -o $@ \
> + -T powerpc/flat.lds --build-id=none \
> $(filter %.o, $^) $(FLATLIBS)
> @echo -n Checking $@ for unsupported reloc types...
> @if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then \
> @@ -50,7 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
> $(OBJCOPY) -O binary $^ >(cat - >>$@)
>
> $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
> - $(LD) -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> + $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>
> powerpc_clean: libfdt_clean asm_offsets_clean
> $(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
> index 899dd5e..1cf277e 100644
> --- a/powerpc/Makefile.ppc64
> +++ b/powerpc/Makefile.ppc64
> @@ -6,6 +6,9 @@
> 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
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host
2016-02-26 17:08 ` Laurent Vivier
@ 2016-02-26 17:42 ` Andrew Jones
-1 siblings, 0 replies; 34+ messages in thread
From: Andrew Jones @ 2016-02-26 17:42 UTC (permalink / raw)
To: Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
One nit though, $SUBJECT is super loooong....
On Fri, Feb 26, 2016 at 06:08:45PM +0100, Laurent Vivier wrote:
> boot_rom will be always big endian (like SLOF is).
> Make the endianness of the test files configurable with
> arch_CFLAGS and arch_LDFLAGS.
>
> For the moment, this only works with big endian.
>
> Of course, once build on a little endian host, these binaries
> can be used on the little endian host to test the
> big endian mode of KVM.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> configure | 2 +-
> powerpc/Makefile.common | 13 ++++++-------
> powerpc/Makefile.ppc64 | 3 +++
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/configure b/configure
> index 7d5702e..a685cca 100755
> --- a/configure
> +++ b/configure
> @@ -7,7 +7,7 @@ ld=ld
> objcopy=objcopy
> objdump=objdump
> ar=ar
> -arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
> +arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
> host=$arch
> cross_prefix=
>
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index cad728e..cc27ac8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -11,6 +11,7 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
>
> ##################################################################
>
> +CFLAGS += $(arch_CFLAGS)
> CFLAGS += -std=gnu99
> CFLAGS += -ffreestanding
> CFLAGS += -Wextra
> @@ -30,13 +31,11 @@ cflatobjs += lib/powerpc/hcall.o
> cflatobjs += lib/powerpc/setup.o
> cflatobjs += lib/powerpc/rtas.o
>
> -libgcc := $(shell $(CC) $(machine) --print-libgcc-file-name)
> -
> -FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc)
> -%.elf: LDFLAGS = $(CFLAGS) -nostdlib -pie
> +FLATLIBS = $(libcflat) $(LIBFDT_archive)
> +%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie
> %.elf: %.o $(FLATLIBS) powerpc/flat.lds
> - $(CC) $(LDFLAGS) -o $@ \
> - -Wl,-T,powerpc/flat.lds,--build-id=none \
> + $(LD) $(LDFLAGS) -o $@ \
> + -T powerpc/flat.lds --build-id=none \
> $(filter %.o, $^) $(FLATLIBS)
> @echo -n Checking $@ for unsupported reloc types...
> @if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then \
> @@ -50,7 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
> $(OBJCOPY) -O binary $^ >(cat - >>$@)
>
> $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
> - $(LD) -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> + $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>
> powerpc_clean: libfdt_clean asm_offsets_clean
> $(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
> index 899dd5e..1cf277e 100644
> --- a/powerpc/Makefile.ppc64
> +++ b/powerpc/Makefile.ppc64
> @@ -6,6 +6,9 @@
> 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
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host
@ 2016-02-26 17:42 ` Andrew Jones
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Jones @ 2016-02-26 17:42 UTC (permalink / raw)
To: Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
One nit though, $SUBJECT is super loooong....
On Fri, Feb 26, 2016 at 06:08:45PM +0100, Laurent Vivier wrote:
> boot_rom will be always big endian (like SLOF is).
> Make the endianness of the test files configurable with
> arch_CFLAGS and arch_LDFLAGS.
>
> For the moment, this only works with big endian.
>
> Of course, once build on a little endian host, these binaries
> can be used on the little endian host to test the
> big endian mode of KVM.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> configure | 2 +-
> powerpc/Makefile.common | 13 ++++++-------
> powerpc/Makefile.ppc64 | 3 +++
> 3 files changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/configure b/configure
> index 7d5702e..a685cca 100755
> --- a/configure
> +++ b/configure
> @@ -7,7 +7,7 @@ ld=ld
> objcopy=objcopy
> objdump=objdump
> ar=ar
> -arch=`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'`
> +arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
> host=$arch
> cross_prefix>
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index cad728e..cc27ac8 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -11,6 +11,7 @@ all: $(TEST_DIR)/boot_rom.bin test_cases
>
> ##################################################################
>
> +CFLAGS += $(arch_CFLAGS)
> CFLAGS += -std=gnu99
> CFLAGS += -ffreestanding
> CFLAGS += -Wextra
> @@ -30,13 +31,11 @@ cflatobjs += lib/powerpc/hcall.o
> cflatobjs += lib/powerpc/setup.o
> cflatobjs += lib/powerpc/rtas.o
>
> -libgcc := $(shell $(CC) $(machine) --print-libgcc-file-name)
> -
> -FLATLIBS = $(libcflat) $(LIBFDT_archive) $(libgcc)
> -%.elf: LDFLAGS = $(CFLAGS) -nostdlib -pie
> +FLATLIBS = $(libcflat) $(LIBFDT_archive)
> +%.elf: LDFLAGS = $(arch_LDFLAGS) -nostdlib -pie
> %.elf: %.o $(FLATLIBS) powerpc/flat.lds
> - $(CC) $(LDFLAGS) -o $@ \
> - -Wl,-T,powerpc/flat.lds,--build-id=none \
> + $(LD) $(LDFLAGS) -o $@ \
> + -T powerpc/flat.lds --build-id=none \
> $(filter %.o, $^) $(FLATLIBS)
> @echo -n Checking $@ for unsupported reloc types...
> @if $(OBJDUMP) -R $@ | grep R_ | grep -v R_PPC64_RELATIVE; then \
> @@ -50,7 +49,7 @@ $(TEST_DIR)/boot_rom.bin: $(TEST_DIR)/boot_rom.elf
> $(OBJCOPY) -O binary $^ >(cat - >>$@)
>
> $(TEST_DIR)/boot_rom.elf: $(TEST_DIR)/boot_rom.o
> - $(LD) -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
> + $(LD) -EB -nostdlib -Ttext=0x100 --entry=start --build-id=none -o $@ $<
>
> powerpc_clean: libfdt_clean asm_offsets_clean
> $(RM) $(TEST_DIR)/*.{o,elf} $(TEST_DIR)/boot_rom.bin \
> diff --git a/powerpc/Makefile.ppc64 b/powerpc/Makefile.ppc64
> index 899dd5e..1cf277e 100644
> --- a/powerpc/Makefile.ppc64
> +++ b/powerpc/Makefile.ppc64
> @@ -6,6 +6,9 @@
> 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
> --
> 2.5.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
2016-02-26 17:08 ` Laurent Vivier
@ 2016-02-26 18:45 ` Andrew Jones
-1 siblings, 0 replies; 34+ messages in thread
From: Andrew Jones @ 2016-02-26 18:45 UTC (permalink / raw)
To: Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
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.
> 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?
>
> +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).
>
> 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.
> +
> +#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.
> +
> +#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
> $(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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
@ 2016-02-26 18:45 ` Andrew Jones
0 siblings, 0 replies; 34+ messages in thread
From: Andrew Jones @ 2016-02-26 18:45 UTC (permalink / raw)
To: Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
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.
> 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?
>
> +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).
>
> 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.
> +
> +#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.
> +
> +#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%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
> $(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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
2016-02-26 18:45 ` Andrew Jones
@ 2016-02-29 13:24 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-29 13:24 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
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
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
@ 2016-02-29 13:24 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-29 13:24 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
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%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 <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
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
2016-02-26 18:45 ` Andrew Jones
@ 2016-02-29 16:06 ` Paolo Bonzini
-1 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2016-02-29 16:06 UTC (permalink / raw)
To: Andrew Jones, Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf
On 26/02/2016 19:45, Andrew Jones wrote:
>> > + 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)
Note that this doesn't apply. I've applied patch 1 though.
Paolo
>> > +
>> > +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.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
@ 2016-02-29 16:06 ` Paolo Bonzini
0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2016-02-29 16:06 UTC (permalink / raw)
To: Andrew Jones, Laurent Vivier; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf
On 26/02/2016 19:45, Andrew Jones wrote:
>> > + 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)
Note that this doesn't apply. I've applied patch 1 though.
Paolo
>> > +
>> > +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.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
2016-02-29 16:06 ` Paolo Bonzini
@ 2016-02-29 16:44 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-29 16:44 UTC (permalink / raw)
To: Paolo Bonzini, Andrew Jones; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf
On 29/02/2016 17:06, Paolo Bonzini wrote:
>
>
> On 26/02/2016 19:45, Andrew Jones wrote:
>>>> + 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)
>
> Note that this doesn't apply. I've applied patch 1 though.
Yes, Drew has changed this part, and anyway I have to rework this.
Thanks,
Laurent
> Paolo
>
>>>> +
>>>> +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.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
@ 2016-02-29 16:44 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-29 16:44 UTC (permalink / raw)
To: Paolo Bonzini, Andrew Jones; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf
On 29/02/2016 17:06, Paolo Bonzini wrote:
>
>
> On 26/02/2016 19:45, Andrew Jones wrote:
>>>> + 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)
>
> Note that this doesn't apply. I've applied patch 1 though.
Yes, Drew has changed this part, and anyway I have to rework this.
Thanks,
Laurent
> Paolo
>
>>>> +
>>>> +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.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
2016-02-26 18:45 ` Andrew Jones
@ 2016-02-29 17:53 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-29 17:53 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
On 26/02/2016 19:45, Andrew Jones wrote:
...
>> 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
>
We can't do that because boot_rom.elf is also a .elf, so the both rules
apply and we end up with in the case of boot_rom.elf (and --endian=little):
CFLAGS += -mlittle-endian -mbig-endian
and it doesn't work. We must overwrite CFLAGS in the case of boot_rom.elf.
But boot_rom.S is a one line of assembly language, we don't need the
other flags, in fact...
Laurent
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness
@ 2016-02-29 17:53 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2016-02-29 17:53 UTC (permalink / raw)
To: Andrew Jones; +Cc: kvm, kvm-ppc, thuth, dgibson, agraf, pbonzini
On 26/02/2016 19:45, Andrew Jones wrote:
...
>> 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
>
We can't do that because boot_rom.elf is also a .elf, so the both rules
apply and we end up with in the case of boot_rom.elf (and --endian=little):
CFLAGS += -mlittle-endian -mbig-endian
and it doesn't work. We must overwrite CFLAGS in the case of boot_rom.elf.
But boot_rom.S is a one line of assembly language, we don't need the
other flags, in fact...
Laurent
^ permalink raw reply [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp
2016-02-26 17:08 ` Laurent Vivier
@ 2019-05-15 0:28 ` Suraj Jitindar Singh
-1 siblings, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-15 0:28 UTC (permalink / raw)
To: kvm; +Cc: kvm-ppc, lvivier, thuth, dgibson, Suraj Jitindar Singh
Currently the handler for a decrementer exception will simply reload the
maximum value (0x7FFFFFFF), which will take ~4 seconds to expire again.
This means that if a vcpu cedes, it will be ~4 seconds between wakeups.
The h_cede_tm test is testing a known breakage when a guest cedes while
suspended. To be sure we cede 500 times to check for the bug. However
since it takes ~4 seconds to be woken up once we've ceded, we only get
through ~20 iterations before we reach the 90 seconds timeout and the
test appears to fail.
Add an option when registering the decrementer handler to specify the
value which should be reloaded by the handler, allowing the timeout to be
chosen.
Modify the spr test to use the max timeout to preserve existing
behaviour.
Modify the h_cede_tm test to use a 10ms timeout to ensure we can perform
500 iterations before hitting the 90 second time limit for a test.
This means the h_cede_tm test now succeeds rather than timing out.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
V1 -> V2:
- Make decr variables static
- Load intial decr value in tm test to ensure known value present
---
lib/powerpc/handlers.c | 7 ++++---
powerpc/sprs.c | 5 +++--
powerpc/tm.c | 4 +++-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
index be8226a..c8721e0 100644
--- a/lib/powerpc/handlers.c
+++ b/lib/powerpc/handlers.c
@@ -12,11 +12,12 @@
/*
* Generic handler for decrementer exceptions (0x900)
- * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
+ * Just reset the decrementer back to the value specified when registering the
+ * handler
*/
-void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
+void dec_except_handler(struct pt_regs *regs __unused, void *data)
{
- uint32_t dec = 0x7FFFFFFF;
+ uint64_t dec = *((uint64_t *) data);
asm volatile ("mtdec %0" : : "r" (dec));
}
diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 6744bd8..0e2e1c9 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -253,6 +253,7 @@ int main(int argc, char **argv)
0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
-1ULL,
};
+ static uint64_t decr = 0x7FFFFFFF; /* Max value */
for (i = 1; i < argc; i++) {
if (!strcmp(argv[i], "-w")) {
@@ -288,8 +289,8 @@ int main(int argc, char **argv)
(void) getchar();
} else {
puts("Sleeping...\n");
- handle_exception(0x900, &dec_except_handler, NULL);
- asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
+ handle_exception(0x900, &dec_except_handler, &decr);
+ asm volatile ("mtdec %0" : : "r" (decr));
hcall(H_CEDE);
}
diff --git a/powerpc/tm.c b/powerpc/tm.c
index bd56baa..c588985 100644
--- a/powerpc/tm.c
+++ b/powerpc/tm.c
@@ -95,11 +95,13 @@ static bool enable_tm(void)
static void test_h_cede_tm(int argc, char **argv)
{
int i;
+ static uint64_t decr = 0x3FFFFF; /* ~10ms */
if (argc > 2)
report_abort("Unsupported argument: '%s'", argv[2]);
- handle_exception(0x900, &dec_except_handler, NULL);
+ handle_exception(0x900, &dec_except_handler, &decr);
+ asm volatile ("mtdec %0" : : "r" (decr));
if (!start_all_cpus(halt, 0))
report_abort("Failed to start secondary cpus");
--
2.13.6
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr
@ 2019-05-15 0:28 ` Suraj Jitindar Singh
0 siblings, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-15 0:28 UTC (permalink / raw)
To: kvm; +Cc: kvm-ppc, lvivier, thuth, dgibson, Suraj Jitindar Singh
Currently the handler for a decrementer exception will simply reload the
maximum value (0x7FFFFFFF), which will take ~4 seconds to expire again.
This means that if a vcpu cedes, it will be ~4 seconds between wakeups.
The h_cede_tm test is testing a known breakage when a guest cedes while
suspended. To be sure we cede 500 times to check for the bug. However
since it takes ~4 seconds to be woken up once we've ceded, we only get
through ~20 iterations before we reach the 90 seconds timeout and the
test appears to fail.
Add an option when registering the decrementer handler to specify the
value which should be reloaded by the handler, allowing the timeout to be
chosen.
Modify the spr test to use the max timeout to preserve existing
behaviour.
Modify the h_cede_tm test to use a 10ms timeout to ensure we can perform
500 iterations before hitting the 90 second time limit for a test.
This means the h_cede_tm test now succeeds rather than timing out.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
V1 -> V2:
- Make decr variables static
- Load intial decr value in tm test to ensure known value present
---
lib/powerpc/handlers.c | 7 ++++---
powerpc/sprs.c | 5 +++--
powerpc/tm.c | 4 +++-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
index be8226a..c8721e0 100644
--- a/lib/powerpc/handlers.c
+++ b/lib/powerpc/handlers.c
@@ -12,11 +12,12 @@
/*
* Generic handler for decrementer exceptions (0x900)
- * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
+ * Just reset the decrementer back to the value specified when registering the
+ * handler
*/
-void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
+void dec_except_handler(struct pt_regs *regs __unused, void *data)
{
- uint32_t dec = 0x7FFFFFFF;
+ uint64_t dec = *((uint64_t *) data);
asm volatile ("mtdec %0" : : "r" (dec));
}
diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 6744bd8..0e2e1c9 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -253,6 +253,7 @@ int main(int argc, char **argv)
0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
-1ULL,
};
+ static uint64_t decr = 0x7FFFFFFF; /* Max value */
for (i = 1; i < argc; i++) {
if (!strcmp(argv[i], "-w")) {
@@ -288,8 +289,8 @@ int main(int argc, char **argv)
(void) getchar();
} else {
puts("Sleeping...\n");
- handle_exception(0x900, &dec_except_handler, NULL);
- asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
+ handle_exception(0x900, &dec_except_handler, &decr);
+ asm volatile ("mtdec %0" : : "r" (decr));
hcall(H_CEDE);
}
diff --git a/powerpc/tm.c b/powerpc/tm.c
index bd56baa..c588985 100644
--- a/powerpc/tm.c
+++ b/powerpc/tm.c
@@ -95,11 +95,13 @@ static bool enable_tm(void)
static void test_h_cede_tm(int argc, char **argv)
{
int i;
+ static uint64_t decr = 0x3FFFFF; /* ~10ms */
if (argc > 2)
report_abort("Unsupported argument: '%s'", argv[2]);
- handle_exception(0x900, &dec_except_handler, NULL);
+ handle_exception(0x900, &dec_except_handler, &decr);
+ asm volatile ("mtdec %0" : : "r" (decr));
if (!start_all_cpus(halt, 0))
report_abort("Failed to start secondary cpus");
--
2.13.6
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default
2019-05-15 0:28 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr Suraj Jitindar Singh
@ 2019-05-15 0:28 ` Suraj Jitindar Singh
-1 siblings, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-15 0:28 UTC (permalink / raw)
To: kvm; +Cc: kvm-ppc, lvivier, thuth, dgibson, Suraj Jitindar Singh
This test was initially designed to test for a known bug where
performing a sequence of H_CEDE hcalls while suspended would cause a
vcpu to lockup in the host. The fix has been available for some time
now, so to increase coverage of this test remove the no-default flag.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
powerpc/unittests.cfg | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index af535b7..1e74948 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -64,7 +64,7 @@ file = emulator.elf
file = tm.elf
smp = 2,threads=2
extra_params = -machine cap-htm=on -append "h_cede_tm"
-groups = nodefault,h_cede_tm
+groups = h_cede_tm
[sprs]
file = sprs.elf
--
2.13.6
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default
@ 2019-05-15 0:28 ` Suraj Jitindar Singh
0 siblings, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-15 0:28 UTC (permalink / raw)
To: kvm; +Cc: kvm-ppc, lvivier, thuth, dgibson, Suraj Jitindar Singh
This test was initially designed to test for a known bug where
performing a sequence of H_CEDE hcalls while suspended would cause a
vcpu to lockup in the host. The fix has been available for some time
now, so to increase coverage of this test remove the no-default flag.
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
powerpc/unittests.cfg | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index af535b7..1e74948 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -64,7 +64,7 @@ file = emulator.elf
file = tm.elf
smp = 2,threads=2
extra_params = -machine cap-htm=on -append "h_cede_tm"
-groups = nodefault,h_cede_tm
+groups = h_cede_tm
[sprs]
file = sprs.elf
--
2.13.6
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp
2019-05-15 0:28 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr Suraj Jitindar Singh
@ 2019-05-15 16:22 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2019-05-15 16:22 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm; +Cc: kvm-ppc, thuth, dgibson
On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
> Currently the handler for a decrementer exception will simply reload the
> maximum value (0x7FFFFFFF), which will take ~4 seconds to expire again.
> This means that if a vcpu cedes, it will be ~4 seconds between wakeups.
>
> The h_cede_tm test is testing a known breakage when a guest cedes while
> suspended. To be sure we cede 500 times to check for the bug. However
> since it takes ~4 seconds to be woken up once we've ceded, we only get
> through ~20 iterations before we reach the 90 seconds timeout and the
> test appears to fail.
>
> Add an option when registering the decrementer handler to specify the
> value which should be reloaded by the handler, allowing the timeout to be
> chosen.
>
> Modify the spr test to use the max timeout to preserve existing
> behaviour.
> Modify the h_cede_tm test to use a 10ms timeout to ensure we can perform
> 500 iterations before hitting the 90 second time limit for a test.
>
> This means the h_cede_tm test now succeeds rather than timing out.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> ---
>
> V1 -> V2:
> - Make decr variables static
> - Load intial decr value in tm test to ensure known value present
> ---
> lib/powerpc/handlers.c | 7 ++++---
> powerpc/sprs.c | 5 +++--
> powerpc/tm.c | 4 +++-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> index be8226a..c8721e0 100644
> --- a/lib/powerpc/handlers.c
> +++ b/lib/powerpc/handlers.c
> @@ -12,11 +12,12 @@
>
> /*
> * Generic handler for decrementer exceptions (0x900)
> - * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
> + * Just reset the decrementer back to the value specified when registering the
> + * handler
> */
> -void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
> +void dec_except_handler(struct pt_regs *regs __unused, void *data)
> {
> - uint32_t dec = 0x7FFFFFFF;
> + uint64_t dec = *((uint64_t *) data);
>
> asm volatile ("mtdec %0" : : "r" (dec));
> }
> diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> index 6744bd8..0e2e1c9 100644
> --- a/powerpc/sprs.c
> +++ b/powerpc/sprs.c
> @@ -253,6 +253,7 @@ int main(int argc, char **argv)
> 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
> -1ULL,
> };
> + static uint64_t decr = 0x7FFFFFFF; /* Max value */
>
> for (i = 1; i < argc; i++) {
> if (!strcmp(argv[i], "-w")) {
> @@ -288,8 +289,8 @@ int main(int argc, char **argv)
> (void) getchar();
> } else {
> puts("Sleeping...\n");
> - handle_exception(0x900, &dec_except_handler, NULL);
> - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
> + handle_exception(0x900, &dec_except_handler, &decr);
> + asm volatile ("mtdec %0" : : "r" (decr));
why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF?
> hcall(H_CEDE);
> }
>
> diff --git a/powerpc/tm.c b/powerpc/tm.c
> index bd56baa..c588985 100644
> --- a/powerpc/tm.c
> +++ b/powerpc/tm.c
> @@ -95,11 +95,13 @@ static bool enable_tm(void)
> static void test_h_cede_tm(int argc, char **argv)
> {
> int i;
> + static uint64_t decr = 0x3FFFFF; /* ~10ms */
>
> if (argc > 2)
> report_abort("Unsupported argument: '%s'", argv[2]);
>
> - handle_exception(0x900, &dec_except_handler, NULL);
> + handle_exception(0x900, &dec_except_handler, &decr);
> + asm volatile ("mtdec %0" : : "r" (decr));
>
> if (!start_all_cpus(halt, 0))
> report_abort("Failed to start secondary cpus");
>
Thanks,
Laurent
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on
@ 2019-05-15 16:22 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2019-05-15 16:22 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm; +Cc: kvm-ppc, thuth, dgibson
On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
> Currently the handler for a decrementer exception will simply reload the
> maximum value (0x7FFFFFFF), which will take ~4 seconds to expire again.
> This means that if a vcpu cedes, it will be ~4 seconds between wakeups.
>
> The h_cede_tm test is testing a known breakage when a guest cedes while
> suspended. To be sure we cede 500 times to check for the bug. However
> since it takes ~4 seconds to be woken up once we've ceded, we only get
> through ~20 iterations before we reach the 90 seconds timeout and the
> test appears to fail.
>
> Add an option when registering the decrementer handler to specify the
> value which should be reloaded by the handler, allowing the timeout to be
> chosen.
>
> Modify the spr test to use the max timeout to preserve existing
> behaviour.
> Modify the h_cede_tm test to use a 10ms timeout to ensure we can perform
> 500 iterations before hitting the 90 second time limit for a test.
>
> This means the h_cede_tm test now succeeds rather than timing out.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> ---
>
> V1 -> V2:
> - Make decr variables static
> - Load intial decr value in tm test to ensure known value present
> ---
> lib/powerpc/handlers.c | 7 ++++---
> powerpc/sprs.c | 5 +++--
> powerpc/tm.c | 4 +++-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> index be8226a..c8721e0 100644
> --- a/lib/powerpc/handlers.c
> +++ b/lib/powerpc/handlers.c
> @@ -12,11 +12,12 @@
>
> /*
> * Generic handler for decrementer exceptions (0x900)
> - * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
> + * Just reset the decrementer back to the value specified when registering the
> + * handler
> */
> -void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
> +void dec_except_handler(struct pt_regs *regs __unused, void *data)
> {
> - uint32_t dec = 0x7FFFFFFF;
> + uint64_t dec = *((uint64_t *) data);
>
> asm volatile ("mtdec %0" : : "r" (dec));
> }
> diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> index 6744bd8..0e2e1c9 100644
> --- a/powerpc/sprs.c
> +++ b/powerpc/sprs.c
> @@ -253,6 +253,7 @@ int main(int argc, char **argv)
> 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
> -1ULL,
> };
> + static uint64_t decr = 0x7FFFFFFF; /* Max value */
>
> for (i = 1; i < argc; i++) {
> if (!strcmp(argv[i], "-w")) {
> @@ -288,8 +289,8 @@ int main(int argc, char **argv)
> (void) getchar();
> } else {
> puts("Sleeping...\n");
> - handle_exception(0x900, &dec_except_handler, NULL);
> - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
> + handle_exception(0x900, &dec_except_handler, &decr);
> + asm volatile ("mtdec %0" : : "r" (decr));
why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF?
> hcall(H_CEDE);
> }
>
> diff --git a/powerpc/tm.c b/powerpc/tm.c
> index bd56baa..c588985 100644
> --- a/powerpc/tm.c
> +++ b/powerpc/tm.c
> @@ -95,11 +95,13 @@ static bool enable_tm(void)
> static void test_h_cede_tm(int argc, char **argv)
> {
> int i;
> + static uint64_t decr = 0x3FFFFF; /* ~10ms */
>
> if (argc > 2)
> report_abort("Unsupported argument: '%s'", argv[2]);
>
> - handle_exception(0x900, &dec_except_handler, NULL);
> + handle_exception(0x900, &dec_except_handler, &decr);
> + asm volatile ("mtdec %0" : : "r" (decr));
>
> if (!start_all_cpus(halt, 0))
> report_abort("Failed to start secondary cpus");
>
Thanks,
Laurent
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default
2019-05-15 0:28 ` Suraj Jitindar Singh
@ 2019-05-15 16:25 ` Laurent Vivier
-1 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2019-05-15 16:25 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm; +Cc: kvm-ppc, thuth, dgibson
On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
> This test was initially designed to test for a known bug where
> performing a sequence of H_CEDE hcalls while suspended would cause a
> vcpu to lockup in the host. The fix has been available for some time
> now, so to increase coverage of this test remove the no-default flag.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> powerpc/unittests.cfg | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index af535b7..1e74948 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -64,7 +64,7 @@ file = emulator.elf
> file = tm.elf
> smp = 2,threads=2
> extra_params = -machine cap-htm=on -append "h_cede_tm"
> -groups = nodefault,h_cede_tm
> +groups = h_cede_tm
>
> [sprs]
> file = sprs.elf
>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default
@ 2019-05-15 16:25 ` Laurent Vivier
0 siblings, 0 replies; 34+ messages in thread
From: Laurent Vivier @ 2019-05-15 16:25 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm; +Cc: kvm-ppc, thuth, dgibson
On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
> This test was initially designed to test for a known bug where
> performing a sequence of H_CEDE hcalls while suspended would cause a
> vcpu to lockup in the host. The fix has been available for some time
> now, so to increase coverage of this test remove the no-default flag.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> powerpc/unittests.cfg | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index af535b7..1e74948 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -64,7 +64,7 @@ file = emulator.elf
> file = tm.elf
> smp = 2,threads=2
> extra_params = -machine cap-htm=on -append "h_cede_tm"
> -groups = nodefault,h_cede_tm
> +groups = h_cede_tm
>
> [sprs]
> file = sprs.elf
>
Reviewed-by: Laurent Vivier <lvivier@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp
2019-05-15 16:22 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on Laurent Vivier
@ 2019-05-15 23:27 ` Suraj Jitindar Singh
-1 siblings, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-15 23:27 UTC (permalink / raw)
To: Laurent Vivier, kvm; +Cc: kvm-ppc, thuth, dgibson
On Wed, 2019-05-15 at 18:22 +0200, Laurent Vivier wrote:
> On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
> > Currently the handler for a decrementer exception will simply
> > reload the
> > maximum value (0x7FFFFFFF), which will take ~4 seconds to expire
> > again.
> > This means that if a vcpu cedes, it will be ~4 seconds between
> > wakeups.
> >
> > The h_cede_tm test is testing a known breakage when a guest cedes
> > while
> > suspended. To be sure we cede 500 times to check for the bug.
> > However
> > since it takes ~4 seconds to be woken up once we've ceded, we only
> > get
> > through ~20 iterations before we reach the 90 seconds timeout and
> > the
> > test appears to fail.
> >
> > Add an option when registering the decrementer handler to specify
> > the
> > value which should be reloaded by the handler, allowing the timeout
> > to be
> > chosen.
> >
> > Modify the spr test to use the max timeout to preserve existing
> > behaviour.
> > Modify the h_cede_tm test to use a 10ms timeout to ensure we can
> > perform
> > 500 iterations before hitting the 90 second time limit for a test.
> >
> > This means the h_cede_tm test now succeeds rather than timing out.
> >
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> >
> > ---
> >
> > V1 -> V2:
> > - Make decr variables static
> > - Load intial decr value in tm test to ensure known value present
> > ---
> > lib/powerpc/handlers.c | 7 ++++---
> > powerpc/sprs.c | 5 +++--
> > powerpc/tm.c | 4 +++-
> > 3 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> > index be8226a..c8721e0 100644
> > --- a/lib/powerpc/handlers.c
> > +++ b/lib/powerpc/handlers.c
> > @@ -12,11 +12,12 @@
> >
> > /*
> > * Generic handler for decrementer exceptions (0x900)
> > - * Just reset the decrementer back to its maximum value
> > (0x7FFFFFFF)
> > + * Just reset the decrementer back to the value specified when
> > registering the
> > + * handler
> > */
> > -void dec_except_handler(struct pt_regs *regs __unused, void *data
> > __unused)
> > +void dec_except_handler(struct pt_regs *regs __unused, void *data)
> > {
> > - uint32_t dec = 0x7FFFFFFF;
> > + uint64_t dec = *((uint64_t *) data);
> >
> > asm volatile ("mtdec %0" : : "r" (dec));
> > }
> > diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> > index 6744bd8..0e2e1c9 100644
> > --- a/powerpc/sprs.c
> > +++ b/powerpc/sprs.c
> > @@ -253,6 +253,7 @@ int main(int argc, char **argv)
> > 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
> > -1ULL,
> > };
> > + static uint64_t decr = 0x7FFFFFFF; /* Max value */
> >
> > for (i = 1; i < argc; i++) {
> > if (!strcmp(argv[i], "-w")) {
> > @@ -288,8 +289,8 @@ int main(int argc, char **argv)
> > (void) getchar();
> > } else {
> > puts("Sleeping...\n");
> > - handle_exception(0x900, &dec_except_handler,
> > NULL);
> > - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
> > + handle_exception(0x900, &dec_except_handler,
> > &decr);
> > + asm volatile ("mtdec %0" : : "r" (decr));
>
> why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF?
Oh yeah, my mistake. I mis-read and thought they were the same. Is
there any reason it was 0x3FFFFFFF? Can this be fixed up when applying
or should I resend?
>
> > hcall(H_CEDE);
> > }
> >
> > diff --git a/powerpc/tm.c b/powerpc/tm.c
> > index bd56baa..c588985 100644
> > --- a/powerpc/tm.c
> > +++ b/powerpc/tm.c
> > @@ -95,11 +95,13 @@ static bool enable_tm(void)
> > static void test_h_cede_tm(int argc, char **argv)
> > {
> > int i;
> > + static uint64_t decr = 0x3FFFFF; /* ~10ms */
> >
> > if (argc > 2)
> > report_abort("Unsupported argument: '%s'",
> > argv[2]);
> >
> > - handle_exception(0x900, &dec_except_handler, NULL);
> > + handle_exception(0x900, &dec_except_handler, &decr);
> > + asm volatile ("mtdec %0" : : "r" (decr));
> >
> > if (!start_all_cpus(halt, 0))
> > report_abort("Failed to start secondary cpus");
> >
>
> Thanks,
> Laurent
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on
@ 2019-05-15 23:27 ` Suraj Jitindar Singh
0 siblings, 0 replies; 34+ messages in thread
From: Suraj Jitindar Singh @ 2019-05-15 23:27 UTC (permalink / raw)
To: Laurent Vivier, kvm; +Cc: kvm-ppc, thuth, dgibson
On Wed, 2019-05-15 at 18:22 +0200, Laurent Vivier wrote:
> On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
> > Currently the handler for a decrementer exception will simply
> > reload the
> > maximum value (0x7FFFFFFF), which will take ~4 seconds to expire
> > again.
> > This means that if a vcpu cedes, it will be ~4 seconds between
> > wakeups.
> >
> > The h_cede_tm test is testing a known breakage when a guest cedes
> > while
> > suspended. To be sure we cede 500 times to check for the bug.
> > However
> > since it takes ~4 seconds to be woken up once we've ceded, we only
> > get
> > through ~20 iterations before we reach the 90 seconds timeout and
> > the
> > test appears to fail.
> >
> > Add an option when registering the decrementer handler to specify
> > the
> > value which should be reloaded by the handler, allowing the timeout
> > to be
> > chosen.
> >
> > Modify the spr test to use the max timeout to preserve existing
> > behaviour.
> > Modify the h_cede_tm test to use a 10ms timeout to ensure we can
> > perform
> > 500 iterations before hitting the 90 second time limit for a test.
> >
> > This means the h_cede_tm test now succeeds rather than timing out.
> >
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> >
> > ---
> >
> > V1 -> V2:
> > - Make decr variables static
> > - Load intial decr value in tm test to ensure known value present
> > ---
> > lib/powerpc/handlers.c | 7 ++++---
> > powerpc/sprs.c | 5 +++--
> > powerpc/tm.c | 4 +++-
> > 3 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> > index be8226a..c8721e0 100644
> > --- a/lib/powerpc/handlers.c
> > +++ b/lib/powerpc/handlers.c
> > @@ -12,11 +12,12 @@
> >
> > /*
> > * Generic handler for decrementer exceptions (0x900)
> > - * Just reset the decrementer back to its maximum value
> > (0x7FFFFFFF)
> > + * Just reset the decrementer back to the value specified when
> > registering the
> > + * handler
> > */
> > -void dec_except_handler(struct pt_regs *regs __unused, void *data
> > __unused)
> > +void dec_except_handler(struct pt_regs *regs __unused, void *data)
> > {
> > - uint32_t dec = 0x7FFFFFFF;
> > + uint64_t dec = *((uint64_t *) data);
> >
> > asm volatile ("mtdec %0" : : "r" (dec));
> > }
> > diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> > index 6744bd8..0e2e1c9 100644
> > --- a/powerpc/sprs.c
> > +++ b/powerpc/sprs.c
> > @@ -253,6 +253,7 @@ int main(int argc, char **argv)
> > 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
> > -1ULL,
> > };
> > + static uint64_t decr = 0x7FFFFFFF; /* Max value */
> >
> > for (i = 1; i < argc; i++) {
> > if (!strcmp(argv[i], "-w")) {
> > @@ -288,8 +289,8 @@ int main(int argc, char **argv)
> > (void) getchar();
> > } else {
> > puts("Sleeping...\n");
> > - handle_exception(0x900, &dec_except_handler,
> > NULL);
> > - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
> > + handle_exception(0x900, &dec_except_handler,
> > &decr);
> > + asm volatile ("mtdec %0" : : "r" (decr));
>
> why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF?
Oh yeah, my mistake. I mis-read and thought they were the same. Is
there any reason it was 0x3FFFFFFF? Can this be fixed up when applying
or should I resend?
>
> > hcall(H_CEDE);
> > }
> >
> > diff --git a/powerpc/tm.c b/powerpc/tm.c
> > index bd56baa..c588985 100644
> > --- a/powerpc/tm.c
> > +++ b/powerpc/tm.c
> > @@ -95,11 +95,13 @@ static bool enable_tm(void)
> > static void test_h_cede_tm(int argc, char **argv)
> > {
> > int i;
> > + static uint64_t decr = 0x3FFFFF; /* ~10ms */
> >
> > if (argc > 2)
> > report_abort("Unsupported argument: '%s'",
> > argv[2]);
> >
> > - handle_exception(0x900, &dec_except_handler, NULL);
> > + handle_exception(0x900, &dec_except_handler, &decr);
> > + asm volatile ("mtdec %0" : : "r" (decr));
> >
> > if (!start_all_cpus(halt, 0))
> > report_abort("Failed to start secondary cpus");
> >
>
> Thanks,
> Laurent
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default
2019-05-15 0:28 ` Suraj Jitindar Singh
@ 2019-05-17 10:13 ` Thomas Huth
-1 siblings, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2019-05-17 10:13 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm; +Cc: kvm-ppc, lvivier, dgibson
On 15/05/2019 02.28, Suraj Jitindar Singh wrote:
> This test was initially designed to test for a known bug where
> performing a sequence of H_CEDE hcalls while suspended would cause a
> vcpu to lockup in the host. The fix has been available for some time
> now, so to increase coverage of this test remove the no-default flag.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> powerpc/unittests.cfg | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index af535b7..1e74948 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -64,7 +64,7 @@ file = emulator.elf
> file = tm.elf
> smp = 2,threads=2
> extra_params = -machine cap-htm=on -append "h_cede_tm"
> -groups = nodefault,h_cede_tm
> +groups = h_cede_tm
>
> [sprs]
> file = sprs.elf
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default
@ 2019-05-17 10:13 ` Thomas Huth
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2019-05-17 10:13 UTC (permalink / raw)
To: Suraj Jitindar Singh, kvm; +Cc: kvm-ppc, lvivier, dgibson
On 15/05/2019 02.28, Suraj Jitindar Singh wrote:
> This test was initially designed to test for a known bug where
> performing a sequence of H_CEDE hcalls while suspended would cause a
> vcpu to lockup in the host. The fix has been available for some time
> now, so to increase coverage of this test remove the no-default flag.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> powerpc/unittests.cfg | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index af535b7..1e74948 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -64,7 +64,7 @@ file = emulator.elf
> file = tm.elf
> smp = 2,threads=2
> extra_params = -machine cap-htm=on -append "h_cede_tm"
> -groups = nodefault,h_cede_tm
> +groups = h_cede_tm
>
> [sprs]
> file = sprs.elf
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp
2019-05-15 23:27 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on Suraj Jitindar Singh
@ 2019-05-17 10:20 ` Thomas Huth
-1 siblings, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2019-05-17 10:20 UTC (permalink / raw)
To: Suraj Jitindar Singh, Laurent Vivier, kvm; +Cc: kvm-ppc, dgibson
On 16/05/2019 01.27, Suraj Jitindar Singh wrote:
> On Wed, 2019-05-15 at 18:22 +0200, Laurent Vivier wrote:
>> On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
>>> Currently the handler for a decrementer exception will simply
>>> reload the
>>> maximum value (0x7FFFFFFF), which will take ~4 seconds to expire
>>> again.
>>> This means that if a vcpu cedes, it will be ~4 seconds between
>>> wakeups.
>>>
>>> The h_cede_tm test is testing a known breakage when a guest cedes
>>> while
>>> suspended. To be sure we cede 500 times to check for the bug.
>>> However
>>> since it takes ~4 seconds to be woken up once we've ceded, we only
>>> get
>>> through ~20 iterations before we reach the 90 seconds timeout and
>>> the
>>> test appears to fail.
>>>
>>> Add an option when registering the decrementer handler to specify
>>> the
>>> value which should be reloaded by the handler, allowing the timeout
>>> to be
>>> chosen.
>>>
>>> Modify the spr test to use the max timeout to preserve existing
>>> behaviour.
>>> Modify the h_cede_tm test to use a 10ms timeout to ensure we can
>>> perform
>>> 500 iterations before hitting the 90 second time limit for a test.
>>>
>>> This means the h_cede_tm test now succeeds rather than timing out.
>>>
>>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>>
>>> ---
>>>
>>> V1 -> V2:
>>> - Make decr variables static
>>> - Load intial decr value in tm test to ensure known value present
>>> ---
>>> lib/powerpc/handlers.c | 7 ++++---
>>> powerpc/sprs.c | 5 +++--
>>> powerpc/tm.c | 4 +++-
>>> 3 files changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
>>> index be8226a..c8721e0 100644
>>> --- a/lib/powerpc/handlers.c
>>> +++ b/lib/powerpc/handlers.c
>>> @@ -12,11 +12,12 @@
>>>
>>> /*
>>> * Generic handler for decrementer exceptions (0x900)
>>> - * Just reset the decrementer back to its maximum value
>>> (0x7FFFFFFF)
>>> + * Just reset the decrementer back to the value specified when
>>> registering the
>>> + * handler
>>> */
>>> -void dec_except_handler(struct pt_regs *regs __unused, void *data
>>> __unused)
>>> +void dec_except_handler(struct pt_regs *regs __unused, void *data)
>>> {
>>> - uint32_t dec = 0x7FFFFFFF;
>>> + uint64_t dec = *((uint64_t *) data);
>>>
>>> asm volatile ("mtdec %0" : : "r" (dec));
>>> }
>>> diff --git a/powerpc/sprs.c b/powerpc/sprs.c
>>> index 6744bd8..0e2e1c9 100644
>>> --- a/powerpc/sprs.c
>>> +++ b/powerpc/sprs.c
>>> @@ -253,6 +253,7 @@ int main(int argc, char **argv)
>>> 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
>>> -1ULL,
>>> };
>>> + static uint64_t decr = 0x7FFFFFFF; /* Max value */
>>>
>>> for (i = 1; i < argc; i++) {
>>> if (!strcmp(argv[i], "-w")) {
>>> @@ -288,8 +289,8 @@ int main(int argc, char **argv)
>>> (void) getchar();
>>> } else {
>>> puts("Sleeping...\n");
>>> - handle_exception(0x900, &dec_except_handler,
>>> NULL);
>>> - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
>>> + handle_exception(0x900, &dec_except_handler,
>>> &decr);
>>> + asm volatile ("mtdec %0" : : "r" (decr));
>>
>> why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF?
>
> Oh yeah, my mistake. I mis-read and thought they were the same. Is
> there any reason it was 0x3FFFFFFF? Can this be fixed up when applying
> or should I resend?
Should be fine to fix this when the patch gets picked up. With the value
fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on
@ 2019-05-17 10:20 ` Thomas Huth
0 siblings, 0 replies; 34+ messages in thread
From: Thomas Huth @ 2019-05-17 10:20 UTC (permalink / raw)
To: Suraj Jitindar Singh, Laurent Vivier, kvm; +Cc: kvm-ppc, dgibson
On 16/05/2019 01.27, Suraj Jitindar Singh wrote:
> On Wed, 2019-05-15 at 18:22 +0200, Laurent Vivier wrote:
>> On 15/05/2019 02:28, Suraj Jitindar Singh wrote:
>>> Currently the handler for a decrementer exception will simply
>>> reload the
>>> maximum value (0x7FFFFFFF), which will take ~4 seconds to expire
>>> again.
>>> This means that if a vcpu cedes, it will be ~4 seconds between
>>> wakeups.
>>>
>>> The h_cede_tm test is testing a known breakage when a guest cedes
>>> while
>>> suspended. To be sure we cede 500 times to check for the bug.
>>> However
>>> since it takes ~4 seconds to be woken up once we've ceded, we only
>>> get
>>> through ~20 iterations before we reach the 90 seconds timeout and
>>> the
>>> test appears to fail.
>>>
>>> Add an option when registering the decrementer handler to specify
>>> the
>>> value which should be reloaded by the handler, allowing the timeout
>>> to be
>>> chosen.
>>>
>>> Modify the spr test to use the max timeout to preserve existing
>>> behaviour.
>>> Modify the h_cede_tm test to use a 10ms timeout to ensure we can
>>> perform
>>> 500 iterations before hitting the 90 second time limit for a test.
>>>
>>> This means the h_cede_tm test now succeeds rather than timing out.
>>>
>>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>>>
>>> ---
>>>
>>> V1 -> V2:
>>> - Make decr variables static
>>> - Load intial decr value in tm test to ensure known value present
>>> ---
>>> lib/powerpc/handlers.c | 7 ++++---
>>> powerpc/sprs.c | 5 +++--
>>> powerpc/tm.c | 4 +++-
>>> 3 files changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
>>> index be8226a..c8721e0 100644
>>> --- a/lib/powerpc/handlers.c
>>> +++ b/lib/powerpc/handlers.c
>>> @@ -12,11 +12,12 @@
>>>
>>> /*
>>> * Generic handler for decrementer exceptions (0x900)
>>> - * Just reset the decrementer back to its maximum value
>>> (0x7FFFFFFF)
>>> + * Just reset the decrementer back to the value specified when
>>> registering the
>>> + * handler
>>> */
>>> -void dec_except_handler(struct pt_regs *regs __unused, void *data
>>> __unused)
>>> +void dec_except_handler(struct pt_regs *regs __unused, void *data)
>>> {
>>> - uint32_t dec = 0x7FFFFFFF;
>>> + uint64_t dec = *((uint64_t *) data);
>>>
>>> asm volatile ("mtdec %0" : : "r" (dec));
>>> }
>>> diff --git a/powerpc/sprs.c b/powerpc/sprs.c
>>> index 6744bd8..0e2e1c9 100644
>>> --- a/powerpc/sprs.c
>>> +++ b/powerpc/sprs.c
>>> @@ -253,6 +253,7 @@ int main(int argc, char **argv)
>>> 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL,
>>> -1ULL,
>>> };
>>> + static uint64_t decr = 0x7FFFFFFF; /* Max value */
>>>
>>> for (i = 1; i < argc; i++) {
>>> if (!strcmp(argv[i], "-w")) {
>>> @@ -288,8 +289,8 @@ int main(int argc, char **argv)
>>> (void) getchar();
>>> } else {
>>> puts("Sleeping...\n");
>>> - handle_exception(0x900, &dec_except_handler,
>>> NULL);
>>> - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF));
>>> + handle_exception(0x900, &dec_except_handler,
>>> &decr);
>>> + asm volatile ("mtdec %0" : : "r" (decr));
>>
>> why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF?
>
> Oh yeah, my mistake. I mis-read and thought they were the same. Is
> there any reason it was 0x3FFFFFFF? Can this be fixed up when applying
> or should I resend?
Should be fine to fix this when the patch gets picked up. With the value
fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2019-05-17 10:20 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-15 0:28 [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp Suraj Jitindar Singh
2019-05-15 0:28 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr Suraj Jitindar Singh
2019-05-15 0:28 ` [kvm-unit-tests PATCH v2 2/2] powerpc: Make h_cede_tm test run by default Suraj Jitindar Singh
2019-05-15 0:28 ` Suraj Jitindar Singh
2019-05-15 16:25 ` Laurent Vivier
2019-05-15 16:25 ` Laurent Vivier
2019-05-17 10:13 ` Thomas Huth
2019-05-17 10:13 ` Thomas Huth
2019-05-15 16:22 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp Laurent Vivier
2019-05-15 16:22 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on Laurent Vivier
2019-05-15 23:27 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp Suraj Jitindar Singh
2019-05-15 23:27 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on Suraj Jitindar Singh
2019-05-17 10:20 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp Thomas Huth
2019-05-17 10:20 ` [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on Thomas Huth
-- strict thread matches above, loose matches on Subject: below --
2016-02-26 17:08 [kvm-unit-tests PATCH v2 0/2] powerpc: add little-endian support Laurent Vivier
2016-02-26 17:08 ` Laurent Vivier
2016-02-26 17:08 ` [kvm-unit-tests PATCH v2 1/2] powerpc: allow to build big-endian binaries on little-endian host Laurent Vivier
2016-02-26 17:08 ` Laurent Vivier
2016-02-26 17:41 ` Andrew Jones
2016-02-26 17:41 ` Andrew Jones
2016-02-26 17:42 ` Andrew Jones
2016-02-26 17:42 ` Andrew Jones
2016-02-26 17:08 ` [kvm-unit-tests PATCH v2 2/2] powerpc: select endianness Laurent Vivier
2016-02-26 17:08 ` Laurent Vivier
2016-02-26 18:45 ` Andrew Jones
2016-02-26 18:45 ` Andrew Jones
2016-02-29 13:24 ` Laurent Vivier
2016-02-29 13:24 ` Laurent Vivier
2016-02-29 16:06 ` Paolo Bonzini
2016-02-29 16:06 ` Paolo Bonzini
2016-02-29 16:44 ` Laurent Vivier
2016-02-29 16:44 ` Laurent Vivier
2016-02-29 17:53 ` Laurent Vivier
2016-02-29 17:53 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.