Re: [Minios-devel] [UNIKRAFT PATCH v2 8/9] plat/common: Add functionality to save and restore extended (x86) registers

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

On 12/19/18 3:58 PM, Yuri Volchkov wrote:
> Hi,
>
> see my notes inline
>
> -Yuri.
>
> Florian Schmidt <florian.schmidt@xxxxxxxxx> writes:
>
> > On creation of a sw_ctx struct, allocate an area sufficiently large to
> > save all extended registers. On context switch, use the appropriate CPU
> > instruction to save and restore those registers to/from that area.
> >
> > Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> > ---
> >   plat/common/include/sw_ctx.h  |  9 +++++--
> >   plat/common/include/x86/cpu.h | 49 +++++++++++++++++++++++++++++++++++
> >   plat/common/sw_ctx.c          | 14 ++++++++--
> >   3 files changed, 68 insertions(+), 4 deletions(-)
> >
> > diff --git a/plat/common/include/sw_ctx.h b/plat/common/include/sw_ctx.h
> > index fae96beb..1b279b25 100644
> > --- a/plat/common/include/sw_ctx.h
> > +++ b/plat/common/include/sw_ctx.h
> > @@ -38,8 +38,13 @@
> >   #include <uk/plat/thread.h>
> >   
> >   struct sw_ctx {
> > -       unsigned long sp;  /* Stack pointer */
> > -       unsigned long ip;  /* Instruction pointer */
> > +       unsigned long sp;               /* Stack pointer */
> > +       unsigned long ip;               /* Instruction pointer */
> > +       unsigned char extregs[];        /* NB, this will typically NOT point to
> > +                                        * the beginning of the extregs area,
> > +                                        * because the extregs area needs to
> > +                                        * be aligned.
> > +                                        */
> You can easily achieve that extregs always pointing to the begining of
> extregs area. If extregs is a pointer, you can just assign it the
> address of this area. E.g.
>
>         ctx = uk_malloc(allocator, sz);
>          ctx.extregs = ALIGN_UP(((uintptr_t)ctx + sizeof(struct sw_ctx)),
>                                 x86_cpu_features.extregs_align);
Hm, yeah, I might do that. I honestly can't remember whether I did it 
like this originally. Your suggestion seems simpler, and probably ever 
so slightly more efficient, because we don't need to calculate the start 
of the memory area on each {save,restore}_extregs. I think the only 
downside to the pointer way is that we "waste" another sizeof(uintptr_t) 
bytes in the struct. However, due to the alignment requirements (64 
bytes for modern CPUs), this memory area is unused anyway. So I'm gonna 
try out this solution and go with it if I don't see any other issue.
> >   };
> >   
> >   void sw_ctx_callbacks_init(struct ukplat_ctx_callbacks *ctx_cbs);
> > diff --git a/plat/common/include/x86/cpu.h b/plat/common/include/x86/cpu.h
> > index fbc229d9..f2a8f0d5 100644
> > --- a/plat/common/include/x86/cpu.h
> > +++ b/plat/common/include/x86/cpu.h
> > @@ -32,6 +32,7 @@
> >   
> >   #include <uk/arch/types.h>
> >   #include <x86/cpu_defs.h>
> > +#include <sw_ctx.h>
> >   #include <stdint.h>
> >   
> >   void halt(void);
> > @@ -55,6 +56,54 @@ struct _x86_features {
> >   
> >   extern struct _x86_features x86_cpu_features;
> >   
> > +static inline void save_extregs(struct sw_ctx *ctx)
> > +{
> > +       uintptr_t r = ALIGN_UP(((uintptr_t)ctx + sizeof(struct sw_ctx)),
> > +                               x86_cpu_features.extregs_align);
> > +
> > +       switch (x86_cpu_features.save) {
> > +       case X86_SAVE_NONE:
> > +               /* nothing to do */
> > +               break;
> > +       case X86_SAVE_FSAVE:
> > +               asm volatile("fsave (%0)" :: "r"(r) : "memory");
> > +               break;
> > +       case X86_SAVE_FXSAVE:
> > +               asm volatile("fxsave (%0)" :: "r"(r) : "memory");
> > +               break;
> > +       case X86_SAVE_XSAVE:
> > +               asm volatile("xsave (%0)" :: "r"(r), "a"(0xffffffff),
> > +                               "d"(0xffffffff) : "memory");
> > +               break;
> > +       case X86_SAVE_XSAVEOPT:
> > +               asm volatile("xsaveopt (%0)" :: "r"(r), "a"(0xffffffff),
> > +                               "d"(0xffffffff) : "memory");
> > +               break;
> > +       }
> > +}
> > +static inline void restore_extregs(struct sw_ctx *ctx)
> > +{
> > +       uintptr_t r = ALIGN_UP(((uintptr_t)ctx + sizeof(struct sw_ctx)),
> > +                               x86_cpu_features.extregs_align);
> > +
> > +       switch (x86_cpu_features.save) {
> > +       case X86_SAVE_NONE:
> > +               /* nothing to do */
> > +               break;
> > +       case X86_SAVE_FSAVE:
> > +               asm volatile("frstor (%0)" :: "r"(r));
> > +               break;
> > +       case X86_SAVE_FXSAVE:
> > +               asm volatile("fxrstor (%0)" :: "r"(r));
> > +               break;
> > +       case X86_SAVE_XSAVE:
> > +       case X86_SAVE_XSAVEOPT:
> > +               asm volatile("xrstor (%0)" :: "r"(r), "a"(0xffffffff),
> > +                               "d"(0xffffffff));
> > +               break;
> > +       }
> > +}
> > +
> >   static inline void _init_cpufeatures(void)
> >   {
> >   #if LINUXUPLAT
> > diff --git a/plat/common/sw_ctx.c b/plat/common/sw_ctx.c
> > index a477753b..79935776 100644
> > --- a/plat/common/sw_ctx.c
> > +++ b/plat/common/sw_ctx.c
> > @@ -37,7 +37,7 @@
> >   #include <uk/alloc.h>
> >   #include <sw_ctx.h>
> >   #include <uk/assert.h>
> > -
> > +#include <x86/cpu.h>
> >   
> >   static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp);
> >   static void  sw_ctx_start(void *ctx) __noreturn;
> > @@ -52,10 +52,14 @@ extern void asm_thread_starter(void);
> >   static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp)
> >   {
> >         struct sw_ctx *ctx;
> > +       size_t sz;
> >   
> >   	UK_ASSERT(allocator != NULL);
> >   
> > -	ctx = uk_malloc(allocator, sizeof(struct sw_ctx));
> > +       sz = ALIGN_UP(sizeof(struct sw_ctx), x86_cpu_features.extregs_align)
> > +               + x86_cpu_features.extregs_size;
> > +       ctx = uk_malloc(allocator, sz);
> > +       uk_pr_debug("Allocating %lu bytes for sw ctx at %p\n", sz, ctx);
> >         if (ctx == NULL) {
> >                 uk_pr_warn("Error allocating software context.");
> >                 return NULL;
> > @@ -63,6 +67,7 @@ static void *sw_ctx_create(struct uk_alloc *allocator, 
> > unsigned long sp)
> >   
> >   	ctx->sp = sp;
> >         ctx->ip = (unsigned long) asm_thread_starter;
> > +       save_extregs(ctx);
> Maybe a small note why we are saving registers without actual context
> switch here? So people like me get less confusion :)
Will do.

> >   
> >   	return ctx;
> >   }
> > @@ -85,6 +90,11 @@ extern void asm_sw_ctx_switch(void *prevctx, void *nextctx);
> >   
> >   static void sw_ctx_switch(void *prevctx, void *nextctx)
> >   {
> > +       struct sw_ctx *p = prevctx;
> > +       struct sw_ctx *n = nextctx;
> > +
> > +       save_extregs(p);
> > +       restore_extregs(n);
> >         asm_sw_ctx_switch(prevctx, nextctx);
> >   }
> >   
> > --
> > 2.19.2
--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

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