Xen PVH booting via multiboot protocol (from pvgrub2-pvh) #482

marmarek · 2020-11-04T19:20:04Z

See QubesOS/qubes-issues#6162 for context.

TODO items:

reorganize headers: avoid cross-platform includes;
deduplicate multiboot parsing (move to bindings/multiboot.c?); alternative: don't parse multiboot info at all - will loose cmdline, but memmap can be retrieved via XENMEM_memory_map hypercall
consider mapping low memory too (multiboot boot information is placed there) - will avoid the need to parse hvm_start_info and multiboot in different places

mato

Looks good, thanks! Various comments in-line.

mato · 2020-11-05T13:38:29Z

bindings/xen/boot.S

@@ -55,6 +74,13 @@ ENTRY(_start32)
    pushl $0
    pushl %ebx


So, %ebx is now either a (struct hvm_start_info *) with direct boot, OR a (struct multiboot_info *), right?

Yes, exactly.

Okay, can you please update both the header comment for this entry point and the comment immediately above to make sense in the new arrangement?

mato · 2020-11-05T13:39:04Z

bindings/xen/boot.S

@@ -20,10 +20,29 @@

 #include "../cpu_x86_64.h"
 #include "xen/elfnote.h"
+#include "../virtio/multiboot.h"


You can leave this as-is, virtio needs some reverse consolidation with Xen anyway, so can do that then.

mato · 2020-11-05T13:39:32Z

bindings/xen/boot.S

+
+.align 4
+_multiboot_header:
+.long MULTIBOOT_HEADER_MAGIC


Nit: Indent the rest of this block by 4 spaces.

mato · 2020-11-05T13:40:34Z

bindings/xen/platform.c

@@ -227,19 +228,64 @@ static void hypercall_init(void)
    assert(HYPERCALL_PAGE[0] != 0xc3);
 }

-void platform_init(const void *arg __attribute__((unused)))
+/* Parse multiboot boot information */
+static void parse_multiboot(const void *arg)


no need for the comment.

mato · 2020-11-05T13:43:07Z

bindings/xen/platform.c


+static void parse_hvm_start_info(const void *arg) {


Nit: Opening { goes on the following line.

mato · 2020-11-05T13:57:29Z

bindings/xen/platform.c

+
+void platform_init(const void *arg)
+{
+    bool multiboot = *((uint32_t*)arg) != XEN_HVM_START_MAGIC_VALUE;


This does too much in one line, including assignment of a truth value based on the negation of another truth value. Please make it more explicit, e.g.:

bool is_xl_boot; if (*(uint32_t *)arg == XEN_HVM_START_MAGIC_VALUE) is_xl_boot = true; else is_xl_boot = false;

and modify below to suit.

mato · 2020-11-05T13:57:43Z

bindings/xen/platform.c

+    bool multiboot = *((uint32_t*)arg) != XEN_HVM_START_MAGIC_VALUE;
+
+    /*
+     * Parse multiboot before setting page tables, as its location may not be


"If booted via multiboot, parse multiboot structure before setting up page tables ..."

mato · 2020-11-05T14:00:46Z

bindings/xen/platform.c

+    console_init();
+
+    /*
+     * Parse Xen HVM/PVH start_info structure after setting hypercall page - it


"If booted directly, parse the Xen hvm_start_info structure after setting up hypercalls"

mato · 2020-11-05T14:02:57Z

bindings/xen/solo5_xen.lds

+    .interp : {
+        *(.interp)
+    } :interp :text
+


Why did this need moving, exactly? It doesn't really matter for Xen, but I'd still like to understand what's going on.

The multiboot header needs to be in the first 8k of the file. Without this change, first 4k were program headers, then the second 4k were just this section, pushing multiboot header beyond the first 8k.

You should be able to accomplish the same thing without re-ordering by placing *(.data.multiboot) into the .text output section as is done in solo5_virtio.lds?

It's already here and it wasn't enough. .text was linked at 8k offset in the file - just beyond what we need. I'm not sure what's the difference with virtio, perhaps more notes here?

Ok, don't worry about it, leave it as-is (and in a separate commit). Thanks.

(... looking at the linker scripts and readelf output, I actually think .interp doesn't belong in :text at all but I'm not going to muck with that now...)

mato · 2020-11-05T14:42:34Z

reorganize headers: avoid cross-platform includes;
deduplicate multiboot parsing (move to bindings/multiboot.c?)

As I mentioned, there is more to sync/de-duplicate between virtio and xen than this, I'm happy to do it at some stage, no need to do it here.

consider mapping low memory too (multiboot boot information is placed there) - will avoid the need to parse hvm_start_info and multiboot in different places

I'd prefer to leave the low memory unmapped as it stops the unikernel from trivially mucking with its pagetables, intentionally or not.

marmarek · 2020-11-05T14:47:40Z

I'd prefer to leave the low memory unmapped as it stops the unikernel from trivially mucking with its pagetables, intentionally or not.

I was considering mapping read-only.

mato · 2020-11-05T14:53:19Z

I was considering mapping read-only.

I like the fact that you can't even introspect your pagetables (or anything else Xen might have "helpfully" put in low memory) without reloading %cr3.

marmarek · 2020-11-05T14:54:41Z

Sure, no problem - as you can see it isn't a blocker.

Add multiboot header pointing at the same entry point. Then, in platform_init distinguish which boot structure is in use based on its magic. Borrow the multiboot structure parsing from virtio bindings.

Currently two boot modes are supported: - native PVH - multiboot (not multiboot2) While hvm_start_info structure (PVH native) has a magic at the start, multiboot protocol set it in separate register (eax), which doesn't survive until platform_init. Add an early sanity check while both magic values are available, otherwise boot structures parsing will blow up.

Multiboot header needs to be in the first 8k of the file.

xaki23 · 2020-11-08T15:57:17Z

tested with a qubes-builder build of mirage-firewall.
build works.
kernel binary starts in both "direct xen" and "via grub" modes.
basic firewall functionality works in both modes.

minor confusion about memory-limits (maxmem setting actualy "works" now), but thats just "different" (compared to minios-pv), not "wrong".

mato

LGTM.

marmarek mentioned this pull request Nov 4, 2020

starting mirage-pvh via grub for templated mirage vms QubesOS/qubes-issues#6162

Closed

mato reviewed Nov 5, 2020

View reviewed changes

marmarek added 3 commits November 6, 2020 14:50

Xen: add multiboot support

c4b4851

Add multiboot header pointing at the same entry point. Then, in platform_init distinguish which boot structure is in use based on its magic. Borrow the multiboot structure parsing from virtio bindings.

Xen: Move .interp after .text

d364601

Multiboot header needs to be in the first 8k of the file.

marmarek force-pushed the xenpvh-multiboot branch from bf78be1 to d364601 Compare November 6, 2020 13:50

mato approved these changes Nov 9, 2020

View reviewed changes

mato merged commit 4c5e949 into Solo5:master Nov 9, 2020

xaki23 mentioned this pull request Nov 14, 2020

With solo5, firewall runs out of memory after a while and can't recover mirage/qubes-mirage-firewall#120

Closed

palainp mentioned this pull request Apr 7, 2022

Why isn't the Mirage Qubes Firewall available as a Qubes Template in the upstream repos mirage/qubes-mirage-firewall#115

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Xen PVH booting via multiboot protocol (from pvgrub2-pvh) #482

Xen PVH booting via multiboot protocol (from pvgrub2-pvh) #482

marmarek commented Nov 4, 2020

mato left a comment

mato Nov 5, 2020

marmarek Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

marmarek Nov 5, 2020 •

edited

Loading

mato Nov 5, 2020

marmarek Nov 5, 2020

mato Nov 5, 2020

mato Nov 5, 2020

mato commented Nov 5, 2020

marmarek commented Nov 5, 2020

mato commented Nov 5, 2020

marmarek commented Nov 5, 2020

xaki23 commented Nov 8, 2020

mato left a comment

Xen PVH booting via multiboot protocol (from pvgrub2-pvh) #482

Xen PVH booting via multiboot protocol (from pvgrub2-pvh) #482

Conversation

marmarek commented Nov 4, 2020

mato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmarek Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mato commented Nov 5, 2020

marmarek commented Nov 5, 2020

mato commented Nov 5, 2020

marmarek commented Nov 5, 2020

xaki23 commented Nov 8, 2020

mato left a comment

Choose a reason for hiding this comment

marmarek Nov 5, 2020 •

edited

Loading