From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Roy Hopkins <roy.hopkins@suse.com>, qemu-devel@nongnu.org
Cc: "Paolo Bonzini" <pbonzini@redhat.com>,
"Daniel P . Berrangé" <berrange@redhat.com>,
"Stefano Garzarella" <sgarzare@redhat.com>,
"Marcelo Tosatti" <mtosatti@redhat.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
"Cornelia Huck" <cohuck@redhat.com>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Sergio Lopez" <slp@redhat.com>,
"Eduardo Habkost" <eduardo@habkost.net>,
"Alistair Francis" <alistair@alistair23.me>,
"Peter Xu" <peterx@redhat.com>,
"David Hildenbrand" <david@redhat.com>,
"Igor Mammedov" <imammedo@redhat.com>,
"Tom Lendacky" <thomas.lendacky@amd.com>,
"Michael Roth" <michael.roth@amd.com>,
"Ani Sinha" <anisinha@redhat.com>,
"Jörg Roedel" <jroedel@suse.com>
Subject: Re: [PATCH v2 04/10] backends/igvm: Implement parsing and processing of IGVM files
Date: Thu, 4 Apr 2024 09:58:13 +0200 [thread overview]
Message-ID: <905678ae-09cd-4e37-8dc1-5b5e5314d0ea@linaro.org> (raw)
In-Reply-To: <9e4268647c2b3dc7fcc59d795575622c4f35a413.1712141833.git.roy.hopkins@suse.com>
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
next prev parent reply other threads:[~2024-04-04 7:59 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1712141833.git.roy.hopkins@suse.com>
2024-04-03 11:11 ` [PATCH v2 01/10] meson: Add optional dependency on IGVM library Roy Hopkins
2024-04-16 14:13 ` Daniel P. Berrangé
2024-05-01 8:53 ` Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 02/10] backends/confidential-guest-support: Add IGVM file parameter Roy Hopkins
2024-04-16 13:29 ` Daniel P. Berrangé
2024-04-03 11:11 ` [PATCH v2 03/10] backends/confidential-guest-support: Add functions to support IGVM Roy Hopkins
2024-04-04 8:00 ` Philippe Mathieu-Daudé
2024-04-16 13:31 ` Daniel P. Berrangé
2024-05-07 14:08 ` Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 04/10] backends/igvm: Implement parsing and processing of IGVM files Roy Hopkins
2024-04-04 7:58 ` Philippe Mathieu-Daudé [this message]
2024-05-07 14:19 ` Roy Hopkins
2024-04-16 14:05 ` Daniel P. Berrangé
2024-05-07 14:25 ` Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 05/10] i386/pc: Process IGVM file during PC initialization if present Roy Hopkins
2024-04-16 14:19 ` Daniel P. Berrangé
2024-04-03 11:11 ` [PATCH v2 06/10] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM Roy Hopkins
2024-04-04 12:36 ` Ani Sinha
2024-05-07 14:32 ` Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 07/10] i386/sev: Refactor setting of reset vector and initial CPU state Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 08/10] i386/sev: Implement ConfidentialGuestSupport functions for SEV Roy Hopkins
2024-04-16 14:30 ` Daniel P. Berrangé
2024-05-07 14:34 ` Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 09/10] docs/system: Add documentation on support for IGVM Roy Hopkins
2024-04-03 11:11 ` [PATCH v2 10/10] docs/interop/firmware.json: Add igvm to FirmwareDevice Roy Hopkins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=905678ae-09cd-4e37-8dc1-5b5e5314d0ea@linaro.org \
--to=philmd@linaro.org \
--cc=alistair@alistair23.me \
--cc=anisinha@redhat.com \
--cc=berrange@redhat.com \
--cc=cohuck@redhat.com \
--cc=david@redhat.com \
--cc=eduardo@habkost.net \
--cc=imammedo@redhat.com \
--cc=jroedel@suse.com \
--cc=marcel.apfelbaum@gmail.com \
--cc=michael.roth@amd.com \
--cc=mst@redhat.com \
--cc=mtosatti@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=roy.hopkins@suse.com \
--cc=sgarzare@redhat.com \
--cc=slp@redhat.com \
--cc=thomas.lendacky@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).