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

[Jan Beulich <jbeulich@xxxxxxxx>]

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
> +}