kernel: Refactoring for multi-arch support#192
Conversation
This is the first step for multi-arch support in the ukvm guest. Arch-specific code (currently only x86_64) is factored out from all over the codebase into cpu_x86_64.c and cpu_x86_64.h. The assembly code in cpu.S has been replaced with either inline functions in cpu_x86_64.h or directly with inline assembly in cpu_x86_64.c. As part of this change the virtio early bootstrap (boot.S) is modified to initialise the CPU into the same state as is used by the ukvm target, so that the two targets can share the cpu_init() code.
The kernel build now detects architecture-dependent portions at build time, allowing for adding additional architectures.
Follow conventions and rename symbols to where they are now defined.
|
@mato I have reviewed the changes. I think they can make ARM64 guest side code landing easily. Regards, |
|
@Weichen81 I would just add a check to force |
kernel/cpu_x86_64.c
Outdated
| } | ||
|
|
||
| /* keeps track of how many stacked "interrupts_disable"'s there are */ | ||
| int intr_depth = 1; |
There was a problem hiding this comment.
Should this be volatile since it may be accessed by nested interrupts?
There was a problem hiding this comment.
Per the comment in idt_fillgate() (https://github.com/mato/solo5/blob/multiarch/kernel/cpu_x86_64.c#L57), we run all interrupt handlers with interrupts disabled. So this should be fine as is.
There was a problem hiding this comment.
Sorry I misread, I wrongly had the impression that the variable was also accessed when handling exceptions/traps.
|
@Kensan @diwillia I have pushed 2ef936d to address the outcome of our discussion about consolidating x86 FPU/SSE unit setup in #190. This compiles and passes all tests on both Linux and FreeBSD. Please take a look and let me know what you think. (Edit: Out of band discussion with @Kensan and @djwillia about this led to the outcome discussed below in #192 (comment)) |
|
@mato The change looks fine to me. A note regarding Muen/#190: I will want to override fpu_init() (by splitting it into a separate compilation unit) since the control register accesses create traps. On Muen I really only want to perform the MXCSR initialization part. Will adapt #190 accordingly once this PR lands on master. |
- Make FPU and SSE unit setup consistent with ukvm target. - Move bootstrap stack and page tables into the correct linker sections. - Rename .bootstrap to .data.multiboot and include it at the start of the .text section. The last two points save us some kilobytes of unnecessary image size and remove the oddity of the virtio text segment "starting" at 0x101000.
This hasn't caught us till now as GCC is much more conservative in enabling FPU/SSE optimizations. Clang on the other hand will happily use these at -O2.
As per the comment, see also discussion of point (3) in mirage/mirage-solo5#15.
The GDT descriptor values should be consistent with those set up by ukvm. Recalculate the values based on those defined in ukvm/ukvm_cpu_x86_64.c and also used them in the virtio bootstrap GDT.
Add a platform_init() hook for plaform-specific initialiazation. This is primarily to support the upcoming Muen port. For virtio, move the resposibility for calling platform_intr_init() to platform_init() since that's where it belongs.
1. Move more platform-specific initialization (virtio multiboot) into platform_init(). This allows us to unify mem.c between virtio and ukvm, which was until now essentially duplicate code. 2. While doing this, clean up the "memory size" vs. "maximum address" confusion in mem.c and improve the stack guard and memory sizing logic. 3. Rename the virtio early boot and startup functions (in boot.S and kernel.c) to better reflect their purposes. This unifies the virtio and ukvm startup flow as much as possible.
1. There are no plans to port virtio to !x86_64, so don't do that. 2. FreeBSD will need extra work due to use of clang, so refuse to build on !x86_64 there as well.
|
I've reworked these changes to incorporate the outcome of the out-of-band discussions around FPU/SSE setup, which was to keep things as they are for now (i.e. do the majority of setup on the monitor side) and add a This led to somewhat more refactoring than I expected, mostly on the virtio side. The outcome of this is that I've managed to unify and clean up While I was at it I also excised / @Weichen81: You'll need to add an appropriate clock for ARM64 and @djwillia Please take a look and let me know if this is OK to merge. |
|
Hi mato,
Thanks for addressing the virtio issue of ARM64. And we are trying to use the arm generic timer to implement the clock functions for ARM64. It will be completed in the next few days.
Regards,
在 2017年5月5日，22:25，Martin Lucina <notifications@github.com<mailto:notifications@github.com>> 写道：
I've reworked these changes to incorporate the outcome of the out-of-band discussions around FPU/SSE setup, which was to keep things as they are for now (i.e. do the majority of setup on the monitor side) and add a platform_init() which can be used for platform-specific initialisation such as LDMXCSR for the Muen port.
This led to somewhat more refactoring than I expected, mostly on the virtio side. The outcome of this is that I've managed to unify and clean up mem.c for both platforms which is a good thing. Also, the virtio initialisation / bootstrap flow is now much clearer. See the individual commits for details.
While I was at it I also excised / #ifdef'd most of the remaining arch-dependent code with the exception of the call to tscclock_init() from time_init() on the ukvm side.
@Weichen81<https://github.com/Weichen81>: You'll need to add an appropriate clock for ARM64 and #ifdef the call to it's init function (if any). I've also addressed your comment about not building virtio on !x86_64.
@djwillia<https://github.com/djwillia> Please take a look and let me know if this is OK to merge.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#192 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AYDF65T-x570C1I4t9d_9xSW50-LPXSyks5r2zFsgaJpZM4NGSwG>.
|
|
|
||
| .text : | ||
| { | ||
| *(.data.multiboot) |
There was a problem hiding this comment.
The multiboot header won't be aligned as before. It seems like it just has to be aligned at 4 bytes (which is taken care of). Just checking (so GCE doesn't have to be tested).
There was a problem hiding this comment.
That's correct. The requirements are that the header be within the first 8k of the executable and be 4-byte aligned, both of which are taken care of. I tested GCE with these commits and everything works.
There was a problem hiding this comment.
Awesome, thanks for the test.
| gdt64: | ||
| .quad 0x0000000000000000 | ||
| .quad 0x00af9b000000ffff /* 64bit CS */ | ||
| .quad GDT_DESC_CODE_VAL /* 64bit CS */ |
There was a problem hiding this comment.
New value is 0x00af99000000ffff which I think changes the limit field. It has a new limit of 0x9ffff (before was 0xbffff). Memory can grow up to 2.5 GB now. Not much difference really but wonder why the change.
There was a problem hiding this comment.
It does not, LIMIT is bits (0 - 15) and (48 - 51) all of which are 1. The values were taken directly from the result of the computation in ukvm_x86_setup_gdt(). The reason I made this change was to deliberately remove any confusion/differences between the values used by ukvm and those used by virtio.
There was a problem hiding this comment.
You are right, sorry for the confusion. The field changed is type which for a code segment means:
before: 0x00af9b000000ffff -> type = 0xb (Execute-Only, accessed)
now: 0x00af99000000ffff -> type = 0x9 (Execute/Read, accessed)
| */ | ||
|
|
||
| #include "multiboot.h" | ||
| #include "../cpu_x86_64.h" |
There was a problem hiding this comment.
might want to rename boot.S to boot_x86_64.S
There was a problem hiding this comment.
We can do that if (unlikely) virtio ever gets ported to a non x86_64 architecture.
|
LGTM (just in case) |
This is the guest-side follow-up to #171, refactoring the kernel side to allow for multi-arch support.
Briefly, this separates out the kernel arch-specific code into
cpu_<architecture>.[ch]andcpu_vectors_<architecture>.S. It also includes the build system infrastructure needed for multiarch. See individual commits for details./cc #151 #190