Re: [PATCH v2 2/2] xen/ppc: Implement a basic exception handler

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 10/20/23 1:22 AM, Jan Beulich wrote:
> On 19.10.2023 22:02, Shawn Anastasio wrote:
>> On 10/18/23 10:43 AM, Jan Beulich wrote:
>>> On 13.10.2023 20:13, Shawn Anastasio wrote:
>>>> --- a/xen/arch/ppc/setup.c
>>>> +++ b/xen/arch/ppc/setup.c
>>>> @@ -11,6 +11,15 @@
>>>>  /* Xen stack for bringing up the first CPU. */
>>>>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE] 
>>>> __aligned(STACK_SIZE);
>>>>
>>>> +void setup_exceptions(void)
>>>> +{
>>>> +    unsigned long lpcr;
>>>> +
>>>> +    /* Set appropriate interrupt location in LPCR */
>>>> +    lpcr = mfspr(SPRN_LPCR);
>>>> +    mtspr(SPRN_LPCR, lpcr | LPCR_AIL_3);
>>>> +}
>>>
>>> Please forgive my lack of PPC knowledge: There's no use of any address
>>> here afaict. Where's the link to (presumably) AIL_VECTOR_BASE? If that
>>> address is kind of magic, I'm then lacking a connection between
>>> XEN_VIRT_START and that constant. (If Xen needed moving around in
>>> address space, it would be nice if changing a single constant would
>>> suffice.)
>>>
>>
>> AIL_VECTOR_BASE is indeed a magic address defined by the ISA for AIL=3.
>> As for the second part of your question, I'm a bit confused as to what
>> you're asking. The ISRs are placed at a position relative to
>> the start of the .text.exceptions section (EXCEPTION_VECTORS_START), so
>> Xen can be arbitrarily shuffled around in address space as long as
>> EXCEPTION_VECTORS_START lies at or before AIL_VECTOR_BASE.
> 
> Well, AIL_VECTOR_BASE is #define-d to a plain constant, not derived
> from EXCEPTION_VECTORS_START. In turn EXCEPTION_VECTORS_START is
> #define-d to a plain constant in patch 1, not derived from
> XEN_VIRT_START. Therefore moving Xen around would require to change
> (at least) 3 #define-s afaict.
>

AIL_VECTOR_BASE needs to be a plain constant since it's fixed by the
ISA. EXCEPTION_VECTORS_START could be defined in terms of XEN_VIRT_START
I suppose, but that would require introducing another plain constant for
representing the offset of EXCEPTION_VECTORS_START from XEN_VIRT_START.

I think the current approach is reasonable, especially since patch 1
introduces a linker assertion that ensures that EXCEPTION_VECTORS_START
matches the actual location of _stext_exceptions, so if one was to
change Xen's address and forgot to change the #define they'd get a
build error.

If you would prefer this to be handled in a different way though, I'm
not opposed.

> Jan

Thanks,
Shawn