[PATCH v1 0/2] AWS Nitro Enclave emulation support

[PATCH v1 0/2] AWS Nitro Enclave emulation support

[Dorjoy Chowdhury]

Hi,

Hope everyone is doing well. I am working on adding AWS Nitro Enclave[1]
emulation support in QEMU. Alexander Graf is mentoring me on this work. This is
a patch series adding, not yet complete, but useful emulation support of nitro
enclaves. I have a gitlab branch where you can view the patches in the gitlab
web UI for each commit:
https://gitlab.com/dorjoy03/qemu/-/tree/nitro-enclave-emulation

AWS nitro enclaves is an Amazon EC2[2] feature that allows creating isolated
execution environments, called enclaves, from Amazon EC2 instances, which are
used for processing highly sensitive data. Enclaves have no persistent storage
and no external networking. The enclave VMs are based on Firecracker microvm
and have a vhost-vsock device for communication with the parent EC2 instance
that spawned it and a Nitro Secure Module (NSM) device for cryptographic
attestation. The parent instance VM always has CID 3 while the enclave VM gets
a dynamic CID. The enclave VMs can communicate with the parent instance over
various ports to CID 3, for example, the init process inside an enclave sends a
heartbeat to port 9000 upon boot, expecting a heartbeat reply, letting the
parent instance know that the enclave VM has successfully booted.

From inside an EC2 instance, nitro-cli[3] is used to spawn an enclave VM using
an EIF (Enclave Image Format)[4] file. EIF files can be built using nitro-cli
as well. There is no official EIF specification apart from the github
aws-nitro-enclaves-image-format repository[4]. An EIF file contains the kernel,
cmdline and ramdisk(s) in different sections which are used to boot the enclave
VM. You can look at the structs in hw/i386/eif.c file for more details about
the EIF file format.

Adding nitro enclave emulation support in QEMU will make the life of AWS Nitro
Enclave users easier as they will be able to test their EIF images locally
without having to run real nitro enclaves which can be difficult for debugging
due to its roots in security. This will also make quick prototyping easier.

In QEMU, the new nitro-enclave machine type is implemented based on the microvm
machine type similar to how AWS Nitro Enclaves are based on Firecracker microvm.
The vhost-vsock device support is already part of this patch series so that the
enclave VM can communicate to CID 3 using vsock. A mandatory 'guest-cid'
machine type option is needed which becomes the CID of the enclave VM. Some
documentation for the new 'nitro-enclave' machine type has also been added. The
NSM device support will be added in the future.

The plan is to eventually make the nitro enclave emulation in QEMU standalone
i.e., without needing to run another VM with CID 3 with proper vsock
communication support. For this to work, one approach could be to teach the
vhost-vsock driver in kernel to forward CID 3 messages to another CID
(set to CID 2 for host) so that users of the nitro-enclave machine type can
run the necessary vsock server/clients in the host machine (some defaults can
be implemented in QEMU as well, for example, sending a reply to the heartbeat)
which will rid them of the cumbersome way of running another whole VM with CID
3. This way, users of nitro-enclave machine in QEMU, could potentially also run
multiple enclaves with their messages for CID 3 forwarded to different CIDs
which, in QEMU side, could then be specified using a new machine type option
(parent-cid) if implemented. I will be posting an email to the linux
virtualization mailing list about this approach asking for feedback and
suggestions soon.

For local testing you need to generate a hello.eif image by first building
nitro-cli locally[5]. Then you can use nitro-cli to build a hello.eif image[6].

You need to build qemu-system-x86_64 after applying the patches and then you
can run the following command to boot a hello.eif image using the new
'nitro-enclave' machine type option in QEMU:

sudo ./qemu-system-x86_64 -M nitro-enclave,guest-cid=8 -kernel path/to/hello.eif -nographic -m 4G --enable-kvm -cpu host

The command needs to be run as sudo because for the vhost-vsock device to work
QEMU needs to be able to open vhost device in host.

Right now, if you just run the nitro-enclave machine, the kernel panics because
the init process exits abnormally because it cannot connect to port 9000 to CID
3 to send its heartbeat message (the connection times out), so another VM with
CID 3 with proper vsock communication support must be run for it to be useful.
But this restriction can be lifted once the approach about forwarding CID 3
messages is implemented if it gets accepted.

Thanks.

Regards,
Dorjoy

[1] https://docs.aws.amazon.com/enclaves/latest/user/nitro-enclave.html
[2] https://aws.amazon.com/ec2/
[3] https://docs.aws.amazon.com/enclaves/latest/user/getting-started.html
[4] https://docs.aws.amazon.com/enclaves/latest/user/getting-started.html
[5] https://github.com/aws/aws-nitro-enclaves-cli/blob/main/docs/ubuntu_20.04_how_to_install_nitro_cli_from_github_sources.md
[6] https://github.com/aws/aws-nitro-enclaves-cli/blob/main/examples/x86_64/hello/README.md

Dorjoy Chowdhury (2):
  machine/microvm: support for loading EIF image
  machine/nitro-enclave: new machine type for AWS nitro enclave

 MAINTAINERS                              |  10 +
 configs/devices/i386-softmmu/default.mak |   1 +
 docs/system/i386/nitro-enclave.rst       |  58 +++
 hw/i386/Kconfig                          |   4 +
 hw/i386/eif.c                            | 486 +++++++++++++++++++++++
 hw/i386/eif.h                            |  20 +
 hw/i386/meson.build                      |   3 +-
 hw/i386/microvm.c                        | 141 ++++++-
 hw/i386/nitro_enclave.c                  | 134 +++++++
 include/hw/i386/nitro_enclave.h          |  38 ++
 10 files changed, 893 insertions(+), 2 deletions(-)
 create mode 100644 docs/system/i386/nitro-enclave.rst
 create mode 100644 hw/i386/eif.c
 create mode 100644 hw/i386/eif.h
 create mode 100644 hw/i386/nitro_enclave.c
 create mode 100644 include/hw/i386/nitro_enclave.h

-- 
2.39.2

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

[Dorjoy Chowdhury]

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
[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 +-
 hw/i386/microvm.c   | 134 +++++++++++-
 4 files changed, 640 insertions(+), 2 deletions(-)
 create mode 100644 hw/i386/eif.c
 create mode 100644 hw/i386/eif.h

diff --git a/hw/i386/eif.c b/hw/i386/eif.c
new file mode 100644
index 0000000000..761c489d33
--- /dev/null
+++ b/hw/i386/eif.c
@@ -0,0 +1,486 @@
+/*
+ * EIF (Enclave Image Format) related helpers
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/bswap.h"
+#include "qapi/error.h"
+#include <zlib.h> /* for crc32 */
+
+#include "hw/i386/eif.h"
+
+#define MAX_SECTIONS 32
+
+/* members are ordered according to field order in .eif file */
+typedef struct EifHeader {
+    uint8_t  magic[4]; /* must be .eif in ascii i.e., [46, 101, 105, 102] */
+    uint16_t version;
+    uint16_t flags;
+    uint64_t default_memory;
+    uint64_t default_cpus;
+    uint16_t reserved;
+    uint16_t section_cnt;
+    uint64_t section_offsets[MAX_SECTIONS];
+    uint64_t section_sizes[MAX_SECTIONS];
+    uint32_t unused;
+    uint32_t eif_crc32;
+} QEMU_PACKED EifHeader;
+
+/* members are ordered according to field order in .eif file */
+typedef struct EifSectionHeader {
+    /*
+     * 0 = invalid, 1 = kernel, 2 = cmdline, 3 = ramdisk, 4 = signature,
+     * 5 = metadata
+     */
+    uint16_t section_type;
+    uint16_t flags;
+    uint64_t section_size;
+} QEMU_PACKED EifSectionHeader;
+
+enum EifSectionTypes {
+    EIF_SECTION_INVALID = 0,
+    EIF_SECTION_KERNEL = 1,
+    EIF_SECTION_CMDLINE = 2,
+    EIF_SECTION_RAMDISK = 3,
+    EIF_SECTION_SIGNATURE = 4,
+    EIF_SECTION_METADATA = 5,
+    EIF_SECTION_MAX = 6,
+};
+
+static const char *section_type_to_string(uint16_t type)
+{
+    const char *str;
+    switch (type) {
+    case EIF_SECTION_INVALID:
+        str = "invalid";
+        break;
+    case EIF_SECTION_KERNEL:
+        str = "kernel";
+        break;
+    case EIF_SECTION_CMDLINE:
+        str = "cmdline";
+        break;
+    case EIF_SECTION_RAMDISK:
+        str = "ramdisk";
+        break;
+    case EIF_SECTION_SIGNATURE:
+        str = "signature";
+        break;
+    case EIF_SECTION_METADATA:
+        str = "metadata";
+        break;
+    default:
+        str = "unknown";
+        break;
+    }
+
+    return str;
+}
+
+static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
+                            Error **errp)
+{
+    size_t got;
+    size_t header_size = sizeof(*header);
+
+    got = fread(header, 1, header_size, f);
+    if (got != header_size) {
+        error_setg(errp, "Failed to read EIF header");
+        return false;
+    }
+
+    if (memcmp(header->magic, ".eif", 4) != 0) {
+        error_setg(errp, "Invalid EIF image. Magic mismatch.");
+        return false;
+    }
+
+    /* Exclude header->eif_crc32 field from CRC calculation */
+    *crc = crc32(*crc, (uint8_t *)header, header_size - 4);
+
+    header->version = be16_to_cpu(header->version);
+    header->flags = be16_to_cpu(header->flags);
+    header->default_memory = be64_to_cpu(header->default_memory);
+    header->default_cpus = be64_to_cpu(header->default_cpus);
+    header->reserved = be16_to_cpu(header->reserved);
+    header->section_cnt = be16_to_cpu(header->section_cnt);
+
+    for (int i = 0; i < MAX_SECTIONS; ++i) {
+        header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
+    }
+
+    for (int i = 0; i < MAX_SECTIONS; ++i) {
+        header->section_sizes[i] = be64_to_cpu(header->section_sizes[i]);
+    }
+
+    header->unused = be32_to_cpu(header->unused);
+    header->eif_crc32 = be32_to_cpu(header->eif_crc32);
+    return true;
+}
+
+static bool read_eif_section_header(FILE *f, EifSectionHeader *section_header,
+                                    uint32_t *crc, Error **errp)
+{
+    size_t got;
+    size_t section_header_size = sizeof(*section_header);
+
+    got = fread(section_header, 1, section_header_size, f);
+    if (got != section_header_size) {
+        error_setg(errp, "Failed to read EIF section header");
+        return false;
+    }
+
+    *crc = crc32(*crc, (uint8_t *)section_header, section_header_size);
+
+    section_header->section_type = be16_to_cpu(section_header->section_type);
+    section_header->flags = be16_to_cpu(section_header->flags);
+    section_header->section_size = be64_to_cpu(section_header->section_size);
+    return true;
+}
+
+/*
+ * Upon success, the caller is responsible for unlinking and freeing *tmp_path.
+ */
+static bool get_tmp_file(const char *template, char **tmp_path, Error **errp)
+{
+    int tmp_fd;
+
+    *tmp_path = NULL;
+    tmp_fd = g_file_open_tmp(template, tmp_path, NULL);
+    if (tmp_fd < 0 || *tmp_path == NULL) {
+        error_setg(errp, "Failed to create temporary file for template %s",
+                   template);
+        return false;
+    }
+
+    close(tmp_fd);
+    return true;
+}
+
+static void safe_fclose(FILE *f)
+{
+    if (f) {
+        fclose(f);
+    }
+}
+
+static void safe_unlink(char *f)
+{
+    if (f) {
+        unlink(f);
+    }
+}
+
+/*
+ * Upon success, the caller is reponsible for unlinking and freeing *kernel_path
+ */
+static bool read_eif_kernel(FILE *f, uint64_t size, char **kernel_path,
+                            uint32_t *crc, Error **errp)
+{
+    size_t got;
+    FILE *tmp_file = NULL;
+    uint8_t *kernel = NULL;
+
+    *kernel_path = NULL;
+    if (!get_tmp_file("eif-kernel-XXXXXX", kernel_path, errp)) {
+        goto cleanup;
+    }
+
+    tmp_file = fopen(*kernel_path, "wb");
+    if (tmp_file == NULL) {
+        error_setg_errno(errp, errno, "Failed to open temporary file %s",
+                         *kernel_path);
+        goto cleanup;
+    }
+
+    kernel = g_malloc(size);
+    got = fread(kernel, 1, size, f);
+    if ((uint64_t) got != size) {
+        error_setg(errp, "Failed to read EIF kernel section data");
+        goto cleanup;
+    }
+
+    got = fwrite(kernel, 1, size, tmp_file);
+    if ((uint64_t) got != size) {
+        error_setg(errp, "Failed to write EIF kernel section data to temporary"
+                   " file");
+        goto cleanup;
+    }
+
+    *crc = crc32(*crc, kernel, size);
+    g_free(kernel);
+    fclose(tmp_file);
+
+    return true;
+
+ cleanup:
+    safe_fclose(tmp_file);
+
+    safe_unlink(*kernel_path);
+    g_free(*kernel_path);
+    *kernel_path = NULL;
+
+    g_free(kernel);
+    return false;
+}
+
+static bool read_eif_cmdline(FILE *f, uint64_t size, char *cmdline,
+                             uint32_t *crc, Error **errp)
+{
+    size_t got = fread(cmdline, 1, size, f);
+    if ((uint64_t) got != size) {
+        error_setg(errp, "Failed to read EIF cmdline section data");
+        return false;
+    }
+
+    *crc = crc32(*crc, (uint8_t *)cmdline, size);
+    return true;
+}
+
+static bool read_eif_ramdisk(FILE *eif, FILE *initrd, uint64_t size,
+                             uint32_t *crc, Error **errp)
+{
+    size_t got;
+    uint8_t *ramdisk = g_malloc(size);
+
+    got = fread(ramdisk, 1, size, eif);
+    if ((uint64_t) got != size) {
+        error_setg(errp, "Failed to read EIF ramdisk section data");
+        goto cleanup;
+    }
+
+    got = fwrite(ramdisk, 1, size, initrd);
+    if ((uint64_t) got != size) {
+        error_setg(errp, "Failed to write EIF ramdisk data to temporary file");
+        goto cleanup;
+    }
+
+    *crc = crc32(*crc, ramdisk, size);
+    g_free(ramdisk);
+    return true;
+
+ cleanup:
+    g_free(ramdisk);
+    return false;
+}
+
+/*
+ * Upon success, the caller is reponsible for unlinking and freeing
+ * *kernel_path, *initrd_path and freeing *cmdline.
+ */
+bool read_eif_file(const char *eif_path, char **kernel_path, char **initrd_path,
+                   char **cmdline, Error **errp)
+{
+    FILE *f = NULL;
+    FILE *initrd_f = NULL;
+    uint32_t crc = 0;
+    EifHeader eif_header;
+    bool seen_sections[EIF_SECTION_MAX] = {false};
+
+    *kernel_path = *initrd_path = *cmdline = NULL;
+
+    f = fopen(eif_path, "rb");
+    if (f == NULL) {
+        error_setg_errno(errp, errno, "Failed to open %s", eif_path);
+        goto cleanup;
+    }
+
+    if (!read_eif_header(f, &eif_header, &crc, errp)) {
+        goto cleanup;
+    }
+
+    if (eif_header.version < 4) {
+        error_setg(errp, "Expected EIF version 4 or greater");
+        goto cleanup;
+    }
+
+    if (eif_header.flags != 0) {
+        error_setg(errp, "Expected EIF flags to be 0");
+        goto cleanup;
+    }
+
+    if (eif_header.section_cnt > MAX_SECTIONS) {
+        error_setg(errp, "EIF header section count must not be greater than "
+                   "%d but found %d", MAX_SECTIONS, eif_header.section_cnt);
+        goto cleanup;
+    }
+
+    for (int i = 0; i < eif_header.section_cnt; ++i) {
+        EifSectionHeader section_header;
+        uint16_t section_type;
+
+        if (fseek(f, eif_header.section_offsets[i], SEEK_SET) != 0) {
+            error_setg_errno(errp, errno, "Failed to offset to %lu in EIF file",
+                             eif_header.section_offsets[i]);
+            goto cleanup;
+        }
+
+        if (!read_eif_section_header(f, &section_header, &crc, errp)) {
+            goto cleanup;
+        }
+
+        if (section_header.flags != 0) {
+            error_setg(errp, "Expected EIF section header flags to be 0");
+            goto cleanup;
+        }
+
+        if (eif_header.section_sizes[i] != section_header.section_size) {
+            error_setg(errp, "EIF section size mismatch between header and "
+                       "section header: header %lu, section header %lu",
+                       eif_header.section_sizes[i],
+                       section_header.section_size);
+            goto cleanup;
+        }
+
+        section_type = section_header.section_type;
+
+        switch (section_type) {
+        case EIF_SECTION_KERNEL:
+            if (seen_sections[EIF_SECTION_KERNEL]) {
+                error_setg(errp, "Invalid EIF image. More than 1 kernel "
+                           "section");
+                goto cleanup;
+            }
+            if (!read_eif_kernel(f, section_header.section_size, kernel_path,
+                                 &crc, errp)) {
+                goto cleanup;
+            }
+
+            break;
+        case EIF_SECTION_CMDLINE:
+        {
+            uint64_t size;
+            if (seen_sections[EIF_SECTION_CMDLINE]) {
+                error_setg(errp, "Invalid EIF image. More than 1 cmdline "
+                           "section");
+                goto cleanup;
+            }
+            size = section_header.section_size;
+            *cmdline = g_malloc(size + 1);
+            if (!read_eif_cmdline(f, size, *cmdline, &crc, errp)) {
+                goto cleanup;
+            }
+            (*cmdline)[size] = '\0';
+
+            break;
+        }
+        case EIF_SECTION_RAMDISK:
+            if (!seen_sections[EIF_SECTION_RAMDISK]) {
+                /*
+                 * If this is the first time we are seeing a ramdisk section,
+                 * we need to create the initrd temporary file.
+                 */
+                if (!get_tmp_file("eif-initrd-XXXXXX", initrd_path, errp)) {
+                    goto cleanup;
+                }
+                initrd_f = fopen(*initrd_path, "wb");
+                if (initrd_f == NULL) {
+                    error_setg_errno(errp, errno, "Failed to open file %s",
+                                     *initrd_path);
+                    goto cleanup;
+                }
+            }
+
+            if (!read_eif_ramdisk(f, initrd_f, section_header.section_size,
+                                  &crc, errp)) {
+                goto cleanup;
+            }
+
+            break;
+        default:
+            /* other sections including invalid or unknown sections */
+        {
+            uint8_t *buf;
+            size_t got;
+            uint64_t size = section_header.section_size;
+            buf = g_malloc(size);
+            got = fread(buf, 1, size, f);
+            if ((uint64_t) got != size) {
+                g_free(buf);
+                error_setg(errp, "Failed to read EIF %s section data",
+                           section_type_to_string(section_type));
+                goto cleanup;
+            }
+            crc = crc32(crc, buf, size);
+            g_free(buf);
+            break;
+        }
+        }
+
+        if (section_type < EIF_SECTION_MAX) {
+            seen_sections[section_type] = true;
+        }
+    }
+
+    if (!seen_sections[EIF_SECTION_KERNEL]) {
+        error_setg(errp, "Invalid EIF image. No kernel section.");
+        goto cleanup;
+    }
+    if (!seen_sections[EIF_SECTION_CMDLINE]) {
+        error_setg(errp, "Invalid EIF image. No cmdline section.");
+        goto cleanup;
+    }
+    if (!seen_sections[EIF_SECTION_RAMDISK]) {
+        error_setg(errp, "Invalid EIF image. No ramdisk section.");
+        goto cleanup;
+    }
+
+    if (eif_header.eif_crc32 != crc) {
+        error_setg(errp, "CRC mismatch. Expected %u but header has %u.",
+                   crc, eif_header.eif_crc32);
+        goto cleanup;
+    }
+
+    fclose(f);
+    fclose(initrd_f);
+    return true;
+
+ cleanup:
+    safe_fclose(f);
+    safe_fclose(initrd_f);
+
+    safe_unlink(*kernel_path);
+    g_free(*kernel_path);
+    *kernel_path = NULL;
+
+    safe_unlink(*initrd_path);
+    g_free(*initrd_path);
+    *initrd_path = NULL;
+
+    g_free(*cmdline);
+    *cmdline = NULL;
+
+    return false;
+}
+
+bool check_if_eif_file(const char *path, bool *is_eif, Error **errp)
+{
+    size_t got;
+    uint8_t buf[4];
+    FILE *f = NULL;
+
+    f = fopen(path, "rb");
+    if (f == NULL) {
+        error_setg_errno(errp, errno, "Failed to open file %s", path);
+        goto cleanup;
+    }
+
+    got = fread(buf, 1, 4, f);
+    if (got != 4) {
+        error_setg(errp, "Failed to read magic value from %s", path);
+        goto cleanup;
+    }
+
+    fclose(f);
+    *is_eif = !memcmp(buf, ".eif", 4);
+    return true;
+
+ cleanup:
+    safe_fclose(f);
+    return false;
+}
diff --git a/hw/i386/eif.h b/hw/i386/eif.h
new file mode 100644
index 0000000000..b71a27062d
--- /dev/null
+++ b/hw/i386/eif.h
@@ -0,0 +1,20 @@
+/*
+ * EIF (Enclave Image Format) related helpers
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#ifndef HW_I386_EIF_H
+#define HW_I386_EIF_H
+
+bool read_eif_file(const char *eif_path, char **kernel_path, char **initrd_path,
+                    char **kernel_cmdline, Error **errp);
+
+bool check_if_eif_file(const char *path, bool *is_eif, Error **errp);
+
+#endif
+
diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index 03aad10df7..e398fc1d74 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -14,7 +14,7 @@ i386_ss.add(when: 'CONFIG_X86_IOMMU', if_true: files('x86-iommu.c'),
 i386_ss.add(when: 'CONFIG_AMD_IOMMU', if_true: files('amd_iommu.c'),
                                       if_false: files('amd_iommu-stub.c'))
 i386_ss.add(when: 'CONFIG_I440FX', if_true: files('pc_piix.c'))
-i386_ss.add(when: 'CONFIG_MICROVM', if_true: files('x86-common.c', 'microvm.c', 'acpi-microvm.c', 'microvm-dt.c'))
+i386_ss.add(when: 'CONFIG_MICROVM', if_true: files('x86-common.c', 'microvm.c', 'acpi-microvm.c', 'microvm-dt.c', 'eif.c'))
 i386_ss.add(when: 'CONFIG_Q35', if_true: files('pc_q35.c'))
 i386_ss.add(when: 'CONFIG_VMMOUSE', if_true: files('vmmouse.c'))
 i386_ss.add(when: 'CONFIG_VMPORT', if_true: files('vmport.c'))
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index fec63cacfa..d52d85bd01 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -34,6 +34,7 @@
 #include "hw/irq.h"
 #include "hw/i386/kvm/clock.h"
 #include "hw/i386/microvm.h"
+#include "hw/i386/eif.h"
 #include "hw/i386/x86.h"
 #include "target/i386/cpu.h"
 #include "hw/intc/i8259.h"
@@ -281,6 +282,127 @@ static void microvm_devices_init(MicrovmMachineState *mms)
     x86_bios_rom_init(x86ms, default_firmware, get_system_memory(), true);
 }
 
+/* Expects file to have offset 0 before this function is called */
+static long get_file_size(FILE *f, Error **errp)
+{
+    long size;
+
+    if (fseek(f, 0, SEEK_END) != 0) {
+        error_setg_errno(errp, errno, "Failed to seek to the end of file");
+        return -1;
+    }
+
+    size = ftell(f);
+    if (size == -1) {
+        error_setg_errno(errp, errno, "Failed to get offset");
+        return -1;
+    }
+
+    if (fseek(f, 0, SEEK_SET) != 0) {
+        error_setg_errno(errp, errno, "Failed to seek back to the start");
+        return -1;
+    }
+
+    return size;
+}
+
+static void load_eif(MicrovmMachineState *mms, FWCfgState *fw_cfg)
+{
+    Error *err;
+    char *eif_kernel, *eif_initrd, *eif_cmdline;
+    MachineState *machine = MACHINE(mms);
+    X86MachineState *x86ms = X86_MACHINE(mms);
+
+    if (!read_eif_file(machine->kernel_filename, &eif_kernel, &eif_initrd,
+                       &eif_cmdline, &err)) {
+        error_report_err(err);
+        exit(1);
+    }
+
+    g_free(machine->kernel_filename);
+    machine->kernel_filename = eif_kernel;
+
+    /*
+     * If an initrd argument was provided, let's concatenate it to the
+     * extracted EIF initrd temporary file.
+     */
+    if (machine->initrd_filename) {
+        long size;
+        size_t got;
+        uint8_t *buf;
+        FILE *initrd_f, *eif_initrd_f;
+
+        initrd_f = fopen(machine->initrd_filename, "rb");
+        if (initrd_f == NULL) {
+            error_setg_errno(&err, errno, "Failed to open initrd file %s",
+                             machine->initrd_filename);
+            goto cleanup;
+        }
+
+        size = get_file_size(initrd_f, &err);
+        if (size == -1) {
+            goto cleanup;
+        }
+
+        buf = g_malloc(size);
+        got = fread(buf, 1, size, initrd_f);
+        if ((uint64_t) got != (uint64_t) size) {
+            error_setg(&err, "Failed to read initrd file %s",
+                       machine->initrd_filename);
+            goto cleanup;
+        }
+
+        eif_initrd_f = fopen(eif_initrd, "ab");
+        if (eif_initrd_f == NULL) {
+            error_setg_errno(&err, errno, "Failed to open EIF initrd file %s",
+                             eif_initrd);
+            goto cleanup;
+        }
+        got = fwrite(buf, 1, size, eif_initrd_f);
+        if ((uint64_t) got != (uint64_t) size) {
+            error_setg(&err, "Failed to append initrd %s to %s",
+                       machine->initrd_filename, eif_initrd);
+            goto cleanup;
+        }
+
+        fclose(initrd_f);
+        fclose(eif_initrd_f);
+
+        g_free(buf);
+        g_free(machine->initrd_filename);
+
+        machine->initrd_filename = eif_initrd;
+    } else {
+        machine->initrd_filename = eif_initrd;
+    }
+
+    /*
+     * If kernel cmdline argument was provided, let's concatenate it to the
+     * extracted EIF kernel cmdline.
+     */
+    if (machine->kernel_cmdline != NULL) {
+        char *cmd = g_strdup_printf("%s %s", eif_cmdline,
+                                    machine->kernel_cmdline);
+        g_free(eif_cmdline);
+        g_free(machine->kernel_cmdline);
+        machine->kernel_cmdline = cmd;
+    } else {
+        machine->kernel_cmdline = eif_cmdline;
+    }
+
+    x86_load_linux(x86ms, fw_cfg, 0, true);
+
+    unlink(machine->kernel_filename);
+    unlink(machine->initrd_filename);
+    return;
+
+ cleanup:
+    error_report_err(err);
+    unlink(eif_kernel);
+    unlink(eif_initrd);
+    exit(1);
+}
+
 static void microvm_memory_init(MicrovmMachineState *mms)
 {
     MachineState *machine = MACHINE(mms);
@@ -330,7 +452,17 @@ static void microvm_memory_init(MicrovmMachineState *mms)
     rom_set_fw(fw_cfg);
 
     if (machine->kernel_filename != NULL) {
-        x86_load_linux(x86ms, fw_cfg, 0, true);
+        Error *err;
+        bool is_eif = false;
+        if (!check_if_eif_file(machine->kernel_filename, &is_eif, &err)) {
+            error_report_err(err);
+            exit(1);
+        }
+        if (is_eif) {
+            load_eif(mms, fw_cfg);
+        } else {
+            x86_load_linux(x86ms, fw_cfg, 0, true);
+        }
     }
 
     if (mms->option_roms) {
-- 
2.39.2

[PATCH v1 2/2] machine/nitro-enclave: new machine type for AWS nitro enclave

[Dorjoy Chowdhury]

AWS nitro enclaves[1] is an Amazon EC2[2] feature that allows creating
isolated execution environments, called enclaves, from Amazon EC2
instances which are used for processing highly sensitive data.
Enclaves have no persistent storage and no external networking. The
enclave VMs are based on Firecracker microvm with a vhost-vsock
device for communication with the parent EC2 instance that spawned
it and a Nitro Secure Module (NSM) device for cryptographic attestation.
The parent instance VM always has CID 3 while the enclave VM gets a
dynamic CID.

This commit adds support for limited AWS nitro enclave emulation using
a new machine type option '-M nitro-enclave'. This new machine type is
based on the 'microvm' machine type (similar to how real nitro enclave
VMs are based on Firecracker microvm) which requires a mandatory
'guest-cid' machine type option which becomes the CID of the running
nitro-enclave VM using a built-in vhost-vsock device.

The code for checking and loading EIF in the microvm machine code added
in the previous commit has been put inside a 'nitro-enclave' machine
type check so that EIF images can only be loaded using a 'nitro-enclave'
machine.

Right now, the emulation support is not complete as nitro-enclave
cannot be run standalone using QEMU. A separate VM with CID 3 must be
run with the necessary vsock communication support that the enclave VM
communicates with. Also the Nitro Secure Module (NSM) device is not
implemented yet.

[1] https://docs.aws.amazon.com/enclaves/latest/user/nitro-enclave.html
[2] https://aws.amazon.com/ec2/

Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
---
 MAINTAINERS                              |  10 ++
 configs/devices/i386-softmmu/default.mak |   1 +
 docs/system/i386/nitro-enclave.rst       |  58 ++++++++++
 hw/i386/Kconfig                          |   4 +
 hw/i386/meson.build                      |   1 +
 hw/i386/microvm.c                        |  21 ++--
 hw/i386/nitro_enclave.c                  | 134 +++++++++++++++++++++++
 include/hw/i386/nitro_enclave.h          |  38 +++++++
 8 files changed, 260 insertions(+), 7 deletions(-)
 create mode 100644 docs/system/i386/nitro-enclave.rst
 create mode 100644 hw/i386/nitro_enclave.c
 create mode 100644 include/hw/i386/nitro_enclave.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 1b79767d61..4ac78da43c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1867,6 +1867,16 @@ F: hw/i386/microvm.c
 F: include/hw/i386/microvm.h
 F: pc-bios/bios-microvm.bin
 
+nitro-enclave
+M: Alexander Graf <graf@amazon.com>
+M: Dorjoy Chowdhury <dorjoychy111@gmail.com>
+S: Maintained
+F: docs/system/i386/nitro-enclave.rst
+F: hw/i386/eif.c
+F: hw/i386/eif.h
+F: hw/i386/nitro_enclave.c
+F: include/hw/i386/nitro_enclave.h
+
 Machine core
 M: Eduardo Habkost <eduardo@habkost.net>
 M: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
diff --git a/configs/devices/i386-softmmu/default.mak b/configs/devices/i386-softmmu/default.mak
index 448e3e3b1b..4faf2f0315 100644
--- a/configs/devices/i386-softmmu/default.mak
+++ b/configs/devices/i386-softmmu/default.mak
@@ -29,3 +29,4 @@
 # CONFIG_I440FX=n
 # CONFIG_Q35=n
 # CONFIG_MICROVM=n
+# CONFIG_NITRO_ENCLAVE=n
diff --git a/docs/system/i386/nitro-enclave.rst b/docs/system/i386/nitro-enclave.rst
new file mode 100644
index 0000000000..0b86b6ab88
--- /dev/null
+++ b/docs/system/i386/nitro-enclave.rst
@@ -0,0 +1,58 @@
+'nitro-enclave' virtual machine (``nitro-enclave``)
+===================================================
+
+``nitro-enclave`` is a machine type which emulates an ``AWS nitro enclave``
+virtual machine. `AWS nitro enclaves`_ is an `Amazon EC2`_ feature that allows
+creating isolated execution environments, called enclaves, from Amazon EC2
+instances which are used for processing highly sensitive data. Enclaves have
+no persistent storage and no external networking. The enclave VMs are based
+on Firecracker microvm with a vhost-vsock device for communication with the
+parent EC2 instance that spawned it and a Nitro Secure Module (NSM) device
+for cryptographic attestation. The parent instance VM always has CID 3 while
+the enclave VM gets a dynamic CID. Enclaves use an EIF (`Enclave Image Format`_)
+file which contains the necessary kernel, cmdline and ramdisk(s) to boot.
+
+In QEMU, ``nitro-enclave`` is a machine type based on ``microvm`` similar to how
+``AWS nitro enclaves`` are based on ``Firecracker``. This is useful for local
+testing of EIF images using QEMU instead of running real AWS Nitro Enclaves
+which can be difficult for debugging due to its roots in security.
+
+.. _AWS nitro enlaves: https://docs.aws.amazon.com/enclaves/latest/user/nitro-enclave.html
+.. _Amazon EC2: https://aws.amazon.com/ec2/
+.. _Enclave Image Format: https://github.com/aws/aws-nitro-enclaves-image-format
+
+
+Limitations
+-----------
+
+AWS nitro enclave emulation support is not complete yet:
+
+- Although support for the vhost-vsock device is implemented, standalone
+nitro-enclave VMs cannot be run right now as nitro-enclave VMs communicate
+with a parent instance VM with CID 3. So another VM with CID 3 must be run
+with necessary vsock communication support.
+- Enclaves also have a Nitro Secure Module (NSM) device which is not implemented
+yet.
+
+
+Using the nitro-enclave machine type
+------------------------------
+
+Machine-specific options
+~~~~~~~~~~~~~~~~~~~~~~~~
+
+It supports the following machine-specific options:
+
+- nitro-enclave.guest-cid=uint32_t (required) (Set nitro enclave VM's CID)
+
+
+Running a nitro-enclave VM
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+A nitro-enclave VM can be run using the following command where ``hello.eif`` is
+an EIF image you would use to spawn a real AWS nitro enclave virtual machine:
+
+  $ sudo qemu-system-x86_64 -M nitro-enclave,guest-cid=8 \
+     -enable-kvm -cpu host -m 512m \
+     -kernel hello.eif \
+     -nographic
diff --git a/hw/i386/Kconfig b/hw/i386/Kconfig
index f4a33b6c08..eba8eaa960 100644
--- a/hw/i386/Kconfig
+++ b/hw/i386/Kconfig
@@ -129,6 +129,10 @@ config MICROVM
     select USB_XHCI_SYSBUS
     select I8254
 
+config NITRO_ENCLAVE
+    default y
+    depends on MICROVM
+
 config X86_IOMMU
     bool
     depends on PC
diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index e398fc1d74..5d8f9747b8 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -15,6 +15,7 @@ i386_ss.add(when: 'CONFIG_AMD_IOMMU', if_true: files('amd_iommu.c'),
                                       if_false: files('amd_iommu-stub.c'))
 i386_ss.add(when: 'CONFIG_I440FX', if_true: files('pc_piix.c'))
 i386_ss.add(when: 'CONFIG_MICROVM', if_true: files('x86-common.c', 'microvm.c', 'acpi-microvm.c', 'microvm-dt.c', 'eif.c'))
+i386_ss.add(when: 'CONFIG_NITRO_ENCLAVE', if_true: files('nitro_enclave.c'))
 i386_ss.add(when: 'CONFIG_Q35', if_true: files('pc_q35.c'))
 i386_ss.add(when: 'CONFIG_VMMOUSE', if_true: files('vmmouse.c'))
 i386_ss.add(when: 'CONFIG_VMPORT', if_true: files('vmport.c'))
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index d52d85bd01..1ba6be8798 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -34,6 +34,7 @@
 #include "hw/irq.h"
 #include "hw/i386/kvm/clock.h"
 #include "hw/i386/microvm.h"
+#include "hw/i386/nitro_enclave.h"
 #include "hw/i386/eif.h"
 #include "hw/i386/x86.h"
 #include "target/i386/cpu.h"
@@ -452,13 +453,19 @@ static void microvm_memory_init(MicrovmMachineState *mms)
     rom_set_fw(fw_cfg);
 
     if (machine->kernel_filename != NULL) {
-        Error *err;
-        bool is_eif = false;
-        if (!check_if_eif_file(machine->kernel_filename, &is_eif, &err)) {
-            error_report_err(err);
-            exit(1);
-        }
-        if (is_eif) {
+        if (object_dynamic_cast(OBJECT(machine), TYPE_NITRO_ENCLAVE_MACHINE)) {
+            Error *err;
+            bool is_eif = false;
+            if (!check_if_eif_file(machine->kernel_filename, &is_eif, &err)) {
+                error_report_err(err);
+                exit(1);
+            }
+            if (!is_eif) {
+                error_report("%s is not a valid EIF file.",
+                             machine->kernel_filename);
+                exit(1);
+            }
+
             load_eif(mms, fw_cfg);
         } else {
             x86_load_linux(x86ms, fw_cfg, 0, true);
diff --git a/hw/i386/nitro_enclave.c b/hw/i386/nitro_enclave.c
new file mode 100644
index 0000000000..5a9b9b04bf
--- /dev/null
+++ b/hw/i386/nitro_enclave.c
@@ -0,0 +1,134 @@
+/*
+ * AWS nitro-enclave machine
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "qapi/visitor.h"
+
+#include "hw/sysbus.h"
+#include "hw/i386/x86.h"
+#include "hw/i386/microvm.h"
+#include "hw/i386/nitro_enclave.h"
+#include "hw/virtio/vhost-vsock.h"
+#include "hw/virtio/virtio-mmio.h"
+
+static BusState *find_virtio_mmio_bus(void)
+{
+    BusChild *kid;
+    BusState *bus = sysbus_get_default();
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        DeviceState *dev = kid->child;
+        if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_MMIO)) {
+            VirtIOMMIOProxy *mmio = VIRTIO_MMIO(OBJECT(dev));
+            VirtioBusState *mmio_virtio_bus = &mmio->bus;
+            BusState *mmio_bus = &mmio_virtio_bus->parent_obj;
+            return mmio_bus;
+        }
+    }
+
+    return NULL;
+}
+
+static void nitro_enclave_devices_init(NitroEnclaveMachineState *nems)
+{
+    DeviceState *dev = qdev_new(TYPE_VHOST_VSOCK);
+    BusState *bus = find_virtio_mmio_bus();
+    if (!bus) {
+        error_report("Failed to find bus for vhost-vsock device.");
+        exit(1);
+    }
+
+    if (nems->guest_cid <= 3) {
+        error_report("Nitro enclave machine option 'guest-cid' must be set "
+                     "with a value greater than or equal to 4");
+        exit(1);
+    }
+
+    qdev_prop_set_uint64(dev, "guest-cid", nems->guest_cid);
+    qdev_realize_and_unref(dev, bus, &error_fatal);
+}
+
+static void nitro_enclave_machine_state_init(MachineState *machine)
+{
+    NitroEnclaveMachineClass *ne_class =
+        NITRO_ENCLAVE_MACHINE_GET_CLASS(machine);
+    NitroEnclaveMachineState *ne_state = NITRO_ENCLAVE_MACHINE(machine);
+
+    ne_class->parent_init(machine);
+    nitro_enclave_devices_init(ne_state);
+}
+
+static void nitro_enclave_machine_initfn(Object *obj)
+{
+    NitroEnclaveMachineState *nems = NITRO_ENCLAVE_MACHINE(obj);
+    MicrovmMachineState *mms = MICROVM_MACHINE(obj);
+    X86MachineState *x86ms = X86_MACHINE(obj);
+
+    nems->guest_cid = 0;
+
+    /* AWS nitro enclaves have PCIE and ACPI disabled */
+    mms->pcie = ON_OFF_AUTO_OFF;
+    x86ms->acpi = ON_OFF_AUTO_OFF;
+}
+
+static void nitro_enclave_get_guest_cid(Object *obj, Visitor *v,
+                                        const char *name, void *opaque,
+                                        Error **errp)
+{
+    NitroEnclaveMachineState *nems = NITRO_ENCLAVE_MACHINE(obj);
+    uint32_t guest_cid = nems->guest_cid;
+
+    visit_type_uint32(v, name, &guest_cid, errp);
+}
+
+static void nitro_enclave_set_guest_cid(Object *obj, Visitor *v,
+                                        const char *name, void *opaque,
+                                        Error **errp)
+{
+    NitroEnclaveMachineState *nems = NITRO_ENCLAVE_MACHINE(obj);
+
+    visit_type_uint32(v, name, &nems->guest_cid, errp);
+}
+
+static void nitro_enclave_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    NitroEnclaveMachineClass *nemc = NITRO_ENCLAVE_MACHINE_CLASS(oc);
+
+    mc->family = "nitro_enclave_i386";
+    mc->desc = "AWS Nitro Enclave";
+
+    nemc->parent_init = mc->init;
+    mc->init = nitro_enclave_machine_state_init;
+
+    object_class_property_add(oc, NITRO_ENCLAVE_GUEST_CID, "uint32_t",
+                              nitro_enclave_get_guest_cid,
+                              nitro_enclave_set_guest_cid,
+                              NULL, NULL);
+    object_class_property_set_description(oc, NITRO_ENCLAVE_GUEST_CID,
+                                          "Set enclave machine's cid");
+}
+
+static const TypeInfo nitro_enclave_machine_info = {
+    .name          = TYPE_NITRO_ENCLAVE_MACHINE,
+    .parent        = TYPE_MICROVM_MACHINE,
+    .instance_size = sizeof(NitroEnclaveMachineState),
+    .instance_init = nitro_enclave_machine_initfn,
+    .class_size    = sizeof(NitroEnclaveMachineClass),
+    .class_init    = nitro_enclave_class_init,
+};
+
+static void nitro_enclave_machine_init(void)
+{
+    type_register_static(&nitro_enclave_machine_info);
+}
+type_init(nitro_enclave_machine_init);
diff --git a/include/hw/i386/nitro_enclave.h b/include/hw/i386/nitro_enclave.h
new file mode 100644
index 0000000000..9dc3afd494
--- /dev/null
+++ b/include/hw/i386/nitro_enclave.h
@@ -0,0 +1,38 @@
+/*
+ * AWS nitro-enclave machine
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#ifndef HW_I386_NITRO_ENCLAVE_H
+#define HW_I386_NITRO_ENCLAVE_H
+
+#include "hw/boards.h"
+#include "hw/i386/microvm.h"
+#include "qom/object.h"
+
+/* Machine type options */
+#define NITRO_ENCLAVE_GUEST_CID  "guest-cid"
+
+struct NitroEnclaveMachineClass {
+    MicrovmMachineClass parent;
+
+    void (*parent_init)(MachineState *state);
+};
+
+struct NitroEnclaveMachineState {
+    MicrovmMachineState parent;
+
+    /* Machine type options */
+    uint32_t guest_cid;
+};
+
+#define TYPE_NITRO_ENCLAVE_MACHINE MACHINE_TYPE_NAME("nitro-enclave")
+OBJECT_DECLARE_TYPE(NitroEnclaveMachineState, NitroEnclaveMachineClass,
+                    NITRO_ENCLAVE_MACHINE)
+
+#endif
-- 
2.39.2

Re: [PATCH v1 0/2] AWS Nitro Enclave emulation support

[Dorjoy Chowdhury]

Hi,
I just noticed the reference URL for number [4] in my cover-letter is incorrect.

On Sat, May 18, 2024 at 2:08 PM Dorjoy Chowdhury <dorjoychy111@gmail.com> wrote:
>
> Hi,
>
> Hope everyone is doing well. I am working on adding AWS Nitro Enclave[1]
> emulation support in QEMU. Alexander Graf is mentoring me on this work. This is
> a patch series adding, not yet complete, but useful emulation support of nitro
> enclaves. I have a gitlab branch where you can view the patches in the gitlab
> web UI for each commit:
> https://gitlab.com/dorjoy03/qemu/-/tree/nitro-enclave-emulation
>
> AWS nitro enclaves is an Amazon EC2[2] feature that allows creating isolated
> execution environments, called enclaves, from Amazon EC2 instances, which are
> used for processing highly sensitive data. Enclaves have no persistent storage
> and no external networking. The enclave VMs are based on Firecracker microvm
> and have a vhost-vsock device for communication with the parent EC2 instance
> that spawned it and a Nitro Secure Module (NSM) device for cryptographic
> attestation. The parent instance VM always has CID 3 while the enclave VM gets
> a dynamic CID. The enclave VMs can communicate with the parent instance over
> various ports to CID 3, for example, the init process inside an enclave sends a
> heartbeat to port 9000 upon boot, expecting a heartbeat reply, letting the
> parent instance know that the enclave VM has successfully booted.
>
> From inside an EC2 instance, nitro-cli[3] is used to spawn an enclave VM using
> an EIF (Enclave Image Format)[4] file. EIF files can be built using nitro-cli
> as well. There is no official EIF specification apart from the github
> aws-nitro-enclaves-image-format repository[4]. An EIF file contains the kernel,
> cmdline and ramdisk(s) in different sections which are used to boot the enclave
> VM. You can look at the structs in hw/i386/eif.c file for more details about
> the EIF file format.
>
> Adding nitro enclave emulation support in QEMU will make the life of AWS Nitro
> Enclave users easier as they will be able to test their EIF images locally
> without having to run real nitro enclaves which can be difficult for debugging
> due to its roots in security. This will also make quick prototyping easier.
>
> In QEMU, the new nitro-enclave machine type is implemented based on the microvm
> machine type similar to how AWS Nitro Enclaves are based on Firecracker microvm.
> The vhost-vsock device support is already part of this patch series so that the
> enclave VM can communicate to CID 3 using vsock. A mandatory 'guest-cid'
> machine type option is needed which becomes the CID of the enclave VM. Some
> documentation for the new 'nitro-enclave' machine type has also been added. The
> NSM device support will be added in the future.
>
> The plan is to eventually make the nitro enclave emulation in QEMU standalone
> i.e., without needing to run another VM with CID 3 with proper vsock
> communication support. For this to work, one approach could be to teach the
> vhost-vsock driver in kernel to forward CID 3 messages to another CID
> (set to CID 2 for host) so that users of the nitro-enclave machine type can
> run the necessary vsock server/clients in the host machine (some defaults can
> be implemented in QEMU as well, for example, sending a reply to the heartbeat)
> which will rid them of the cumbersome way of running another whole VM with CID
> 3. This way, users of nitro-enclave machine in QEMU, could potentially also run
> multiple enclaves with their messages for CID 3 forwarded to different CIDs
> which, in QEMU side, could then be specified using a new machine type option
> (parent-cid) if implemented. I will be posting an email to the linux
> virtualization mailing list about this approach asking for feedback and
> suggestions soon.
>
> For local testing you need to generate a hello.eif image by first building
> nitro-cli locally[5]. Then you can use nitro-cli to build a hello.eif image[6].
>
> You need to build qemu-system-x86_64 after applying the patches and then you
> can run the following command to boot a hello.eif image using the new
> 'nitro-enclave' machine type option in QEMU:
>
> sudo ./qemu-system-x86_64 -M nitro-enclave,guest-cid=8 -kernel path/to/hello.eif -nographic -m 4G --enable-kvm -cpu host
>
> The command needs to be run as sudo because for the vhost-vsock device to work
> QEMU needs to be able to open vhost device in host.
>
> Right now, if you just run the nitro-enclave machine, the kernel panics because
> the init process exits abnormally because it cannot connect to port 9000 to CID
> 3 to send its heartbeat message (the connection times out), so another VM with
> CID 3 with proper vsock communication support must be run for it to be useful.
> But this restriction can be lifted once the approach about forwarding CID 3
> messages is implemented if it gets accepted.
>
> Thanks.
>
> Regards,
> Dorjoy
>
> [1] https://docs.aws.amazon.com/enclaves/latest/user/nitro-enclave.html
> [2] https://aws.amazon.com/ec2/
> [3] https://docs.aws.amazon.com/enclaves/latest/user/getting-started.html
> [4] https://docs.aws.amazon.com/enclaves/latest/user/getting-started.html

It should be this instead:
[4] https://github.com/aws/aws-nitro-enclaves-image-format

Sorry about that.

Regards,
Dorjoy

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

[Daniel P. Berrangé]

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.

> 
> 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
> [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 +-
>  hw/i386/microvm.c   | 134 +++++++++++-
>  4 files changed, 640 insertions(+), 2 deletions(-)
>  create mode 100644 hw/i386/eif.c
>  create mode 100644 hw/i386/eif.h
> 
> diff --git a/hw/i386/eif.c b/hw/i386/eif.c
> new file mode 100644
> index 0000000000..761c489d33
> --- /dev/null
> +++ b/hw/i386/eif.c
> @@ -0,0 +1,486 @@
> +/*
> + * EIF (Enclave Image Format) related helpers
> + *
> + * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/bswap.h"
> +#include "qapi/error.h"
> +#include <zlib.h> /* for crc32 */
> +
> +#include "hw/i386/eif.h"
> +
> +#define MAX_SECTIONS 32
> +
> +/* members are ordered according to field order in .eif file */
> +typedef struct EifHeader {
> +    uint8_t  magic[4]; /* must be .eif in ascii i.e., [46, 101, 105, 102] */
> +    uint16_t version;
> +    uint16_t flags;
> +    uint64_t default_memory;
> +    uint64_t default_cpus;
> +    uint16_t reserved;
> +    uint16_t section_cnt;
> +    uint64_t section_offsets[MAX_SECTIONS];
> +    uint64_t section_sizes[MAX_SECTIONS];
> +    uint32_t unused;
> +    uint32_t eif_crc32;
> +} QEMU_PACKED EifHeader;
> +
> +/* members are ordered according to field order in .eif file */
> +typedef struct EifSectionHeader {
> +    /*
> +     * 0 = invalid, 1 = kernel, 2 = cmdline, 3 = ramdisk, 4 = signature,
> +     * 5 = metadata
> +     */
> +    uint16_t section_type;
> +    uint16_t flags;
> +    uint64_t section_size;
> +} QEMU_PACKED EifSectionHeader;
> +
> +enum EifSectionTypes {
> +    EIF_SECTION_INVALID = 0,
> +    EIF_SECTION_KERNEL = 1,
> +    EIF_SECTION_CMDLINE = 2,
> +    EIF_SECTION_RAMDISK = 3,
> +    EIF_SECTION_SIGNATURE = 4,
> +    EIF_SECTION_METADATA = 5,
> +    EIF_SECTION_MAX = 6,
> +};
> +
> +static const char *section_type_to_string(uint16_t type)
> +{
> +    const char *str;
> +    switch (type) {
> +    case EIF_SECTION_INVALID:
> +        str = "invalid";
> +        break;
> +    case EIF_SECTION_KERNEL:
> +        str = "kernel";
> +        break;
> +    case EIF_SECTION_CMDLINE:
> +        str = "cmdline";
> +        break;
> +    case EIF_SECTION_RAMDISK:
> +        str = "ramdisk";
> +        break;
> +    case EIF_SECTION_SIGNATURE:
> +        str = "signature";
> +        break;
> +    case EIF_SECTION_METADATA:
> +        str = "metadata";
> +        break;
> +    default:
> +        str = "unknown";
> +        break;
> +    }
> +
> +    return str;
> +}
> +
> +static bool read_eif_header(FILE *f, EifHeader *header, uint32_t *crc,
> +                            Error **errp)
> +{
> +    size_t got;
> +    size_t header_size = sizeof(*header);
> +
> +    got = fread(header, 1, header_size, f);
> +    if (got != header_size) {
> +        error_setg(errp, "Failed to read EIF header");
> +        return false;
> +    }
> +
> +    if (memcmp(header->magic, ".eif", 4) != 0) {
> +        error_setg(errp, "Invalid EIF image. Magic mismatch.");
> +        return false;
> +    }
> +
> +    /* Exclude header->eif_crc32 field from CRC calculation */
> +    *crc = crc32(*crc, (uint8_t *)header, header_size - 4);
> +
> +    header->version = be16_to_cpu(header->version);
> +    header->flags = be16_to_cpu(header->flags);
> +    header->default_memory = be64_to_cpu(header->default_memory);
> +    header->default_cpus = be64_to_cpu(header->default_cpus);
> +    header->reserved = be16_to_cpu(header->reserved);
> +    header->section_cnt = be16_to_cpu(header->section_cnt);
> +
> +    for (int i = 0; i < MAX_SECTIONS; ++i) {
> +        header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
> +    }
> +
> +    for (int i = 0; i < MAX_SECTIONS; ++i) {
> +        header->section_sizes[i] = be64_to_cpu(header->section_sizes[i]);
> +    }
> +
> +    header->unused = be32_to_cpu(header->unused);
> +    header->eif_crc32 = be32_to_cpu(header->eif_crc32);
> +    return true;
> +}
> +
> +static bool read_eif_section_header(FILE *f, EifSectionHeader *section_header,
> +                                    uint32_t *crc, Error **errp)
> +{
> +    size_t got;
> +    size_t section_header_size = sizeof(*section_header);
> +
> +    got = fread(section_header, 1, section_header_size, f);
> +    if (got != section_header_size) {
> +        error_setg(errp, "Failed to read EIF section header");
> +        return false;
> +    }
> +
> +    *crc = crc32(*crc, (uint8_t *)section_header, section_header_size);
> +
> +    section_header->section_type = be16_to_cpu(section_header->section_type);
> +    section_header->flags = be16_to_cpu(section_header->flags);
> +    section_header->section_size = be64_to_cpu(section_header->section_size);
> +    return true;
> +}
> +
> +/*
> + * Upon success, the caller is responsible for unlinking and freeing *tmp_path.
> + */
> +static bool get_tmp_file(const char *template, char **tmp_path, Error **errp)
> +{
> +    int tmp_fd;
> +
> +    *tmp_path = NULL;
> +    tmp_fd = g_file_open_tmp(template, tmp_path, NULL);
> +    if (tmp_fd < 0 || *tmp_path == NULL) {
> +        error_setg(errp, "Failed to create temporary file for template %s",
> +                   template);
> +        return false;
> +    }
> +
> +    close(tmp_fd);
> +    return true;
> +}
> +
> +static void safe_fclose(FILE *f)
> +{
> +    if (f) {
> +        fclose(f);
> +    }
> +}
> +
> +static void safe_unlink(char *f)
> +{
> +    if (f) {
> +        unlink(f);
> +    }
> +}
> +
> +/*
> + * Upon success, the caller is reponsible for unlinking and freeing *kernel_path
> + */
> +static bool read_eif_kernel(FILE *f, uint64_t size, char **kernel_path,
> +                            uint32_t *crc, Error **errp)
> +{
> +    size_t got;
> +    FILE *tmp_file = NULL;
> +    uint8_t *kernel = NULL;
> +
> +    *kernel_path = NULL;
> +    if (!get_tmp_file("eif-kernel-XXXXXX", kernel_path, errp)) {
> +        goto cleanup;
> +    }
> +
> +    tmp_file = fopen(*kernel_path, "wb");
> +    if (tmp_file == NULL) {
> +        error_setg_errno(errp, errno, "Failed to open temporary file %s",
> +                         *kernel_path);
> +        goto cleanup;
> +    }
> +
> +    kernel = g_malloc(size);
> +    got = fread(kernel, 1, size, f);
> +    if ((uint64_t) got != size) {
> +        error_setg(errp, "Failed to read EIF kernel section data");
> +        goto cleanup;
> +    }
> +
> +    got = fwrite(kernel, 1, size, tmp_file);
> +    if ((uint64_t) got != size) {
> +        error_setg(errp, "Failed to write EIF kernel section data to temporary"
> +                   " file");
> +        goto cleanup;
> +    }
> +
> +    *crc = crc32(*crc, kernel, size);
> +    g_free(kernel);
> +    fclose(tmp_file);
> +
> +    return true;
> +
> + cleanup:
> +    safe_fclose(tmp_file);
> +
> +    safe_unlink(*kernel_path);
> +    g_free(*kernel_path);
> +    *kernel_path = NULL;
> +
> +    g_free(kernel);
> +    return false;
> +}
> +
> +static bool read_eif_cmdline(FILE *f, uint64_t size, char *cmdline,
> +                             uint32_t *crc, Error **errp)
> +{
> +    size_t got = fread(cmdline, 1, size, f);
> +    if ((uint64_t) got != size) {
> +        error_setg(errp, "Failed to read EIF cmdline section data");
> +        return false;
> +    }
> +
> +    *crc = crc32(*crc, (uint8_t *)cmdline, size);
> +    return true;
> +}
> +
> +static bool read_eif_ramdisk(FILE *eif, FILE *initrd, uint64_t size,
> +                             uint32_t *crc, Error **errp)
> +{
> +    size_t got;
> +    uint8_t *ramdisk = g_malloc(size);
> +
> +    got = fread(ramdisk, 1, size, eif);
> +    if ((uint64_t) got != size) {
> +        error_setg(errp, "Failed to read EIF ramdisk section data");
> +        goto cleanup;
> +    }
> +
> +    got = fwrite(ramdisk, 1, size, initrd);
> +    if ((uint64_t) got != size) {
> +        error_setg(errp, "Failed to write EIF ramdisk data to temporary file");
> +        goto cleanup;
> +    }
> +
> +    *crc = crc32(*crc, ramdisk, size);
> +    g_free(ramdisk);
> +    return true;
> +
> + cleanup:
> +    g_free(ramdisk);
> +    return false;
> +}
> +
> +/*
> + * Upon success, the caller is reponsible for unlinking and freeing
> + * *kernel_path, *initrd_path and freeing *cmdline.
> + */
> +bool read_eif_file(const char *eif_path, char **kernel_path, char **initrd_path,
> +                   char **cmdline, Error **errp)
> +{
> +    FILE *f = NULL;
> +    FILE *initrd_f = NULL;
> +    uint32_t crc = 0;
> +    EifHeader eif_header;
> +    bool seen_sections[EIF_SECTION_MAX] = {false};
> +
> +    *kernel_path = *initrd_path = *cmdline = NULL;
> +
> +    f = fopen(eif_path, "rb");
> +    if (f == NULL) {
> +        error_setg_errno(errp, errno, "Failed to open %s", eif_path);
> +        goto cleanup;
> +    }
> +
> +    if (!read_eif_header(f, &eif_header, &crc, errp)) {
> +        goto cleanup;
> +    }
> +
> +    if (eif_header.version < 4) {
> +        error_setg(errp, "Expected EIF version 4 or greater");
> +        goto cleanup;
> +    }
> +
> +    if (eif_header.flags != 0) {
> +        error_setg(errp, "Expected EIF flags to be 0");
> +        goto cleanup;
> +    }
> +
> +    if (eif_header.section_cnt > MAX_SECTIONS) {
> +        error_setg(errp, "EIF header section count must not be greater than "
> +                   "%d but found %d", MAX_SECTIONS, eif_header.section_cnt);
> +        goto cleanup;
> +    }
> +
> +    for (int i = 0; i < eif_header.section_cnt; ++i) {
> +        EifSectionHeader section_header;
> +        uint16_t section_type;
> +
> +        if (fseek(f, eif_header.section_offsets[i], SEEK_SET) != 0) {
> +            error_setg_errno(errp, errno, "Failed to offset to %lu in EIF file",
> +                             eif_header.section_offsets[i]);
> +            goto cleanup;
> +        }
> +
> +        if (!read_eif_section_header(f, &section_header, &crc, errp)) {
> +            goto cleanup;
> +        }
> +
> +        if (section_header.flags != 0) {
> +            error_setg(errp, "Expected EIF section header flags to be 0");
> +            goto cleanup;
> +        }
> +
> +        if (eif_header.section_sizes[i] != section_header.section_size) {
> +            error_setg(errp, "EIF section size mismatch between header and "
> +                       "section header: header %lu, section header %lu",
> +                       eif_header.section_sizes[i],
> +                       section_header.section_size);
> +            goto cleanup;
> +        }
> +
> +        section_type = section_header.section_type;
> +
> +        switch (section_type) {
> +        case EIF_SECTION_KERNEL:
> +            if (seen_sections[EIF_SECTION_KERNEL]) {
> +                error_setg(errp, "Invalid EIF image. More than 1 kernel "
> +                           "section");
> +                goto cleanup;
> +            }
> +            if (!read_eif_kernel(f, section_header.section_size, kernel_path,
> +                                 &crc, errp)) {
> +                goto cleanup;
> +            }
> +
> +            break;
> +        case EIF_SECTION_CMDLINE:
> +        {
> +            uint64_t size;
> +            if (seen_sections[EIF_SECTION_CMDLINE]) {
> +                error_setg(errp, "Invalid EIF image. More than 1 cmdline "
> +                           "section");
> +                goto cleanup;
> +            }
> +            size = section_header.section_size;
> +            *cmdline = g_malloc(size + 1);
> +            if (!read_eif_cmdline(f, size, *cmdline, &crc, errp)) {
> +                goto cleanup;
> +            }
> +            (*cmdline)[size] = '\0';
> +
> +            break;
> +        }
> +        case EIF_SECTION_RAMDISK:
> +            if (!seen_sections[EIF_SECTION_RAMDISK]) {
> +                /*
> +                 * If this is the first time we are seeing a ramdisk section,
> +                 * we need to create the initrd temporary file.
> +                 */
> +                if (!get_tmp_file("eif-initrd-XXXXXX", initrd_path, errp)) {
> +                    goto cleanup;
> +                }
> +                initrd_f = fopen(*initrd_path, "wb");
> +                if (initrd_f == NULL) {
> +                    error_setg_errno(errp, errno, "Failed to open file %s",
> +                                     *initrd_path);
> +                    goto cleanup;
> +                }
> +            }
> +
> +            if (!read_eif_ramdisk(f, initrd_f, section_header.section_size,
> +                                  &crc, errp)) {
> +                goto cleanup;
> +            }
> +
> +            break;
> +        default:
> +            /* other sections including invalid or unknown sections */
> +        {
> +            uint8_t *buf;
> +            size_t got;
> +            uint64_t size = section_header.section_size;
> +            buf = g_malloc(size);
> +            got = fread(buf, 1, size, f);
> +            if ((uint64_t) got != size) {
> +                g_free(buf);
> +                error_setg(errp, "Failed to read EIF %s section data",
> +                           section_type_to_string(section_type));
> +                goto cleanup;
> +            }
> +            crc = crc32(crc, buf, size);
> +            g_free(buf);
> +            break;
> +        }
> +        }
> +
> +        if (section_type < EIF_SECTION_MAX) {
> +            seen_sections[section_type] = true;
> +        }
> +    }
> +
> +    if (!seen_sections[EIF_SECTION_KERNEL]) {
> +        error_setg(errp, "Invalid EIF image. No kernel section.");
> +        goto cleanup;
> +    }
> +    if (!seen_sections[EIF_SECTION_CMDLINE]) {
> +        error_setg(errp, "Invalid EIF image. No cmdline section.");
> +        goto cleanup;
> +    }
> +    if (!seen_sections[EIF_SECTION_RAMDISK]) {
> +        error_setg(errp, "Invalid EIF image. No ramdisk section.");
> +        goto cleanup;
> +    }
> +
> +    if (eif_header.eif_crc32 != crc) {
> +        error_setg(errp, "CRC mismatch. Expected %u but header has %u.",
> +                   crc, eif_header.eif_crc32);
> +        goto cleanup;
> +    }
> +
> +    fclose(f);
> +    fclose(initrd_f);
> +    return true;
> +
> + cleanup:
> +    safe_fclose(f);
> +    safe_fclose(initrd_f);
> +
> +    safe_unlink(*kernel_path);
> +    g_free(*kernel_path);
> +    *kernel_path = NULL;
> +
> +    safe_unlink(*initrd_path);
> +    g_free(*initrd_path);
> +    *initrd_path = NULL;
> +
> +    g_free(*cmdline);
> +    *cmdline = NULL;
> +
> +    return false;
> +}
> +
> +bool check_if_eif_file(const char *path, bool *is_eif, Error **errp)
> +{
> +    size_t got;
> +    uint8_t buf[4];
> +    FILE *f = NULL;
> +
> +    f = fopen(path, "rb");
> +    if (f == NULL) {
> +        error_setg_errno(errp, errno, "Failed to open file %s", path);
> +        goto cleanup;
> +    }
> +
> +    got = fread(buf, 1, 4, f);
> +    if (got != 4) {
> +        error_setg(errp, "Failed to read magic value from %s", path);
> +        goto cleanup;
> +    }
> +
> +    fclose(f);
> +    *is_eif = !memcmp(buf, ".eif", 4);
> +    return true;
> +
> + cleanup:
> +    safe_fclose(f);
> +    return false;
> +}
> diff --git a/hw/i386/eif.h b/hw/i386/eif.h
> new file mode 100644
> index 0000000000..b71a27062d
> --- /dev/null
> +++ b/hw/i386/eif.h
> @@ -0,0 +1,20 @@
> +/*
> + * EIF (Enclave Image Format) related helpers
> + *
> + * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#ifndef HW_I386_EIF_H
> +#define HW_I386_EIF_H
> +
> +bool read_eif_file(const char *eif_path, char **kernel_path, char **initrd_path,
> +                    char **kernel_cmdline, Error **errp);
> +
> +bool check_if_eif_file(const char *path, bool *is_eif, Error **errp);
> +
> +#endif
> +
> diff --git a/hw/i386/meson.build b/hw/i386/meson.build
> index 03aad10df7..e398fc1d74 100644
> --- a/hw/i386/meson.build
> +++ b/hw/i386/meson.build
> @@ -14,7 +14,7 @@ i386_ss.add(when: 'CONFIG_X86_IOMMU', if_true: files('x86-iommu.c'),
>  i386_ss.add(when: 'CONFIG_AMD_IOMMU', if_true: files('amd_iommu.c'),
>                                        if_false: files('amd_iommu-stub.c'))
>  i386_ss.add(when: 'CONFIG_I440FX', if_true: files('pc_piix.c'))
> -i386_ss.add(when: 'CONFIG_MICROVM', if_true: files('x86-common.c', 'microvm.c', 'acpi-microvm.c', 'microvm-dt.c'))
> +i386_ss.add(when: 'CONFIG_MICROVM', if_true: files('x86-common.c', 'microvm.c', 'acpi-microvm.c', 'microvm-dt.c', 'eif.c'))
>  i386_ss.add(when: 'CONFIG_Q35', if_true: files('pc_q35.c'))
>  i386_ss.add(when: 'CONFIG_VMMOUSE', if_true: files('vmmouse.c'))
>  i386_ss.add(when: 'CONFIG_VMPORT', if_true: files('vmport.c'))
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index fec63cacfa..d52d85bd01 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -34,6 +34,7 @@
>  #include "hw/irq.h"
>  #include "hw/i386/kvm/clock.h"
>  #include "hw/i386/microvm.h"
> +#include "hw/i386/eif.h"
>  #include "hw/i386/x86.h"
>  #include "target/i386/cpu.h"
>  #include "hw/intc/i8259.h"
> @@ -281,6 +282,127 @@ static void microvm_devices_init(MicrovmMachineState *mms)
>      x86_bios_rom_init(x86ms, default_firmware, get_system_memory(), true);
>  }
>  
> +/* Expects file to have offset 0 before this function is called */
> +static long get_file_size(FILE *f, Error **errp)
> +{
> +    long size;
> +
> +    if (fseek(f, 0, SEEK_END) != 0) {
> +        error_setg_errno(errp, errno, "Failed to seek to the end of file");
> +        return -1;
> +    }
> +
> +    size = ftell(f);
> +    if (size == -1) {
> +        error_setg_errno(errp, errno, "Failed to get offset");
> +        return -1;
> +    }
> +
> +    if (fseek(f, 0, SEEK_SET) != 0) {
> +        error_setg_errno(errp, errno, "Failed to seek back to the start");
> +        return -1;
> +    }
> +
> +    return size;
> +}
> +
> +static void load_eif(MicrovmMachineState *mms, FWCfgState *fw_cfg)
> +{
> +    Error *err;
> +    char *eif_kernel, *eif_initrd, *eif_cmdline;
> +    MachineState *machine = MACHINE(mms);
> +    X86MachineState *x86ms = X86_MACHINE(mms);
> +
> +    if (!read_eif_file(machine->kernel_filename, &eif_kernel, &eif_initrd,
> +                       &eif_cmdline, &err)) {
> +        error_report_err(err);
> +        exit(1);
> +    }
> +
> +    g_free(machine->kernel_filename);
> +    machine->kernel_filename = eif_kernel;
> +
> +    /*
> +     * If an initrd argument was provided, let's concatenate it to the
> +     * extracted EIF initrd temporary file.
> +     */
> +    if (machine->initrd_filename) {
> +        long size;
> +        size_t got;
> +        uint8_t *buf;
> +        FILE *initrd_f, *eif_initrd_f;
> +
> +        initrd_f = fopen(machine->initrd_filename, "rb");
> +        if (initrd_f == NULL) {
> +            error_setg_errno(&err, errno, "Failed to open initrd file %s",
> +                             machine->initrd_filename);
> +            goto cleanup;
> +        }
> +
> +        size = get_file_size(initrd_f, &err);
> +        if (size == -1) {
> +            goto cleanup;
> +        }
> +
> +        buf = g_malloc(size);
> +        got = fread(buf, 1, size, initrd_f);
> +        if ((uint64_t) got != (uint64_t) size) {
> +            error_setg(&err, "Failed to read initrd file %s",
> +                       machine->initrd_filename);
> +            goto cleanup;
> +        }
> +
> +        eif_initrd_f = fopen(eif_initrd, "ab");
> +        if (eif_initrd_f == NULL) {
> +            error_setg_errno(&err, errno, "Failed to open EIF initrd file %s",
> +                             eif_initrd);
> +            goto cleanup;
> +        }
> +        got = fwrite(buf, 1, size, eif_initrd_f);
> +        if ((uint64_t) got != (uint64_t) size) {
> +            error_setg(&err, "Failed to append initrd %s to %s",
> +                       machine->initrd_filename, eif_initrd);
> +            goto cleanup;
> +        }
> +
> +        fclose(initrd_f);
> +        fclose(eif_initrd_f);
> +
> +        g_free(buf);
> +        g_free(machine->initrd_filename);
> +
> +        machine->initrd_filename = eif_initrd;
> +    } else {
> +        machine->initrd_filename = eif_initrd;
> +    }
> +
> +    /*
> +     * If kernel cmdline argument was provided, let's concatenate it to the
> +     * extracted EIF kernel cmdline.
> +     */
> +    if (machine->kernel_cmdline != NULL) {
> +        char *cmd = g_strdup_printf("%s %s", eif_cmdline,
> +                                    machine->kernel_cmdline);
> +        g_free(eif_cmdline);
> +        g_free(machine->kernel_cmdline);
> +        machine->kernel_cmdline = cmd;
> +    } else {
> +        machine->kernel_cmdline = eif_cmdline;
> +    }
> +
> +    x86_load_linux(x86ms, fw_cfg, 0, true);
> +
> +    unlink(machine->kernel_filename);
> +    unlink(machine->initrd_filename);
> +    return;
> +
> + cleanup:
> +    error_report_err(err);
> +    unlink(eif_kernel);
> +    unlink(eif_initrd);
> +    exit(1);
> +}
> +
>  static void microvm_memory_init(MicrovmMachineState *mms)
>  {
>      MachineState *machine = MACHINE(mms);
> @@ -330,7 +452,17 @@ static void microvm_memory_init(MicrovmMachineState *mms)
>      rom_set_fw(fw_cfg);
>  
>      if (machine->kernel_filename != NULL) {
> -        x86_load_linux(x86ms, fw_cfg, 0, true);
> +        Error *err;
> +        bool is_eif = false;
> +        if (!check_if_eif_file(machine->kernel_filename, &is_eif, &err)) {
> +            error_report_err(err);
> +            exit(1);
> +        }
> +        if (is_eif) {
> +            load_eif(mms, fw_cfg);
> +        } else {
> +            x86_load_linux(x86ms, fw_cfg, 0, true);
> +        }
>      }
>  
>      if (mms->option_roms) {
> -- 
> 2.39.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dorjoy Chowdhury]

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?

Thanks.

Regards,
Dorjoy

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

[Philippe Mathieu-Daudé]

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

> [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]?

>   hw/i386/microvm.c   | 134 +++++++++++-
>   4 files changed, 640 insertions(+), 2 deletions(-)
>   create mode 100644 hw/i386/eif.c
>   create mode 100644 hw/i386/eif.h

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

[Dorjoy Chowdhury]

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?

Regards,
Dorjoy

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

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

[Dorjoy Chowdhury]

Hey Alex,

On Mon, May 27, 2024 at 9:51 PM Alexander Graf <graf@amazon.com> wrote:
>
>
> 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.
>

Understood. I will move the eif.c and eif.h files to the hw/core
directory. Thanks.

Regards,
Dorjoy

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

[Alexander Graf]

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.

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

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

[Kevin Wolf]

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