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

[Alexander Graf]

On 27.05.24 16:52, Dorjoy Chowdhury wrote:
> Hi Philippe,
> Thank you for reviewing.
>
> On Mon, May 27, 2024 at 4:47 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> > Hi Dorjoy,
> >
> > On 18/5/24 10:07, 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.
> > >
> > > Although not useful for the microvm machine itself, this is needed
> > > as the following commit adds support for a new machine type
> > > 'nitro-enclave' which uses the microvm machine type as parent. The
> > > code for checking and loading EIF will be put inside a 'nitro-enclave'
> > > machine type check in the following commit so that microvm cannot load
> > > EIF because it shouldn't.
> > >
> > > [1] https://github.com/aws/aws-nitro-enclaves-image-format
> > The documentation is rather scarse...
> Do you mean documentation about EIF file format?  If so, yes, right
> now there is no specification other than the github repo for EIF.
>
> > > [2] https://aws.amazon.com/ec2/nitro/nitro-enclaves/
> > >
> > > Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
> > > ---
> > >    hw/i386/eif.c       | 486 ++++++++++++++++++++++++++++++++++++++++++++
> > >    hw/i386/eif.h       |  20 ++
> > >    hw/i386/meson.build |   2 +-
> > ... still it seems a generic loader, not restricted to x86.
> >
> > Maybe better add it as hw/core/loader-eif.[ch]?
> Yes, the code in eif.c is architecture agnostic. So it could make
> sense to move the files to hw/core. But I don't think the names should
> have "loader" prefix as there is no loading logic in eif.c. There is
> only logic for parsing and extracting kernel, intird, cmdline etc.
> Because nitro-enclave machine type is based on microvm which only
> supports x86 now, I think it also makes sense to keep the files inside
> hw/i386 for now as we can only really load x86 kernel using it. Maybe
> if we, in the future, add support for other architectures, then we can
> move them to hw/core. What do you think?


I think it makes sense to put EIF parsing into generic code from the 
start. Nitro Enclaves supports Aarch64 with the same EIF semantics. In 
fact, it would be pretty simple to do a sub-machine-class similar to the 
x86 NE one for arm based on -M virt as a follow-up and by making the EIF 
logic x86 only we're only making our lives harder for that future.


Alex




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