Re: [PATCH 2/3] xen/ppc: Add .text.exceptions section for exception vectors

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/29/23 4:28 AM, Andrew Cooper wrote:
> On 29/09/2023 12:19 am, Shawn Anastasio wrote:
>> On Power, the exception vectors must lie at a fixed address, depending
>> on the state of the Alternate Interrupt Location (AIL) field of the
>> Logical Partition Control Register (LPCR). Create a .text.exceptions
>> section in the linker script at an address suitable for AIL=3 plus an
>> accompanying assertion to pave the way for implementing exception
>> support.
> 
> Thanks - this is a perfect level of detail as far as I'm concerned as a
> PPC novice.
> 
>>
>> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>>  xen/arch/ppc/include/asm/config.h | 3 +++
>>  xen/arch/ppc/xen.lds.S            | 7 +++++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/xen/arch/ppc/include/asm/config.h 
>> b/xen/arch/ppc/include/asm/config.h
>> index a11a09c570..e012b75beb 100644
>> --- a/xen/arch/ppc/include/asm/config.h
>> +++ b/xen/arch/ppc/include/asm/config.h
>> @@ -42,6 +42,9 @@
>>  
>>  #define XEN_VIRT_START _AC(0xc000000000000000, UL)
>>  
>> +/* Fixed address for start of the section containing exception vectors */
>> +#define EXCEPTION_VECTORS_START _AC(0xc000000000000100, UL)
> 
> The patch looks fine, but a PPC question.  Does AIL=3 really mean a hard
> coded address at 0xc000000000000100 ?
> 
> Or is it +0x100 from something else that happens to be programmed to
> XEN_VIRT_START ?
> 

AIL=3 means a hardcoded address at 0xc000000000004000, actually, but by
placing the section earlier we can take advantage of some of the space
in between ...0100 and ...4000 for things like the {h_,}exception_common
routines introduced in the next patch instead of just having the linker
pad it with nops.

(Now that I look closely though, I see that I erroneously placed those
routines in .text rather than .text.exceptions in the next patch --
that's something I'll fix for v2).

>> +
>>  #define VMAP_VIRT_START (XEN_VIRT_START + GB(1))
>>  #define VMAP_VIRT_SIZE  GB(1)
>>  
>> diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S
>> index 9e46035155..9e888d7383 100644
>> --- a/xen/arch/ppc/xen.lds.S
>> +++ b/xen/arch/ppc/xen.lds.S
>> @@ -24,6 +24,10 @@ SECTIONS
>>          _stext = .;            /* Text section */
>>          *(.text.header)
>>  
>> +        . = ALIGN(256);
>> +        _stext_exceptions = .;
> 
> If this is really only used for the linker assertion, then it wants to be
> 
>     HIDDEN(_stext_exceptions = .);
> 
> otherwise the debugging symbols will have _stext_exceptions typically
> hiding exc_sysreset in the disassembly and symbol table.
>

It is indeed only used for the assertion -- I'll make this change in v2.

>> +        *(.text.exceptions)
>> +
>>          *(.text.cold)
>>          *(.text.unlikely .text.*_unlikely .text.unlikely.*)
>>  
>> @@ -184,3 +188,6 @@ ASSERT(IS_ALIGNED(__bss_end,        POINTER_ALIGN), 
>> "__bss_end is misaligned")
>>  
>>  ASSERT(!SIZEOF(.got),      ".got non-empty")
>>  ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
>> +
>> +ASSERT(_stext_exceptions == EXCEPTION_VECTORS_START, \
>> +       ".text.exceptions not at expected location -- .text.header too 
>> big?");
> 
> No need for ; at the end, and no need for \ either.
>

Will fix.

> As I said for patch 1, we're now at 4.18-rc1.   Does this need to be
> included now, or wait for 4.19?  There's something to be said for having
> a basic exception handler, but it is technically a new feature...
>

I don't think there's any pressing need to bring this into 4.18, unless
the burden of doing so is trivial.

> ~Andrew

Thanks,
Shawn