[v2,04/10] backends/igvm: Implement parsing and processing of IGVM files

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

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(&parameter_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, &parameter_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

On Wed, Apr 03, 2024 at 12:11:35PM +0100, 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
> 
> 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)
> +    igvm_file_init(cgs, errp);
> +#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)
> +
> +#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;
> +
> +
> +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[] = {
> +    { 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));
> +         ++handler) {

Normal style is post-increment (ie handler++), also you can replace the
sizeof calculation with "G_N_ELEMENTS(directive_handlers)"

> +        if (directive_handlers[handler].type == type) {

if (directive_handlers[handler].type != type) {
  continue
}

means we can reduce indent in the rest of the block

> +            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;
> +}
> +
> +static void *igvm_prepare_memory(uint64_t addr, uint64_t size,
> +                                 int region_identifier, Error **errp)
> +{
> +    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_setg(
> +                errp,
> +                "Processing of IGVM file failed: Could not prepare memory "
> +                "at address 0x%lX due to existing non-RAM region",
> +                addr);
> +            return NULL;
> +        }
> +
> +        gpa_region_size = int128_make64(size);
> +        if (int128_lt(mrs.size, gpa_region_size)) {
> +            memory_region_unref(mrs.mr);
> +            error_setg(
> +                errp,
> +                "Processing of IGVM file failed: Could not prepare memory "
> +                "at address 0x%lX: region size exceeded",
> +                addr);
> +            return NULL;
> +        }
> +        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.
> +         */
> +        g_autofree char *region_name =
> +            g_strdup_printf("igvm.%X", region_identifier);
> +        igvm_pages = g_malloc(sizeof(*igvm_pages));
> +        if (!memory_region_init_ram(igvm_pages, NULL, region_name, size,
> +                                    errp)) {
> +            return NULL;
> +        }
> +        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 -1;
> +    }
> +}
> +
> +static bool page_attrs_equal(IgvmHandle igvm, int i,
> +                             const IGVM_VHS_PAGE_DATA *page_1,
> +                             const IGVM_VHS_PAGE_DATA *page_2)
> +{
> +    IgvmHandle data_handle1, data_handle2;
> +
> +    /*
> +     * If one page has data and the other doesn't then this results in different
> +     * page types: NORMAL vs ZERO.
> +     */
> +    data_handle1 = igvm_get_header_data(igvm, HEADER_SECTION_DIRECTIVE, i - 1);
> +    data_handle2 = igvm_get_header_data(igvm, HEADER_SECTION_DIRECTIVE, i);
> +    if ((data_handle1 == IGVMAPI_NO_DATA) &&
> +        (data_handle2 != IGVMAPI_NO_DATA)) {
> +        return false;
> +    } else if ((data_handle1 != IGVMAPI_NO_DATA) &&
> +               (data_handle2 == IGVMAPI_NO_DATA)) {
> +        return false;
> +    }
> +    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));
> +}
> +
> +static int igvm_process_mem_region(ConfidentialGuestSupport *cgs,
> +                                   IgvmHandle igvm, int start_index,
> +                                   uint64_t gpa_start, int page_count,
> +                                   const IgvmPageDataFlags *flags,
> +                                   const IgvmPageDataType page_type,
> +                                   Error **errp)
> +{
> +    ERRP_GUARD();
> +    uint8_t *region;
> +    IgvmHandle data_handle;
> +    const void *data;
> +    uint32_t data_size;
> +    int i;
> +    bool zero = true;
> +    const uint64_t page_size = flags->is_2mb_page ? 0x200000 : 0x1000;
> +    int result;
> +    int cgs_page_type;
> +
> +    region = igvm_prepare_memory(gpa_start, page_count * page_size, start_index,
> +                                 errp);
> +    if (!region) {
> +        return -1;
> +    }
> +
> +    for (i = 0; i < page_count; ++i) {
> +        data_handle = igvm_get_header_data(igvm, HEADER_SECTION_DIRECTIVE,
> +                                           i + start_index);
> +        if (data_handle == IGVMAPI_NO_DATA) {
> +            /* No data indicates a zero page */
> +            memset(&region[i * page_size], 0, page_size);
> +        } else if (data_handle < 0) {
> +            error_setg(
> +                errp,
> +                "IGVM file contains invalid page data for directive with "
> +                "index %d",
> +                i + start_index);
> +            return -1;
> +        } else {
> +            zero = false;
> +            data_size = igvm_get_buffer_size(igvm, data_handle);
> +            if (data_size < page_size) {
> +                memset(&region[i * page_size], 0, page_size);
> +            } else if (data_size > page_size) {
> +                error_setg(errp,
> +                           "IGVM file contains page data with invalid size for "
> +                           "directive with index %d",
> +                           i + start_index);
> +                return -1;
> +            }
> +            data = igvm_get_buffer(igvm, data_handle);
> +            memcpy(&region[i * page_size], data, data_size);
> +            igvm_free_buffer(igvm, data_handle);
> +        }
> +    }
> +
> +    cgs_page_type = igvm_type_to_cgs_type(page_type, flags->unmeasured, zero);
> +    if (cgs_page_type < 0) {
> +        error_setg(
> +            errp,
> +            "Invalid page type in IGVM file. Directives: %d to %d, "
> +            "page type: %d",
> +            start_index, start_index + page_count, page_type);
> +        return -1;
> +    }
> +
> +    result = cgs->set_guest_state(gpa_start, region, page_size * page_count,
> +                                  cgs_page_type, 0, errp);
> +    if ((result < 0) && !*errp) {
> +        error_setg(errp, "IGVM set guest state failed with code %d", result);
> +        return -1;
> +    }

It is bad practice to have a method which only fills 'errp' in some
error conditions. We should expect that any impl of set_guest_state
always sets 'errp' if it returns -1, and thus not need this error_setg
call. Same point later on.

Removing this check means we cna also remove the ERRP_GUARD.

> +    return 0;
> +}
> +
> +static int process_mem_page(ConfidentialGuestSupport *cgs, int i,
> +                            const IGVM_VHS_PAGE_DATA *page_data, Error **errp)
> +{
> +    ERRP_GUARD();

Nothing dereferences 'errp' so this ERRP_GUARD is redundant.

> +    static IGVM_VHS_PAGE_DATA prev_page_data;
> +    static uint64_t region_start;
> +    static int region_start_i;
> +    static int last_i;
> +    static int page_count;
> +
> +    if (page_data) {
> +        if (page_count == 0) {
> +            region_start = page_data->gpa;
> +            region_start_i = i;
> +        } else {
> +            if (!page_attrs_equal(cgs->igvm, i, page_data, &prev_page_data) ||
> +                ((prev_page_data.gpa +
> +                  (prev_page_data.flags.is_2mb_page ? 0x200000 : 0x1000)) !=
> +                 page_data->gpa) ||
> +                (last_i != (i - 1))) {
> +                /* End of current region */
> +                if (igvm_process_mem_region(cgs, cgs->igvm, region_start_i,
> +                                        region_start, page_count,
> +                                        &prev_page_data.flags,
> +                                        prev_page_data.data_type, errp) < 0) {
> +                    return -1;
> +                }
> +                page_count = 0;
> +                region_start = page_data->gpa;
> +                region_start_i = i;
> +            }
> +        }
> +        memcpy(&prev_page_data, page_data, sizeof(prev_page_data));
> +        last_i = i;
> +        ++page_count;
> +    } else {
> +        if (page_count > 0) {
> +            if (igvm_process_mem_region(cgs, cgs->igvm, region_start_i,
> +                                    region_start, page_count,
> +                                    &prev_page_data.flags,
> +                                    prev_page_data.data_type, errp) < 0) {
> +                return -1;
> +            }
> +            page_count = 0;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int directive_page_data(ConfidentialGuestSupport *cgs, int i,
> +                               uint32_t compatibility_mask,
> +                               const uint8_t *header_data, Error **errp)
> +{
> +    const IGVM_VHS_PAGE_DATA *page_data =
> +        (const IGVM_VHS_PAGE_DATA *)header_data;
> +    if (page_data->compatibility_mask & compatibility_mask) {
> +        return process_mem_page(cgs, i, page_data, errp);
> +    }
> +    return 0;
> +}
> +
> +static int directive_vp_context(ConfidentialGuestSupport *cgs, int i,
> +                                uint32_t compatibility_mask,
> +                                const uint8_t *header_data, Error **errp)
> +{
> +    ERRP_GUARD();
> +    const IGVM_VHS_VP_CONTEXT *vp_context =
> +        (const IGVM_VHS_VP_CONTEXT *)header_data;
> +    IgvmHandle data_handle;
> +    uint8_t *data;
> +    int result;
> +
> +    if (vp_context->compatibility_mask & compatibility_mask) {
> +        data_handle =
> +            igvm_get_header_data(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> +        if (data_handle < 0) {
> +            error_setg(errp, "Invalid VP context in IGVM file. Error code: %X",
> +                       data_handle);
> +            return -1;
> +        }
> +
> +        data = (uint8_t *)igvm_get_buffer(cgs->igvm, data_handle);
> +        result = cgs->set_guest_state(
> +            vp_context->gpa, data, igvm_get_buffer_size(cgs->igvm, data_handle),
> +            CGS_PAGE_TYPE_VMSA, vp_context->vp_index, errp);
> +        igvm_free_buffer(cgs->igvm, data_handle);
> +        if (result != 0) {

Other places check 'result < 0'.

> +            if (!*errp) {

Again, we  should expect set_guest_state to have correctly
reported errors already.

> +                error_setg(errp,
> +                           "IGVM: Failed to set CPU context: error_code=%d",
> +                           result);
> +            }
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int directive_parameter_area(ConfidentialGuestSupport *cgs, int i,
> +                                    uint32_t compatibility_mask,
> +                                    const uint8_t *header_data, Error **errp)
> +{
> +    const IGVM_VHS_PARAMETER_AREA *param_area =
> +        (const IGVM_VHS_PARAMETER_AREA *)header_data;
> +    IgvmParameterData *param_entry;
> +
> +    param_entry = g_new0(IgvmParameterData, 1);
> +    param_entry->size = param_area->number_of_bytes;
> +    param_entry->index = param_area->parameter_area_index;
> +    param_entry->data = g_malloc0(param_entry->size);
> +
> +    QTAILQ_INSERT_TAIL(&parameter_data, param_entry, next);
> +    return 0;
> +}
> +
> +static int directive_parameter_insert(ConfidentialGuestSupport *cgs, int i,
> +                                      uint32_t compatibility_mask,
> +                                      const uint8_t *header_data, Error **errp)
> +{
> +    ERRP_GUARD();
> +    const IGVM_VHS_PARAMETER_INSERT *param =
> +        (const IGVM_VHS_PARAMETER_INSERT *)header_data;
> +    IgvmParameterData *param_entry;
> +    int result;
> +    void *region;
> +
> +    QTAILQ_FOREACH(param_entry, &parameter_data, next)
> +    {
> +        if (param_entry->index == param->parameter_area_index) {
> +            region =
> +                igvm_prepare_memory(param->gpa, param_entry->size, i, errp);
> +            if (!region) {
> +                return -1;
> +            }
> +            memcpy(region, param_entry->data, param_entry->size);
> +            g_free(param_entry->data);
> +            param_entry->data = NULL;
> +
> +            result = cgs->set_guest_state(param->gpa, region, param_entry->size,
> +                                          CGS_PAGE_TYPE_UNMEASURED, 0, errp);
> +            if (result != 0) {

Use '< 0'

> +                if (!*errp) {

Redundant

> +                    error_setg(errp,
> +                               "IGVM: Failed to set guest state: error_code=%d",
> +                               result);
> +                }
> +                return -1;
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int cmp_mm_entry(const void *a, const void *b)
> +{
> +    const IGVM_VHS_MEMORY_MAP_ENTRY *entry_a =
> +        (const IGVM_VHS_MEMORY_MAP_ENTRY *)a;
> +    const IGVM_VHS_MEMORY_MAP_ENTRY *entry_b =
> +        (const IGVM_VHS_MEMORY_MAP_ENTRY *)b;
> +    if (entry_a->starting_gpa_page_number < entry_b->starting_gpa_page_number) {
> +        return -1;
> +    } else if (entry_a->starting_gpa_page_number >
> +               entry_b->starting_gpa_page_number) {
> +        return 1;
> +    } else {
> +        return 0;
> +    }
> +}
> +
> +static int directive_memory_map(ConfidentialGuestSupport *cgs, int i,
> +                                uint32_t compatibility_mask,
> +                                const uint8_t *header_data, Error **errp)
> +{
> +    const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
> +    IgvmParameterData *param_entry;
> +    int max_entry_count;
> +    int entry = 0;
> +    IGVM_VHS_MEMORY_MAP_ENTRY *mm_entry;
> +    ConfidentialGuestMemoryMapEntry cgmm_entry;
> +    int retval = 0;
> +
> +    /* Find the parameter area that should hold the memory map */
> +    QTAILQ_FOREACH(param_entry, &parameter_data, next)
> +    {
> +        if (param_entry->index == param->parameter_area_index) {
> +            max_entry_count =
> +                param_entry->size / sizeof(IGVM_VHS_MEMORY_MAP_ENTRY);
> +            mm_entry = (IGVM_VHS_MEMORY_MAP_ENTRY *)param_entry->data;
> +
> +            retval = cgs->get_mem_map_entry(entry, &cgmm_entry, errp);
> +            while (retval == 0) {
> +                if (entry > max_entry_count) {
> +                    error_setg(
> +                        errp,
> +                        "IGVM: guest memory map size exceeds parameter area defined in IGVM file");
> +                    return -1;
> +                }
> +                mm_entry[entry].starting_gpa_page_number = cgmm_entry.gpa >> 12;
> +                mm_entry[entry].number_of_pages = cgmm_entry.size >> 12;
> +
> +                switch (cgmm_entry.type) {
> +                case CGS_MEM_RAM:
> +                    mm_entry[entry].entry_type = MEMORY;
> +                    break;
> +                case CGS_MEM_RESERVED:
> +                    mm_entry[entry].entry_type = PLATFORM_RESERVED;
> +                    break;
> +                case CGS_MEM_ACPI:
> +                    mm_entry[entry].entry_type = PLATFORM_RESERVED;
> +                    break;
> +                case CGS_MEM_NVS:
> +                    mm_entry[entry].entry_type = PERSISTENT;
> +                    break;
> +                case CGS_MEM_UNUSABLE:
> +                    mm_entry[entry].entry_type = PLATFORM_RESERVED;
> +                    break;
> +                }
> +                retval = cgs->get_mem_map_entry(++entry, &cgmm_entry, errp);
> +            }
> +            if (retval < 0) {
> +                return retval;
> +            }
> +            /* The entries need to be sorted */
> +            qsort(mm_entry, entry, sizeof(IGVM_VHS_MEMORY_MAP_ENTRY),
> +                  cmp_mm_entry);
> +
> +            break;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int directive_vp_count(ConfidentialGuestSupport *cgs, int i,
> +                              uint32_t compatibility_mask,
> +                              const uint8_t *header_data, Error **errp)
> +{
> +    const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
> +    IgvmParameterData *param_entry;
> +    uint32_t *vp_count;
> +    CPUState *cpu;
> +
> +    QTAILQ_FOREACH(param_entry, &parameter_data, next)
> +    {
> +        if (param_entry->index == param->parameter_area_index) {
> +            vp_count = (uint32_t *)(param_entry->data + param->byte_offset);
> +            *vp_count = 0;
> +            CPU_FOREACH(cpu)
> +            {
> +                (*vp_count)++;
> +            }
> +            break;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int directive_environment_info(ConfidentialGuestSupport *cgs, int i,
> +                                      uint32_t compatibility_mask,
> +                                      const uint8_t *header_data, Error **errp)
> +{
> +    const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
> +    IgvmParameterData *param_entry;
> +    IgvmEnvironmentInfo *environmental_state;
> +
> +    QTAILQ_FOREACH(param_entry, &parameter_data, next)
> +    {
> +        if (param_entry->index == param->parameter_area_index) {
> +            environmental_state =
> +                (IgvmEnvironmentInfo *)(param_entry->data + param->byte_offset);
> +            environmental_state->memory_is_shared = 1;
> +            break;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int directive_required_memory(ConfidentialGuestSupport *cgs, int i,
> +                                     uint32_t compatibility_mask,
> +                                     const uint8_t *header_data, Error **errp)
> +{
> +    ERRP_GUARD();
> +    const IGVM_VHS_REQUIRED_MEMORY *mem =
> +        (const IGVM_VHS_REQUIRED_MEMORY *)header_data;
> +    uint8_t *region;
> +    int result;
> +
> +    if (mem->compatibility_mask & compatibility_mask) {
> +        region = igvm_prepare_memory(mem->gpa, mem->number_of_bytes, i, errp);
> +        if (!region) {
> +            return -1;
> +        }
> +        result = cgs->set_guest_state(mem->gpa, region, mem->number_of_bytes,
> +                                      CGS_PAGE_TYPE_REQUIRED_MEMORY, 0, errp);
> +        if (result < 0) {
> +            if (!*errp) {

Redynudat

> +                error_setg(errp,
> +                           "IGVM: Failed to set guest state: error_code=%d",
> +                           result);
> +            }
> +            return -1;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static uint32_t supported_platform_compat_mask(ConfidentialGuestSupport *cgs,
> +                                               Error **errp)
> +{
> +    int32_t result;
> +    int i;
> +    IgvmHandle header_handle;
> +    IGVM_VHS_SUPPORTED_PLATFORM *platform;
> +    uint32_t compatibility_mask = 0;
> +
> +    result = igvm_header_count(cgs->igvm, HEADER_SECTION_PLATFORM);
> +    if (result < 0) {
> +        error_setg(errp,
> +                   "Invalid platform header count in IGVM file. Error code: %X",
> +                   result);
> +        return 0;
> +    }
> +
> +    for (i = 0; i < (int)result; ++i) {
> +        IgvmVariableHeaderType typ =
> +            igvm_get_header_type(cgs->igvm, HEADER_SECTION_PLATFORM, i);
> +        if (typ == IGVM_VHT_SUPPORTED_PLATFORM) {
> +            header_handle =
> +                igvm_get_header(cgs->igvm, HEADER_SECTION_PLATFORM, i);
> +            if (header_handle < 0) {
> +                error_setg(errp,
> +                           "Invalid platform header in IGVM file. "
> +                           "Index: %d, Error code: %X",
> +                           i, header_handle);
> +                return 0;
> +            }
> +            platform =
> +                (IGVM_VHS_SUPPORTED_PLATFORM *)(igvm_get_buffer(cgs->igvm,
> +                                                                header_handle) +
> +                                                sizeof(
> +                                                    IGVM_VHS_VARIABLE_HEADER));
> +            /* Currently only support SEV-SNP. */
> +            if (platform->platform_type == SEV_SNP) {
> +                /*
> +                 * IGVM does not define a platform types of SEV or SEV_ES.
> +                 * Translate SEV_SNP into CGS_PLATFORM_SEV_ES and
> +                 * CGS_PLATFORM_SEV and let the cgs function implementations
> +                 * check whether each IGVM directive results in an operation
> +                 * that is supported by the particular derivative of SEV.
> +                 */
> +                if (cgs->check_support(
> +                        CGS_PLATFORM_SEV_ES, platform->platform_version,
> +                        platform->highest_vtl, platform->shared_gpa_boundary) ||
> +                    cgs->check_support(
> +                        CGS_PLATFORM_SEV, platform->platform_version,
> +                        platform->highest_vtl, platform->shared_gpa_boundary)) {
> +                    compatibility_mask = platform->compatibility_mask;
> +                    break;
> +                }
> +            }
> +            igvm_free_buffer(cgs->igvm, header_handle);
> +        }
> +    }
> +    if (compatibility_mask == 0) {
> +        error_setg(
> +            errp,
> +            "IGVM file does not describe a compatible supported platform");
> +    }
> +    return compatibility_mask;
> +}
> +
> +int igvm_file_init(ConfidentialGuestSupport *cgs, Error **errp)
> +{
> +    g_autofree uint8_t *buf = NULL;
> +    unsigned long len;
> +    g_autoptr(GError) gerr = NULL;
> +
> +    if (!cgs->igvm_filename) {
> +        return 0;
> +    }

I'd prefer to see this check in the caller, as it makes it clearer
that the igvm logic won't be run when the filename is unset.

> +
> +    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;
> +    }
> +
> +    cgs->igvm = igvm_new_from_binary(buf, len);
> +    if (cgs->igvm < 0) {
> +        error_setg(errp, "Unable to parse IGVM file %s: %d", cgs->igvm_filename,
> +                   cgs->igvm);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +int igvm_process(ConfidentialGuestSupport *cgs, Error **errp)
> +{
> +    int32_t result;
> +    int i;
> +    uint32_t compatibility_mask;
> +    IgvmParameterData *parameter;
> +    int retval = 0;

Change to 'retval = -1'

> +
> +    /*
> +     * If this is not a Confidential guest or no IGVM has been provided then
> +     * this is a no-op.
> +     */
> +    if (!cgs->igvm) {
> +        return 0;
> +    }

Again I'd prefedr the caller to do this check and avoid calling
igvm_process, to make it clear it is a no-op.

> +
> +    /*
> +     * 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) {

Is 0 a valid header count, or should this be '<=' instead of '<' ?

> +        error_setg(
> +            errp, "Invalid directive header count in IGVM file. Error code: %X",
> +            result);
> +        return -1;
> +    }
> +
> +    QTAILQ_INIT(&parameter_data);
> +
> +    for (i = 0; i < (int)result; ++i) {

Preferrably  'i++ ' for more common style.

> +        IgvmVariableHeaderType type =
> +            igvm_get_header_type(cgs->igvm, HEADER_SECTION_DIRECTIVE, i);
> +        if (directive(type, cgs, i, compatibility_mask, errp) < 0) {
> +            retval = -1;
> +            break;

Replace two lines with 'goto cleanup'.

> +        }
> +    }
> +
> +    /*
> +     * 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) {

Can remove this condition check

> +        retval = process_mem_page(cgs, i, NULL, errp);
> +    }
> +

Here add:

  cleanup:

> +    QTAILQ_FOREACH(parameter, &parameter_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'))
>  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"
> +#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
> @@ -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;

This should report an error and return -1, since reaching
this would be a error scenario

> +}
> +
> +static inline int igvm_process(ConfidentialGuestSupport *cgs, Error **errp)
> +{

Also should report an error

> +}
> +
> +#endif
> +
> +#endif
> -- 
> 2.43.0
> 

With regards,
Daniel