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? Thanks. Regards, Dorjoy
