Re: [PATCH 3/3] xen/arm: stack check instrumentation

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 7/29/24 16:37, Julien Grall wrote:
> Hi,
> 
> On 29/07/2024 20:40, Stewart Hildebrand wrote:
>> On 7/29/24 14:50, Julien Grall wrote:
>>> Hi again,
>>>
>>> On 29/07/2024 15:24, Stewart Hildebrand wrote:
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index aac6c599f878..b4890eec7ec4 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -2325,6 +2325,51 @@ void asmlinkage leave_hypervisor_to_guest(void)
>>>>            arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
>>>>    }
>>>>    +#ifdef CONFIG_DEBUG
>>>> +DEFINE_PER_CPU(unsigned int, stack_check_nesting);
>>>> +DEFINE_PER_CPU(unsigned char *, stack_base);
>>>
>>> While looking at the code, I just realized that this should be equivalent 
>>> to current->arch.base.
>>
>> current->arch.stack
>>
>> That's true only after the idle vcpu stacks have been allocated.
>>
>>> So do we actually need stack_base?
>>
>> This is a way to enable stack instrumentation much earlier during boot
>> when we are still using the static boot stack.
> 
> Sure. But we are only partially checking the static boot stack.

The stack checking begins just a few lines into C world, after the
percpu_init_areas(), set_processor_id(), and stack_check_init() calls in
arch/arm/setup.c:start_xen().

> I am not entirely sure if there is any value for that because at that point 
> the stack would be barely used.

The entirety of start_xen() uses the boot stack, and it makes plenty of
device tree parsing calls (where there is recursion) and performs domain
creation, which hits the stack significantly.

arch/arm/dom0less-build.c: In function ‘construct_domU’:
arch/arm/dom0less-build.c:742:19: warning: stack usage is 7824 bytes 
[-Wstack-usage=]
  742 | static int __init construct_domU(struct domain *d,
      |                   ^~~~~~~~~~~~~~

arch/arm/domain_build.c: In function ‘make_memory_node’:
arch/arm/domain_build.c:788:12: warning: stack usage is 4720 bytes 
[-Wstack-usage=]
  788 | int __init make_memory_node(const struct kernel_info *kinfo, int 
addrcells,
      |            ^~~~~~~~~~~~~~~~

arch/arm/domain_build.c: In function ‘construct_dom0’:
arch/arm/domain_build.c:2120:19: warning: stack usage is 7776 bytes 
[-Wstack-usage=]
 2120 | static int __init construct_dom0(struct domain *d)
      |                   ^~~~~~~~~~~~~~

> 
> Anyway, this is only for debug build so far, so I am ok to keep it.
> 
> Cheers,
>