On 22.05.24 19:23, Dorjoy Chowdhury wrote:
Hi Daniel,
Thanks for reviewing.
On Wed, May 22, 2024 at 9:32 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
On Sat, May 18, 2024 at 02:07:52PM +0600, Dorjoy Chowdhury wrote:
An EIF (Enclave Image Format)[1] image is used to boot an AWS nitro
enclave[2] virtual machine. The EIF file contains the necessary
kernel, cmdline, ramdisk(s) sections to boot.
This commit adds support for loading EIF image using the microvm
machine code. For microvm to boot from an EIF file, the kernel and
ramdisk(s) are extracted into a temporary kernel and a temporary
initrd file which are then hooked into the regular x86 boot mechanism
along with the extracted cmdline.
I can understand why you did it this way, but I feel its pretty
gross to be loading everything into memory, writing it back to
disk, and then immediately loading it all back into memory.
The root problem is the x86_load_linux() method, which directly
accesses the machine properties:
const char *kernel_filename = machine->kernel_filename;
const char *initrd_filename = machine->initrd_filename;
const char *dtb_filename = machine->dtb;
const char *kernel_cmdline = machine->kernel_cmdline;
To properly handle this, I'd say we need to introduce an abstraction
for loading the kernel/inittrd/cmdlkine data.
ie on the X86MachineClass object, we should introduce four virtual
methods
uint8_t *(*load_kernel)(X86MachineState *machine);
uint8_t *(*load_initrd)(X86MachineState *machine);
uint8_t *(*load_dtb)(X86MachineState *machine);
uint8_t *(*load_cmdline)(X86MachineState *machine);
The default impl of these four methods should just following the
existing logic, of reading and returning the data associated with
the kernel_filename, initrd_filename, dtb and kernel_cmdline
properties.
The Nitro machine sub-class, however, can provide an alternative
impl of thse virtual methods which returns data directly from
the EIF file instead.
Great suggestion! I agree the implementation path you suggested would
look much nicer as a whole. Now that I looked a bit into the
"x86_load_linux" implementation, it looks like pretty much everything
is tied to expecting a filename. For example, after reading the header
from the kernel_filename x86_load_linux calls into load_multiboot,
load_elf (which in turn calls load_elf64, 32) and they all expect a
filename. I think I would need to refactor all of them to instead work
with (uint8_t *) buffers instead, right? Also in case of
initrd_filename the existing code maps the file using
g_mapped_file_new to the X86MachineState member initrd_mapped_file. So
that will need to be looked into and refactored. Please correct me if
I misunderstood something about the way to implement your suggested
approach.
If I am understanding this right, this probably requires a lot of work
which will also probably not be straightforward to implement or test.
Right now, the way the code is, it's easy to see that the existing
code paths are still correct as they are not changed and the new
nitro-enclave machine code just hooks into them. As the loading to
memory, writing to disk and loading back to memory only is in the
execution path of the new nitro-enclave machine type, I think the way
the patch is right now, is a good first implementation. What do you
think?
I think the "real" fix here is to move all of the crufty loader logic
from "file access" to "block layer access". Along with that, create a
generic helper function (like this[1]) that opens all
-kernel/-initrd/-dtb files through the block layer and calls a board
specific callback to perform the load.
With that in place, we would have a reentrant code path for the EIF
case: EIF could plug into the generic x86 loader and when it detects
EIF, create internal block devices that reference the existing file plus
an offset/limit setting to ensure it only accesses the correct range in
the target file. It can then simply reinvoke the x86 loader with the new
block device objects.
With that in place, we could also finally support -kernel
http://.../vmlinuz command line invocations which currently only works
on block devices.
However, I do agree that the above is significant effort to get going
and shouldn't hold back the Nitro Enclave machine model.
Alex
[1]
https://github.com/agraf/qemu/commit/e49b7a18f2d8a386e5f207c567ad9ab2e3cb5429
Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597
