Re: [Minios-devel] [UNIKRAFT PATCH] plat: Configure stack size page order

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 04.09.19 09:15, Costin Lupu wrote:
> Hi Simon,
>
> Please see inline.
>
> On 8/30/19 4:51 PM, Simon Kuenzer wrote:
> > Hey Costin,
> >
> > Thanks a lot for this patch. I am currently having a look but need some
> > clarifications.
> >
> > 1) Why did you expose the option int the platform submenu? It is
> > obviously changing something in architecture headers?
> Well, actually it is neither. I think it's actually a scheduling
> abstraction, but one that is currently used for other stacks as well
> (interrupts, traps). So it's true that it's weird to put it here, but
> it's not an arch specific config either. This stack size value should be
> the same regardless any arch or platform.
I completely agree with this and I think we are going to introduce 
supporting different stack sizes later. The whole discussion is related 
what we had with patch ID 735752 (see patchwork.unikraft.org). Although 
this patch is also an intermediate fix, I tend to put this option 
directly under 'Architecture Selection' because it should be independent 
to any platform. The other reason is that we have the stack size 
currently defined within the arch headers.
> > 2) Did you check the interrupt stack for Xen on x86? It seems that this
> > one is just sized to PAGE_SIZE. I think this can get critical for thread
> > current retrieval, right? See: plat/xen/x86/arch_events.c and
> > plat/xen/x86/traps.c .
> Well actually I see that the interrupt stack in
> plat/xen/x86/arch_events.c has the right size, STACK_SIZE.
>
> In deed, the trap stack in plat/xen/x86/traps.c is PAGE_SIZE and it
> should be fixed.
>
> > Do you by chance remember why we have the boot stack twice as big?
> > See: xen/x86/setup.c
> I don't remember, but it was the same with the interrupt stack in
> plat/xen/x86/arch_events.c because the alignment was done at runtime. It
> might be the same reason.
I understand. Probably also something to re-visit later but not with 
this patch...
> > 3) More as a note: Xen on Arm32 seems not to follow any STACK_SIZE
> > definition at all. We should probably put a note on this somewhere. I am
> > not sure if it is worth fixing it - who knows what we are going to do
> > with this architecture-platform-combination. I rather expect that we are
> > going towards Arm64 for Xen in the future.
> I'm not sure I follow. This patch fixes that and sets the same stack
> size for ARM.
Arm32 uses some internal hard-coded values and does not use the 
STACK_SIZE definition. I think we should adopt this with this patch, too.
In general, I am fine with introducing this stack size configuration as 
option.
Thanks,

Simon

> > Thanks,
> >
> > Simon
> >
> > On 27.08.19 09:56, Costin Lupu wrote:
> > > This patch adds a config option for configuring the stack size page
> > > order. We
> > > need this for supporting large stacks.
> > >
> > > Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> > > ---
> > >    arch/arm/arm/include/uk/asm/limits.h    | 2 +-
> > >    arch/arm/arm64/include/uk/asm/limits.h  | 2 +-
> > >    arch/x86/x86_64/include/uk/asm/limits.h | 2 +-
> > >    plat/Config.uk                          | 9 +++++++++
> > >    4 files changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm/arm/include/uk/asm/limits.h
> > > b/arch/arm/arm/include/uk/asm/limits.h
> > > index 085761c3..e2298d6b 100644
> > > --- a/arch/arm/arm/include/uk/asm/limits.h
> > > +++ b/arch/arm/arm/include/uk/asm/limits.h
> > > @@ -39,7 +39,7 @@
> > >    #define __PAGE_MASK        (~((__PAGE_SIZE) - 1))
> > >    #endif
> > >    -#define __STACK_SIZE_PAGE_ORDER    2
> > > +#define __STACK_SIZE_PAGE_ORDER    CONFIG_STACK_SIZE_PAGE_ORDER
> > >    #define __STACK_SIZE        (__PAGE_SIZE * (1 <<
> > > __STACK_SIZE_PAGE_ORDER))
> > >      #define __WORDSIZE        32
> > > diff --git a/arch/arm/arm64/include/uk/asm/limits.h
> > > b/arch/arm/arm64/include/uk/asm/limits.h
> > > index cec05641..fb70f2ba 100644
> > > --- a/arch/arm/arm64/include/uk/asm/limits.h
> > > +++ b/arch/arm/arm64/include/uk/asm/limits.h
> > > @@ -40,7 +40,7 @@
> > >    #define __PAGE_MASK        (~((__PAGE_SIZE) - 1))
> > >    #endif
> > >    -#define __STACK_SIZE_PAGE_ORDER    4
> > > +#define __STACK_SIZE_PAGE_ORDER    CONFIG_STACK_SIZE_PAGE_ORDER
> > >    #define __STACK_SIZE    (__PAGE_SIZE * (1 << __STACK_SIZE_PAGE_ORDER))
> > >    #define __STACK_ALIGN_SIZE    16
> > >    diff --git a/arch/x86/x86_64/include/uk/asm/limits.h
> > > b/arch/x86/x86_64/include/uk/asm/limits.h
> > > index a969bd17..21814044 100644
> > > --- a/arch/x86/x86_64/include/uk/asm/limits.h
> > > +++ b/arch/x86/x86_64/include/uk/asm/limits.h
> > > @@ -39,7 +39,7 @@
> > >    #define __PAGE_MASK        (~((__PAGE_SIZE) - 1))
> > >    #endif
> > >    -#define __STACK_SIZE_PAGE_ORDER    4
> > > +#define __STACK_SIZE_PAGE_ORDER    CONFIG_STACK_SIZE_PAGE_ORDER
> > >    #define __STACK_SIZE        (__PAGE_SIZE * (1 <<
> > > __STACK_SIZE_PAGE_ORDER))
> > >      #define __WORDSIZE        64
> > > diff --git a/plat/Config.uk b/plat/Config.uk
> > > index 8a878eb0..d0b99bd5 100644
> > > --- a/plat/Config.uk
> > > +++ b/plat/Config.uk
> > > @@ -25,3 +25,12 @@ config HZ
> > >        help
> > >            Configure the timer interrupt frequency.
> > >            Only change this if you know what you're doing.
> > > +
> > > +config STACK_SIZE_PAGE_ORDER
> > > +    int
> > > +    prompt "Stack size page order"
> > > +    default 4
> > > +    help
> > > +        Indirectly configures the stack size by changing the stack
> > > size page
> > > +        order. Stack size is equal with 2^order * page size (e.g. 4KB).
> > > +        Only change this if you know what you're doing.
> > _______________________________________________
> > Minios-devel mailing list
> > Minios-devel@xxxxxxxxxxxxxxxxxxxx
> > https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel