Re: [PATCH 2/3] xen/ppc: Relocate kernel to physical address 0 on boot

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 7/31/23 10:46 AM, Jan Beulich wrote:
> On 29.07.2023 00:21, Shawn Anastasio wrote:
>> Introduce a small assembly loop in `start` to copy the kernel to
>> physical address 0 before continuing. This ensures that the physical
>> address lines up with XEN_VIRT_START (0xc000000000000000) and allows us
>> to identity map the kernel when the MMU is set up in the next patch.
> 
> So PPC guarantees there's always a reasonable amount of memory at 0,
> and that's available for use?

Both Linux and FreeBSD rely on this being the case, so it's essentially
a de facto standard, though I'm not aware of any specification that
guarantees it.

>> --- a/xen/arch/ppc/ppc64/head.S
>> +++ b/xen/arch/ppc/ppc64/head.S
>> @@ -18,6 +18,33 @@ ENTRY(start)
>>      addis   %r2, %r12, .TOC.-1b@ha
>>      addi    %r2, %r2, .TOC.-1b@l
>>  
>> +    /*
>> +     * Copy Xen to physical address zero and jump to XEN_VIRT_START
>> +     * (0xc000000000000000). This works because the hardware will ignore 
>> the top
>> +     * four address bits when the MMU is off.
>> +     */
>> +    LOAD_REG_ADDR(%r1, start)
> 
> I think you really mean _start here (which is missing from the linker
> script),

The PIC patch series fixes the missing _start definition in the linker
script. In the cover letter of v2 I'll add a clear note that this series
is based on that one.

> not start. See also Andrew's recent related RISC-V change.

Good point. In practice this worked because the `start` function was the
first thing in the first section of the linker script, but of course
using _start here is more correct.

> 
>> +    LOAD_IMM64(%r12, XEN_VIRT_START)
>> +
>> +    /* If we're at the correct address, skip copy */
>> +    cmpld   %r1, %r12
>> +    beq     .L_correct_address
> 
> Can this ever be the case, especially with the MMU-off behavior you
> describe in the comment above? Wouldn't you need to ignore the top
> four bits in the comparison?

It will always be the case after the code jumps to XEN_VIRT_START after
the copy takes place. I could have it jump past the copy loop entirely,
but then I'd need to duplicate the TOC setup.

>> +    /* Copy bytes until _end */
>> +    LOAD_REG_ADDR(%r11, _end)
>> +    addi    %r1, %r1, -8
>> +    li      %r13, -8
>> +.L_copy_xen:
>> +    ldu     %r10, 8(%r1)
>> +    stdu    %r10, 8(%r13)
>> +    cmpld   %r1, %r11
>> +    blt     .L_copy_xen
>> +
>> +    /* Jump to XEN_VIRT_START */
>> +    mtctr   %r12
>> +    bctr
>> +.L_correct_address:
> 
> Can the two regions potentially overlap? Looking at the ELF header
> it's not clear to me what guarantees there are that this can't
> happen.

As I understand it, any bootloader that placed the kernel at a low
enough address for this to be an issue wouldn't be able to boot Linux or
FreeBSD, so in practice it's a safe bet that this won't be the case.

> Jan

Thanks,
Shawn