[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 04/10] backends/igvm: Implement parsing and processing of
|
From: |
Philippe Mathieu-Daudé |
|
Subject: |
Re: [PATCH v2 04/10] backends/igvm: Implement parsing and processing of IGVM files |
|
Date: |
Thu, 4 Apr 2024 09:58:13 +0200 |
|
User-agent: |
Mozilla Thunderbird |
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.hopkins@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.
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.hopkins@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;
};
+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.
+ ++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.
+ 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])
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).
@@ -0,0 +1,36 @@
+/*
+ * QEMU IGVM configuration backend for Confidential Guests
+ *
+ * Copyright (C) 2023-2024 SUSE
+ *
+ * Authors:
+ * Roy Hopkins <roy.hopkins@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