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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> 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 ...

> +
>   #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));


Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel