* [PATCH v3 0/3] Early serial on Power
@ 2023-07-06 19:04 Shawn Anastasio
2023-07-06 19:04 ` [PATCH v3 1/3] xen/ppc: Set up a basic C environment Shawn Anastasio
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-06 19:04 UTC (permalink / raw)
To: xen-devel; +Cc: Timothy Pearson, Andrew Cooper, Jan Beulich, Shawn Anastasio
Hello all,
This series adds support for early serial printing on Power, as well as
a simple CI smoke test modeled after the riscv one.
The first patch is responsible for setting up a basic C environment with
an initial stack while the second sets up an Open Firmware serial console
and primitive early_printk infrastructure.
This will currently only run on QEMU pseries VMs, since the firmware
interface on bare metal differs significantly. Support for bare metal
will be added in a future series.
Thanks,
Shawn
--
Changes in v3:
- Set up r2 TOC pointer in start
- Change indirect call to start_xen to a direct one
- Use 32-bit constant loads for stack pointer, etc. in start
- Our load address is < 4GB so this works fine
- In a future series we'll get -fPIC working and change all of
these immediate address loads to TOC-relative ones.
- Move .bss initialization to patch 2
- Move cpu0_boot_stack declaration to setup.c
- Bump stack size down to one page (64k), previous was way overkill.
- Remove unnecessary STACK_FRAME_OVERHEAD component from of-call.S
stack frame size calculation
- Add assertion that `struct cpu_user_regs` is stack-aligned
- Move magic `or 31,31,31` to appropriately-named macro
- Fix formatting of loop in setup.c
- Add newline in arch/ppc/Makefile to preserve alphabetical ordering
per group
- Rebase on staging, including the final version of
'xen/types: Rework stdint vs __{u,s}$N types'
Changes in v2:
- Split main patch into two - one for basic C environment setup and
one for serial
- Mark OpenFirmware functions and early_printk functions as __init and
change boot-of.o to boot-of.init.o in Makefile
- Change <xen/lib.h> include to <xen/stdarg.h> and drop skeleton
headers that are no longer necessary for build as a result
- Add loop to clear .bss before jumping to C environment that was
accidentally excluded from the first series
- Move common asm macros from processor.h to asm-defns.h
- Change note in head.S about preserved registers to a multi-line
comment so it's more noticeable
- Drop reg-defs.h and use '%'-prefixed register names in assembly
- This is necessary since -mregnames, which allows standard
non-prefixed register names without manual macro definitions,
is not supported by LLVM's assembler.
- Drop inline asm swab routines in favor of __builtin_bswap family
- Fix up types.h in accordance with (as of now, unmerged)
'xen/types: Rework stdint vs __{u,s}$N types'
- Remove unnecessary braces for single-line statements
- Remove unnecessary license text when SPDX header is present
- Fix alphabetical ordering of object declarations in Makefile
- Drop 'extern' from enter_of prototype, ensure prototypes have
argument names
Shawn Anastasio (3):
xen/ppc: Set up a basic C environment
xen/ppc: Implement early serial printk on pseries
automation: Add smoke test for ppc64le
automation/gitlab-ci/test.yaml | 20 ++++
automation/scripts/qemu-smoke-ppc64le.sh | 27 +++++
xen/arch/ppc/Kconfig.debug | 5 +
xen/arch/ppc/Makefile | 4 +
xen/arch/ppc/boot-of.c | 100 ++++++++++++++++
xen/arch/ppc/configs/ppc64_defconfig | 1 +
xen/arch/ppc/early_printk.c | 28 +++++
xen/arch/ppc/include/asm/asm-defns.h | 57 ++++++++++
xen/arch/ppc/include/asm/boot.h | 23 ++++
xen/arch/ppc/include/asm/byteorder.h | 12 ++
xen/arch/ppc/include/asm/config.h | 5 +-
xen/arch/ppc/include/asm/early_printk.h | 15 +++
xen/arch/ppc/include/asm/msr.h | 51 +++++++++
xen/arch/ppc/include/asm/processor.h | 139 +++++++++++++++++++++++
xen/arch/ppc/include/asm/types.h | 21 ++++
xen/arch/ppc/ppc64/Makefile | 1 +
xen/arch/ppc/ppc64/asm-offsets.c | 59 ++++++++++
xen/arch/ppc/ppc64/head.S | 49 ++++----
xen/arch/ppc/ppc64/of-call.S | 83 ++++++++++++++
xen/arch/ppc/setup.c | 32 ++++++
20 files changed, 711 insertions(+), 21 deletions(-)
create mode 100755 automation/scripts/qemu-smoke-ppc64le.sh
create mode 100644 xen/arch/ppc/boot-of.c
create mode 100644 xen/arch/ppc/early_printk.c
create mode 100644 xen/arch/ppc/include/asm/asm-defns.h
create mode 100644 xen/arch/ppc/include/asm/boot.h
create mode 100644 xen/arch/ppc/include/asm/byteorder.h
create mode 100644 xen/arch/ppc/include/asm/early_printk.h
create mode 100644 xen/arch/ppc/include/asm/msr.h
create mode 100644 xen/arch/ppc/include/asm/processor.h
create mode 100644 xen/arch/ppc/include/asm/types.h
create mode 100644 xen/arch/ppc/ppc64/of-call.S
create mode 100644 xen/arch/ppc/setup.c
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 1/3] xen/ppc: Set up a basic C environment
2023-07-06 19:04 [PATCH v3 0/3] Early serial on Power Shawn Anastasio
@ 2023-07-06 19:04 ` Shawn Anastasio
2023-07-17 15:38 ` Jan Beulich
2023-07-06 19:04 ` [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries Shawn Anastasio
2023-07-06 19:04 ` [PATCH v3 3/3] automation: Add smoke test for ppc64le Shawn Anastasio
2 siblings, 1 reply; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-06 19:04 UTC (permalink / raw)
To: xen-devel; +Cc: Timothy Pearson, Andrew Cooper, Jan Beulich, Shawn Anastasio
Update ppc64/head.S to set up an initial boot stack, zero the .bss
section, and jump to C.
Also refactor the endian fixup trampoline into its own macro, since it
will need to be used in multiple places, including every time we make a
call into firmware (see next commit).
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/ppc/Makefile | 2 ++
xen/arch/ppc/include/asm/asm-defns.h | 40 ++++++++++++++++++++++++++++
xen/arch/ppc/include/asm/config.h | 2 +-
xen/arch/ppc/ppc64/head.S | 40 ++++++++++++++--------------
xen/arch/ppc/setup.c | 19 +++++++++++++
5 files changed, 82 insertions(+), 21 deletions(-)
create mode 100644 xen/arch/ppc/include/asm/asm-defns.h
create mode 100644 xen/arch/ppc/setup.c
diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
index 98220648af..530fba2121 100644
--- a/xen/arch/ppc/Makefile
+++ b/xen/arch/ppc/Makefile
@@ -1,5 +1,7 @@
obj-$(CONFIG_PPC64) += ppc64/
+obj-y += setup.o
+
$(TARGET): $(TARGET)-syms
cp -f $< $@
diff --git a/xen/arch/ppc/include/asm/asm-defns.h b/xen/arch/ppc/include/asm/asm-defns.h
new file mode 100644
index 0000000000..6ea35f6edb
--- /dev/null
+++ b/xen/arch/ppc/include/asm/asm-defns.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef _ASM_PPC_ASM_DEFNS_H
+#define _ASM_PPC_ASM_DEFNS_H
+
+/*
+ * Load a 64-bit immediate value into the specified GPR.
+ */
+#define LOAD_IMM64(reg, val) \
+ lis reg, (val) @highest; \
+ ori reg, reg, (val) @higher; \
+ rldicr reg, reg, 32, 31; \
+ oris reg, reg, (val) @h; \
+ ori reg, reg, (val) @l;
+
+#define LOAD_IMM32(reg, val) \
+ lis reg, (val) @h; \
+ ori reg, reg, (val) @l; \
+
+/*
+ * Depending on how we were booted, the CPU could be running in either
+ * Little Endian or Big Endian mode. The following trampoline from Linux
+ * cleverly uses an instruction that encodes to a NOP if the CPU's
+ * endianness matches the assumption of the assembler (LE, in our case)
+ * or a branch to code that performs the endian switch in the other case.
+ */
+#define FIXUP_ENDIAN \
+ tdi 0, 0, 0x48; /* Reverse endian of b . + 8 */ \
+ b . + 44; /* Skip trampoline if endian is good */ \
+ .long 0xa600607d; /* mfmsr r11 */ \
+ .long 0x01006b69; /* xori r11,r11,1 */ \
+ .long 0x00004039; /* li r10,0 */ \
+ .long 0x6401417d; /* mtmsrd r10,1 */ \
+ .long 0x05009f42; /* bcl 20,31,$+4 */ \
+ .long 0xa602487d; /* mflr r10 */ \
+ .long 0x14004a39; /* addi r10,r10,20 */ \
+ .long 0xa6035a7d; /* mtsrr0 r10 */ \
+ .long 0xa6037b7d; /* mtsrr1 r11 */ \
+ .long 0x2400004c /* rfid */
+
+#endif /* _ASM_PPC_ASM_DEFNS_H */
diff --git a/xen/arch/ppc/include/asm/config.h b/xen/arch/ppc/include/asm/config.h
index b9a6814f00..01ca5d0803 100644
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -43,7 +43,7 @@
#define SMP_CACHE_BYTES (1 << 6)
-#define STACK_ORDER 2
+#define STACK_ORDER 0
#define STACK_SIZE (PAGE_SIZE << STACK_ORDER)
/* 288 bytes below the stack pointer must be preserved by interrupt handlers */
diff --git a/xen/arch/ppc/ppc64/head.S b/xen/arch/ppc/ppc64/head.S
index 2fcefb40d8..7d848611cc 100644
--- a/xen/arch/ppc/ppc64/head.S
+++ b/xen/arch/ppc/ppc64/head.S
@@ -1,30 +1,30 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <asm/asm-defns.h>
+
.section .text.header, "ax", %progbits
ENTRY(start)
/*
- * Depending on how we were booted, the CPU could be running in either
- * Little Endian or Big Endian mode. The following trampoline from Linux
- * cleverly uses an instruction that encodes to a NOP if the CPU's
- * endianness matches the assumption of the assembler (LE, in our case)
- * or a branch to code that performs the endian switch in the other case.
+ * NOTE: argument registers (r3-r9) must be preserved until the C entrypoint
*/
- tdi 0, 0, 0x48 /* Reverse endian of b . + 8 */
- b . + 44 /* Skip trampoline if endian is good */
- .long 0xa600607d /* mfmsr r11 */
- .long 0x01006b69 /* xori r11,r11,1 */
- .long 0x00004039 /* li r10,0 */
- .long 0x6401417d /* mtmsrd r10,1 */
- .long 0x05009f42 /* bcl 20,31,$+4 */
- .long 0xa602487d /* mflr r10 */
- .long 0x14004a39 /* addi r10,r10,20 */
- .long 0xa6035a7d /* mtsrr0 r10 */
- .long 0xa6037b7d /* mtsrr1 r11 */
- .long 0x2400004c /* rfid */
-
- /* Now that the endianness is confirmed, continue */
-1: b 1b
+ FIXUP_ENDIAN
+
+ /* set up the TOC pointer */
+ LOAD_IMM32(%r2, .TOC.)
+
+ /* set up the initial stack */
+ LOAD_IMM32(%r1, cpu0_boot_stack)
+ li %r11, 0
+ stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
+
+ /* call the C entrypoint */
+ bl start_xen
+
+ /* should never return */
+ trap
.size start, . - start
.type start, %function
+
+ .section .init.data, "aw", @progbits
diff --git a/xen/arch/ppc/setup.c b/xen/arch/ppc/setup.c
new file mode 100644
index 0000000000..73106474b2
--- /dev/null
+++ b/xen/arch/ppc/setup.c
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#include <xen/init.h>
+
+/* Xen stack for bringing up the first CPU. */
+unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
+
+/* Macro to adjust thread priority for hardware multithreading */
+#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
+
+void __init noreturn start_xen(unsigned long r3, unsigned long r4,
+ unsigned long r5, unsigned long r6,
+ unsigned long r7)
+{
+ for ( ; ; )
+ /* Set current hardware thread to very low priority */
+ HMT_very_low();
+
+ unreachable();
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries
2023-07-06 19:04 [PATCH v3 0/3] Early serial on Power Shawn Anastasio
2023-07-06 19:04 ` [PATCH v3 1/3] xen/ppc: Set up a basic C environment Shawn Anastasio
@ 2023-07-06 19:04 ` Shawn Anastasio
2023-07-17 16:17 ` Jan Beulich
2023-07-06 19:04 ` [PATCH v3 3/3] automation: Add smoke test for ppc64le Shawn Anastasio
2 siblings, 1 reply; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-06 19:04 UTC (permalink / raw)
To: xen-devel; +Cc: Timothy Pearson, Andrew Cooper, Jan Beulich, Shawn Anastasio
On typical Power VMs (e.g. QEMU's -M pseries), a variety of services
including an early serial console are provided by Open Firmware.
Implement the required interfaces to call into Open Firmware and write
to the serial console.
Since Open Firmware runs in 32-bit Big Endian mode and Xen runs in
64-bit Little Endian mode, a thunk is required to save/restore
any potentially-clobbered registers as well as to perform the
required endianness switch. Thankfully, linux already has such
a routine, which was imported into ppc64/of-call.S.
Support for bare metal (PowerNV) will be implemented in a future
patch.
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
xen/arch/ppc/Kconfig.debug | 5 +
xen/arch/ppc/Makefile | 2 +
xen/arch/ppc/boot-of.c | 100 +++++++++++++++++
xen/arch/ppc/configs/ppc64_defconfig | 1 +
xen/arch/ppc/early_printk.c | 28 +++++
xen/arch/ppc/include/asm/asm-defns.h | 17 +++
xen/arch/ppc/include/asm/boot.h | 23 ++++
xen/arch/ppc/include/asm/byteorder.h | 12 ++
xen/arch/ppc/include/asm/config.h | 3 +
xen/arch/ppc/include/asm/early_printk.h | 15 +++
xen/arch/ppc/include/asm/msr.h | 51 +++++++++
xen/arch/ppc/include/asm/processor.h | 139 ++++++++++++++++++++++++
xen/arch/ppc/include/asm/types.h | 21 ++++
xen/arch/ppc/ppc64/Makefile | 1 +
xen/arch/ppc/ppc64/asm-offsets.c | 59 ++++++++++
xen/arch/ppc/ppc64/head.S | 9 ++
xen/arch/ppc/ppc64/of-call.S | 83 ++++++++++++++
xen/arch/ppc/setup.c | 19 +++-
18 files changed, 585 insertions(+), 3 deletions(-)
create mode 100644 xen/arch/ppc/boot-of.c
create mode 100644 xen/arch/ppc/early_printk.c
create mode 100644 xen/arch/ppc/include/asm/boot.h
create mode 100644 xen/arch/ppc/include/asm/byteorder.h
create mode 100644 xen/arch/ppc/include/asm/early_printk.h
create mode 100644 xen/arch/ppc/include/asm/msr.h
create mode 100644 xen/arch/ppc/include/asm/processor.h
create mode 100644 xen/arch/ppc/include/asm/types.h
create mode 100644 xen/arch/ppc/ppc64/of-call.S
diff --git a/xen/arch/ppc/Kconfig.debug b/xen/arch/ppc/Kconfig.debug
index e69de29bb2..608c9ff832 100644
--- a/xen/arch/ppc/Kconfig.debug
+++ b/xen/arch/ppc/Kconfig.debug
@@ -0,0 +1,5 @@
+config EARLY_PRINTK
+ bool "Enable early printk"
+ default DEBUG
+ help
+ Enables early printk debug messages
diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
index 530fba2121..e1d4590d67 100644
--- a/xen/arch/ppc/Makefile
+++ b/xen/arch/ppc/Makefile
@@ -1,5 +1,7 @@
obj-$(CONFIG_PPC64) += ppc64/
+obj-y += boot-of.init.o
+obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
obj-y += setup.o
$(TARGET): $(TARGET)-syms
diff --git a/xen/arch/ppc/boot-of.c b/xen/arch/ppc/boot-of.c
new file mode 100644
index 0000000000..6c69ca94ba
--- /dev/null
+++ b/xen/arch/ppc/boot-of.c
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright IBM Corp. 2005, 2006, 2007
+ * Copyright Raptor Engineering, LLC
+ *
+ * Authors: Jimi Xenidis <jimix@watson.ibm.com>
+ * Hollis Blanchard <hollisb@us.ibm.com>
+ * Shawn Anastasio <sanastasio@raptorengineering.com>
+ */
+
+#include <xen/init.h>
+#include <xen/stdarg.h>
+#include <xen/types.h>
+#include <asm/boot.h>
+#include <asm/byteorder.h>
+#include <asm/early_printk.h>
+
+#define ADDR(x) (uint32_t)(unsigned long)(x)
+
+/* OF entrypoint*/
+static unsigned long __initdata of_vec;
+
+/* OF device handles*/
+static int __initdata bof_chosen;
+static int __initdata of_out;
+
+static int __init of_call(const char *service, uint32_t nargs, uint32_t nrets,
+ int32_t rets[], ...)
+{
+ int rc;
+ va_list args;
+ int i;
+ struct of_service s = { 0 };
+
+ s.ofs_service = cpu_to_be32(ADDR(service));
+ s.ofs_nargs = cpu_to_be32(nargs);
+ s.ofs_nrets = cpu_to_be32(nrets);
+
+ /* copy all the params into the args array */
+ va_start(args, rets);
+
+ for ( i = 0; i < nargs; i++ )
+ s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t));
+
+ va_end(args);
+
+ rc = enter_of(&s, of_vec);
+
+ /* copy all return values to the output rets array */
+ for ( i = 0; i < nrets; i++ )
+ rets[i] = be32_to_cpu(s.ofs_args[i + nargs]);
+
+ return rc;
+}
+
+static int __init of_finddevice(const char *devspec)
+{
+ int rets[1] = { OF_FAILURE };
+
+ of_call("finddevice", 1, 1, rets, devspec);
+ return rets[0];
+}
+
+static int __init of_getprop(int ph, const char *name, void *buf, uint32_t buflen)
+{
+ int rets[1] = { OF_FAILURE };
+
+ of_call("getprop", 4, 1, rets, ph, ADDR(name), buf, buflen);
+ return rets[0];
+}
+
+int __init of_write(int ih, const char *addr, uint32_t len)
+{
+ int rets[1] = { OF_FAILURE };
+
+ of_call("write", 3, 1, rets, ih, addr, len);
+ return rets[0];
+}
+
+static void __init of_putchar(char c)
+{
+ if ( c == '\n' )
+ {
+ char buf = '\r';
+ of_write(of_out, &buf, 1);
+ }
+ of_write(of_out, &c, 1);
+}
+
+void __init boot_of_init(unsigned long vec)
+{
+ of_vec = vec;
+
+ /* Get a handle to the default console */
+ bof_chosen = of_finddevice("/chosen");
+ of_getprop(bof_chosen, "stdout", &of_out, sizeof(of_out));
+ of_out = be32_to_cpu(of_out);
+
+ early_printk_init(of_putchar);
+}
diff --git a/xen/arch/ppc/configs/ppc64_defconfig b/xen/arch/ppc/configs/ppc64_defconfig
index 8783eb3488..f7cc075e45 100644
--- a/xen/arch/ppc/configs/ppc64_defconfig
+++ b/xen/arch/ppc/configs/ppc64_defconfig
@@ -10,4 +10,5 @@
CONFIG_PPC64=y
CONFIG_DEBUG=y
CONFIG_DEBUG_INFO=y
+CONFIG_EARLY_PRINTK=y
CONFIG_EXPERT=y
diff --git a/xen/arch/ppc/early_printk.c b/xen/arch/ppc/early_printk.c
new file mode 100644
index 0000000000..c5014945ef
--- /dev/null
+++ b/xen/arch/ppc/early_printk.c
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#include <xen/init.h>
+#include <asm/boot.h>
+
+static void (*putchar_func)(char);
+
+void __init early_printk_init(void (*putchar)(char))
+{
+ putchar_func = putchar;
+}
+
+void __init early_puts(const char *s, size_t nr)
+{
+ if ( !putchar_func )
+ return;
+
+ while ( nr-- > 0 )
+ putchar_func(*s++);
+}
+
+void __init early_printk(const char *s)
+{
+ if ( !putchar_func )
+ return;
+
+ while ( *s )
+ putchar_func(*s++);
+}
diff --git a/xen/arch/ppc/include/asm/asm-defns.h b/xen/arch/ppc/include/asm/asm-defns.h
index 6ea35f6edb..9d7328f4a2 100644
--- a/xen/arch/ppc/include/asm/asm-defns.h
+++ b/xen/arch/ppc/include/asm/asm-defns.h
@@ -37,4 +37,21 @@
.long 0xa6037b7d; /* mtsrr1 r11 */ \
.long 0x2400004c /* rfid */
+
+/* Taken from Linux kernel source (arch/powerpc/boot/crt0.S) */
+.macro OP_REGS op, width, start, end, base, offset
+ .Lreg=\start
+ .rept (\end - \start + 1)
+ \op .Lreg,\offset+\width*.Lreg(\base)
+ .Lreg=.Lreg+1
+ .endr
+.endm
+
+#define SAVE_GPRS(start, end, base) OP_REGS std, 8, start, end, base, 0
+#define REST_GPRS(start, end, base) OP_REGS ld, 8, start, end, base, 0
+#define SAVE_GPR(n, base) SAVE_GPRS(n, n, base)
+#define REST_GPR(n, base) REST_GPRS(n, n, base)
+#define SAVE_NVGPRS(base) SAVE_GPRS(14, 31, base)
+#define REST_NVGPRS(base) REST_GPRS(14, 31, base)
+
#endif /* _ASM_PPC_ASM_DEFNS_H */
diff --git a/xen/arch/ppc/include/asm/boot.h b/xen/arch/ppc/include/asm/boot.h
new file mode 100644
index 0000000000..9b8a7c43c2
--- /dev/null
+++ b/xen/arch/ppc/include/asm/boot.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASM_PPC_BOOT_H
+#define _ASM_PPC_BOOT_H
+
+#include <xen/types.h>
+
+/* a collection of interfaces used during boot. */
+enum {
+ OF_FAILURE = -1,
+ OF_SUCCESS = 0,
+};
+
+struct of_service {
+ __be32 ofs_service;
+ __be32 ofs_nargs;
+ __be32 ofs_nrets;
+ __be32 ofs_args[10];
+};
+
+int enter_of(struct of_service *args, unsigned long entry);
+void boot_of_init(unsigned long vec);
+
+#endif /* _ASM_PPC_BOOT_H */
diff --git a/xen/arch/ppc/include/asm/byteorder.h b/xen/arch/ppc/include/asm/byteorder.h
new file mode 100644
index 0000000000..2b5f6b9f63
--- /dev/null
+++ b/xen/arch/ppc/include/asm/byteorder.h
@@ -0,0 +1,12 @@
+#ifndef _ASM_PPC_BYTEORDER_H
+#define _ASM_PPC_BYTEORDER_H
+
+#define __arch__swab16 __builtin_bswap16
+#define __arch__swab32 __builtin_bswap32
+#define __arch__swab64 __builtin_bswap64
+
+#define __BYTEORDER_HAS_U64__
+
+#include <xen/byteorder/little_endian.h>
+
+#endif /* _ASM_PPC_BYTEORDER_H */
diff --git a/xen/arch/ppc/include/asm/config.h b/xen/arch/ppc/include/asm/config.h
index 01ca5d0803..cb27d2781e 100644
--- a/xen/arch/ppc/include/asm/config.h
+++ b/xen/arch/ppc/include/asm/config.h
@@ -52,6 +52,9 @@
/* size of minimum stack frame; C code can write into the caller's stack */
#define STACK_FRAME_OVERHEAD 32
+/* ELFv2 ABI mandates 16 byte alignment */
+#define STACK_ALIGN 16
+
#endif /* __PPC_CONFIG_H__ */
/*
* Local variables:
diff --git a/xen/arch/ppc/include/asm/early_printk.h b/xen/arch/ppc/include/asm/early_printk.h
new file mode 100644
index 0000000000..d1d8b416f4
--- /dev/null
+++ b/xen/arch/ppc/include/asm/early_printk.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASM_PPC_EARLY_PRINTK_H
+#define _ASM_PPC_EARLY_PRINTK_H
+
+#include <xen/early_printk.h>
+
+#ifdef CONFIG_EARLY_PRINTK
+void early_printk_init(void (*putchar)(char));
+void early_printk(const char *s);
+#else
+static inline void early_printk_init(void (*putchar)(char)) {}
+static inline void early_printk(const char *s) {}
+#endif
+
+#endif /* _ASM_PPC_EARLY_PRINTK_H */
diff --git a/xen/arch/ppc/include/asm/msr.h b/xen/arch/ppc/include/asm/msr.h
new file mode 100644
index 0000000000..144511e5c3
--- /dev/null
+++ b/xen/arch/ppc/include/asm/msr.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) IBM Corp. 2005
+ * Copyright Raptor Engineering, LLC
+ *
+ * Authors: Jimi Xenidis <jimix@watson.ibm.com>
+ * Shawn Anastasio <sanastasio@raptorengineering.com>
+ */
+
+#ifndef _ASM_PPC_MSR_H
+#define _ASM_PPC_MSR_H
+
+#include <xen/const.h>
+
+/* Flags in MSR: */
+#define MSR_SF _AC(0x8000000000000000, ULL)
+#define MSR_TA _AC(0x4000000000000000, ULL)
+#define MSR_ISF _AC(0x2000000000000000, ULL)
+#define MSR_HV _AC(0x1000000000000000, ULL)
+#define MSR_VMX _AC(0x0000000002000000, ULL)
+#define MSR_MER _AC(0x0000000000200000, ULL)
+#define MSR_POW _AC(0x0000000000040000, ULL)
+#define MSR_ILE _AC(0x0000000000010000, ULL)
+#define MSR_EE _AC(0x0000000000008000, ULL)
+#define MSR_PR _AC(0x0000000000004000, ULL)
+#define MSR_FP _AC(0x0000000000002000, ULL)
+#define MSR_ME _AC(0x0000000000001000, ULL)
+#define MSR_FE0 _AC(0x0000000000000800, ULL)
+#define MSR_SE _AC(0x0000000000000400, ULL)
+#define MSR_BE _AC(0x0000000000000200, ULL)
+#define MSR_FE1 _AC(0x0000000000000100, ULL)
+#define MSR_IP _AC(0x0000000000000040, ULL)
+#define MSR_IR _AC(0x0000000000000020, ULL)
+#define MSR_DR _AC(0x0000000000000010, ULL)
+#define MSR_PMM _AC(0x0000000000000004, ULL)
+#define MSR_RI _AC(0x0000000000000002, ULL)
+#define MSR_LE _AC(0x0000000000000001, ULL)
+
+/* MSR bits set on the systemsim simulator */
+#define MSR_SIM _AC(0x0000000020000000, ULL)
+#define MSR_SYSTEMSIM _AC(0x0000000010000000, ULL)
+
+/* On a trap, srr1's copy of msr defines some bits as follows: */
+#define MSR_TRAP_FE _AC(0x0000000000100000, ULL) /* Floating Point Exception */
+#define MSR_TRAP_IOP _AC(0x0000000000080000, ULL) /* Illegal Instruction */
+#define MSR_TRAP_PRIV _AC(0x0000000000040000, ULL) /* Privileged Instruction */
+#define MSR_TRAP _AC(0x0000000000020000, ULL) /* Trap Instruction */
+#define MSR_TRAP_NEXT _AC(0x0000000000010000, ULL) /* PC is next instruction */
+#define MSR_TRAP_BITS (MSR_TRAP_FE|MSR_TRAP_IOP|MSR_TRAP_PRIV|MSR_TRAP)
+
+#endif /* _ASM_PPC_MSR_H */
diff --git a/xen/arch/ppc/include/asm/processor.h b/xen/arch/ppc/include/asm/processor.h
new file mode 100644
index 0000000000..2d2e582f08
--- /dev/null
+++ b/xen/arch/ppc/include/asm/processor.h
@@ -0,0 +1,139 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright IBM Corp. 2005, 2006, 2007
+ * Copyright Raptor Engineering, LLC
+ *
+ * Authors: Hollis Blanchard <hollisb@us.ibm.com>
+ * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
+ * Timothy Pearson <tpearson@raptorengineering.com>
+ * Shawn Anastasio <sanastasio@raptorengineering.com>
+ */
+
+#ifndef _ASM_PPC_PROCESSOR_H
+#define _ASM_PPC_PROCESSOR_H
+
+#include <xen/config.h>
+#include <xen/types.h>
+
+#define IOBMP_BYTES 8192
+#define IOBMP_INVALID_OFFSET 0x8000
+
+/* Processor Version Register (PVR) field extraction */
+
+#define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
+#define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* Revison field */
+
+#define __is_processor(pv) (PVR_VER(mfspr(SPRN_PVR)) == (pv))
+
+/*
+ * IBM has further subdivided the standard PowerPC 16-bit version and
+ * revision subfields of the PVR for the PowerPC 403s into the following:
+ */
+
+#define PVR_FAM(pvr) (((pvr) >> 20) & 0xFFF) /* Family field */
+#define PVR_MEM(pvr) (((pvr) >> 16) & 0xF) /* Member field */
+#define PVR_CORE(pvr) (((pvr) >> 12) & 0xF) /* Core field */
+#define PVR_CFG(pvr) (((pvr) >> 8) & 0xF) /* Configuration field */
+#define PVR_MAJ(pvr) (((pvr) >> 4) & 0xF) /* Major revision field */
+#define PVR_MIN(pvr) (((pvr) >> 0) & 0xF) /* Minor revision field */
+
+/* Processor Version Numbers */
+
+#define PVR_403GA 0x00200000
+#define PVR_403GB 0x00200100
+#define PVR_403GC 0x00200200
+#define PVR_403GCX 0x00201400
+#define PVR_405GP 0x40110000
+#define PVR_STB03XXX 0x40310000
+#define PVR_NP405H 0x41410000
+#define PVR_NP405L 0x41610000
+#define PVR_601 0x00010000
+#define PVR_602 0x00050000
+#define PVR_603 0x00030000
+#define PVR_603e 0x00060000
+#define PVR_603ev 0x00070000
+#define PVR_603r 0x00071000
+#define PVR_604 0x00040000
+#define PVR_604e 0x00090000
+#define PVR_604r 0x000A0000
+#define PVR_620 0x00140000
+#define PVR_740 0x00080000
+#define PVR_750 PVR_740
+#define PVR_740P 0x10080000
+#define PVR_750P PVR_740P
+#define PVR_7400 0x000C0000
+#define PVR_7410 0x800C0000
+#define PVR_7450 0x80000000
+#define PVR_8540 0x80200000
+#define PVR_8560 0x80200000
+/*
+ * For the 8xx processors, all of them report the same PVR family for
+ * the PowerPC core. The various versions of these processors must be
+ * differentiated by the version number in the Communication Processor
+ * Module (CPM).
+ */
+#define PVR_821 0x00500000
+#define PVR_823 PVR_821
+#define PVR_850 PVR_821
+#define PVR_860 PVR_821
+#define PVR_8240 0x00810100
+#define PVR_8245 0x80811014
+#define PVR_8260 PVR_8240
+
+/* 64-bit processors */
+#define PVR_NORTHSTAR 0x0033
+#define PVR_PULSAR 0x0034
+#define PVR_POWER4 0x0035
+#define PVR_ICESTAR 0x0036
+#define PVR_SSTAR 0x0037
+#define PVR_POWER4p 0x0038
+#define PVR_970 0x0039
+#define PVR_POWER5 0x003A
+#define PVR_POWER5p 0x003B
+#define PVR_970FX 0x003C
+#define PVR_POWER6 0x003E
+#define PVR_POWER7 0x003F
+#define PVR_630 0x0040
+#define PVR_630p 0x0041
+#define PVR_970MP 0x0044
+#define PVR_970GX 0x0045
+#define PVR_POWER7p 0x004A
+#define PVR_POWER8E 0x004B
+#define PVR_POWER8NVL 0x004C
+#define PVR_POWER8 0x004D
+#define PVR_POWER9 0x004E
+#define PVR_POWER10 0x0080
+#define PVR_BE 0x0070
+#define PVR_PA6T 0x0090
+
+/* Macro to adjust thread priority for hardware multithreading */
+#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
+
+#ifndef __ASSEMBLY__
+
+#include <xen/types.h>
+
+/* User-accessible registers: nost of these need to be saved/restored
+ * for every nested Xen invocation. */
+struct cpu_user_regs
+{
+ uint64_t gprs[32];
+ uint64_t lr;
+ uint64_t ctr;
+ uint64_t srr0;
+ uint64_t srr1;
+ uint64_t pc;
+ uint64_t msr;
+ uint64_t fpscr;
+ uint64_t xer;
+ uint64_t hid4; /* debug only */
+ uint64_t dar; /* debug only */
+ uint32_t dsisr; /* debug only */
+ uint32_t cr;
+ uint32_t __pad; /* good spot for another 32bit reg */
+ uint32_t entry_vector;
+};
+
+#endif
+
+#endif /* _ASM_PPC_PROCESSOR_H */
diff --git a/xen/arch/ppc/include/asm/types.h b/xen/arch/ppc/include/asm/types.h
new file mode 100644
index 0000000000..cee08e111a
--- /dev/null
+++ b/xen/arch/ppc/include/asm/types.h
@@ -0,0 +1,21 @@
+/* from xen/arch/x86/include/asm/types.h */
+
+#ifndef _ASM_PPC_TYPES_H
+#define _ASM_PPC_TYPES_H
+
+typedef signed char s8;
+typedef unsigned char u8;
+
+typedef signed short s16;
+typedef unsigned short u16;
+
+typedef signed int s32;
+typedef unsigned int u32;
+
+typedef signed long s64;
+typedef unsigned long u64;
+typedef unsigned long paddr_t;
+#define INVALID_PADDR (~0UL)
+#define PRIpaddr "016lx"
+
+#endif /* _ASM_PPC_TYPES_H */
diff --git a/xen/arch/ppc/ppc64/Makefile b/xen/arch/ppc/ppc64/Makefile
index 3340058c08..f4956daaa9 100644
--- a/xen/arch/ppc/ppc64/Makefile
+++ b/xen/arch/ppc/ppc64/Makefile
@@ -1 +1,2 @@
obj-y += head.o
+obj-y += of-call.o
diff --git a/xen/arch/ppc/ppc64/asm-offsets.c b/xen/arch/ppc/ppc64/asm-offsets.c
index e69de29bb2..7fc29d5db1 100644
--- a/xen/arch/ppc/ppc64/asm-offsets.c
+++ b/xen/arch/ppc/ppc64/asm-offsets.c
@@ -0,0 +1,59 @@
+/*
+ * Generate definitions needed by assembly language modules.
+ * This code generates raw asm output which is post-processed
+ * to extract and format the required data.
+ */
+
+#include <asm/processor.h>
+
+#define DEFINE(_sym, _val) \
+ asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \
+ : : "i" (_val) )
+#define BLANK() \
+ asm volatile ( "\n.ascii\"==><==\"" : : )
+#define OFFSET(_sym, _str, _mem) \
+ DEFINE(_sym, offsetof(_str, _mem));
+
+/* base-2 logarithm */
+#define __L2(_x) (((_x) & 0x00000002) ? 1 : 0)
+#define __L4(_x) (((_x) & 0x0000000c) ? ( 2 + __L2( (_x)>> 2)) : __L2( _x))
+#define __L8(_x) (((_x) & 0x000000f0) ? ( 4 + __L4( (_x)>> 4)) : __L4( _x))
+#define __L16(_x) (((_x) & 0x0000ff00) ? ( 8 + __L8( (_x)>> 8)) : __L8( _x))
+#define LOG_2(_x) (((_x) & 0xffff0000) ? (16 + __L16((_x)>>16)) : __L16(_x))
+
+void __dummy__(void)
+{
+ DEFINE(GPR_WIDTH, sizeof(unsigned long));
+ DEFINE(FPR_WIDTH, sizeof(double));
+
+ OFFSET(UREGS_gprs, struct cpu_user_regs, gprs);
+ OFFSET(UREGS_r0, struct cpu_user_regs, gprs[0]);
+ OFFSET(UREGS_r1, struct cpu_user_regs, gprs[1]);
+ OFFSET(UREGS_r13, struct cpu_user_regs, gprs[13]);
+ OFFSET(UREGS_srr0, struct cpu_user_regs, srr0);
+ OFFSET(UREGS_srr1, struct cpu_user_regs, srr1);
+ OFFSET(UREGS_pc, struct cpu_user_regs, pc);
+ OFFSET(UREGS_msr, struct cpu_user_regs, msr);
+ OFFSET(UREGS_lr, struct cpu_user_regs, lr);
+ OFFSET(UREGS_ctr, struct cpu_user_regs, ctr);
+ OFFSET(UREGS_xer, struct cpu_user_regs, xer);
+ OFFSET(UREGS_hid4, struct cpu_user_regs, hid4);
+ OFFSET(UREGS_dar, struct cpu_user_regs, dar);
+ OFFSET(UREGS_dsisr, struct cpu_user_regs, dsisr);
+ OFFSET(UREGS_cr, struct cpu_user_regs, cr);
+ OFFSET(UREGS_fpscr, struct cpu_user_regs, fpscr);
+ DEFINE(UREGS_sizeof, sizeof(struct cpu_user_regs));
+}
+
+/* TODO: Replace with BUILD_BUG_ON + IS_ALIGNED once we can use <xen/lib.h> */
+_Static_assert(sizeof(struct cpu_user_regs) % STACK_ALIGN == 0,
+ "struct cpu_user_regs not stack aligned!");
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/ppc/ppc64/head.S b/xen/arch/ppc/ppc64/head.S
index 7d848611cc..8497d8f37e 100644
--- a/xen/arch/ppc/ppc64/head.S
+++ b/xen/arch/ppc/ppc64/head.S
@@ -18,6 +18,15 @@ ENTRY(start)
li %r11, 0
stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
+ /* clear .bss */
+ LOAD_IMM32(%r14, __bss_start)
+ LOAD_IMM32(%r15, __bss_end)
+1:
+ std %r11, 0(%r14)
+ addi %r14, %r14, 8
+ cmpld %r14, %r15
+ blt 1b
+
/* call the C entrypoint */
bl start_xen
diff --git a/xen/arch/ppc/ppc64/of-call.S b/xen/arch/ppc/ppc64/of-call.S
new file mode 100644
index 0000000000..5f13e3b327
--- /dev/null
+++ b/xen/arch/ppc/ppc64/of-call.S
@@ -0,0 +1,83 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Adapted from Linux's arch/powerpc/kernel/entry_64.S, with the
+ * following copyright notice:
+ *
+ * PowerPC version
+ * Copyright (C) 1995-1996 Gary Thomas (gdt@linuxppc.org)
+ * Rewritten by Cort Dougan (cort@cs.nmt.edu) for PReP
+ * Copyright (C) 1996 Cort Dougan <cort@cs.nmt.edu>
+ * Adapted for Power Macintosh by Paul Mackerras.
+ * Low-level exception handlers and MMU support
+ * rewritten by Paul Mackerras.
+ * Copyright (C) 1996 Paul Mackerras.
+ * MPC8xx modifications Copyright (C) 1997 Dan Malek (dmalek@jlc.net).
+ */
+
+#include <asm/asm-offsets.h>
+#include <asm/asm-defns.h>
+#include <asm/msr.h>
+
+/* size of minimum stack frame that can hold an entire cpu_user_regs struct */
+#define STACK_SWITCH_FRAME_SIZE UREGS_sizeof
+
+ .section .init.text, "ax", @progbits
+
+ENTRY(enter_of)
+ mflr %r0
+ std %r0, 16(%r1)
+ stdu %r1,-STACK_SWITCH_FRAME_SIZE(%r1) /* Save SP and create stack space */
+
+ /*
+ * Because PROM is running in 32b mode, it clobbers the high order half
+ * of all registers that it saves. We therefore save those registers
+ * PROM might touch to the stack. (%r0, %r3-%r13 are caller saved)
+ */
+ SAVE_GPR(2, %r1)
+ SAVE_GPR(13, %r1)
+ SAVE_NVGPRS(%r1)
+ mfcr %r10
+ mfmsr %r11
+ std %r10, UREGS_cr(%r1)
+ std %r11, UREGS_msr(%r1)
+
+ /* Put PROM address in SRR0 */
+ mtsrr0 %r4
+
+ /* Setup our trampoline return addr in LR */
+ bcl 20, 31,.+4
+0: mflr %r4
+ addi %r4, %r4,(1f - 0b)
+ mtlr %r4
+
+ /* Prepare a 32-bit mode big endian MSR */
+ LOAD_IMM64(%r12, MSR_SF | MSR_LE)
+ andc %r11, %r11, %r12
+ mtsrr1 %r11
+ rfid
+
+1: /* Return from OF */
+ FIXUP_ENDIAN
+
+ /* Just make sure that %r1 top 32 bits didn't get corrupt by OF */
+ rldicl %r1, %r1, 0, 32
+
+ /* Restore the MSR (back to 64 bits) */
+ ld %r0, UREGS_msr(%r1)
+ mtmsrd %r0
+ isync
+
+ /* Restore other registers */
+ REST_GPR(2, %r1)
+ REST_GPR(13, %r1)
+ REST_NVGPRS(%r1)
+ ld %r4, UREGS_cr(%r1)
+ mtcr %r4
+
+ addi %r1, %r1, STACK_SWITCH_FRAME_SIZE
+ ld %r0, 16(%r1)
+ mtlr %r0
+ blr
+
+ .size enter_of, . - enter_of
+ .type enter_of, %function
diff --git a/xen/arch/ppc/setup.c b/xen/arch/ppc/setup.c
index 73106474b2..7c623a49f5 100644
--- a/xen/arch/ppc/setup.c
+++ b/xen/arch/ppc/setup.c
@@ -1,16 +1,29 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#include <xen/init.h>
+#include <asm/boot.h>
+#include <asm/early_printk.h>
+#include <asm/processor.h>
/* Xen stack for bringing up the first CPU. */
unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
-/* Macro to adjust thread priority for hardware multithreading */
-#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
-
void __init noreturn start_xen(unsigned long r3, unsigned long r4,
unsigned long r5, unsigned long r6,
unsigned long r7)
{
+ if ( r5 )
+ {
+ /* OpenFirmware boot protocol */
+ boot_of_init(r5);
+ }
+ else
+ {
+ /* kexec boot: Unimplemented */
+ __builtin_trap();
+ }
+
+ early_printk("Hello, ppc64le!\n");
+
for ( ; ; )
/* Set current hardware thread to very low priority */
HMT_very_low();
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/3] automation: Add smoke test for ppc64le
2023-07-06 19:04 [PATCH v3 0/3] Early serial on Power Shawn Anastasio
2023-07-06 19:04 ` [PATCH v3 1/3] xen/ppc: Set up a basic C environment Shawn Anastasio
2023-07-06 19:04 ` [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries Shawn Anastasio
@ 2023-07-06 19:04 ` Shawn Anastasio
2 siblings, 0 replies; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-06 19:04 UTC (permalink / raw)
To: xen-devel
Cc: Timothy Pearson, Andrew Cooper, Jan Beulich, Shawn Anastasio,
Stefano Stabellini
Add an initial smoke test that boots xen on a ppc64/pseries machine and
checks for a magic string. Based on the riscv smoke test.
Eventually the powernv9 (POWER9 bare metal) machine type will want to be
tested as well, but for now we only boot on pseries.
Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
automation/gitlab-ci/test.yaml | 20 ++++++++++++++++++
automation/scripts/qemu-smoke-ppc64le.sh | 27 ++++++++++++++++++++++++
2 files changed, 47 insertions(+)
create mode 100755 automation/scripts/qemu-smoke-ppc64le.sh
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index d5cb238b0a..45e8ddb7a3 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -71,6 +71,19 @@
tags:
- x86_64
+.qemu-ppc64le:
+ extends: .test-jobs-common
+ variables:
+ CONTAINER: debian:bullseye-ppc64le
+ LOGFILE: qemu-smoke-ppc64le.log
+ artifacts:
+ paths:
+ - smoke.serial
+ - '*.log'
+ when: always
+ tags:
+ - x86_64
+
.xilinx-arm64:
extends: .test-jobs-common
variables:
@@ -444,3 +457,10 @@ qemu-smoke-riscv64-gcc:
- ./automation/scripts/qemu-smoke-riscv64.sh 2>&1 | tee ${LOGFILE}
needs:
- archlinux-current-gcc-riscv64-debug
+
+qemu-smoke-ppc64le-pseries-gcc:
+ extends: .qemu-ppc64le
+ script:
+ - ./automation/scripts/qemu-smoke-ppc64le.sh pseries-5.2 2>&1 | tee ${LOGFILE}
+ needs:
+ - debian-bullseye-gcc-ppc64le-debug
diff --git a/automation/scripts/qemu-smoke-ppc64le.sh b/automation/scripts/qemu-smoke-ppc64le.sh
new file mode 100755
index 0000000000..eb55221221
--- /dev/null
+++ b/automation/scripts/qemu-smoke-ppc64le.sh
@@ -0,0 +1,27 @@
+#!/bin/bash
+
+set -ex
+
+# machine type from first arg passed directly to qemu -M
+machine=$1
+
+# Run the test
+rm -f smoke.serial
+set +e
+
+touch smoke.serial
+
+timeout -k 1 20 \
+qemu-system-ppc64 \
+ -M $machine \
+ -m 2g \
+ -smp 1 \
+ -vga none \
+ -monitor none \
+ -nographic \
+ -serial file:smoke.serial \
+ -kernel binaries/xen
+
+set -e
+(grep -q "Hello, ppc64le!" smoke.serial) || exit 1
+exit 0
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/3] xen/ppc: Set up a basic C environment
2023-07-06 19:04 ` [PATCH v3 1/3] xen/ppc: Set up a basic C environment Shawn Anastasio
@ 2023-07-17 15:38 ` Jan Beulich
2023-07-17 16:00 ` Shawn Anastasio
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-07-17 15:38 UTC (permalink / raw)
To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 06.07.2023 21:04, Shawn Anastasio wrote:
> --- a/xen/arch/ppc/include/asm/config.h
> +++ b/xen/arch/ppc/include/asm/config.h
> @@ -43,7 +43,7 @@
>
> #define SMP_CACHE_BYTES (1 << 6)
>
> -#define STACK_ORDER 2
> +#define STACK_ORDER 0
> #define STACK_SIZE (PAGE_SIZE << STACK_ORDER)
In which way is this related to the change at hand? Aren't you going to
need to undo this rather sooner than later?
> --- a/xen/arch/ppc/ppc64/head.S
> +++ b/xen/arch/ppc/ppc64/head.S
> @@ -1,30 +1,30 @@
> /* SPDX-License-Identifier: GPL-2.0-or-later */
>
> +#include <asm/asm-defns.h>
> +
> .section .text.header, "ax", %progbits
>
> ENTRY(start)
> /*
> - * Depending on how we were booted, the CPU could be running in either
> - * Little Endian or Big Endian mode. The following trampoline from Linux
> - * cleverly uses an instruction that encodes to a NOP if the CPU's
> - * endianness matches the assumption of the assembler (LE, in our case)
> - * or a branch to code that performs the endian switch in the other case.
> + * NOTE: argument registers (r3-r9) must be preserved until the C entrypoint
> */
> - tdi 0, 0, 0x48 /* Reverse endian of b . + 8 */
> - b . + 44 /* Skip trampoline if endian is good */
> - .long 0xa600607d /* mfmsr r11 */
> - .long 0x01006b69 /* xori r11,r11,1 */
> - .long 0x00004039 /* li r10,0 */
> - .long 0x6401417d /* mtmsrd r10,1 */
> - .long 0x05009f42 /* bcl 20,31,$+4 */
> - .long 0xa602487d /* mflr r10 */
> - .long 0x14004a39 /* addi r10,r10,20 */
> - .long 0xa6035a7d /* mtsrr0 r10 */
> - .long 0xa6037b7d /* mtsrr1 r11 */
> - .long 0x2400004c /* rfid */
> -
> - /* Now that the endianness is confirmed, continue */
> -1: b 1b
> + FIXUP_ENDIAN
> +
> + /* set up the TOC pointer */
> + LOAD_IMM32(%r2, .TOC.)
> +
> + /* set up the initial stack */
> + LOAD_IMM32(%r1, cpu0_boot_stack)
Wouldn't this (and perhaps also .TOC.) better be calculated in a
PC-relative manner? Or is the plan to have Xen linked to an address
below 4Gb?
> + li %r11, 0
> + stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
> +
> + /* call the C entrypoint */
> + bl start_xen
> +
> + /* should never return */
> + trap
>
> .size start, . - start
> .type start, %function
> +
> + .section .init.data, "aw", @progbits
What's this good for when no data follows?
> --- /dev/null
> +++ b/xen/arch/ppc/setup.c
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#include <xen/init.h>
> +
> +/* Xen stack for bringing up the first CPU. */
> +unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
This yields the entire array as zero-initialized. At which point I
don't see a need for the store in head.S.
> +/* Macro to adjust thread priority for hardware multithreading */
> +#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
Nit: Style. Wants to be
#define HMT_very_low() asm volatile ( "or %r31, %r31, %r31" )
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/3] xen/ppc: Set up a basic C environment
2023-07-17 15:38 ` Jan Beulich
@ 2023-07-17 16:00 ` Shawn Anastasio
2023-07-17 16:24 ` Jan Beulich
0 siblings, 1 reply; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-17 16:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 7/17/23 10:38 AM, Jan Beulich wrote:
> On 06.07.2023 21:04, Shawn Anastasio wrote:
>> --- a/xen/arch/ppc/include/asm/config.h
>> +++ b/xen/arch/ppc/include/asm/config.h
>> @@ -43,7 +43,7 @@
>>
>> #define SMP_CACHE_BYTES (1 << 6)
>>
>> -#define STACK_ORDER 2
>> +#define STACK_ORDER 0
>> #define STACK_SIZE (PAGE_SIZE << STACK_ORDER)
>
> In which way is this related to the change at hand? Aren't you going to
> need to undo this rather sooner than later?
I noticed the stack order being too large when I moved the stack
declaration to .c per Andrew's recommendation for v2. Since we're using
64k pages, I don't see why the stack would need to be this big. A quick
look at ARM shows they have a stack order of 3, which would yield a
stack size of just 32k.
>> --- a/xen/arch/ppc/ppc64/head.S
>> +++ b/xen/arch/ppc/ppc64/head.S
>> @@ -1,30 +1,30 @@
>> /* SPDX-License-Identifier: GPL-2.0-or-later */
>>
>> +#include <asm/asm-defns.h>
>> +
>> .section .text.header, "ax", %progbits
>>
>> ENTRY(start)
>> /*
>> - * Depending on how we were booted, the CPU could be running in either
>> - * Little Endian or Big Endian mode. The following trampoline from Linux
>> - * cleverly uses an instruction that encodes to a NOP if the CPU's
>> - * endianness matches the assumption of the assembler (LE, in our case)
>> - * or a branch to code that performs the endian switch in the other case.
>> + * NOTE: argument registers (r3-r9) must be preserved until the C entrypoint
>> */
>> - tdi 0, 0, 0x48 /* Reverse endian of b . + 8 */
>> - b . + 44 /* Skip trampoline if endian is good */
>> - .long 0xa600607d /* mfmsr r11 */
>> - .long 0x01006b69 /* xori r11,r11,1 */
>> - .long 0x00004039 /* li r10,0 */
>> - .long 0x6401417d /* mtmsrd r10,1 */
>> - .long 0x05009f42 /* bcl 20,31,$+4 */
>> - .long 0xa602487d /* mflr r10 */
>> - .long 0x14004a39 /* addi r10,r10,20 */
>> - .long 0xa6035a7d /* mtsrr0 r10 */
>> - .long 0xa6037b7d /* mtsrr1 r11 */
>> - .long 0x2400004c /* rfid */
>> -
>> - /* Now that the endianness is confirmed, continue */
>> -1: b 1b
>> + FIXUP_ENDIAN
>> +
>> + /* set up the TOC pointer */
>> + LOAD_IMM32(%r2, .TOC.)
>> +
>> + /* set up the initial stack */
>> + LOAD_IMM32(%r1, cpu0_boot_stack)
>
> Wouldn't this (and perhaps also .TOC.) better be calculated in a
> PC-relative manner? Or is the plan to have Xen linked to an address
> below 4Gb?
As mentioned previously, I am planning to enable the PIC code model in
my next series in order to accommodate booting on the PowerNV firmware
type which has a different load address. That patch will change the
initial TOC load to a simulated PC-relative one (pre-POWER10 doesn't
have proper PC-relative loads/stores) and the rest to TOC-relative.
>> + li %r11, 0
>> + stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
>> +
>> + /* call the C entrypoint */
>> + bl start_xen
>> +
>> + /* should never return */
>> + trap
>>
>> .size start, . - start
>> .type start, %function
>> +
>> + .section .init.data, "aw", @progbits
>
> What's this good for when no data follows?
Good catch. With the stack no longer declared in head.S this is
unnecessary. Will remove in v3.
>> --- /dev/null
>> +++ b/xen/arch/ppc/setup.c
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#include <xen/init.h>
>> +
>> +/* Xen stack for bringing up the first CPU. */
>> +unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
>
> This yields the entire array as zero-initialized. At which point I
> don't see a need for the store in head.S.
Okay, fair enough. Given that the array is zero-initialized the stdu
could be replaced with an `addi %r1, %r1, -STACK_FRAME_OVERHEAD`, and
the load of zero to %r11 could be deferred to the second patch in this
series where it's used in the .bss clearing loop.
That said I don't really see the harm with keeping the standard
idiomatic `stdu` for the initial stack frame setup. Other than
performance, which isn't a concern here in early setup code.
>> +/* Macro to adjust thread priority for hardware multithreading */
>> +#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
>
> Nit: Style. Wants to be
>
> #define HMT_very_low() asm volatile ( "or %r31, %r31, %r31" )
Will fix in v3.
> Jan
Thanks,
Shawn
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries
2023-07-06 19:04 ` [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries Shawn Anastasio
@ 2023-07-17 16:17 ` Jan Beulich
2023-07-17 18:32 ` Shawn Anastasio
0 siblings, 1 reply; 12+ messages in thread
From: Jan Beulich @ 2023-07-17 16:17 UTC (permalink / raw)
To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 06.07.2023 21:04, Shawn Anastasio wrote:
> --- a/xen/arch/ppc/Makefile
> +++ b/xen/arch/ppc/Makefile
> @@ -1,5 +1,7 @@
> obj-$(CONFIG_PPC64) += ppc64/
>
> +obj-y += boot-of.init.o
> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
While in this case I could accept the name as is to be the same as Arm's,
as said before it would be nice if new files could avoid underscores (and
use dashes instead) unless strictly required by something.
Also, with boot-of.c using early_printk.c, shouldn't the latter also
build into early_printk.init.o?
> --- /dev/null
> +++ b/xen/arch/ppc/boot-of.c
> @@ -0,0 +1,100 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright IBM Corp. 2005, 2006, 2007
Judging from the years the file was taken from somewhere. Is the license
there permitting "2.0 or later"? (For files [partly] taken from somewhere,
a clarification as to the originals' licenses would be helpful to have in
the description, or maybe in the post-commit-message area.)
> + * Copyright Raptor Engineering, LLC
> + *
> + * Authors: Jimi Xenidis <jimix@watson.ibm.com>
> + * Hollis Blanchard <hollisb@us.ibm.com>
> + * Shawn Anastasio <sanastasio@raptorengineering.com>
> + */
> +
> +#include <xen/init.h>
> +#include <xen/stdarg.h>
> +#include <xen/types.h>
> +#include <asm/boot.h>
> +#include <asm/byteorder.h>
> +#include <asm/early_printk.h>
> +
> +#define ADDR(x) (uint32_t)(unsigned long)(x)
Besides needing to be wrapped in parentheses, this likely needs a comment:
It is hard to see how in 64-bit code uint32_t can suffice as an address.
Unless (see the question on patch 1) you intend to link/run Xen at an
address firmly below 4Gb, now and going forward.
> +/* OF entrypoint*/
> +static unsigned long __initdata of_vec;
> +
> +/* OF device handles*/
> +static int __initdata bof_chosen;
> +static int __initdata of_out;
> +
> +static int __init of_call(const char *service, uint32_t nargs, uint32_t nrets,
> + int32_t rets[], ...)
Nit: Indentation.
> +{
> + int rc;
> + va_list args;
> + int i;
unsigned int?
> + struct of_service s = { 0 };
> +
> + s.ofs_service = cpu_to_be32(ADDR(service));
> + s.ofs_nargs = cpu_to_be32(nargs);
> + s.ofs_nrets = cpu_to_be32(nrets);
> +
> + /* copy all the params into the args array */
Nit: Style (comments want to start with a capital letter). Again below.
> + va_start(args, rets);
> +
> + for ( i = 0; i < nargs; i++ )
> + s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t));
> +
> + va_end(args);
> +
> + rc = enter_of(&s, of_vec);
> +
> + /* copy all return values to the output rets array */
> + for ( i = 0; i < nrets; i++ )
> + rets[i] = be32_to_cpu(s.ofs_args[i + nargs]);
> +
> + return rc;
> +}
> +
> +static int __init of_finddevice(const char *devspec)
> +{
> + int rets[1] = { OF_FAILURE };
Hmm, of_call() uses int32_t. Again below several times.
> + of_call("finddevice", 1, 1, rets, devspec);
This could do with using ARRAY_SIZE(rets). Same again below.
Also don't you need to wrap devspec in ADDR()? Similar issues
then again below.
> + return rets[0];
> +}
> +
> +static int __init of_getprop(int ph, const char *name, void *buf, uint32_t buflen)
> +{
> + int rets[1] = { OF_FAILURE };
> +
> + of_call("getprop", 4, 1, rets, ph, ADDR(name), buf, buflen);
> + return rets[0];
> +}
> +
> +int __init of_write(int ih, const char *addr, uint32_t len)
> +{
> + int rets[1] = { OF_FAILURE };
> +
> + of_call("write", 3, 1, rets, ih, addr, len);
> + return rets[0];
> +}
> +
> +static void __init of_putchar(char c)
> +{
> + if ( c == '\n' )
> + {
> + char buf = '\r';
> + of_write(of_out, &buf, 1);
> + }
> + of_write(of_out, &c, 1);
> +}
> +
> +void __init boot_of_init(unsigned long vec)
> +{
> + of_vec = vec;
> +
> + /* Get a handle to the default console */
> + bof_chosen = of_finddevice("/chosen");
> + of_getprop(bof_chosen, "stdout", &of_out, sizeof(of_out));
> + of_out = be32_to_cpu(of_out);
> +
> + early_printk_init(of_putchar);
> +}
Considering that bof_chosen is used just here, why does it need to be
a file-scope variable?
> --- /dev/null
> +++ b/xen/arch/ppc/early_printk.c
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#include <xen/init.h>
> +#include <asm/boot.h>
> +
> +static void (*putchar_func)(char);
__initdata? (Connected to the question of building into .init.o.)
> --- /dev/null
> +++ b/xen/arch/ppc/include/asm/processor.h
> @@ -0,0 +1,139 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright IBM Corp. 2005, 2006, 2007
> + * Copyright Raptor Engineering, LLC
> + *
> + * Authors: Hollis Blanchard <hollisb@us.ibm.com>
> + * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
> + * Timothy Pearson <tpearson@raptorengineering.com>
> + * Shawn Anastasio <sanastasio@raptorengineering.com>
> + */
> +
> +#ifndef _ASM_PPC_PROCESSOR_H
> +#define _ASM_PPC_PROCESSOR_H
> +
> +#include <xen/config.h>
No need to - this is force-included by the Makefile machinery.
> +#include <xen/types.h>
Note this. Why ...
> +#define IOBMP_BYTES 8192
> +#define IOBMP_INVALID_OFFSET 0x8000
> +
> +/* Processor Version Register (PVR) field extraction */
> +
> +#define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
> +#define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* Revison field */
> +
> +#define __is_processor(pv) (PVR_VER(mfspr(SPRN_PVR)) == (pv))
> +
> +/*
> + * IBM has further subdivided the standard PowerPC 16-bit version and
> + * revision subfields of the PVR for the PowerPC 403s into the following:
> + */
> +
> +#define PVR_FAM(pvr) (((pvr) >> 20) & 0xFFF) /* Family field */
> +#define PVR_MEM(pvr) (((pvr) >> 16) & 0xF) /* Member field */
> +#define PVR_CORE(pvr) (((pvr) >> 12) & 0xF) /* Core field */
> +#define PVR_CFG(pvr) (((pvr) >> 8) & 0xF) /* Configuration field */
> +#define PVR_MAJ(pvr) (((pvr) >> 4) & 0xF) /* Major revision field */
> +#define PVR_MIN(pvr) (((pvr) >> 0) & 0xF) /* Minor revision field */
> +
> +/* Processor Version Numbers */
> +
> +#define PVR_403GA 0x00200000
> +#define PVR_403GB 0x00200100
> +#define PVR_403GC 0x00200200
> +#define PVR_403GCX 0x00201400
> +#define PVR_405GP 0x40110000
> +#define PVR_STB03XXX 0x40310000
> +#define PVR_NP405H 0x41410000
> +#define PVR_NP405L 0x41610000
> +#define PVR_601 0x00010000
> +#define PVR_602 0x00050000
> +#define PVR_603 0x00030000
> +#define PVR_603e 0x00060000
> +#define PVR_603ev 0x00070000
> +#define PVR_603r 0x00071000
> +#define PVR_604 0x00040000
> +#define PVR_604e 0x00090000
> +#define PVR_604r 0x000A0000
> +#define PVR_620 0x00140000
> +#define PVR_740 0x00080000
> +#define PVR_750 PVR_740
> +#define PVR_740P 0x10080000
> +#define PVR_750P PVR_740P
> +#define PVR_7400 0x000C0000
> +#define PVR_7410 0x800C0000
> +#define PVR_7450 0x80000000
> +#define PVR_8540 0x80200000
> +#define PVR_8560 0x80200000
> +/*
> + * For the 8xx processors, all of them report the same PVR family for
> + * the PowerPC core. The various versions of these processors must be
> + * differentiated by the version number in the Communication Processor
> + * Module (CPM).
> + */
> +#define PVR_821 0x00500000
> +#define PVR_823 PVR_821
> +#define PVR_850 PVR_821
> +#define PVR_860 PVR_821
> +#define PVR_8240 0x00810100
> +#define PVR_8245 0x80811014
> +#define PVR_8260 PVR_8240
> +
> +/* 64-bit processors */
> +#define PVR_NORTHSTAR 0x0033
> +#define PVR_PULSAR 0x0034
> +#define PVR_POWER4 0x0035
> +#define PVR_ICESTAR 0x0036
> +#define PVR_SSTAR 0x0037
> +#define PVR_POWER4p 0x0038
> +#define PVR_970 0x0039
> +#define PVR_POWER5 0x003A
> +#define PVR_POWER5p 0x003B
> +#define PVR_970FX 0x003C
> +#define PVR_POWER6 0x003E
> +#define PVR_POWER7 0x003F
> +#define PVR_630 0x0040
> +#define PVR_630p 0x0041
> +#define PVR_970MP 0x0044
> +#define PVR_970GX 0x0045
> +#define PVR_POWER7p 0x004A
> +#define PVR_POWER8E 0x004B
> +#define PVR_POWER8NVL 0x004C
> +#define PVR_POWER8 0x004D
> +#define PVR_POWER9 0x004E
> +#define PVR_POWER10 0x0080
> +#define PVR_BE 0x0070
> +#define PVR_PA6T 0x0090
> +
> +/* Macro to adjust thread priority for hardware multithreading */
> +#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <xen/types.h>
... another time here? Or is the first one perhaps too early?
> +/* User-accessible registers: nost of these need to be saved/restored
> + * for every nested Xen invocation. */
Nit: Comment style. Multi-line comments want to be
/*
* User-accessible registers: most of these need to be saved/restored
* for every nested Xen invocation.
*/
(also note the s/nost/most/ I did)
> --- a/xen/arch/ppc/ppc64/head.S
> +++ b/xen/arch/ppc/ppc64/head.S
> @@ -18,6 +18,15 @@ ENTRY(start)
> li %r11, 0
> stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
>
> + /* clear .bss */
> + LOAD_IMM32(%r14, __bss_start)
> + LOAD_IMM32(%r15, __bss_end)
Question regarding addressing again.
> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/of-call.S
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Adapted from Linux's arch/powerpc/kernel/entry_64.S, with the
> + * following copyright notice:
> + *
> + * PowerPC version
> + * Copyright (C) 1995-1996 Gary Thomas (gdt@linuxppc.org)
> + * Rewritten by Cort Dougan (cort@cs.nmt.edu) for PReP
> + * Copyright (C) 1996 Cort Dougan <cort@cs.nmt.edu>
> + * Adapted for Power Macintosh by Paul Mackerras.
> + * Low-level exception handlers and MMU support
> + * rewritten by Paul Mackerras.
> + * Copyright (C) 1996 Paul Mackerras.
> + * MPC8xx modifications Copyright (C) 1997 Dan Malek (dmalek@jlc.net).
> + */
> +
> +#include <asm/asm-offsets.h>
> +#include <asm/asm-defns.h>
> +#include <asm/msr.h>
> +
> +/* size of minimum stack frame that can hold an entire cpu_user_regs struct */
> +#define STACK_SWITCH_FRAME_SIZE UREGS_sizeof
> +
> + .section .init.text, "ax", @progbits
> +
> +ENTRY(enter_of)
> + mflr %r0
> + std %r0, 16(%r1)
> + stdu %r1,-STACK_SWITCH_FRAME_SIZE(%r1) /* Save SP and create stack space */
> +
> + /*
> + * Because PROM is running in 32b mode, it clobbers the high order half
> + * of all registers that it saves. We therefore save those registers
> + * PROM might touch to the stack. (%r0, %r3-%r13 are caller saved)
> + */
> + SAVE_GPR(2, %r1)
> + SAVE_GPR(13, %r1)
> + SAVE_NVGPRS(%r1)
> + mfcr %r10
> + mfmsr %r11
> + std %r10, UREGS_cr(%r1)
> + std %r11, UREGS_msr(%r1)
> +
> + /* Put PROM address in SRR0 */
> + mtsrr0 %r4
> +
> + /* Setup our trampoline return addr in LR */
> + bcl 20, 31,.+4
> +0: mflr %r4
> + addi %r4, %r4,(1f - 0b)
Nit: Missing blank after last comma. I also wonder whether the parentheses
are doing any good here.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/3] xen/ppc: Set up a basic C environment
2023-07-17 16:00 ` Shawn Anastasio
@ 2023-07-17 16:24 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-07-17 16:24 UTC (permalink / raw)
To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 17.07.2023 18:00, Shawn Anastasio wrote:
> On 7/17/23 10:38 AM, Jan Beulich wrote:
>> On 06.07.2023 21:04, Shawn Anastasio wrote:
>>> --- a/xen/arch/ppc/include/asm/config.h
>>> +++ b/xen/arch/ppc/include/asm/config.h
>>> @@ -43,7 +43,7 @@
>>>
>>> #define SMP_CACHE_BYTES (1 << 6)
>>>
>>> -#define STACK_ORDER 2
>>> +#define STACK_ORDER 0
>>> #define STACK_SIZE (PAGE_SIZE << STACK_ORDER)
>>
>> In which way is this related to the change at hand? Aren't you going to
>> need to undo this rather sooner than later?
>
> I noticed the stack order being too large when I moved the stack
> declaration to .c per Andrew's recommendation for v2. Since we're using
> 64k pages, I don't see why the stack would need to be this big. A quick
> look at ARM shows they have a stack order of 3, which would yield a
> stack size of just 32k.
Oh, I forgot page size is 64k. May I suggest to have a BUILD_BUG_ON()
somewhere, such that switching to e.g. 4k pages (the need for which
cannot be ruled out yet) will make obvious that an adjustment is
necessary. (Alternatively accommodate this case here right away.)
>>> --- a/xen/arch/ppc/ppc64/head.S
>>> +++ b/xen/arch/ppc/ppc64/head.S
>>> @@ -1,30 +1,30 @@
>>> /* SPDX-License-Identifier: GPL-2.0-or-later */
>>>
>>> +#include <asm/asm-defns.h>
>>> +
>>> .section .text.header, "ax", %progbits
>>>
>>> ENTRY(start)
>>> /*
>>> - * Depending on how we were booted, the CPU could be running in either
>>> - * Little Endian or Big Endian mode. The following trampoline from Linux
>>> - * cleverly uses an instruction that encodes to a NOP if the CPU's
>>> - * endianness matches the assumption of the assembler (LE, in our case)
>>> - * or a branch to code that performs the endian switch in the other case.
>>> + * NOTE: argument registers (r3-r9) must be preserved until the C entrypoint
>>> */
>>> - tdi 0, 0, 0x48 /* Reverse endian of b . + 8 */
>>> - b . + 44 /* Skip trampoline if endian is good */
>>> - .long 0xa600607d /* mfmsr r11 */
>>> - .long 0x01006b69 /* xori r11,r11,1 */
>>> - .long 0x00004039 /* li r10,0 */
>>> - .long 0x6401417d /* mtmsrd r10,1 */
>>> - .long 0x05009f42 /* bcl 20,31,$+4 */
>>> - .long 0xa602487d /* mflr r10 */
>>> - .long 0x14004a39 /* addi r10,r10,20 */
>>> - .long 0xa6035a7d /* mtsrr0 r10 */
>>> - .long 0xa6037b7d /* mtsrr1 r11 */
>>> - .long 0x2400004c /* rfid */
>>> -
>>> - /* Now that the endianness is confirmed, continue */
>>> -1: b 1b
>>> + FIXUP_ENDIAN
>>> +
>>> + /* set up the TOC pointer */
>>> + LOAD_IMM32(%r2, .TOC.)
>>> +
>>> + /* set up the initial stack */
>>> + LOAD_IMM32(%r1, cpu0_boot_stack)
>>
>> Wouldn't this (and perhaps also .TOC.) better be calculated in a
>> PC-relative manner? Or is the plan to have Xen linked to an address
>> below 4Gb?
>
> As mentioned previously, I am planning to enable the PIC code model in
> my next series in order to accommodate booting on the PowerNV firmware
> type which has a different load address. That patch will change the
> initial TOC load to a simulated PC-relative one (pre-POWER10 doesn't
> have proper PC-relative loads/stores) and the rest to TOC-relative.
Okay. Perhaps worth mentioning in the description, so the question
won't need asking again. What about addresses being confined to 32
bits, though?
>>> --- /dev/null
>>> +++ b/xen/arch/ppc/setup.c
>>> @@ -0,0 +1,19 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +#include <xen/init.h>
>>> +
>>> +/* Xen stack for bringing up the first CPU. */
>>> +unsigned char __initdata cpu0_boot_stack[STACK_SIZE] __aligned(STACK_SIZE);
>>
>> This yields the entire array as zero-initialized. At which point I
>> don't see a need for the store in head.S.
>
> Okay, fair enough. Given that the array is zero-initialized the stdu
> could be replaced with an `addi %r1, %r1, -STACK_FRAME_OVERHEAD`, and
> the load of zero to %r11 could be deferred to the second patch in this
> series where it's used in the .bss clearing loop.
>
> That said I don't really see the harm with keeping the standard
> idiomatic `stdu` for the initial stack frame setup. Other than
> performance, which isn't a concern here in early setup code.
I'm not going to insist that you switch, but as you can see this can
raise questions.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries
2023-07-17 16:17 ` Jan Beulich
@ 2023-07-17 18:32 ` Shawn Anastasio
2023-07-17 18:40 ` Shawn Anastasio
2023-07-18 6:36 ` Jan Beulich
0 siblings, 2 replies; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-17 18:32 UTC (permalink / raw)
To: Jan Beulich; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 7/17/23 11:17 AM, Jan Beulich wrote:
> On 06.07.2023 21:04, Shawn Anastasio wrote:
>> --- a/xen/arch/ppc/Makefile
>> +++ b/xen/arch/ppc/Makefile
>> @@ -1,5 +1,7 @@
>> obj-$(CONFIG_PPC64) += ppc64/
>>
>> +obj-y += boot-of.init.o
>> +obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>
> While in this case I could accept the name as is to be the same as Arm's,
> as said before it would be nice if new files could avoid underscores (and
> use dashes instead) unless strictly required by something.
>
> Also, with boot-of.c using early_printk.c, shouldn't the latter also
> build into early_printk.init.o?
Yes, good point. I'll change it to early_printk.init.o.
>> --- /dev/null
>> +++ b/xen/arch/ppc/boot-of.c
>> @@ -0,0 +1,100 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright IBM Corp. 2005, 2006, 2007
>
> Judging from the years the file was taken from somewhere. Is the license
> there permitting "2.0 or later"? (For files [partly] taken from somewhere,
> a clarification as to the originals' licenses would be helpful to have in
> the description, or maybe in the post-commit-message area.)
The original license of the file that this was derived from
(xen/arch/powerpc/boot_of.c from Xen 3.2) is GPL v2.0 or later.
In any case where I'm deriving code from existing files, I'm always
using the original license of the derived code. Should I still clarify
this in the header comment?
>> + * Copyright Raptor Engineering, LLC
>> + *
>> + * Authors: Jimi Xenidis <jimix@watson.ibm.com>
>> + * Hollis Blanchard <hollisb@us.ibm.com>
>> + * Shawn Anastasio <sanastasio@raptorengineering.com>
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/stdarg.h>
>> +#include <xen/types.h>
>> +#include <asm/boot.h>
>> +#include <asm/byteorder.h>
>> +#include <asm/early_printk.h>
>> +
>> +#define ADDR(x) (uint32_t)(unsigned long)(x)
>
> Besides needing to be wrapped in parentheses, this likely needs a comment:
> It is hard to see how in 64-bit code uint32_t can suffice as an address.
> Unless (see the question on patch 1) you intend to link/run Xen at an
> address firmly below 4Gb, now and going forward.
Calls to the Open Firmware client interface necessitate that the CPU be
running with both the MMU off and in 32-bit mode (MSR_SF unset).
Obviously these two criteria will not always be true for Xen itself, but
we can guarantee that Xen's physical address lies below 4GB. In fact,
the mere act of being booted by Open Firmware should guarantee that this
is the case.
This also came up earlier, but the Open Firmware interface will only
need to be used during early boot. Once the MMU is up it shouldn't be
necessary to use it, but if it does end up being required this could be
changed to use __pa() or equivalent (plus the uint32_t cast).
I'll definitely add a comment explaining the situation, as well as the
extra parenthesis.
>> +/* OF entrypoint*/
>> +static unsigned long __initdata of_vec;
>> +
>> +/* OF device handles*/
>> +static int __initdata bof_chosen;
>> +static int __initdata of_out;
>> +
>> +static int __init of_call(const char *service, uint32_t nargs, uint32_t nrets,
>> + int32_t rets[], ...)
>
> Nit: Indentation.
Will fix.
>> +{
>> + int rc;
>> + va_list args;
>> + int i;
>
> unsigned int?
I might as well change it to uint32_t to be in line with nargs.
>
>> + struct of_service s = { 0 };
>> +
>> + s.ofs_service = cpu_to_be32(ADDR(service));
>> + s.ofs_nargs = cpu_to_be32(nargs);
>> + s.ofs_nrets = cpu_to_be32(nrets);
>> +
>> + /* copy all the params into the args array */
>
> Nit: Style (comments want to start with a capital letter). Again below.
Will fix.
>> + va_start(args, rets);
>> +
>> + for ( i = 0; i < nargs; i++ )
>> + s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t));
>> +
>> + va_end(args);
>> +
>> + rc = enter_of(&s, of_vec);
>> +
>> + /* copy all return values to the output rets array */
>> + for ( i = 0; i < nrets; i++ )
>> + rets[i] = be32_to_cpu(s.ofs_args[i + nargs]);
>> +
>> + return rc;
>> +}
>> +
>> +static int __init of_finddevice(const char *devspec)
>> +{
>> + int rets[1] = { OF_FAILURE };
>
> Hmm, of_call() uses int32_t. Again below several times.
Good catch. I'll switch all of these to int32_t for consistency.
>
>> + of_call("finddevice", 1, 1, rets, devspec);
>
> This could do with using ARRAY_SIZE(rets). Same again below.
Sure, will do this.
> Also don't you need to wrap devspec in ADDR()? Similar issues
> then again below.
Yes, good catch. I'll fix this and the other bare pointers.
>> + return rets[0];
>> +}
>> +
>> +static int __init of_getprop(int ph, const char *name, void *buf, uint32_t buflen)
>> +{
>> + int rets[1] = { OF_FAILURE };
>> +
>> + of_call("getprop", 4, 1, rets, ph, ADDR(name), buf, buflen);
>> + return rets[0];
>> +}
>> +
>> +int __init of_write(int ih, const char *addr, uint32_t len)
>> +{
>> + int rets[1] = { OF_FAILURE };
>> +
>> + of_call("write", 3, 1, rets, ih, addr, len);
>> + return rets[0];
>> +}
>> +
>> +static void __init of_putchar(char c)
>> +{
>> + if ( c == '\n' )
>> + {
>> + char buf = '\r';
>> + of_write(of_out, &buf, 1);
>> + }
>> + of_write(of_out, &c, 1);
>> +}
>> +
>> +void __init boot_of_init(unsigned long vec)
>> +{
>> + of_vec = vec;
>> +
>> + /* Get a handle to the default console */
>> + bof_chosen = of_finddevice("/chosen");
>> + of_getprop(bof_chosen, "stdout", &of_out, sizeof(of_out));
>> + of_out = be32_to_cpu(of_out);
>> +
>> + early_printk_init(of_putchar);
>> +}
>
> Considering that bof_chosen is used just here, why does it need to be
> a file-scope variable?
Good point. The original code had it as a file-scope variable to be used
by other routines, but I've pruned them from this version which only
does the bare-minimum required for console printing. I'll change this to
a function-scoped variable.
>> --- /dev/null
>> +++ b/xen/arch/ppc/early_printk.c
>> @@ -0,0 +1,28 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +#include <xen/init.h>
>> +#include <asm/boot.h>
>> +
>> +static void (*putchar_func)(char);
>
> __initdata? (Connected to the question of building into .init.o.)
Since I'm going to change this to build to .init.o, would this
automatically be put into the correct .init section? Would it still be
preferable style-wise to mark it as __initdata?
>> --- /dev/null
>> +++ b/xen/arch/ppc/include/asm/processor.h
>> @@ -0,0 +1,139 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright IBM Corp. 2005, 2006, 2007
>> + * Copyright Raptor Engineering, LLC
>> + *
>> + * Authors: Hollis Blanchard <hollisb@us.ibm.com>
>> + * Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>> + * Timothy Pearson <tpearson@raptorengineering.com>
>> + * Shawn Anastasio <sanastasio@raptorengineering.com>
>> + */
>> +
>> +#ifndef _ASM_PPC_PROCESSOR_H
>> +#define _ASM_PPC_PROCESSOR_H
>> +
>> +#include <xen/config.h>
>
> No need to - this is force-included by the Makefile machinery.
Ah, thanks. I'll remove this.
>> +#include <xen/types.h>
>
> Note this. Why ...
See below
>
>> +#define IOBMP_BYTES 8192
>> +#define IOBMP_INVALID_OFFSET 0x8000
>> +
>> +/* Processor Version Register (PVR) field extraction */
>> +
>> +#define PVR_VER(pvr) (((pvr) >> 16) & 0xFFFF) /* Version field */
>> +#define PVR_REV(pvr) (((pvr) >> 0) & 0xFFFF) /* Revison field */
>> +
>> +#define __is_processor(pv) (PVR_VER(mfspr(SPRN_PVR)) == (pv))
>> +
>> +/*
>> + * IBM has further subdivided the standard PowerPC 16-bit version and
>> + * revision subfields of the PVR for the PowerPC 403s into the following:
>> + */
>> +
>> +#define PVR_FAM(pvr) (((pvr) >> 20) & 0xFFF) /* Family field */
>> +#define PVR_MEM(pvr) (((pvr) >> 16) & 0xF) /* Member field */
>> +#define PVR_CORE(pvr) (((pvr) >> 12) & 0xF) /* Core field */
>> +#define PVR_CFG(pvr) (((pvr) >> 8) & 0xF) /* Configuration field */
>> +#define PVR_MAJ(pvr) (((pvr) >> 4) & 0xF) /* Major revision field */
>> +#define PVR_MIN(pvr) (((pvr) >> 0) & 0xF) /* Minor revision field */
>> +
>> +/* Processor Version Numbers */
>> +
>> +#define PVR_403GA 0x00200000
>> +#define PVR_403GB 0x00200100
>> +#define PVR_403GC 0x00200200
>> +#define PVR_403GCX 0x00201400
>> +#define PVR_405GP 0x40110000
>> +#define PVR_STB03XXX 0x40310000
>> +#define PVR_NP405H 0x41410000
>> +#define PVR_NP405L 0x41610000
>> +#define PVR_601 0x00010000
>> +#define PVR_602 0x00050000
>> +#define PVR_603 0x00030000
>> +#define PVR_603e 0x00060000
>> +#define PVR_603ev 0x00070000
>> +#define PVR_603r 0x00071000
>> +#define PVR_604 0x00040000
>> +#define PVR_604e 0x00090000
>> +#define PVR_604r 0x000A0000
>> +#define PVR_620 0x00140000
>> +#define PVR_740 0x00080000
>> +#define PVR_750 PVR_740
>> +#define PVR_740P 0x10080000
>> +#define PVR_750P PVR_740P
>> +#define PVR_7400 0x000C0000
>> +#define PVR_7410 0x800C0000
>> +#define PVR_7450 0x80000000
>> +#define PVR_8540 0x80200000
>> +#define PVR_8560 0x80200000
>> +/*
>> + * For the 8xx processors, all of them report the same PVR family for
>> + * the PowerPC core. The various versions of these processors must be
>> + * differentiated by the version number in the Communication Processor
>> + * Module (CPM).
>> + */
>> +#define PVR_821 0x00500000
>> +#define PVR_823 PVR_821
>> +#define PVR_850 PVR_821
>> +#define PVR_860 PVR_821
>> +#define PVR_8240 0x00810100
>> +#define PVR_8245 0x80811014
>> +#define PVR_8260 PVR_8240
>> +
>> +/* 64-bit processors */
>> +#define PVR_NORTHSTAR 0x0033
>> +#define PVR_PULSAR 0x0034
>> +#define PVR_POWER4 0x0035
>> +#define PVR_ICESTAR 0x0036
>> +#define PVR_SSTAR 0x0037
>> +#define PVR_POWER4p 0x0038
>> +#define PVR_970 0x0039
>> +#define PVR_POWER5 0x003A
>> +#define PVR_POWER5p 0x003B
>> +#define PVR_970FX 0x003C
>> +#define PVR_POWER6 0x003E
>> +#define PVR_POWER7 0x003F
>> +#define PVR_630 0x0040
>> +#define PVR_630p 0x0041
>> +#define PVR_970MP 0x0044
>> +#define PVR_970GX 0x0045
>> +#define PVR_POWER7p 0x004A
>> +#define PVR_POWER8E 0x004B
>> +#define PVR_POWER8NVL 0x004C
>> +#define PVR_POWER8 0x004D
>> +#define PVR_POWER9 0x004E
>> +#define PVR_POWER10 0x0080
>> +#define PVR_BE 0x0070
>> +#define PVR_PA6T 0x0090
>> +
>> +/* Macro to adjust thread priority for hardware multithreading */
>> +#define HMT_very_low() asm volatile (" or %r31, %r31, %r31 ")
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +#include <xen/types.h>
>
> ... another time here? Or is the first one perhaps too early?
Yes, the first include was a mistake -- this is the intended location.
Will fix.
>
>> +/* User-accessible registers: nost of these need to be saved/restored
>> + * for every nested Xen invocation. */
>
> Nit: Comment style. Multi-line comments want to be
>
> /*
> * User-accessible registers: most of these need to be saved/restored
> * for every nested Xen invocation.
> */
>
> (also note the s/nost/most/ I did)
Will fix.
>
>> --- a/xen/arch/ppc/ppc64/head.S
>> +++ b/xen/arch/ppc/ppc64/head.S
>> @@ -18,6 +18,15 @@ ENTRY(start)
>> li %r11, 0
>> stdu %r11, -STACK_FRAME_OVERHEAD(%r1)
>>
>> + /* clear .bss */
>> + LOAD_IMM32(%r14, __bss_start)
>> + LOAD_IMM32(%r15, __bss_end)
>
> Question regarding addressing again.
Hopefully the above explanation answered your questions, but just to
summarize: Xen will always reside within the first 4G of physical
address space.
Also, as mentioned in your review of patch 1/3, this will soon be
changed to use TOC-relative addressing instead of immediate addressing.
>
>> --- /dev/null
>> +++ b/xen/arch/ppc/ppc64/of-call.S
>> @@ -0,0 +1,83 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Adapted from Linux's arch/powerpc/kernel/entry_64.S, with the
>> + * following copyright notice:
>> + *
>> + * PowerPC version
>> + * Copyright (C) 1995-1996 Gary Thomas (gdt@linuxppc.org)
>> + * Rewritten by Cort Dougan (cort@cs.nmt.edu) for PReP
>> + * Copyright (C) 1996 Cort Dougan <cort@cs.nmt.edu>
>> + * Adapted for Power Macintosh by Paul Mackerras.
>> + * Low-level exception handlers and MMU support
>> + * rewritten by Paul Mackerras.
>> + * Copyright (C) 1996 Paul Mackerras.
>> + * MPC8xx modifications Copyright (C) 1997 Dan Malek (dmalek@jlc.net).
>> + */
>> +
>> +#include <asm/asm-offsets.h>
>> +#include <asm/asm-defns.h>
>> +#include <asm/msr.h>
>> +
>> +/* size of minimum stack frame that can hold an entire cpu_user_regs struct */
>> +#define STACK_SWITCH_FRAME_SIZE UREGS_sizeof
>> +
>> + .section .init.text, "ax", @progbits
>> +
>> +ENTRY(enter_of)
>> + mflr %r0
>> + std %r0, 16(%r1)
>> + stdu %r1,-STACK_SWITCH_FRAME_SIZE(%r1) /* Save SP and create stack space */
>> +
>> + /*
>> + * Because PROM is running in 32b mode, it clobbers the high order half
>> + * of all registers that it saves. We therefore save those registers
>> + * PROM might touch to the stack. (%r0, %r3-%r13 are caller saved)
>> + */
>> + SAVE_GPR(2, %r1)
>> + SAVE_GPR(13, %r1)
>> + SAVE_NVGPRS(%r1)
>> + mfcr %r10
>> + mfmsr %r11
>> + std %r10, UREGS_cr(%r1)
>> + std %r11, UREGS_msr(%r1)
>> +
>> + /* Put PROM address in SRR0 */
>> + mtsrr0 %r4
>> +
>> + /* Setup our trampoline return addr in LR */
>> + bcl 20, 31,.+4
>> +0: mflr %r4
>> + addi %r4, %r4,(1f - 0b)
>
> Nit: Missing blank after last comma. I also wonder whether the parentheses
> are doing any good here.
Will fix the blank. The parentheses here are also unnecessary, so I can
remove them.
> Jan
Thanks,
Shawn
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries
2023-07-17 18:32 ` Shawn Anastasio
@ 2023-07-17 18:40 ` Shawn Anastasio
2023-07-18 6:41 ` Jan Beulich
2023-07-18 6:36 ` Jan Beulich
1 sibling, 1 reply; 12+ messages in thread
From: Shawn Anastasio @ 2023-07-17 18:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
Quick followup,
On 7/17/23 1:32 PM, Shawn Anastasio wrote:
> On 7/17/23 11:17 AM, Jan Beulich wrote:
>> This could do with using ARRAY_SIZE(rets). Same again below.
>
> Sure, will do this.
ARRAY_SIZE is defined in <xen/lib.h> which can't yet be included due to
missing headers. I could copy its definition into this file along with a
TODO comment to fix it once the include works.
If I were to go down that route, I'd also probably not include the
__must_be_array assertion and related machinery that the <xen/lib.h>
version has, since it'd require a large portion of <xen/lib.h> to be
copy/pasted.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries
2023-07-17 18:32 ` Shawn Anastasio
2023-07-17 18:40 ` Shawn Anastasio
@ 2023-07-18 6:36 ` Jan Beulich
1 sibling, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-07-18 6:36 UTC (permalink / raw)
To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 17.07.2023 20:32, Shawn Anastasio wrote:
> On 7/17/23 11:17 AM, Jan Beulich wrote:
>> On 06.07.2023 21:04, Shawn Anastasio wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/ppc/boot-of.c
>>> @@ -0,0 +1,100 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright IBM Corp. 2005, 2006, 2007
>>
>> Judging from the years the file was taken from somewhere. Is the license
>> there permitting "2.0 or later"? (For files [partly] taken from somewhere,
>> a clarification as to the originals' licenses would be helpful to have in
>> the description, or maybe in the post-commit-message area.)
>
> The original license of the file that this was derived from
> (xen/arch/powerpc/boot_of.c from Xen 3.2) is GPL v2.0 or later.
>
> In any case where I'm deriving code from existing files, I'm always
> using the original license of the derived code. Should I still clarify
> this in the header comment?
I think it would be good to mention explicitly, as 2.0-only is the
common expectation.
>>> +/* OF entrypoint*/
>>> +static unsigned long __initdata of_vec;
>>> +
>>> +/* OF device handles*/
>>> +static int __initdata bof_chosen;
>>> +static int __initdata of_out;
>>> +
>>> +static int __init of_call(const char *service, uint32_t nargs, uint32_t nrets,
>>> + int32_t rets[], ...)
>>
>> Nit: Indentation.
>
> Will fix.
>
>>> +{
>>> + int rc;
>>> + va_list args;
>>> + int i;
>>
>> unsigned int?
>
> I might as well change it to uint32_t to be in line with nargs.
Please don't. See ./CODING_STYLE for when it is okay to use fixed-
width types.
>>> + va_start(args, rets);
>>> +
>>> + for ( i = 0; i < nargs; i++ )
>>> + s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t));
>>> +
>>> + va_end(args);
>>> +
>>> + rc = enter_of(&s, of_vec);
>>> +
>>> + /* copy all return values to the output rets array */
>>> + for ( i = 0; i < nrets; i++ )
>>> + rets[i] = be32_to_cpu(s.ofs_args[i + nargs]);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +static int __init of_finddevice(const char *devspec)
>>> +{
>>> + int rets[1] = { OF_FAILURE };
>>
>> Hmm, of_call() uses int32_t. Again below several times.
>
> Good catch. I'll switch all of these to int32_t for consistency.
Here, for example, it is (because of being used to interface with
firmware).
>>> --- /dev/null
>>> +++ b/xen/arch/ppc/early_printk.c
>>> @@ -0,0 +1,28 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#include <xen/init.h>
>>> +#include <asm/boot.h>
>>> +
>>> +static void (*putchar_func)(char);
>>
>> __initdata? (Connected to the question of building into .init.o.)
>
> Since I'm going to change this to build to .init.o, would this
> automatically be put into the correct .init section? Would it still be
> preferable style-wise to mark it as __initdata?
No, it would complain that .data or .bss is non-empty. Automatic
conversion occurs only for things you can't control at the source
level, e.g. string literals.
Jan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries
2023-07-17 18:40 ` Shawn Anastasio
@ 2023-07-18 6:41 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2023-07-18 6:41 UTC (permalink / raw)
To: Shawn Anastasio; +Cc: Timothy Pearson, Andrew Cooper, xen-devel
On 17.07.2023 20:40, Shawn Anastasio wrote:
> Quick followup,
>
> On 7/17/23 1:32 PM, Shawn Anastasio wrote:
>> On 7/17/23 11:17 AM, Jan Beulich wrote:
>>> This could do with using ARRAY_SIZE(rets). Same again below.
>>
>> Sure, will do this.
>
> ARRAY_SIZE is defined in <xen/lib.h> which can't yet be included due to
> missing headers. I could copy its definition into this file along with a
> TODO comment to fix it once the include works.
>
> If I were to go down that route, I'd also probably not include the
> __must_be_array assertion and related machinery that the <xen/lib.h>
> version has, since it'd require a large portion of <xen/lib.h> to be
> copy/pasted.
You (like anyone else) could review (and then offer R-b) my "common:
move a few macros out of xen/lib.h" [1]. That's of course only a tiny
first step (covering little more that what I need in that series). You
(or I or anyone else) could then put a patch on top moving ARRAY_SIZE()
(and its prereqs) to the new header as well. Since the new header has
no other prereqs, it should be usable as is, without the need for any
arch headers, both here and similarly in the RISC-V work.
Jan
[1] https://lists.xen.org/archives/html/xen-devel/2023-07/msg00582.html
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-07-18 6:41 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-06 19:04 [PATCH v3 0/3] Early serial on Power Shawn Anastasio
2023-07-06 19:04 ` [PATCH v3 1/3] xen/ppc: Set up a basic C environment Shawn Anastasio
2023-07-17 15:38 ` Jan Beulich
2023-07-17 16:00 ` Shawn Anastasio
2023-07-17 16:24 ` Jan Beulich
2023-07-06 19:04 ` [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries Shawn Anastasio
2023-07-17 16:17 ` Jan Beulich
2023-07-17 18:32 ` Shawn Anastasio
2023-07-17 18:40 ` Shawn Anastasio
2023-07-18 6:41 ` Jan Beulich
2023-07-18 6:36 ` Jan Beulich
2023-07-06 19:04 ` [PATCH v3 3/3] automation: Add smoke test for ppc64le Shawn Anastasio
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.