|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH v2 7/9] plat: Add global struct to keep x86 CPU information
Hi,
comments are inline.
Cheers, Yuri.
Florian Schmidt <florian.schmidt@xxxxxxxxx> writes:
> Currently, all information relates to the additional registers that can
> be available on x86 CPUs, and how to save and restore them.
>
> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> ---
> plat/common/include/x86/cpu.h | 63 +++++++++++++++++++++++++++++++---
> plat/common/x86/cpu_features.c | 37 ++++++++++++++++++++
> plat/kvm/Makefile.uk | 1 +
> plat/kvm/x86/setup.c | 2 ++
> plat/linuxu/Makefile.uk | 1 +
> plat/linuxu/setup.c | 7 ++++
> plat/xen/Makefile.uk | 1 +
> plat/xen/x86/setup.c | 2 ++
> 8 files changed, 109 insertions(+), 5 deletions(-)
> create mode 100644 plat/common/x86/cpu_features.c
>
> diff --git a/plat/common/include/x86/cpu.h b/plat/common/include/x86/cpu.h
> index 001e9cac..fbc229d9 100644
> --- a/plat/common/include/x86/cpu.h
> +++ b/plat/common/include/x86/cpu.h
> @@ -31,16 +31,69 @@
> #define __PLAT_COMMON_X86_CPU_H__
>
> #include <uk/arch/types.h>
> +#include <x86/cpu_defs.h>
> +#include <stdint.h>
>
> void halt(void);
> void system_off(void);
>
> -static inline void cpuid(__u32 leaf, __u32 *eax, __u32 *ebx,
> - __u32 *ecx, __u32 *edx)
> +enum save_cmd {
> + X86_SAVE_NONE,
> + X86_SAVE_FSAVE,
> + X86_SAVE_FXSAVE,
> + X86_SAVE_XSAVE,
> + X86_SAVE_XSAVEOPT
> +};
> +
> +struct _x86_features {
> + unsigned long extregs_size; /* Size of the extregs area */
> + unsigned long extregs_align; /* Alignment of the extregs area */
> + enum save_cmd save; /* which CPU instruction to use for
> + * saving/restoring extregs.
> + */
> +};
> +
> +extern struct _x86_features x86_cpu_features;
> +
> +static inline void _init_cpufeatures(void)
> {
> - asm volatile("cpuid"
> - : "=a"(*eax), "=b"(*ebx), "=c"(*ecx), "=d"(*edx)
> - : "0"(leaf));
> +#if LINUXUPLAT
> + __u32 ecx, edx;
> +#else
> + unsigned long cr4;
> +#endif
> + __u32 eax, ebx;
> +
> +#if LINUXUPLAT
> + asm volatile("cpuid" : "=c"(ecx), "=d"(edx) : "a"(1) : "ebx");
1) Let's just fire the cpuid anyways. This way we will get rid of all
conditional compilations in this function.
We are following the coding style from linux. And it says "Wherever
possible, don’t use preprocessor conditionals (#if, #ifdef) in .c
files; doing so makes code harder to read and logic harder to
follow." It is possible here.
2) The cpuid instruction modifies eax. However eax is listed only in the
inputs. See the "Warning: Do not modify the contents of input-only
operands" from
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#OutputOperands
I guess what we need to do here, is to add "=a(eax)" to the input
list.
This apples to all 3 cpuid calls in this function
> + if (ecx & X86_CPUID1_ECX_OSXSAVE) {
> +#else
> + asm volatile("mov %%cr4, %0" : "=g"(cr4));
> + if (cr4 & X86_CR4_OSXSAVE) {
> +#endif
> + asm volatile("cpuid" : "=a"(eax) : "a"(0xd), "c"(1)
> + : "ebx", "edx");
> + if (eax & X86_CPUIDD1_EAX_XSAVEOPT)
> + x86_cpu_features.save = X86_SAVE_XSAVEOPT;
> + else
> + x86_cpu_features.save = X86_SAVE_XSAVE;
> + asm volatile("cpuid" : "=b"(ebx) : "a"(0xd), "c"(0) : "edx");
> + x86_cpu_features.extregs_size = ebx;
> + x86_cpu_features.extregs_align = 64;
> + }
> +#if LINUXUPLAT
> + else if (edx & X86_CPUID1_EDX_FXSR) {
> +#else
> + else if (cr4 & X86_CR4_OSFXSR) {
> +#endif
> + x86_cpu_features.save = X86_SAVE_FXSAVE;
> + x86_cpu_features.extregs_size = 512;
> + x86_cpu_features.extregs_align = 16;
> + } else {
> + x86_cpu_features.save = X86_SAVE_FSAVE;
> + x86_cpu_features.extregs_size = 108;
> + x86_cpu_features.extregs_align = 1;
> + }
> }
>
> unsigned long read_cr2(void);
> diff --git a/plat/common/x86/cpu_features.c b/plat/common/x86/cpu_features.c
> new file mode 100644
> index 00000000..07097397
> --- /dev/null
> +++ b/plat/common/x86/cpu_features.c
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *
> + * Copyright (c) 2018, 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.
> + */
> +
> +#include <x86/cpu.h>
> +
> +struct _x86_features x86_cpu_features;
> diff --git a/plat/kvm/Makefile.uk b/plat/kvm/Makefile.uk
> index b4e03223..92b9a379 100644
> --- a/plat/kvm/Makefile.uk
> +++ b/plat/kvm/Makefile.uk
> @@ -26,6 +26,7 @@ LIBKVMPLAT_CFLAGS += -DKVMPLAT
> ##
> LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/trace.c|common
> LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/traps.c|common
> +LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/cpu_features.c|common
> LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/cpu_native.c|common
> ifeq ($(CONFIG_HAVE_SCHED),y)
> LIBKVMPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/thread_start.S|common
> diff --git a/plat/kvm/x86/setup.c b/plat/kvm/x86/setup.c
> index 47a78dcf..c17a7dd5 100644
> --- a/plat/kvm/x86/setup.c
> +++ b/plat/kvm/x86/setup.c
> @@ -27,6 +27,7 @@
> */
>
> #include <string.h>
> +#include <x86/cpu.h>
> #include <x86/traps.h>
> #include <kvm/console.h>
> #include <kvm/intctrl.h>
> @@ -118,6 +119,7 @@ void _libkvmplat_entry(void *arg)
> {
> struct multiboot_info *mi = (struct multiboot_info *)arg;
>
> + _init_cpufeatures();
> _libkvmplat_init_console();
> traps_init();
> intctrl_init();
> diff --git a/plat/linuxu/Makefile.uk b/plat/linuxu/Makefile.uk
> index 3c59de44..35b456b5 100644
> --- a/plat/linuxu/Makefile.uk
> +++ b/plat/linuxu/Makefile.uk
> @@ -19,6 +19,7 @@ LIBLINUXUPLAT_CINCLUDES-y +=
> -I$(UK_PLAT_COMMON_BASE)/include
> LIBLINUXUPLAT_ASFLAGS += -DLINUXUPLAT
> LIBLINUXUPLAT_CFLAGS += -DLINUXUPLAT
>
> +LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/cpu_features.c|common
> LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_32) +=
> $(LIBLINUXUPLAT_BASE)/x86/entry32.S
> LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(LIBLINUXUPLAT_BASE)/x86/entry64.S
> LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_ARM_32) +=
> $(LIBLINUXUPLAT_BASE)/arm/entry32.S
> diff --git a/plat/linuxu/setup.c b/plat/linuxu/setup.c
> index 5fbf54b1..c6b910fa 100644
> --- a/plat/linuxu/setup.c
> +++ b/plat/linuxu/setup.c
> @@ -45,6 +45,9 @@
> #include <uk/plat/bootstrap.h>
> #include <uk/assert.h>
> #include <uk/errptr.h>
> +#if defined __X86_64__
> +#include <x86/cpu.h>
> +#endif
Just include <cpu.h>. It has necessary architecture resolution
inside. Furthermore, we could add and empty _init_cpufeatures() to
arm(64)/cpu.h and we will not just get rid of the next "#ifdef
__X86_64__", but also will encourage consistency for implementing
similar mechanisms for those platforms.
Another thing, instead of adding compile guards to x86/cpu.h, it is
probably better to include <cpu.h> everywhere. And add a guard
preventing including <x86/cpu.h> directly.
>
> struct liblinuxuplat_opts _liblinuxuplat_opts = { 0 };
>
> @@ -150,6 +153,10 @@ void _liblinuxuplat_entry(int argc, char *argv[])
> int ret;
> void *pret;
>
> +#if defined __X86_64__
> + _init_cpufeatures();
> +#endif
> +
> /*
> * Initialize platform console
> */
> diff --git a/plat/xen/Makefile.uk b/plat/xen/Makefile.uk
> index 43866e4b..07002d29 100644
> --- a/plat/xen/Makefile.uk
> +++ b/plat/xen/Makefile.uk
> @@ -33,6 +33,7 @@ LIBXENPLAT_SRCS-y +=
> $(UK_PLAT_COMMON_BASE)/memory.c|common
>
> LIBXENPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/trace.c|common
> LIBXENPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/traps.c|common
> +LIBXENPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/cpu_features.c|common
> ifeq ($(CONFIG_HAVE_SCHED),y)
> LIBXENPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/x86/thread_start.S|common
> LIBXENPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
> $(UK_PLAT_COMMON_BASE)/thread.c|common
> diff --git a/plat/xen/x86/setup.c b/plat/xen/x86/setup.c
> index a41d5cb3..60a9f9e6 100644
> --- a/plat/xen/x86/setup.c
> +++ b/plat/xen/x86/setup.c
> @@ -74,6 +74,7 @@
> #include <uk/plat/config.h>
> #include <uk/plat/console.h>
> #include <uk/plat/bootstrap.h>
> +#include <x86/cpu.h>
>
> #include <xen/xen.h>
> #include <common/console.h>
> @@ -170,6 +171,7 @@ void _libxenplat_x86entry(void *start_info) __noreturn;
> void _libxenplat_x86entry(void *start_info)
> {
> _init_traps();
> + _init_cpufeatures();
> HYPERVISOR_start_info = (start_info_t *)start_info;
> _libxenplat_prepare_console(); /* enables buffering for console */
>
> --
> 2.19.2
>
--
Yuri Volchkov
Software Specialist
NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |