Re: [PATCH v2 1/3] xen/ppc: Set up a basic C environment

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 6/22/23 8:26 PM, Shawn Anastasio wrote:
> On 6/22/23 5:49 PM, Andrew Cooper wrote:
>> On 22/06/2023 9:57 pm, Shawn Anastasio wrote:
>>> 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@xxxxxxxxxxxxxxxxxxxxx>
>>
>> Thankyou for making this change - it definitely simplifies things.
> 
> No problem.
> 
>> I've done a bit of reading around, and it's rather sad that things prior
>> to Power10 don't have PC-relative addressing.  So the LOAD_IMM64()'s do
>> look necessary for the stack and bss.  I guess that means we can't be
>> sensibly be position independent in head.S?
> 
> Prior to the introduction of pcrel loads/stores in P10, PIC is achieved
> using a Table of Contents (TOC) whose base address is kept in r2 and can
> be used as a relative base to address other symbols. I don't have -fPIC
> enabled in this series yet (it's in the series I was hoping to submit
> after this one), so for now I'm just loading the addresses as immediates
> directly.
> 
>> Also, why store 0 onto the stack ?
> 
> On the ELFv2 ABI which we're using here, sp+0 is reserved for the "back
> chain" pointer which is used to store the address of the caller's stack
> frame and is used to support backtraces.
> 
> At the top of the stack, we need to make sure the first back chain
> pointer is zero to ensure that anything walking the stack via these
> pointers eventually terminates.
> 
>>> +
>>> +    /* clear .bss */
>>> +    LOAD_IMM64(%r14, __bss_start)
>>> +    LOAD_IMM64(%r15, __bss_end)
>>> +1:
>>> +    std %r11, 0(%r14)
>>> +    addi %r14, %r14, 8
>>> +    cmpld %r14, %r15
>>> +    blt 1b
>>
>> This loop is correct, except for the corner case of this patch alone,
>> where the BSS is empty and this will write one word out-of-bounds.
>>
>> For RISC-V, we put a temporary "char bss_tmp;" in setup.c, and I suggest
>> you do the same here, deleting it at a later point when there's real
>> data in the bss.
> 
> Good catch. I actually introduce a .bss variable in patch 2 of this
> series, so perhaps it would make the most sense for me to move this loop
> to that patch?
> 
> Also it might make sense to have an assert in the linker script checking
> that sizeof(.bss) > 0, though past early bring-up an empty .bss is
> probably a pretty unlikely scenario...
> 
>>> +
>>> +    /* call the C entrypoint */
>>> +    LOAD_IMM64(%r12, start_xen)
>>> +    mtctr %r12
>>> +    bctrl
>>
>> Why is this a LOAD_IMM64(), and not just:
>>
>>     b start_xen
>>
>> ?  From the same reading around, PPC64 seems to have +/- 32M addressing
>> for branches, and the entire x86 hypervisor (.init included) is within 3M.
> 
> Good question. You're right that here it's entirely unnecessary. Once we
> enable -fPIC, though, the calling convention for functions changes a bit
> and necessitates that in certain scenarios r12 contains the entrypoint
> of the function being called.

Turns out I was actually wrong here -- changing the indirect load +
branch to a direct branch does actually break the code here, but for a
reason other than what I anticipated. Even with PIC disabled, r2 needs
to contain a valid TOC pointer. The function address doesn't need to be
contained in r12 to calculate it since it's an absolute address rather
than function-relative, but using a simple direct branch here causes the
linker to skip past the TOC calculation prologue in `start_xen` as an
optimization, since it assumes that r2 is already set up. Since we
haven't set up r2, though, this results in the program using a
garbage TOC pointer at run-time.

I'll just set up the TOC before making the call in `start` to fix this.

Thanks,
Shawn