[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH 3/4] plat: switch thread-local storage area on context switch



Hi Florian,

Please see my comments inline.

On 4/15/19 1:03 PM, Florian Schmidt wrote:
> On x86, the standard way is writing to an MSR to switch the fs register,
> which is used as the pointer to the TLS area. However, this is a
> privileged instruction, so for the linuxu platform, use a system call
> provided by Linux that does exactly that instead.
> 
> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> ---
>  arch/x86/x86_64/include/uk/asm/thread.h     |  5 +++
>  include/uk/plat/thread.h                    |  8 ++--
>  lib/uksched/thread.c                        |  5 ++-
>  plat/common/include/sw_ctx.h                |  1 +
>  plat/common/include/thread_switch.h         | 43 +++++++++++++++++++
>  plat/common/include/x86/thread_switch.h     | 41 ++++++++++++++++++
>  plat/common/sw_ctx.c                        | 10 ++++-
>  plat/linuxu/include/linuxu/syscall-x86_64.h |  1 +
>  plat/linuxu/include/linuxu/syscall.h        |  7 ++++
>  plat/linuxu/include/thread_switch.h         | 46 +++++++++++++++++++++
>  10 files changed, 160 insertions(+), 7 deletions(-)
>  create mode 100644 plat/common/include/thread_switch.h
>  create mode 100644 plat/common/include/x86/thread_switch.h
>  create mode 100644 plat/linuxu/include/thread_switch.h
> 
> diff --git a/arch/x86/x86_64/include/uk/asm/thread.h 
> b/arch/x86/x86_64/include/uk/asm/thread.h
> index 6e95848d..f07eb521 100644
> --- a/arch/x86/x86_64/include/uk/asm/thread.h
> +++ b/arch/x86/x86_64/include/uk/asm/thread.h
> @@ -62,3 +62,8 @@ static inline void tls_copy(void *tls_area)
>       memset(tls_area+tls_data_len, 0, tls_bss_len);
>       *((uintptr_t *)(tls_area+tls_len)) = (uintptr_t)(tls_area+tls_len);
>  }
> +
> +static inline void *tls_pointer(void *tls_area)
> +{
> +     return tls_area + (_tls_end - _tls_start);
> +}
> diff --git a/include/uk/plat/thread.h b/include/uk/plat/thread.h
> index 69fc5e28..4b349ea7 100644
> --- a/include/uk/plat/thread.h
> +++ b/include/uk/plat/thread.h
> @@ -51,7 +51,8 @@ enum ukplat_ctx_type {
>  struct uk_alloc;
>  
>  typedef void *(*ukplat_ctx_create_func_t)
> -             (struct uk_alloc *allocator, unsigned long sp);
> +             (struct uk_alloc *allocator, unsigned long sp,
> +              unsigned long tlsp);
>  typedef void  (*ukplat_ctx_start_func_t)
>               (void *ctx);
>  typedef void  (*ukplat_ctx_switch_func_t)
> @@ -72,12 +73,13 @@ int ukplat_ctx_callbacks_init(struct ukplat_ctx_callbacks 
> *ctx_cbs,
>  
>  static inline
>  void *ukplat_thread_ctx_create(struct ukplat_ctx_callbacks *cbs,
> -             struct uk_alloc *allocator, unsigned long sp)
> +             struct uk_alloc *allocator, unsigned long sp,
> +             unsigned long tlsp)
>  {
>       UK_ASSERT(cbs != NULL);
>       UK_ASSERT(allocator != NULL);
>  
> -     return cbs->create_cb(allocator, sp);
> +     return cbs->create_cb(allocator, sp, tlsp);
>  }
>  
>  void ukplat_thread_ctx_destroy(struct uk_alloc *allocator, void *ctx);
> diff --git a/lib/uksched/thread.c b/lib/uksched/thread.c
> index a97fad9e..45f79206 100644
> --- a/lib/uksched/thread.c
> +++ b/lib/uksched/thread.c
> @@ -39,7 +39,7 @@
>  #include <uk/wait.h>
>  #include <uk/print.h>
>  #include <uk/assert.h>
> -
> +#include <uk/arch/thread.h>
>  
>  /* Pushes the specified value onto the stack of the specified thread */
>  static void stack_push(unsigned long *sp, unsigned long value)
> @@ -103,7 +103,8 @@ int uk_thread_init(struct uk_thread *thread,
>       init_sp(&sp, stack, function, arg);
>  
>       /* Call platform specific setup. */
> -     thread->ctx = ukplat_thread_ctx_create(cbs, allocator, sp);
> +     thread->ctx = ukplat_thread_ctx_create(cbs, allocator, sp,
> +                     (uintptr_t)tls_pointer(tls));
>       if (thread->ctx == NULL)
>               return -1;
>  
> diff --git a/plat/common/include/sw_ctx.h b/plat/common/include/sw_ctx.h
> index d52fe65b..366c6a8f 100644
> --- a/plat/common/include/sw_ctx.h
> +++ b/plat/common/include/sw_ctx.h
> @@ -41,6 +41,7 @@
>  struct sw_ctx {
>       unsigned long sp;       /* Stack pointer */
>       unsigned long ip;       /* Instruction pointer */
> +     unsigned long tlsp;     /* thread-local storage pointer */
>       uintptr_t extregs;      /* Pointer to an area to which extended
>                                * registers are saved on context switch.
>                                */
> diff --git a/plat/common/include/thread_switch.h 
> b/plat/common/include/thread_switch.h
> new file mode 100644
> index 00000000..b04c38c5
> --- /dev/null
> +++ b/plat/common/include/thread_switch.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS 
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +#ifndef __PLAT_CMN_THREAD_SWITCH_H__
> +#define __PLAT_CMN_THREAD_SWITCH_H__
> +
> +#if defined(__X86_64__)
> +#include <x86/thread_switch.h>
> +#else
> +#error "Add thread_switch.h for current architecture."
> +#endif
> +
> +#endif /* __PLAT_CMN_THREAD_SWITCH_H__ */
> diff --git a/plat/common/include/x86/thread_switch.h 
> b/plat/common/include/x86/thread_switch.h
> new file mode 100644
> index 00000000..f8252916
> --- /dev/null
> +++ b/plat/common/include/x86/thread_switch.h

For this file the location is correct, according to my comments for the
previous patch. However, I would use the same header for both sets of
changes and rename it to tls.h .

> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS 
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +#ifndef __PLAT_CMN_X86_THREAD_SWITCH_H__
> +#define __PLAT_CMN_X86_THREAD_SWITCH_H__
> +
> +#define MSR_FS_BASE 0xc0000100

This is a CPU related macro, please move it to
plat/common/include/x86/cpu_defs.h where all the CPU related macros are
defined.

> +
> +#define set_tls_pointer(ptr) wrmsrl(MSR_FS_BASE, ptr)
> +
> +#endif /* __PLAT_CMN_X86_THREAD_SWITCH_H__ */
> diff --git a/plat/common/sw_ctx.c b/plat/common/sw_ctx.c
> index 06795244..088d1233 100644
> --- a/plat/common/sw_ctx.c
> +++ b/plat/common/sw_ctx.c
> @@ -39,9 +39,11 @@
>  #include <uk/alloc.h>
>  #include <sw_ctx.h>
>  #include <uk/assert.h>
> +#include <thread_switch.h>
>  #include <x86/cpu.h>
>  
> -static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp);
> +static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp,
> +                             unsigned long tlsp);
>  static void  sw_ctx_start(void *ctx) __noreturn;
>  static void  sw_ctx_switch(void *prevctx, void *nextctx);
>  
> @@ -51,7 +53,8 @@ static void  sw_ctx_switch(void *prevctx, void *nextctx);
>   */
>  extern void asm_thread_starter(void);
>  
> -static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp)
> +static void *sw_ctx_create(struct uk_alloc *allocator, unsigned long sp,
> +                             unsigned long tlsp)
>  {
>       struct sw_ctx *ctx;
>       size_t sz;
> @@ -68,6 +71,7 @@ static void *sw_ctx_create(struct uk_alloc *allocator, 
> unsigned long sp)
>       }
>  
>       ctx->sp = 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);
> @@ -86,6 +90,7 @@ static void sw_ctx_start(void *ctx)
>  
>       UK_ASSERT(sw_ctx != NULL);
>  
> +     set_tls_pointer(sw_ctx->tlsp);
>       /* Switch stacks and run the thread */
>       asm_ctx_start(sw_ctx->sp, sw_ctx->ip);
>  
> @@ -101,6 +106,7 @@ static void sw_ctx_switch(void *prevctx, void *nextctx)
>  
>       save_extregs(p);
>       restore_extregs(n);
> +     set_tls_pointer(n->tlsp);
>       asm_sw_ctx_switch(prevctx, nextctx);
>  }
>  
> diff --git a/plat/linuxu/include/linuxu/syscall-x86_64.h 
> b/plat/linuxu/include/linuxu/syscall-x86_64.h
> index 26820dcd..553f0ba4 100644
> --- a/plat/linuxu/include/linuxu/syscall-x86_64.h
> +++ b/plat/linuxu/include/linuxu/syscall-x86_64.h
> @@ -48,6 +48,7 @@
>  #define __SC_RT_SIGPROCMASK 14
>  #define __SC_IOCTL  16
>  #define __SC_EXIT   60
> +#define __SC_ARCH_PRCTL       158
>  #define __SC_TIMER_CREATE     222
>  #define __SC_TIMER_SETTIME    223
>  #define __SC_TIMER_GETTIME    224
> diff --git a/plat/linuxu/include/linuxu/syscall.h 
> b/plat/linuxu/include/linuxu/syscall.h
> index d378d265..d5b66698 100644
> --- a/plat/linuxu/include/linuxu/syscall.h
> +++ b/plat/linuxu/include/linuxu/syscall.h
> @@ -123,6 +123,13 @@ static inline int sys_sigprocmask(int how,
>                             sizeof(k_sigset_t));
>  }
>  
> +static inline int sys_arch_prctl(int code, unsigned long addr)
> +{
> +     return (int) syscall2(__SC_ARCH_PRCTL,
> +                           (long) code,
> +                           (long) addr);
> +}
> +
>  static inline int sys_pselect6(int nfds,
>               k_fd_set *readfds, k_fd_set *writefds, k_fd_set *exceptfds,
>               const struct k_timespec *timeout, const void *sigmask)
> diff --git a/plat/linuxu/include/thread_switch.h 
> b/plat/linuxu/include/thread_switch.h
> new file mode 100644
> index 00000000..3a0f962f
> --- /dev/null
> +++ b/plat/linuxu/include/thread_switch.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS 
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +#ifndef __PLAT_THREAD_SWITCH_H__
> +#define __PLAT_THREAD_SWITCH_H__
> +
> +#include <linuxu/syscall.h>
> +
> +#define ARCH_SET_FS 0x1002
> +
> +static inline void set_tls_pointer(unsigned long arg)
> +{
> +     sys_arch_prctl(ARCH_SET_FS, arg);
> +}
> +
> +#endif /* __PLAT_THREAD_SWITCH_H__ */
> 

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.