Re: [Minios-devel] [UNIKRAFT PATCH 2/3] plat/common: Implement a few extra registers stub helpers on arm64

["Justin He (Arm Technology China)" <Justin.He@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2019年9月26日 17:10
> To: Justin He (Arm Technology China) <Justin.He@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx; Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Cc: Sharan.Santhanam@xxxxxxxxx; Felipe Huici <felipe.huici@xxxxxxxxx>;
> Kaly Xin (Arm Technology China) <Kaly.Xin@xxxxxxx>;
> Santiago.Pagani@xxxxxxxxx
> Subject: Re: [UNIKRAFT PATCH 2/3] plat/common: Implement a few extra
> registers stub helpers on arm64
>
> Hi,
>
> On 9/26/19 9:20 AM, Jia He wrote:
> > On arm64, we don't need the extra registers during context switching so
> > far. This patch decouple the arch specific structures/functions info arch
> > related files.
>
> AFAICT, the patch adding x86 header in sw_ctx.c were committed last
> January. It would be good to understand how this is suddently an issue.
>
> If this is because of one series not yet merged, then it should be
> squashed in it. If not, an explanation in the commit message to explain
> the exact problem (possibly compilation issue?) is the best.

Sure, I will explain it in more detail.
>
> >
> > Signed-off-by: Jia He <justin.he@xxxxxxx>
> > ---
> >   plat/common/include/arm/arm64/cpu.h | 25
> +++++++++++++++++++++++++
> >   plat/common/include/x86/cpu.h       | 10 ++++++++++
> >   plat/common/sw_ctx.c                | 10 +++++-----
> >   3 files changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/plat/common/include/arm/arm64/cpu.h
> b/plat/common/include/arm/arm64/cpu.h
> > index 1495192..298424e 100644
> > --- a/plat/common/include/arm/arm64/cpu.h
> > +++ b/plat/common/include/arm/arm64/cpu.h
> > @@ -32,7 +32,12 @@
> >    * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> >    */
> >
> > +#ifndef __PLAT_COMMON_ARM64_CPU_H__
> > +#define __PLAT_COMMON_ARM64_CPU_H__
>
> TBH, this belongs to a separate patch with ...
Okay, I will isolate this part as another patch
>
> > +
> >   #include <inttypes.h>
> > +#include <uk/essentials.h>
> > +#include <sw_ctx.h>
> >
> >   /* Define macros to access IO registers */
> >   #define __IOREG_READ(bits) \
> > @@ -108,3 +113,23 @@ int32_t smcc_psci_smc_call(uint32_t, uint64_t,
> uint64_t, uint64_t);
> >   void halt(void);
> >   void reset(void);
> >   void system_off(void);
> > +
> > +static inline unsigned long get_extregs_align(void)
> > +{
> > +   return 1;
> > +}
> > +
> > +static inline unsigned long get_extregs_size(void)
> > +{
> > +   return 0;
> > +}
> > +
> > +static inline void save_extregs(struct sw_ctx *ctx __unused)
> > +{
> > +}
> > +
> > +static inline void restore_extregs(struct sw_ctx *ctx __unused)
> > +{
> > +}
> > +
> > +#endif /* __PLAT_COMMON_ARM64_CPU_H__ */
> > diff --git a/plat/common/include/x86/cpu.h
> b/plat/common/include/x86/cpu.h
> > index 8acd71e..838bcea 100644
> > --- a/plat/common/include/x86/cpu.h
> > +++ b/plat/common/include/x86/cpu.h
> > @@ -56,6 +56,16 @@ struct _x86_features {
> >
> >   extern struct _x86_features x86_cpu_features;
> >
> > +static inline unsigned long get_extregs_align(void)
> > +{
> > +   return x86_cpu_features.extregs_align;
> > +}
> > +
> > +static inline unsigned long get_extregs_size(void)
> > +{
> > +   return x86_cpu_features.extregs_size;
> > +}
> > +
> >   static inline void save_extregs(struct sw_ctx *ctx)
> >   {
> >     switch (x86_cpu_features.save) {
> > diff --git a/plat/common/sw_ctx.c b/plat/common/sw_ctx.c
> > index 88a377f..74b935c 100644
> > --- a/plat/common/sw_ctx.c
> > +++ b/plat/common/sw_ctx.c
> > @@ -40,7 +40,7 @@
> >   #include <sw_ctx.h>
> >   #include <uk/assert.h>
> >   #include <tls.h>
> > -#include <x86/cpu.h>
> > +#include <cpu.h>
> >
> >   static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp,
> >                             unsigned long tlsp);
> > @@ -61,8 +61,8 @@ static void *sw_ctx_create(struct uk_alloc *allocator,
> unsigned long sp,
> >
> >     UK_ASSERT(allocator != NULL);
> >
> > -   sz = ALIGN_UP(sizeof(struct sw_ctx), x86_cpu_features.extregs_align)
> > -           + x86_cpu_features.extregs_size;
> > +   sz = ALIGN_UP(sizeof(struct sw_ctx), get_extregs_align())
> > +           + get_extregs_size();
> >     ctx = uk_malloc(allocator, sz);
> >     uk_pr_debug("Allocating %lu bytes for sw ctx at %p\n", sz, ctx);
> >     if (ctx == NULL) {
> > @@ -74,9 +74,9 @@ static void *sw_ctx_create(struct uk_alloc *allocator,
> unsigned long sp,
> >     ctx->tlsp = tlsp;
> >     ctx->ip = (unsigned long) asm_thread_starter;
> >     ctx->extregs = ALIGN_UP(((uintptr_t)ctx + sizeof(struct sw_ctx)),
> > -                           x86_cpu_features.extregs_align);
> > +                           get_extregs_align());
> >     // Initialize extregs area: zero out, then save a valid layout to it.
> > -   memset((void *)ctx->extregs, 0, x86_cpu_features.extregs_size);
> > +   memset((void *)ctx->extregs, 0, get_extregs_size());
>
> This is a bit a waste for Arm and also quite dangerous because extregs
> is not going to be NULL but pointing just after the end of the allocated
> space.
>
> Couldn't we try to abstract it further? Maybe by providing a function
> that will allocate the structure and initialize arch specific things?
> Something like:
>
> ctx = arch_alloc_ctx(allocator);
>
> where arch_alloc_ctx for x86 would be implemented as
>
> sz = ALIGN_UP(sizeof(struct sw_ctx), ...) + ...;
> ctx = uk_malloc(..., sz);
> ctx->extregs = ...
> memset(ctx->extregs, 0, ...)
> save_ctx_regs();
> return ctx;
>
> For arm, it would be:
>
> return uk_malloc(..., sizeof(struct sw_ctx));
>
Ok, I will try your suggestions.
Unfortunately, when I rebased my patches to latest unikraft tree. I met more
issues (at first glance not related to this patch series). I will take more 
time to
dig into it.

--
Cheers,
Justin (Jia He)


>
> Cheers,
>
> --
> Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel