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

["Shawn Anastasio" <shawn@xxxxxxxxxx>]

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