-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Xen: retrieve mem_size via a hypercall and the E820 map #516
Conversation
@palainp if you've some spare minutes, could you check whether this PR together with qubes & multiboot (a) works (i.e. boots) and (b) the cmdline is preserved if you pass arguments? |
I haven't done much testing other than starting the unikernels but it looks good to me, qubes-mirage-firewall compiles and runs fine with Qubes 4.1, both with multiboot in a template and direct kernel mode. |
Sure, I'll look for the arguments side. |
build fails here in ways that are probably related to my lack of opam fu.
and the result is
please advise. |
My pin is
EDIT: |
The correct pin is
|
doesnt work here.
if i unpin mirage so it goes to 4.1.1, the mirage-firewall make fails
|
Thanks for your experiments. @xaki23 since solo5 moved towards MirageOS4, and this PR is against the master branch, it won't work smoothly. But I cherry-picked that commit to the 0.6 line (which works well with MirageOS 3):
This should result in a solo5-bindings-xen for MirageOS 3 that includes the multiboot-mem-size patch (this PR). @palainp has some outstanding PRs for qubes-firewall to use MirageOS 4. I'll hopefully soon have time to figure out the details and merge that line of work. |
thank you for the backport, but i actually went in the other direction now and used @palainp mirage4 branch of mirage firewall. opam 2.1.1 so both this PR and https://github.com/palainp/qubes-mirage-firewall/tree/mirage4 LGTM, thanks! 👍 |
@xaki23 cool. can you try to pass boot parameters to the VM? i.e. a |
or |
tried that and --help. |
I vaguely remember qubes-mirage-firewall intentionally ignores cmdline, due to qubes used to force some linux-specific values there. |
that should work with no-default-kernelopts these days? |
Testing with
Flood the logfile (it loops over the
Whereas the command line is ignored with multiboot:
gives:
|
yes, thats it. if i remove that line, rebuild, it does actualy respond to commandline options. in direct-boot mode the "-l *:debug" works, i am seeing DBG level messages (from qubes.db).
a --help seems to work in both modes, but gets sortof trapped in a loop, spewing the help again and again and again... |
ha! remembered something. it has to be...
note the "placeholder" part. because something eats the first arg. still looping on the --help though. |
Thanks for the tip ! It's eaten there: https://github.com/hannesm/solo5/blob/2c18fdab70d572faf026dc33e871fe0dbab551c7/bindings/xen/platform.c#L243 |
Once again, thanks for your valuable and quick comments. For me as a slow person (still suffering post-corona), would you mind to answer some minor questions once more? @marmarek oh thanks, well spotted (the @palainp @xaki23 thanks again for spotting that -- should the following code block just be removed from platform.c (so we don't need a /*
* Skip the first token in the cmdline as it is an opaque "name" for
* the kernel coming from the bootloader.
*/
for (; *mi_cmdline; mi_cmdline++, cmdline_len--) {
if (*mi_cmdline == ' ') {
mi_cmdline++;
cmdline_len--;
break;
}
}
I don't quite understanding the looping. The |
from my pov, yes.
no need to support anything older than 4.1 really. i dont think it actualy still is supported, the qrexec version changes come to mind. 4.0 is on the way out, and people can just continue to run older mirage-vm versions on it.
yes, it keeps spewing the help again and again and again. (not sure about "infinite". technically i didnt wait long enough to confirm that.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, the indentation level in the file seems to be 4 spaces
I don't really know if a user can give directives to grub to add this opaque kernel name before the command line arguments, but I am also in favor of deleting this code (and stick to the mirage-builder
I will track for the differences between mirage-xen and mirage-solo5 :) |
I didn't noticed that before because the log flooding is at high speed, but the "restart help message" appears before the help message is completly printed. For example @xaki23 you can try to
|
i can confirm the looped help is truncated/overlapping.
|
This unifies the three approaches to memory map: - multiboot - start_info - E820 Since the hypercall and E820 map works in both settings, cut the code complexity and always use the XENMEM_memory_map. See QubesOS/qubes-issues#6162 and mirage/qubes-mirage-firewall#120
@palainp wrote
Thanks, force-pushed to adhere to these 4 spaces. About the boot loop -- to me this sounds like an unrelated issue that we should investigate, but it has not the top priority (I'm focusing on a release to get the qubes-mirage-firewall back into shape first). Does that sound good to you? |
Thanks ! I agree with that, it can be investigated later. |
Thanks all for this work 👍. We probably should keep track your last issue about the boot loop somewhere. It seems that everyone have an agreement with this PR, so let's merge and cut a relaase! |
This unifies the three approaches to memory map:
Since the hypercall and E820 map works in both settings, cut the code complexity
and always use the XENMEM_memory_map.
See QubesOS/qubes-issues#6162 and
mirage/qubes-mirage-firewall#120