[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 Yuri,
On 12/18/18 5:44 PM, Yuri Volchkov wrote:
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.
OK, so the main reason I did this is that cpuid is much slower than
reading from cr4. Linux userspace can't read from it though, because
that's a privileged operation, so I used cpuid in that case. Granted,
_init_cpufeatures isn't very time-critical, because it's only called
once at boot time.
Alternatively, we can do what the section you cite suggests in the
following: "Instead, use such conditionals in a header file defining
functions for use in those .c files, providing no-op stub versions in
the #else case, and then call those functions unconditionally from .c
files."
So instead we could have an _init_cpufeatures_unprivileged() and an
_init_cpufeatures_privileged() and then #define _init_cpufeatures() to
either one, depending on which platform we use. That would remove the
#if's from the logic inside the function, but give us slight code
duplication.
So I guess it's a tradeoff between execution speed, readability, and no
duplication: choose two out of three.
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
Hmm... indeed. So I guess it should be like this then:
asm volatile("cpuid" : "=a"(eax),"=c"(ecx),"=d"(edx) : "0"(1) : "ebx");
And the others accordingly.
+ 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.
Good point.
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.
I guess we could, yeah.
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.
OK, I see what you mean, but do you think this should be part of this
patch series?
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
--
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
|
