Re: [PATCH v1 1/2] machine/microvm: support for loading EIF image

[Kevin Wolf <kwolf@redhat.com>]

Am 31.05.2024 um 09:54 hat Alexander Graf geschrieben:
> 
> 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é <berrange@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.

Not sure if I would call that a "real fix", it's more like a hack.
Kernel images aren't block devices and their size may not even be
aligned to 512, which is the minimum block size the block layer
supports. So there might be some complications around that area.

That said, even if it's an abuse of the block layer, it might be a
useful abuse that saves you writing quite a bit of FILE * based logic
providing similar functionality, so who I am to stop you...

> 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.

Kevin