On Fri, 2024-03-01 at 16:51 +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:10PM +0000, Roy Hopkins wrote:
> > This commit adds an implementation of an IGVM loader which parses the
> > file specified as a pararameter to ConfidentialGuestSupport and provides
> > a function that uses the interface in the same object to configure and
> > populate guest memory based on the contents of the file.
> >
> > The IGVM file is parsed when a filename is provided but the code to
> > process the IGVM file is not yet hooked into target systems. This will
> > follow in a later commit.
> >
> > Signed-off-by: Roy Hopkins <roy.hopk...@suse.com>
> > ---
> > backends/confidential-guest-support.c | 4 +
> > backends/igvm.c | 718 ++++++++++++++++++++++
> > backends/meson.build | 1 +
> > include/exec/confidential-guest-support.h | 5 +
> > include/exec/igvm.h | 35 ++
> > 5 files changed, 763 insertions(+)
> > create mode 100644 backends/igvm.c
> > create mode 100644 include/exec/igvm.h
> >
> > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > guest-support.c
> > index 42628be8d7..cb7651a8d0 100644
> > --- a/backends/confidential-guest-support.c
> > +++ b/backends/confidential-guest-support.c
> > @@ -15,6 +15,7 @@
> >
> > #include "exec/confidential-guest-support.h"
> > #include "qemu/error-report.h"
> > +#include "exec/igvm.h"
> >
> > OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> > confidential_guest_support,
> > @@ -33,6 +34,9 @@ static void set_igvm(Object *obj, const char *value, Error
> > **errp)
> > ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> > g_free(cgs->igvm_filename);
> > cgs->igvm_filename = g_strdup(value);
> > +#if defined(CONFIG_IGVM)
> > + igvm_file_init(cgs);
> > +#endif
> > }
> > #endif
> >
> > diff --git a/backends/igvm.c b/backends/igvm.c
> > new file mode 100644
> > index 0000000000..a6261d796f
> > --- /dev/null
> > +++ b/backends/igvm.c
> > @@ -0,0 +1,718 @@
> > +/*
> > + * QEMU IGVM configuration backend for Confidential Guests
> > + *
> > + * Copyright (C) 2023-2024 SUSE
> > + *
> > + * Authors:
> > + * Roy Hopkins <roy.hopk...@suse.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#if defined(CONFIG_IGVM)
> > +
> > +#include "exec/confidential-guest-support.h"
> > +#include "qemu/queue.h"
> > +#include "qemu/typedefs.h"
> > +
> > +#include "exec/igvm.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/boards.h"
> > +#include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> > +
> > +#include <igvm/igvm.h>
> > +#include <igvm/igvm_defs.h>
> > +#include <linux/kvm.h>
> > +
> > +typedef struct IgvmParameterData {
> > + QTAILQ_ENTRY(IgvmParameterData) next;
> > + uint8_t *data;
> > + uint32_t size;
> > + uint32_t index;
> > +} IgvmParameterData;
> > +
> > +static QTAILQ_HEAD(, IgvmParameterData) parameter_data;
> > +
> > +static void directive_page_data(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_vp_context(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_parameter_insert(ConfidentialGuestSupport *cgs, int
> > i,
> > + uint32_t compatibility_mask);
> > +static void directive_memory_map(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_vp_count(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +static void directive_environment_info(ConfidentialGuestSupport *cgs, int
> > i,
> > + uint32_t compatibility_mask);
> > +static void directive_required_memory(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +
> > +struct IGVMDirectiveHandler {
> > + uint32_t type;
> > + void (*handler)(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask);
> > +};
> > +
> > +static struct IGVMDirectiveHandler directive_handlers[] = {
> > + { IGVM_VHT_PAGE_DATA, directive_page_data },
> > + { IGVM_VHT_VP_CONTEXT, directive_vp_context },
> > + { IGVM_VHT_PARAMETER_AREA, directive_parameter_area },
> > + { IGVM_VHT_PARAMETER_INSERT, directive_parameter_insert },
> > + { IGVM_VHT_MEMORY_MAP, directive_memory_map },
> > + { IGVM_VHT_VP_COUNT_PARAMETER, directive_vp_count },
> > + { IGVM_VHT_ENVIRONMENT_INFO_PARAMETER, directive_environment_info },
> > + { IGVM_VHT_REQUIRED_MEMORY, directive_required_memory },
> > +};
> > +
> > +static void directive(uint32_t type, ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask)
> > +{
> > + size_t handler;
> > + for (handler = 0; handler < (sizeof(directive_handlers) /
> > + sizeof(struct IGVMDirectiveHandler));
> > + ++handler) {
> > + if (directive_handlers[handler].type == type) {
> > + directive_handlers[handler].handler(cgs, i,
> > compatibility_mask);
> > + return;
> > + }
> > + }
> > + warn_report("IGVM: Unknown directive encountered when processing file:
> > %X",
> > + type);
> > +}
>
> If a directive in a IGVM file has functional effects on the
> guest behaviour it does not feel right to just ignore it and
> carry on launching the guest in a likely incorrect state.
>
> IOW, I think this should be a error, not a warning, and
> this method ought to have an 'Error **errp' parameter.
Agreed. I'll update the code to generate errors on any failure or unknown
directives/configuration from the IGVM file.
>
> > +
> > +static void igvm_handle_error(int32_t result, const char *msg)
> > +{
> > + if (result < 0) {
> > + error_report("Processing of IGVM file failed: %s (code: %d)", msg,
> > + (int)result);
> > + exit(EXIT_FAILURE);
> > + }
> > +}
>
> IMHO all the code below should have "Error **errp" parameters
> to report and return to the caller. The top level CGS code
> that calls into IVGM can use 'error_report_err'. As such
> I think this method shouldn't exist, and code shoud directly
> call error_setg.
Right, V2 will include a rework of the error handling using a combination of
return codes and "Error **errp" as recommended in "error.h". This method has
been removed as part of that rework.
>
> > +
> > +static void *igvm_prepare_memory(uint64_t addr, uint64_t size,
> > + int region_identifier)
> > +{
> > + MemoryRegion *igvm_pages = NULL;
> > + Int128 gpa_region_size;
> > + MemoryRegionSection mrs =
> > + memory_region_find(get_system_memory(), addr, size);
> > + if (mrs.mr) {
> > + if (!memory_region_is_ram(mrs.mr)) {
> > + memory_region_unref(mrs.mr);
> > + error_report(
> > + "Processing of IGVM file failed: Could not prepare memory "
> > + "at address 0x%lX due to existing non-RAM region",
> > + addr);
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + gpa_region_size = int128_make64(size);
> > + if (int128_lt(mrs.size, gpa_region_size)) {
> > + memory_region_unref(mrs.mr);
> > + error_report(
> > + "Processing of IGVM file failed: Could not prepare memory "
> > + "at address 0x%lX: region size exceeded",
> > + addr);
> > + exit(EXIT_FAILURE);
> > + }
> > + return qemu_map_ram_ptr(mrs.mr->ram_block,
> > mrs.offset_within_region);
> > + } else {
> > + /*
> > + * The region_identifier is the is the index of the IGVM directive
> > that
> > + * contains the page with the lowest GPA in the region. This will
> > + * generate a unique region name.
> > + */
> > + char region_name[22];
> > + snprintf(region_name, sizeof(region_name), "igvm.%X",
> > + region_identifier);
>
> IMO it is preferrable to use
>
> g_autofree char *region_name = g_strdup_printf("igvm.%X",
> region_identifier);
Ok, I'll make this change.
>
> > + igvm_pages = g_malloc(sizeof(*igvm_pages));
> > + memory_region_init_ram(igvm_pages, NULL, region_name, size,
> > + &error_fatal);
> > + memory_region_add_subregion(get_system_memory(), addr, igvm_pages);
> > + return memory_region_get_ram_ptr(igvm_pages);
> > + }
> > +}
> > +
> > +static int igvm_type_to_cgs_type(IgvmPageDataType memory_type, bool
> > unmeasured,
> > + bool zero)
> > +{
> > + switch (memory_type) {
> > + case NORMAL: {
> > + if (unmeasured) {
> > + return CGS_PAGE_TYPE_UNMEASURED;
> > + } else {
> > + return zero ? CGS_PAGE_TYPE_ZERO :
> > + CGS_PAGE_TYPE_NORMAL;
> > + }
> > + }
> > + case SECRETS:
> > + return CGS_PAGE_TYPE_SECRETS;
> > + case CPUID_DATA:
> > + return CGS_PAGE_TYPE_CPUID;
> > + case CPUID_XF:
> > + return CGS_PAGE_TYPE_CPUID;
> > + default:
> > + return CGS_PAGE_TYPE_UNMEASURED;
> > + }
> > +}
> > +
> > +static bool page_attrs_equal(const IGVM_VHS_PAGE_DATA *page_1,
> > + const IGVM_VHS_PAGE_DATA *page_2)
> > +{
> > + return ((*(const uint32_t *)&page_1->flags ==
> > + *(const uint32_t *)&page_2->flags) &&
> > + (page_1->data_type == page_2->data_type) &&
> > + (page_1->compatibility_mask == page_2->compatibility_mask));
> > +}
>
>
>
>
> > +void igvm_file_init(ConfidentialGuestSupport *cgs)
> > +{
> > + FILE *igvm_file = NULL;
> > + uint8_t *igvm_buf = NULL;
> > +
> > + if (cgs->igvm_filename) {
>
> Invert this condition and return immediately, so we
> don't have the entire method body uneccessarily indented.
Ok.
>
> > + IgvmHandle igvm;
> > + unsigned long igvm_length;
> > +
> > + igvm_file = fopen(cgs->igvm_filename, "rb");
> > + if (!igvm_file) {
> > + error_report("IGVM file not found '%s'", cgs->igvm_filename);
> > + goto error_out;
> > + }
> > +
> > + fseek(igvm_file, 0, SEEK_END);
> > + igvm_length = ftell(igvm_file);
> > + fseek(igvm_file, 0, SEEK_SET);
> > +
> > + igvm_buf = g_new(uint8_t, igvm_length);
> > + if (!igvm_buf) {
> > + error_report(
> > + "Could not allocate buffer to read file IGVM file '%s'",
> > + cgs->igvm_filename);
> > + goto error_out;
> > + }
> > + if (fread(igvm_buf, 1, igvm_length, igvm_file) != igvm_length) {
> > + error_report("Unable to load IGVM file '%s'", cgs-
> > >igvm_filename);
> > + goto error_out;
> > + }
> > +
> > + igvm = igvm_new_from_binary(igvm_buf, igvm_length);
> > + if (igvm < 0) {
> > + error_report("Parsing IGVM file '%s' failed with error_code
> > %d",
> > + cgs->igvm_filename, igvm);
> > + goto error_out;
> > + }
> > + fclose(igvm_file);
> > + g_free(igvm_buf);
> > +
> > + cgs->igvm = igvm;
> > + }
> > + return;
> > +
> > +error_out:
> > + free(igvm_buf);
> > + if (igvm_file) {
> > + fclose(igvm_file);
> > + }
> > + exit(EXIT_FAILURE);
> > +}
>
> This can be massively simplified to:
>
> g_autofree uint8_t *buf = NULL;
> unsigned long len;
> g_autoptr(GError) gerr = NULL;
>
> if (!cgs->igvm_filename) {
> return 0;
> }
>
> if (!g_file_get_contents(cgs->igvm_filename, (gchar**)&buf, &len, &gerr))
> {
> error_setg(errp, "Unable to load %s: %s", cgs->igvm_filename, gerr-
> >message);
> return -1;
> }
>
> if ((cgs->igvm = igvm_new_from_binary(buf, len)) < 0) {
> error_setg(errp, "Unable to parse IGVM %s: %s", cgs->igvm_filename,
> cgs->igvm);
> return -1;
> }
>
> return 0;
>
Much nicer! Thank you.
> > +
> > +void igvm_process(ConfidentialGuestSupport *cgs)
> > +{
> > + int32_t result;
> > + int i;
> > + uint32_t compatibility_mask;
> > + IgvmParameterData *parameter;
> > +
> > + /*
> > + * If this is not a Confidential guest or no IGVM has been provided
> > then
> > + * this is a no-op.
> > + */
> > + if (!cgs || !cgs->igvm) {
> > + return;
> > + }
>
> The caller should not be invoking this method if cgs is NULL.
Ok.
>
> > +
> > + QTAILQ_INIT(¶meter_data);
> > +
> > + /*
> > + * Check that the IGVM file provides configuration for the current
> > + * platform
> > + */
> > + compatibility_mask = supported_platform_compat_mask(cgs);
> > + if (compatibility_mask == 0) {
> > + error_report(
> > + "IGVM file does not describe a compatible supported platform");
> > + exit(EXIT_FAILURE);
> > + }
> > +
> > + result = igvm_header_count(cgs->igvm, HEADER_SECTION_DIRECTIVE);
> > + igvm_handle_error(result, "Failed to read directive header count");
> > + for (i = 0; i < (int)result; ++i) {
> > + IgvmVariableHeaderType type =
> > + igvm_get_header_type(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> > + directive(type, cgs, i, compatibility_mask);
> > + }
> > +
> > + /*
> > + * Contiguous pages of data with compatible flags are grouped together
> > in
> > + * order to reduce the number of memory regions we create. Make sure
> > the
> > + * last group is processed with this call.
> > + */
> > + process_mem_page(cgs, i, NULL);
> > +
> > + QTAILQ_FOREACH(parameter, ¶meter_data, next)
> > + {
> > + g_free(parameter->data);
> > + parameter->data = NULL;
> > + }
> > +}
> > +
> > +#endif
> > diff --git a/backends/meson.build b/backends/meson.build
> > index d550ac19f7..d092850a07 100644
> > --- a/backends/meson.build
> > +++ b/backends/meson.build
> > @@ -32,6 +32,7 @@ system_ss.add(when: gio, if_true: files('dbus-vmstate.c'))
> > system_ss.add(when: 'CONFIG_SGX', if_true: files('hostmem-epc.c'))
> > if igvm.found()
> > system_ss.add(igvm)
> > + system_ss.add(files('igvm.c'))
> > endif
> >
> > subdir('tpm')
> > diff --git a/include/exec/confidential-guest-support.h
> > b/include/exec/confidential-guest-support.h
> > index c43a1a32f1..1a017a8fda 100644
> > --- a/include/exec/confidential-guest-support.h
> > +++ b/include/exec/confidential-guest-support.h
> > @@ -27,6 +27,10 @@
> > #include "igvm/igvm.h"
> > #endif
> >
> > +#if defined(CONFIG_IGVM)
> > +#include "igvm/igvm.h"
> > +#endif
> > +
> > #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport,
> > CONFIDENTIAL_GUEST_SUPPORT)
> >
> > @@ -95,6 +99,7 @@ struct ConfidentialGuestSupport {
> > * Virtual Machine (IGVM) format.
> > */
> > char *igvm_filename;
> > + IgvmHandle igvm;
> > #endif
> >
> > /*
> > diff --git a/include/exec/igvm.h b/include/exec/igvm.h
> > new file mode 100644
> > index 0000000000..6f40a3239c
> > --- /dev/null
> > +++ b/include/exec/igvm.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * QEMU IGVM configuration backend for Confidential Guests
> > + *
> > + * Copyright (C) 2023-2024 SUSE
> > + *
> > + * Authors:
> > + * Roy Hopkins <roy.hopk...@suse.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef EXEC_IGVM_H
> > +#define EXEC_IGVM_H
> > +
> > +#include "exec/confidential-guest-support.h"
> > +
> > +#if defined(CONFIG_IGVM)
> > +
> > +void igvm_file_init(ConfidentialGuestSupport *cgs);
> > +void igvm_process(ConfidentialGuestSupport *cgs);
>
> Both of these should gain an "Error *errp" parameter and 'int'
> return type and leave the error_report + exit to the caller.
>
This will be addressed as per my first comment above.
> > +
> > +#else
> > +
> > +static inline void igvm_file_init(ConfidentialGuestSupport *cgs)
> > +{
> > +}
> > +
> > +static inline void igvm_process(ConfidentialGuestSupport *cgs)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +#endif
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
Daniel - Many thanks for taking the time to look at this.
Regards,
Roy
