On 6/7/19 5:52 PM, Jan Kiszka wrote: > On 07.06.19 17:26, Ralf Ramsauer wrote: >> >> >> On 5/31/19 8:39 PM, Ralf Ramsauer wrote: >>> >>> >>> On 5/31/19 12:04 PM, Jan Kiszka wrote: >>>> On 31.05.19 17:52, Ralf Ramsauer wrote: >>>>> On 5/30/19 3:00 AM, Jan Kiszka wrote: >>>>>> On 21.05.19 16:34, Ralf Ramsauer wrote: >>>>>>> Recent gcc versions emit SSE instructions for 32-bit inmates >>>>>>> (e.g., in >>>>>>> hex2str or cmdline_parse routines). Inmates aren't able to execute >>>>>>> those >>>>>>> instructions as SSE is not enabled and will crash. >>>>>>> >>>>>>> Enabling SSE is the same code for 32 and 64 bit x86 and straight >>>>>>> forward: Lookup SSE availability via cpuid and enable OSFXSR in >>>>>>> cr4. If >>>>>>> SSE is not available, stop the inmate. >>>>>>> >>>>>>> If AVX is available, activate it (XCR0). >>>>>>> >>>>>>> Lookup features that need no explicit activation. >>>>>>> >>>>>>> Signed-off-by: Ralf Ramsauer <[email protected]> >>>>>>> --- >>>>>>> inmates/lib/x86/Makefile | 6 +- >>>>>>> inmates/lib/x86/cpu-features.c | 103 >>>>>>> +++++++++++++++++++++++++++++ >>>>>>> inmates/lib/x86/header-32.S | 9 ++- >>>>>>> inmates/lib/x86/header-64.S | 9 ++- >>>>>>> inmates/lib/x86/include/asm/regs.h | 36 ++++++++++ >>>>>>> 5 files changed, 158 insertions(+), 5 deletions(-) >>>>>>> create mode 100644 inmates/lib/x86/cpu-features.c >>>>>>> >>>>>>> diff --git a/inmates/lib/x86/Makefile b/inmates/lib/x86/Makefile >>>>>>> index e474ffd0..ed3b04d5 100644 >>>>>>> --- a/inmates/lib/x86/Makefile >>>>>>> +++ b/inmates/lib/x86/Makefile >>>>>>> @@ -40,7 +40,7 @@ include $(INMATES_LIB)/Makefile.lib >>>>>>> >>>>>>> always := lib.a lib32.a >>>>>>> >>>>>>> -TARGETS := header-common.o ioapic.o printk.o setup.o smp.o uart.o >>>>>>> +TARGETS := cpu-features.o header-common.o ioapic.o printk.o setup.o >>>>>>> smp.o uart.o >>>>>>> TARGETS += ../alloc.o ../pci.o ../string.o ../cmdline.o >>>>>>> ../setup.o >>>>>>> TARGETS += ../uart-8250.o ../printk.o >>>>>>> TARGETS_32_ONLY := header-32.o >>>>>>> @@ -57,6 +57,10 @@ $(obj)/lib32.a: $(addprefix $(obj)/,$(lib32-y)) >>>>>>> >>>>>>> targets += header-32.o >>>>>>> >>>>>>> +# Code of this object is called before SSE/AVX is available. Ensure >>>>>>> that the >>>>>>> +# compiler won't generate unsupported instructions for this file. >>>>>>> +CFLAGS_cpu-features.o += -mno-sse >>>>>>> + >>>>>>> $(obj)/%-32.o: c_flags += -m32 >>>>>>> $(obj)/%-32.o: $(src)/%.c >>>>>>> $(call if_changed_rule,cc_o_c) >>>>>>> diff --git a/inmates/lib/x86/cpu-features.c >>>>>>> b/inmates/lib/x86/cpu-features.c >>>>>>> new file mode 100644 >>>>>>> index 00000000..9cf98543 >>>>>>> --- /dev/null >>>>>>> +++ b/inmates/lib/x86/cpu-features.c >>>>>>> @@ -0,0 +1,103 @@ >>>>>>> +/* >>>>>>> + * Jailhouse, a Linux-based partitioning hypervisor >>>>>>> + * >>>>>>> + * Copyright (c) OTH Regensburg, 2019 >>>>>>> + * >>>>>>> + * Authors: >>>>>>> + * Ralf Ramsauer <[email protected]> >>>>>>> + * >>>>>>> + * This work is licensed under the terms of the GNU GPL, version >>>>>>> 2. See >>>>>>> + * the COPYING file in the top-level directory. >>>>>>> + * >>>>>>> + * Alternatively, you can use or redistribute this file under the >>>>>>> following >>>>>>> + * BSD license: >>>>>>> + * >>>>>>> + * 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. >>>>>>> + * >>>>>>> + * 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. >>>>>>> + */ >>>>>>> + >>>>>>> +#include <inmate.h> >>>>>>> +#include <asm/regs.h> >>>>>>> + >>>>>>> +/* Must only be called from assembler via jmp */ >>>>>>> +void arch_init_features(void); >>>>>>> + >>>>>>> +struct x86_cpu_features x86_cpu_features >>>>>>> __attribute__((section(".data"))); >>>>>>> + >>>>>>> +/* >>>>>>> + * We arrive here very early, and we don't have a stack. Take care. >>>>>>> + * >>>>>>> + * Every booting CPU will call this function. We make the >>>>>>> assumption >>>>>>> that all >>>>>>> + * CPUs have the same feature set. So we don't need any locks when >>>>>>> writing to >>>>>>> + * x86_cpu_features. >>>>> >>>>> Any comments on this? Is it okay that this code will run on each CPU, >>>>> even if only required on the primary CPU? >>>>> >>>> >>>> Running once on all CPUs is harmless - we are not in a hurry. >>>> >>>> Running without stack is a problem, see below. >>> >>> Depends. It's perfectly fine to have no stack if we don't use it (even >>> if we had an corrupt stack…). >>> >>>> >>>>>>> + */ >>>>>>> +void __attribute__((naked, noreturn, section(".boot"))) >>>>>> >>>>>> What's "naked" supposed to do here? The compilers says it will ignore >>>>>> it. >>>>> >>>>> Huh? Does it produce a warning? >>>> >>>> Yes, gcc 7.4 said so. >>>> >>>>> >>>>> naked ensures that there's no pro- and epilogue sequences. And as the >>>>> comment states: We won't have a stack that early. >>>>> >>>>> BTW: That raises a question: Where do we set the stack for secondary >>>>> CPUs on x86? >>>> >>>> In header.S, always to stack_top... Looks like the ARM issue is still >>>> present here. >>> >>> I remember that I once fixed some corrupted stack things for ARM >>> inmates, but it was a bit different there. >>> >>> Trying to interpret your words: We're currently running on an >>> overlapping stack for secondary (all) CPUs, right? Should probably be >>> fixed first. >>> >>> Hmm. On x86 we're hlting all secondary CPUs very early and wait for a >>> SIPI. Besides setting ap_entry, we could allocate a valid stack on the >>> primary CPU inside smp_start_cpu() and pass it. >> >> MMIO stuff is done, let's get back to this topic. :-) > > Yeah, right... I was close to pushing "--no-sse" today when I recalled > that you are on this ;) > >> >> The problem that I initially wanted to solve was missing SSE support, >> turns out secondary CPUs in x86 have a broken stack. >> >> There might be a clean solution to solve all issues, including the dirty >> "naked jmp around"-hack: >> >> Let's reconsider the early boot phase of x86 inmates. In comparison to >> ARM, for example, we start all CPUs but halt secondary CPUs and wait for >> SIPIs. >> >> Now, we could provide a stack symbol that points to a free, valid stack >> that a CPU can grab. Initially, (by compile time) the symbol holds the >> address of the stack for the primary CPU. It's yet unclear who will win >> the race to be the primary CPU. >> >> We could cmpxchg the content of this symbol *very* early. Whoever wins >> is the primary CPU, everyone else will read zero and simply put the CPU >> in hlt state. > > That's what we do today already: Only the first CPU gets the static > stack, the rest enter a hlt loop until their inmate-triggered wakeup > without touching that stack. No problem here.
Hmm. We go through start16 start32 and start64 until we reach the point where we check the CPU ID, and setup the stack. And everything we do during this path (lgdtl, cr0/4 setup, …) will be executed twice: On the initial startup, and on the SIPI. When we receive the SIPI, can't we simply assume that everything but the stack is already set up and in place? So all we would have to do is setup the stack, get the entry point and call it. Or am I missing something here? > >> >> When starting secondary CPUs, smp_start_cpu() alloc()s a new stack from >> the heap, sets the stack symbol (and sets ap_entry) and kicks the CPU. >> Now the same startup code will run, but the secondary CPU grabs the >> stack and boots. > > Right, we need to make "mov $stack_top,%rsp" configurable, and the > problem should be solved. For the first CPU, it can remain that static one. > >> >> This has some advantages: >> - We have a valid stack very early, and can jump to C back and forth >> with regular calls instead of the hacky hack above. >> - Startup code will only run once when needed, and not multiple times >> as it is the case at the moment > > Startup should happen via INIT/SIPI, so some bits have to be run again. Ok, maybe I lack some architectural knowledge: in which state do we arrive when we receive the SIPI? And why is it written twice? > Moreover, we need to let them run in order to fill smp_cpu_ids. > >> - Every secondary CPU gets its own clean stack (which is broken atm) >> - Still we maintain shared startup code for primary and secondary >> CPUs. >> >> Does that sound reasonable? I'd just like to hear some feedback before I >> start hacking, as this could become a bit more complex, yet another >> time. :-) > > Minus that some of the bits you defined are already there - yes. :) Well, I thought that there could be a shortcut, but seems i'm wrong. Thanks Ralf > > Jan > -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/ce3aaf2a-e8a1-9e07-ea14-0ee9360271b4%40oth-regensburg.de. For more options, visit https://groups.google.com/d/optout.
