[PATCH v3 2/4] xen: Add files needed for minimal ppc64le build

Shawn Anastasio posted 4 patches 1 year, 5 months ago
There is a newer version of this series
[PATCH v3 2/4] xen: Add files needed for minimal ppc64le build
Posted by Shawn Anastasio 1 year, 5 months ago
Add the build system changes required to build for ppc64le (POWER8+).
As of now the resulting image simply boots to an infinite loop.

$ make XEN_TARGET_ARCH=ppc64 -C xen openpower_defconfig
$ make XEN_TARGET_ARCH=ppc64 SUBSYSTEMS=xen -C xen build

This port targets POWER8+ CPUs running in Little Endian mode specifically,
and does not boot on older machines. Additionally, this initial skeleton
only implements the PaPR/pseries boot protocol which allows it to be
booted in a standard QEMU virtual machine:

$ qemu-system-ppc64 -M pseries-5.2 -m 256M -kernel xen/xen

Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
---
 config/ppc64.mk                          |   5 +
 xen/Makefile                             |   5 +-
 xen/arch/ppc/Kconfig                     |  42 ++++++
 xen/arch/ppc/Kconfig.debug               |   0
 xen/arch/ppc/Makefile                    |  16 +++
 xen/arch/ppc/Rules.mk                    |   0
 xen/arch/ppc/arch.mk                     |  11 ++
 xen/arch/ppc/configs/openpower_defconfig |  13 ++
 xen/arch/ppc/include/asm/config.h        |  63 +++++++++
 xen/arch/ppc/include/asm/page-bits.h     |   7 +
 xen/arch/ppc/ppc64/Makefile              |   1 +
 xen/arch/ppc/ppc64/asm-offsets.c         |   0
 xen/arch/ppc/ppc64/head.S                |  27 ++++
 xen/arch/ppc/xen.lds.S                   | 173 +++++++++++++++++++++++
 14 files changed, 361 insertions(+), 2 deletions(-)
 create mode 100644 config/ppc64.mk
 create mode 100644 xen/arch/ppc/Kconfig
 create mode 100644 xen/arch/ppc/Kconfig.debug
 create mode 100644 xen/arch/ppc/Makefile
 create mode 100644 xen/arch/ppc/Rules.mk
 create mode 100644 xen/arch/ppc/arch.mk
 create mode 100644 xen/arch/ppc/configs/openpower_defconfig
 create mode 100644 xen/arch/ppc/include/asm/config.h
 create mode 100644 xen/arch/ppc/include/asm/page-bits.h
 create mode 100644 xen/arch/ppc/ppc64/Makefile
 create mode 100644 xen/arch/ppc/ppc64/asm-offsets.c
 create mode 100644 xen/arch/ppc/ppc64/head.S
 create mode 100644 xen/arch/ppc/xen.lds.S

diff --git a/config/ppc64.mk b/config/ppc64.mk
new file mode 100644
index 0000000000..597f0668c3
--- /dev/null
+++ b/config/ppc64.mk
@@ -0,0 +1,5 @@
+CONFIG_PPC := y
+CONFIG_PPC64 := y
+CONFIG_PPC_$(XEN_OS) := y
+
+CONFIG_XEN_INSTALL_SUFFIX :=
diff --git a/xen/Makefile b/xen/Makefile
index e89fc461fc..db5454fb58 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -38,7 +38,7 @@ EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi
 ARCH=$(XEN_TARGET_ARCH)
 SRCARCH=$(shell echo $(ARCH) | \
           sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-              -e s'/riscv.*/riscv/g')
+              -e s'/riscv.*/riscv/g' -e s'/ppc.*/ppc/g')
 export ARCH SRCARCH
 
 # Allow someone to change their config file
@@ -244,7 +244,7 @@ include $(XEN_ROOT)/Config.mk
 export TARGET_SUBARCH  := $(XEN_TARGET_ARCH)
 export TARGET_ARCH     := $(shell echo $(XEN_TARGET_ARCH) | \
                             sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
-                                -e s'/riscv.*/riscv/g')
+                                -e s'/riscv.*/riscv/g' -e s'/ppc.*/ppc/g')
 
 export CONFIG_SHELL := $(SHELL)
 export CC CXX LD NM OBJCOPY OBJDUMP ADDR2LINE
@@ -563,6 +563,7 @@ _clean:
 	$(Q)$(MAKE) $(clean)=xsm
 	$(Q)$(MAKE) $(clean)=crypto
 	$(Q)$(MAKE) $(clean)=arch/arm
+	$(Q)$(MAKE) $(clean)=arch/ppc
 	$(Q)$(MAKE) $(clean)=arch/riscv
 	$(Q)$(MAKE) $(clean)=arch/x86
 	$(Q)$(MAKE) $(clean)=test
diff --git a/xen/arch/ppc/Kconfig b/xen/arch/ppc/Kconfig
new file mode 100644
index 0000000000..a0a70adef4
--- /dev/null
+++ b/xen/arch/ppc/Kconfig
@@ -0,0 +1,42 @@
+config PPC
+	def_bool y
+
+config PPC64
+	def_bool y
+	select 64BIT
+
+config ARCH_DEFCONFIG
+	string
+	default "arch/ppc/configs/openpower_defconfig"
+
+menu "Architecture Features"
+
+source "arch/Kconfig"
+
+endmenu
+
+menu "ISA Selection"
+
+choice
+	prompt "Base ISA"
+	default POWER_ISA_2_07B if PPC64
+	help
+	  This selects the base ISA version that Xen will target.
+
+config POWER_ISA_2_07B
+	bool "Power ISA 2.07B"
+	help
+	  Target version 2.07B of the Power ISA (POWER8)
+
+config POWER_ISA_3_00
+	bool "Power ISA 3.00"
+	help
+	  Target version 3.00 of the Power ISA (POWER9)
+
+endchoice
+
+endmenu
+
+source "common/Kconfig"
+
+source "drivers/Kconfig"
diff --git a/xen/arch/ppc/Kconfig.debug b/xen/arch/ppc/Kconfig.debug
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/ppc/Makefile b/xen/arch/ppc/Makefile
new file mode 100644
index 0000000000..10b101cf9c
--- /dev/null
+++ b/xen/arch/ppc/Makefile
@@ -0,0 +1,16 @@
+obj-$(CONFIG_PPC64) += ppc64/
+
+$(TARGET): $(TARGET)-syms
+	cp -f $< $@
+
+$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
+	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) -o $@
+	$(NM) -pa --format=sysv $(@D)/$(@F) \
+		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
+		>$(@D)/$(@F).map
+
+$(obj)/xen.lds: $(src)/xen.lds.S FORCE
+	$(call if_changed_dep,cpp_lds_S)
+
+.PHONY: include
+include:
diff --git a/xen/arch/ppc/Rules.mk b/xen/arch/ppc/Rules.mk
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/ppc/arch.mk b/xen/arch/ppc/arch.mk
new file mode 100644
index 0000000000..c185a5028a
--- /dev/null
+++ b/xen/arch/ppc/arch.mk
@@ -0,0 +1,11 @@
+########################################
+# Power-specific definitions
+
+ppc-march-$(CONFIG_POWER_ISA_2_07B) := power8
+ppc-march-$(CONFIG_POWER_ISA_3_00) := power9
+
+CFLAGS += -mcpu=$(ppc-march-y) -mstrict-align -mcmodel=large -mabi=elfv2 -mno-altivec -mno-vsx
+
+# TODO: Drop override when more of the build is working
+override ALL_OBJS-y = arch/$(TARGET_ARCH)/built_in.o
+override ALL_LIBS-y =
diff --git a/xen/arch/ppc/configs/openpower_defconfig b/xen/arch/ppc/configs/openpower_defconfig
new file mode 100644
index 0000000000..8783eb3488
--- /dev/null
+++ b/xen/arch/ppc/configs/openpower_defconfig
@@ -0,0 +1,13 @@
+# CONFIG_SCHED_CREDIT is not set
+# CONFIG_SCHED_RTDS is not set
+# CONFIG_SCHED_NULL is not set
+# CONFIG_SCHED_ARINC653 is not set
+# CONFIG_TRACEBUFFER is not set
+# CONFIG_HYPFS is not set
+# CONFIG_GRANT_TABLE is not set
+# CONFIG_SPECULATIVE_HARDEN_ARRAY is not set
+
+CONFIG_PPC64=y
+CONFIG_DEBUG=y
+CONFIG_DEBUG_INFO=y
+CONFIG_EXPERT=y
diff --git a/xen/arch/ppc/include/asm/config.h b/xen/arch/ppc/include/asm/config.h
new file mode 100644
index 0000000000..7a2862ef7a
--- /dev/null
+++ b/xen/arch/ppc/include/asm/config.h
@@ -0,0 +1,63 @@
+#ifndef __PPC_CONFIG_H__
+#define __PPC_CONFIG_H__
+
+#include <xen/const.h>
+#include <xen/page-size.h>
+
+#if defined(CONFIG_PPC64)
+#define LONG_BYTEORDER 3
+#define ELFSIZE        64
+#define MAX_VIRT_CPUS  1024u
+#else
+#error "Unsupported PowerPC variant"
+#endif
+
+#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
+#define BITS_PER_LONG  (BYTES_PER_LONG << 3)
+#define POINTER_ALIGN  BYTES_PER_LONG
+
+#define BITS_PER_LLONG 64
+
+/* xen_ulong_t is always 64 bits */
+#define BITS_PER_XEN_ULONG 64
+
+#define CONFIG_PPC_L1_CACHE_SHIFT  7
+#define CONFIG_PAGEALLOC_MAX_ORDER 18
+#define CONFIG_DOMU_MAX_ORDER      9
+#define CONFIG_HWDOM_MAX_ORDER     10
+
+#define OPT_CONSOLE_STR "dtuart"
+#define INVALID_VCPU_ID MAX_VIRT_CPUS
+
+/* Linkage for PPC */
+#ifdef __ASSEMBLY__
+#define ALIGN .align 2
+
+#define ENTRY(name)                                                            \
+    .globl name;                                                               \
+    ALIGN;                                                                     \
+    name:
+#endif
+
+#define XEN_VIRT_START _AT(UL, 0x400000)
+
+#define SMP_CACHE_BYTES (1 << 6)
+
+#define STACK_ORDER 2
+#define STACK_SIZE  (PAGE_SIZE << STACK_ORDER)
+
+/* 288 bytes below the stack pointer must be preserved by interrupt handlers */
+#define STACK_VOLATILE_AREA 288
+
+/* size of minimum stack frame; C code can write into the caller's stack */
+#define STACK_FRAME_OVERHEAD 32
+
+#endif /* __PPC_CONFIG_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/ppc/include/asm/page-bits.h b/xen/arch/ppc/include/asm/page-bits.h
new file mode 100644
index 0000000000..4c01bf9716
--- /dev/null
+++ b/xen/arch/ppc/include/asm/page-bits.h
@@ -0,0 +1,7 @@
+#ifndef __PPC_PAGE_BITS_H__
+#define __PPC_PAGE_BITS_H__
+
+#define PAGE_SHIFT              16 /* 64 KiB Pages */
+#define PADDR_BITS              48
+
+#endif /* __PPC_PAGE_BITS_H__ */
diff --git a/xen/arch/ppc/ppc64/Makefile b/xen/arch/ppc/ppc64/Makefile
new file mode 100644
index 0000000000..3340058c08
--- /dev/null
+++ b/xen/arch/ppc/ppc64/Makefile
@@ -0,0 +1 @@
+obj-y += head.o
diff --git a/xen/arch/ppc/ppc64/asm-offsets.c b/xen/arch/ppc/ppc64/asm-offsets.c
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/xen/arch/ppc/ppc64/head.S b/xen/arch/ppc/ppc64/head.S
new file mode 100644
index 0000000000..e24331f95c
--- /dev/null
+++ b/xen/arch/ppc/ppc64/head.S
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+.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.
+     */
+    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
diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
new file mode 100644
index 0000000000..89fcdc9c00
--- /dev/null
+++ b/xen/arch/ppc/xen.lds.S
@@ -0,0 +1,173 @@
+#include <xen/xen.lds.h>
+
+#undef ENTRY
+#undef ALIGN
+
+OUTPUT_ARCH(powerpc:common64)
+ENTRY(start)
+
+PHDRS
+{
+    text PT_LOAD ;
+#if defined(BUILD_ID)
+    note PT_NOTE ;
+#endif
+}
+
+/**
+ * OF's base load address is 0x400000 (XEN_VIRT_START).
+ * By defining sections this way, we can keep our virtual address base at 0x400000
+ * while keeping the physical base at 0x0.
+ *
+ * Without this, OF incorrectly loads .text at 0x400000 + 0x400000 = 0x800000.
+ * Taken from x86/xen.lds.S
+ */
+#ifdef CONFIG_LD_IS_GNU
+# define DECL_SECTION(x) x : AT(ADDR(#x) - XEN_VIRT_START)
+#else
+# define DECL_SECTION(x) x : AT(ADDR(x) - XEN_VIRT_START)
+#endif
+
+SECTIONS
+{
+    . = XEN_VIRT_START;
+
+    DECL_SECTION(.text) {
+        _stext = .;            /* Text section */
+        *(.text.header)
+
+        *(.text.cold)
+        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
+
+        *(.text)
+#ifdef CONFIG_CC_SPLIT_SECTIONS
+        *(.text.*)
+#endif
+
+        *(.fixup)
+        *(.gnu.warning)
+        . = ALIGN(POINTER_ALIGN);
+        _etext = .;             /* End of text section */
+    } :text
+
+    . = ALIGN(PAGE_SIZE);
+    DECL_SECTION(.rodata) {
+        _srodata = .;          /* Read-only data */
+        *(.rodata)
+        *(.rodata.*)
+        *(.data.rel.ro)
+        *(.data.rel.ro.*)
+
+        VPCI_ARRAY
+
+        . = ALIGN(POINTER_ALIGN);
+        _erodata = .;        /* End of read-only data */
+    } :text
+
+    #if defined(BUILD_ID)
+    . = ALIGN(4);
+    DECL_SECTION(.note.gnu.build-id) {
+        __note_gnu_build_id_start = .;
+        *(.note.gnu.build-id)
+        __note_gnu_build_id_end = .;
+    } :note :text
+    #endif
+    _erodata = .;                /* End of read-only data */
+
+    . = ALIGN(PAGE_SIZE);
+    DECL_SECTION(.data.ro_after_init) {
+        __ro_after_init_start = .;
+        *(.data.ro_after_init)
+        . = ALIGN(PAGE_SIZE);
+        __ro_after_init_end = .;
+    } : text
+
+    DECL_SECTION(.data.read_mostly) {
+        *(.data.read_mostly)
+    } :text
+
+    . = ALIGN(PAGE_SIZE);
+    DECL_SECTION(.data) {                    /* Data */
+        *(.data.page_aligned)
+        . = ALIGN(8);
+        __start_schedulers_array = .;
+        *(.data.schedulers)
+        __end_schedulers_array = .;
+
+        HYPFS_PARAM
+
+        *(.data .data.*)
+        CONSTRUCTORS
+    } :text
+
+    . = ALIGN(PAGE_SIZE);             /* Init code and data */
+    __init_begin = .;
+    DECL_SECTION(.init.text) {
+        _sinittext = .;
+        *(.init.text)
+        _einittext = .;
+        . = ALIGN(PAGE_SIZE);        /* Avoid mapping alt insns executable */
+    } :text
+
+    . = ALIGN(PAGE_SIZE);
+    DECL_SECTION(.init.data) {
+        *(.init.rodata)
+        *(.init.rodata.*)
+
+        . = ALIGN(POINTER_ALIGN);
+        __setup_start = .;
+        *(.init.setup)
+        __setup_end = .;
+
+        __initcall_start = .;
+        *(.initcallpresmp.init)
+        __presmp_initcall_end = .;
+        *(.initcall1.init)
+        __initcall_end = .;
+
+        LOCK_PROFILE_DATA
+
+        *(.init.data)
+        *(.init.data.rel)
+        *(.init.data.rel.*)
+
+        . = ALIGN(8);
+        __ctors_start = .;
+        *(.ctors)
+        *(.init_array)
+        *(SORT(.init_array.*))
+        __ctors_end = .;
+    } :text
+    . = ALIGN(POINTER_ALIGN);
+    __init_end = .;
+
+    DECL_SECTION(.bss) {                     /* BSS */
+        __bss_start = .;
+        *(.bss.stack_aligned)
+        . = ALIGN(PAGE_SIZE);
+        *(.bss.page_aligned)
+        . = ALIGN(PAGE_SIZE);
+        __per_cpu_start = .;
+        *(.bss.percpu.page_aligned)
+        *(.bss.percpu)
+        . = ALIGN(SMP_CACHE_BYTES);
+        *(.bss.percpu.read_mostly)
+        . = ALIGN(SMP_CACHE_BYTES);
+        __per_cpu_data_end = .;
+        *(.bss .bss.*)
+        . = ALIGN(POINTER_ALIGN);
+        __bss_end = .;
+    } :text
+    _end = . ;
+
+    /* Section for the device tree blob (if any). */
+    DECL_SECTION(.dtb) { *(.dtb) } :text
+
+    DWARF2_DEBUG_SECTIONS
+
+    DISCARD_SECTIONS
+
+    STABS_DEBUG_SECTIONS
+
+    ELF_DETAILS_SECTIONS
+}
-- 
2.30.2
Re: [PATCH v3 2/4] xen: Add files needed for minimal ppc64le build
Posted by Jan Beulich 1 year, 5 months ago
On 13.06.2023 16:50, Shawn Anastasio wrote:
> --- /dev/null
> +++ b/xen/arch/ppc/Makefile
> @@ -0,0 +1,16 @@
> +obj-$(CONFIG_PPC64) += ppc64/
> +
> +$(TARGET): $(TARGET)-syms
> +	cp -f $< $@
> +
> +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) -o $@
> +	$(NM) -pa --format=sysv $(@D)/$(@F) \
> +		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
> +		>$(@D)/$(@F).map

Elsewhere we recently switched these uses of $(@D)/$(@F) to just $@.
Please can you do so here as well?

> --- /dev/null
> +++ b/xen/arch/ppc/arch.mk
> @@ -0,0 +1,11 @@
> +########################################
> +# Power-specific definitions
> +
> +ppc-march-$(CONFIG_POWER_ISA_2_07B) := power8
> +ppc-march-$(CONFIG_POWER_ISA_3_00) := power9
> +
> +CFLAGS += -mcpu=$(ppc-march-y) -mstrict-align -mcmodel=large -mabi=elfv2 -mno-altivec -mno-vsx

Wouldn't it make sense to also pass -mlittle here, such that a tool
chain defaulting to big-endian can still be used?

> --- /dev/null
> +++ b/xen/arch/ppc/ppc64/head.S
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +.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.
> +     */
> +    tdi 0, 0, 0x48    /* Reverse endian of b . + 8          */
> +    b $ + 44          /* Skip trampoline if endian is good  */

If I get this right, $ and . are interchangable on Power? If not,
then all is fine and there likely is a reason to use . in the
comment but $ in the code. But if so, it would be nice if both
could match, and I guess with other architectures in mind . would
be preferable.

> --- /dev/null
> +++ b/xen/arch/ppc/xen.lds.S
> @@ -0,0 +1,173 @@
> +#include <xen/xen.lds.h>
> +
> +#undef ENTRY
> +#undef ALIGN
> +
> +OUTPUT_ARCH(powerpc:common64)
> +ENTRY(start)
> +
> +PHDRS
> +{
> +    text PT_LOAD ;
> +#if defined(BUILD_ID)
> +    note PT_NOTE ;
> +#endif
> +}
> +
> +/**
> + * OF's base load address is 0x400000 (XEN_VIRT_START).
> + * By defining sections this way, we can keep our virtual address base at 0x400000
> + * while keeping the physical base at 0x0.
> + *
> + * Without this, OF incorrectly loads .text at 0x400000 + 0x400000 = 0x800000.
> + * Taken from x86/xen.lds.S
> + */
> +#ifdef CONFIG_LD_IS_GNU
> +# define DECL_SECTION(x) x : AT(ADDR(#x) - XEN_VIRT_START)
> +#else
> +# define DECL_SECTION(x) x : AT(ADDR(x) - XEN_VIRT_START)
> +#endif
> +
> +SECTIONS
> +{
> +    . = XEN_VIRT_START;
> +
> +    DECL_SECTION(.text) {
> +        _stext = .;            /* Text section */
> +        *(.text.header)
> +
> +        *(.text.cold)
> +        *(.text.unlikely .text.*_unlikely .text.unlikely.*)
> +
> +        *(.text)
> +#ifdef CONFIG_CC_SPLIT_SECTIONS
> +        *(.text.*)
> +#endif
> +
> +        *(.fixup)
> +        *(.gnu.warning)
> +        . = ALIGN(POINTER_ALIGN);
> +        _etext = .;             /* End of text section */
> +    } :text
> +
> +    . = ALIGN(PAGE_SIZE);
> +    DECL_SECTION(.rodata) {
> +        _srodata = .;          /* Read-only data */
> +        *(.rodata)
> +        *(.rodata.*)
> +        *(.data.rel.ro)
> +        *(.data.rel.ro.*)
> +
> +        VPCI_ARRAY
> +
> +        . = ALIGN(POINTER_ALIGN);
> +        _erodata = .;        /* End of read-only data */
> +    } :text
> +
> +    #if defined(BUILD_ID)
> +    . = ALIGN(4);
> +    DECL_SECTION(.note.gnu.build-id) {
> +        __note_gnu_build_id_start = .;
> +        *(.note.gnu.build-id)
> +        __note_gnu_build_id_end = .;
> +    } :note :text
> +    #endif
> +    _erodata = .;                /* End of read-only data */
> +
> +    . = ALIGN(PAGE_SIZE);
> +    DECL_SECTION(.data.ro_after_init) {
> +        __ro_after_init_start = .;
> +        *(.data.ro_after_init)
> +        . = ALIGN(PAGE_SIZE);
> +        __ro_after_init_end = .;
> +    } : text
> +
> +    DECL_SECTION(.data.read_mostly) {
> +        *(.data.read_mostly)
> +    } :text
> +
> +    . = ALIGN(PAGE_SIZE);
> +    DECL_SECTION(.data) {                    /* Data */
> +        *(.data.page_aligned)
> +        . = ALIGN(8);
> +        __start_schedulers_array = .;
> +        *(.data.schedulers)
> +        __end_schedulers_array = .;
> +
> +        HYPFS_PARAM
> +
> +        *(.data .data.*)
> +        CONSTRUCTORS
> +    } :text
> +
> +    . = ALIGN(PAGE_SIZE);             /* Init code and data */
> +    __init_begin = .;
> +    DECL_SECTION(.init.text) {
> +        _sinittext = .;
> +        *(.init.text)
> +        _einittext = .;
> +        . = ALIGN(PAGE_SIZE);        /* Avoid mapping alt insns executable */
> +    } :text
> +
> +    . = ALIGN(PAGE_SIZE);
> +    DECL_SECTION(.init.data) {
> +        *(.init.rodata)
> +        *(.init.rodata.*)
> +
> +        . = ALIGN(POINTER_ALIGN);
> +        __setup_start = .;
> +        *(.init.setup)
> +        __setup_end = .;
> +
> +        __initcall_start = .;
> +        *(.initcallpresmp.init)
> +        __presmp_initcall_end = .;
> +        *(.initcall1.init)
> +        __initcall_end = .;
> +
> +        LOCK_PROFILE_DATA
> +
> +        *(.init.data)
> +        *(.init.data.rel)
> +        *(.init.data.rel.*)
> +
> +        . = ALIGN(8);
> +        __ctors_start = .;
> +        *(.ctors)
> +        *(.init_array)
> +        *(SORT(.init_array.*))
> +        __ctors_end = .;
> +    } :text
> +    . = ALIGN(POINTER_ALIGN);
> +    __init_end = .;

Up to here I think I agree with all uses of ALIGN(), but ...

> +    DECL_SECTION(.bss) {                     /* BSS */
> +        __bss_start = .;
> +        *(.bss.stack_aligned)
> +        . = ALIGN(PAGE_SIZE);
> +        *(.bss.page_aligned)

... the one between the two .bss parts looks unmotivated. Within
a section definition ALIGN() typically only makes sense when followed
by a label definition, like ...

> +        . = ALIGN(PAGE_SIZE);
> +        __per_cpu_start = .;

... here.

> +        *(.bss.percpu.page_aligned)
> +        *(.bss.percpu)
> +        . = ALIGN(SMP_CACHE_BYTES);

This one is an exception, as it is intended to separate the read-mostly
part such that there's no shared cache line.

Jan

> +        *(.bss.percpu.read_mostly)
> +        . = ALIGN(SMP_CACHE_BYTES);
> +        __per_cpu_data_end = .;
> +        *(.bss .bss.*)
> +        . = ALIGN(POINTER_ALIGN);
> +        __bss_end = .;
> +    } :text
> +    _end = . ;
> +
> +    /* Section for the device tree blob (if any). */
> +    DECL_SECTION(.dtb) { *(.dtb) } :text
> +
> +    DWARF2_DEBUG_SECTIONS
> +
> +    DISCARD_SECTIONS
> +
> +    STABS_DEBUG_SECTIONS
> +
> +    ELF_DETAILS_SECTIONS
> +}
Re: [PATCH v3 2/4] xen: Add files needed for minimal ppc64le build
Posted by Shawn Anastasio 1 year, 5 months ago
On Wed Jun 14, 2023 at 10:51 AM CDT, Jan Beulich wrote:
> On 13.06.2023 16:50, Shawn Anastasio wrote:
> > --- /dev/null
> > +++ b/xen/arch/ppc/Makefile
> > @@ -0,0 +1,16 @@
> > +obj-$(CONFIG_PPC64) += ppc64/
> > +
> > +$(TARGET): $(TARGET)-syms
> > +	cp -f $< $@
> > +
> > +$(TARGET)-syms: $(objtree)/prelink.o $(obj)/xen.lds
> > +	$(LD) $(XEN_LDFLAGS) -T $(obj)/xen.lds -N $< $(build_id_linker) -o $@
> > +	$(NM) -pa --format=sysv $(@D)/$(@F) \
> > +		| $(objtree)/tools/symbols --all-symbols --xensyms --sysv --sort \
> > +		>$(@D)/$(@F).map
>
> Elsewhere we recently switched these uses of $(@D)/$(@F) to just $@.
> Please can you do so here as well?

Sure, will fix in v4.

> > --- /dev/null
> > +++ b/xen/arch/ppc/arch.mk
> > @@ -0,0 +1,11 @@
> > +########################################
> > +# Power-specific definitions
> > +
> > +ppc-march-$(CONFIG_POWER_ISA_2_07B) := power8
> > +ppc-march-$(CONFIG_POWER_ISA_3_00) := power9
> > +
> > +CFLAGS += -mcpu=$(ppc-march-y) -mstrict-align -mcmodel=large -mabi=elfv2 -mno-altivec -mno-vsx
>
> Wouldn't it make sense to also pass -mlittle here, such that a tool
> chain defaulting to big-endian can still be used?

Good call. On this topic, I suppose I'll also add -m64 to allow 32-bit
toolchains to be used as well.

> > --- /dev/null
> > +++ b/xen/arch/ppc/ppc64/head.S
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +.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.
> > +     */
> > +    tdi 0, 0, 0x48    /* Reverse endian of b . + 8          */
> > +    b $ + 44          /* Skip trampoline if endian is good  */
>
> If I get this right, $ and . are interchangable on Power? If not,
> then all is fine and there likely is a reason to use . in the
> comment but $ in the code. But if so, it would be nice if both
> could match, and I guess with other architectures in mind . would
> be preferable.

As hinted by the comment, this code was directly inherited from Linux
and I'm not sure why the original author chose '$' instead of '.'. That
said, as far as I can tell you are correct about the two being
interchangeable, and changing the $ to . results in the exact same
machine code.

I can go ahead and make the change for consistency in v4.

> > +    DECL_SECTION(.bss) {                     /* BSS */
> > +        __bss_start = .;
> > +        *(.bss.stack_aligned)
> > +        . = ALIGN(PAGE_SIZE);
> > +        *(.bss.page_aligned)
>
> ... the one between the two .bss parts looks unmotivated. Within
> a section definition ALIGN() typically only makes sense when followed
> by a label definition, like ...

Correct me if I'm wrong, but wouldn't the ALIGN here serve to ensure
that the subsequent '.bss.page_aligned' section has the correct alignment
that its name implies?

> Jan

Thanks,
Shawn
Re: [PATCH v3 2/4] xen: Add files needed for minimal ppc64le build
Posted by Jan Beulich 1 year, 4 months ago
On 14.06.2023 18:36, Shawn Anastasio wrote:
> On Wed Jun 14, 2023 at 10:51 AM CDT, Jan Beulich wrote:
>> On 13.06.2023 16:50, Shawn Anastasio wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/ppc/arch.mk
>>> @@ -0,0 +1,11 @@
>>> +########################################
>>> +# Power-specific definitions
>>> +
>>> +ppc-march-$(CONFIG_POWER_ISA_2_07B) := power8
>>> +ppc-march-$(CONFIG_POWER_ISA_3_00) := power9
>>> +
>>> +CFLAGS += -mcpu=$(ppc-march-y) -mstrict-align -mcmodel=large -mabi=elfv2 -mno-altivec -mno-vsx
>>
>> Wouldn't it make sense to also pass -mlittle here, such that a tool
>> chain defaulting to big-endian can still be used?
> 
> Good call. On this topic, I suppose I'll also add -m64 to allow 32-bit
> toolchains to be used as well.

Turns out this isn't quite enough. When trying to test my little bit of
re-basing of Anthony's series, I ran into ld complaining about little
endian input when the target format is big endian (like for the compiler
I'm using a default configured, i.e. big-endian, binutils build). Looks
like we need to pass "-m elf64lppc" to the linker; I'll see if that
helps (and where exactly to put it).

Jan
Re: [PATCH v3 2/4] xen: Add files needed for minimal ppc64le build
Posted by Jan Beulich 1 year, 5 months ago
On 14.06.2023 18:36, Shawn Anastasio wrote:
> On Wed Jun 14, 2023 at 10:51 AM CDT, Jan Beulich wrote:
>> On 13.06.2023 16:50, Shawn Anastasio wrote:
>>> +    DECL_SECTION(.bss) {                     /* BSS */
>>> +        __bss_start = .;
>>> +        *(.bss.stack_aligned)
>>> +        . = ALIGN(PAGE_SIZE);
>>> +        *(.bss.page_aligned)
>>
>> ... the one between the two .bss parts looks unmotivated. Within
>> a section definition ALIGN() typically only makes sense when followed
>> by a label definition, like ...
> 
> Correct me if I'm wrong, but wouldn't the ALIGN here serve to ensure
> that the subsequent '.bss.page_aligned' section has the correct alignment
> that its name implies?

Yes and no. Thing is that every contribution to .bss.page_aligned already
needs to specify page alignment itself, or else it may break if any earlier
contribution was page-aligned, but not a full page in size (which I think
the compiler wouldn't allow to happen, but assembly code can result in
such). Note how this very ALIGN() was recently dropped from RISC-V code,
and my respective Arm side patch is also about to go in.

Jan
Re: [PATCH v3 2/4] xen: Add files needed for minimal ppc64le build
Posted by Shawn Anastasio 1 year, 5 months ago
On Thu Jun 15, 2023 at 1:47 AM CDT, Jan Beulich wrote:
> On 14.06.2023 18:36, Shawn Anastasio wrote:
> > On Wed Jun 14, 2023 at 10:51 AM CDT, Jan Beulich wrote:
> >> On 13.06.2023 16:50, Shawn Anastasio wrote:
> >>> +    DECL_SECTION(.bss) {                     /* BSS */
> >>> +        __bss_start = .;
> >>> +        *(.bss.stack_aligned)
> >>> +        . = ALIGN(PAGE_SIZE);
> >>> +        *(.bss.page_aligned)
> >>
> >> ... the one between the two .bss parts looks unmotivated. Within
> >> a section definition ALIGN() typically only makes sense when followed
> >> by a label definition, like ...
> > 
> > Correct me if I'm wrong, but wouldn't the ALIGN here serve to ensure
> > that the subsequent '.bss.page_aligned' section has the correct alignment
> > that its name implies?
>
> Yes and no. Thing is that every contribution to .bss.page_aligned already
> needs to specify page alignment itself, or else it may break if any earlier
> contribution was page-aligned, but not a full page in size (which I think
> the compiler wouldn't allow to happen, but assembly code can result in
> such). Note how this very ALIGN() was recently dropped from RISC-V code,
> and my respective Arm side patch is also about to go in.

That makes sense, thanks. I'll get rid of the ALIGN here in v4.

> Jan

Thanks,
Shawn