On Thu, 2024-04-04 at 09:58 +0200, Philippe Mathieu-Daudé wrote:
> Hi Roy,
>
> On 3/4/24 13:11, 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 | 745 ++++++++++++++++++++++
> > backends/meson.build | 1 +
> > include/exec/confidential-guest-support.h | 5 +
> > include/exec/igvm.h | 36 ++
> > 5 files changed, 791 insertions(+)
> > create mode 100644 backends/igvm.c
> > create mode 100644 include/exec/igvm.h
>
> Consider enabling scripts/git.orderfile.
Ok, will do.
>
> > diff --git a/backends/confidential-guest-support.c b/backends/confidential-
> > guest-support.c
> > index cb0bc543c0..adfe447334 100644
> > --- a/backends/confidential-guest-support.c
> > +++ b/backends/confidential-guest-support.c
> > @@ -16,6 +16,7 @@
> > #include "exec/confidential-guest-support.h"
> > #include "qemu/error-report.h"
> > #include "qapi/error.h"
> > +#include "exec/igvm.h"
> >
> > OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
> > confidential_guest_support,
> > @@ -34,6 +35,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)
>
> You don't need the #ifdef'ry because if CONFIG_IGVM you defined
> an inlined function which returns 0.
>
> > + igvm_file_init(cgs, errp);
>
> You are deliberately ignoring the return value. Should the prototype
> return void? Or at least a boolean, since the return value is (-1, 0).
>
> > +#endif
> > }
> > #endif
> >
> > diff --git a/backends/igvm.c b/backends/igvm.c
> > new file mode 100644
> > index 0000000000..87e6032a2e
> > --- /dev/null
> > +++ b/backends/igvm.c
> > @@ -0,0 +1,745 @@
> > +/*
> > + * 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)
>
> This file is only compiled when CONFIG_IGVM is set, so no need for
> this guard.
>
> > +#include "exec/confidential-guest-support.h"
> > +#include "qemu/queue.h"
> > +#include "qemu/typedefs.h"
>
> No need to include "qemu/typedefs.h", we get it via "qemu/osdep.h".
>
> > +#include "exec/igvm.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/boards.h"
>
> What is used from "hw/board.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;
> Can we store this in ConfidentialGuestSupport instead?
>
> Possibly forward-declaring a structure, using an opaque
> pointer in ConfidentialGuestSupport ...:
>
> typedef struct QemuIvgm QemuIvgm;
>
> struct ConfidentialGuestSupport {
> ...
> QemuIvgm *ivgm;
> ...
> };
>
> ... and defining the struct here in igvm.c:
>
> struct QemuIvgm {
> char *filename;
> IgvmHandle handle;
> QTAILQ_HEAD(, IgvmParameterData) parameter_data;
> };
>
That make senses. In fact, I've added QemuIgvm to hold the context of the file
as it is being processed but this is only scoped to igvm_process_file() so I
will keep it as a local variable in that function instead of adding it to
ConfidentialGuestSupport.
> > +static int directive_page_data(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error **errp);
> > +static int directive_vp_context(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error **errp);
> > +static int directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error
> > **errp);
> > +static int directive_parameter_insert(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error
> > **errp);
> > +static int directive_memory_map(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error **errp);
> > +static int directive_vp_count(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error **errp);
> > +static int directive_environment_info(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error
> > **errp);
> > +static int directive_required_memory(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask,
> > + const uint8_t *header_data, Error
> > **errp);
> > +
> > +struct IGVMDirectiveHandler {
> > + uint32_t type;
> > + int (*handler)(ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask, const uint8_t *header_data,
> > + Error **errp);
> > +};
> > +
> > +static struct IGVMDirectiveHandler directive_handlers[] = {
>
> const.
>
> > + { 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 int directive(uint32_t type, ConfidentialGuestSupport *cgs, int i,
> > + uint32_t compatibility_mask, Error **errp)
> > +{
> > + size_t handler;
> > + IgvmHandle header_handle;
> > + const uint8_t *header_data;
> > + int result;
> > +
> > + for (handler = 0; handler < (sizeof(directive_handlers) /
> > + sizeof(struct IGVMDirectiveHandler));
>
> We have ARRAY_SIZE(), which is easier to read.
Noted.
>
> > + ++handler) {
> > + if (directive_handlers[handler].type == type) {
> > + header_handle =
> > + igvm_get_header(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> > + if (header_handle < 0) {
> > + error_setg(
> > + errp,
> > + "IGVM file is invalid: Failed to read directive header
> > (code: %d)",
> > + (int)header_handle);
> > + return -1;
> > + }
> > + header_data = igvm_get_buffer(cgs->igvm, header_handle) +
> > + sizeof(IGVM_VHS_VARIABLE_HEADER);
> > + result = directive_handlers[handler].handler(
> > + cgs, i, compatibility_mask, header_data, errp);
> > + igvm_free_buffer(cgs->igvm, header_handle);
> > + return result;
> > + }
> > + }
> > + error_setg(errp,
> > + "IGVM: Unknown directive type encountered when processing
> > file: "
> > + "(type 0x%X)",
> > + type);
> > + return -1;
> > +}
>
> [...]
>
> > +int igvm_process(ConfidentialGuestSupport *cgs, Error **errp)
> > +{
> > + int32_t result;
> > + int i;
>
> Since 'i' is never set with a negative value, it can be declared
> as unsigned.
>
> > + uint32_t compatibility_mask;
> > + IgvmParameterData *parameter;
> > + int retval = 0;
> > +
> > + /*
> > + * If this is not a Confidential guest or no IGVM has been provided
> > then
> > + * this is a no-op.
> > + */
> > + if (!cgs->igvm) {
> > + return 0;
> > + }
> > +
> > + /*
> > + * Check that the IGVM file provides configuration for the current
> > + * platform
> > + */
> > + compatibility_mask = supported_platform_compat_mask(cgs, errp);
> > + if (compatibility_mask == 0) {
> > + return -1;
> > + }
> > +
> > + result = igvm_header_count(cgs->igvm, HEADER_SECTION_DIRECTIVE);
> > + if (result < 0) {
> > + error_setg(
> > + errp, "Invalid directive header count in IGVM file. Error code:
> > %X",
> > + result);
> > + return -1;
> > + }
> > +
> > + QTAILQ_INIT(¶meter_data);
> > +
> > + for (i = 0; i < (int)result; ++i) {
>
> Well, 'i' is clearly unsigned.
>
> I'd rename s/result/header_count/ and s/i/header_index/ here and in all
> the callees.
>
Ok. I'm also going to move what was 'i' but represents the current header index
into the new QemuIgvm context structure as that is being passed around to all
the handlers.
> > + IgvmVariableHeaderType type =
> > + igvm_get_header_type(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> > + if (directive(type, cgs, i, compatibility_mask, errp) < 0) {
> > + retval = -1;
> > + break;
> > + }
> > + }
> > +
> > + /*
> > + * 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.
> > + */
> > + if (retval == 0) {
> > + retval = process_mem_page(cgs, i, NULL, errp);
> > + }
> > +
> > + QTAILQ_FOREACH(parameter, ¶meter_data, next)
> > + {
> > + g_free(parameter->data);
> > + parameter->data = NULL;
> > + }
> > +
> > + return retval;
> > +}
> > +
> > +#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'))
>
> You want in the same line to propagate the library flags to the built
> objects:
>
> system_ss.add([files('igvm.c'), igvm])
>
Ok.
> > endif
> >
> > subdir('tpm')
> > diff --git a/include/exec/confidential-guest-support.h
> > b/include/exec/confidential-guest-support.h
> > index a8ad84fa07..9419e91249 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"
>
> You already included it in the previous commit ;)
>
> > +#endif
> > +
> > #define TYPE_CONFIDENTIAL_GUEST_SUPPORT "confidential-guest-support"
> > OBJECT_DECLARE_SIMPLE_TYPE(ConfidentialGuestSupport,
> > CONFIDENTIAL_GUEST_SUPPORT)
> >
> > @@ -93,6 +97,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..59594f047e
> > --- /dev/null
> > +++ b/include/exec/igvm.h
>
> Please move to include/sysemu/ (confidential-guest-support.h will soon
> be moved there).
>
Do you mean just the header file? Is backends/igvm.c the correct location?
> > @@ -0,0 +1,36 @@
> > +/*
> > + * 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)
> > +
> > +int igvm_file_init(ConfidentialGuestSupport *cgs, Error **errp);
> > +int igvm_process(ConfidentialGuestSupport *cgs, Error **erp);
> > +
> > +#else
> > +
> > +static inline int igvm_file_init(ConfidentialGuestSupport *cgs, Error
> > **errp)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline int igvm_process(ConfidentialGuestSupport *cgs, Error **errp)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +#endif
>
Thanks for the reviews.
I'll be addressing all the above comments that I haven't explicitly commented on
in the next version of the patch series. Thanks.
Regards,
Roy
