[PATCH v2 01/10] meson: Add optional dependency on IGVM library

[PATCH v2 01/10] meson: Add optional dependency on IGVM library

[Roy Hopkins]

The IGVM library allows Independent Guest Virtual Machine files to be
parsed and processed. IGVM files are used to configure guest memory
layout, initial processor state and other configuration pertaining to
secure virtual machines.

This adds the --enable-igvm configure option, enabled by default, which
attempts to locate and link against the IGVM library via pkgconfig and
sets CONFIG_IGVM if found.

The library is added to the system_ss target in backends/meson.build
where the IGVM parsing will be performed by the ConfidentialGuestSupport
object.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 backends/meson.build          | 3 +++
 meson.build                   | 8 ++++++++
 meson_options.txt             | 2 ++
 scripts/meson-buildoptions.sh | 3 +++
 4 files changed, 16 insertions(+)

diff --git a/backends/meson.build b/backends/meson.build
index 8b2b111497..d550ac19f7 100644
--- a/backends/meson.build
+++ b/backends/meson.build
@@ -30,5 +30,8 @@ if have_vhost_user_crypto
 endif
 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)
+endif
 
 subdir('tpm')
diff --git a/meson.build b/meson.build
index c9c3217ba4..f0b5a29ce7 100644
--- a/meson.build
+++ b/meson.build
@@ -1232,6 +1232,12 @@ if host_os == 'linux' and (have_system or have_tools)
                        method: 'pkg-config',
                        required: get_option('libudev'))
 endif
+igvm = not_found
+if not get_option('igvm').auto() or have_system
+  igvm = dependency('igvm',
+                       method: 'pkg-config',
+                       required: get_option('igvm'))
+endif
 
 mpathlibs = [libudev]
 mpathpersist = not_found
@@ -2320,6 +2326,7 @@ config_host_data.set('CONFIG_CFI', get_option('cfi'))
 config_host_data.set('CONFIG_SELINUX', selinux.found())
 config_host_data.set('CONFIG_XEN_BACKEND', xen.found())
 config_host_data.set('CONFIG_LIBDW', libdw.found())
+config_host_data.set('CONFIG_IGVM', igvm.found())
 if xen.found()
   # protect from xen.version() having less than three components
   xen_version = xen.version().split('.') + ['0', '0']
@@ -4456,6 +4463,7 @@ summary_info += {'seccomp support':   seccomp}
 summary_info += {'GlusterFS support': glusterfs}
 summary_info += {'hv-balloon support': hv_balloon}
 summary_info += {'TPM support':       have_tpm}
+summary_info += {'IGVM support':      igvm}
 summary_info += {'libssh support':    libssh}
 summary_info += {'lzo support':       lzo}
 summary_info += {'snappy support':    snappy}
diff --git a/meson_options.txt b/meson_options.txt
index 0a99a059ec..4eaba64f4b 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -109,6 +109,8 @@ option('dbus_display', type: 'feature', value: 'auto',
        description: '-display dbus support')
 option('tpm', type : 'feature', value : 'auto',
        description: 'TPM support')
+option('igvm', type: 'feature', value: 'auto',
+       description: 'Independent Guest Virtual Machine (IGVM) file support')
 
 # Do not enable it by default even for Mingw32, because it doesn't
 # work on Wine.
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 680fa3f581..38a8183625 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -126,6 +126,7 @@ meson_options_help() {
   printf "%s\n" '  hv-balloon      hv-balloon driver (requires Glib 2.68+ GTree API)'
   printf "%s\n" '  hvf             HVF acceleration support'
   printf "%s\n" '  iconv           Font glyph conversion support'
+  printf "%s\n" '  igvm            IGVM file support'
   printf "%s\n" '  jack            JACK sound support'
   printf "%s\n" '  keyring         Linux keyring support'
   printf "%s\n" '  kvm             KVM acceleration support'
@@ -342,6 +343,8 @@ _meson_option_parse() {
     --iasl=*) quote_sh "-Diasl=$2" ;;
     --enable-iconv) printf "%s" -Diconv=enabled ;;
     --disable-iconv) printf "%s" -Diconv=disabled ;;
+    --enable-igvm) printf "%s" -Digvm=enabled ;;
+    --disable-igvm) printf "%s" -Digvm=disabled ;;
     --includedir=*) quote_sh "-Dincludedir=$2" ;;
     --enable-install-blobs) printf "%s" -Dinstall_blobs=true ;;
     --disable-install-blobs) printf "%s" -Dinstall_blobs=false ;;
-- 
2.43.0

Re: [PATCH v2 01/10] meson: Add optional dependency on IGVM library

[Daniel P. Berrangé]

On Wed, Apr 03, 2024 at 12:11:32PM +0100, Roy Hopkins wrote:
> The IGVM library allows Independent Guest Virtual Machine files to be
> parsed and processed. IGVM files are used to configure guest memory
> layout, initial processor state and other configuration pertaining to
> secure virtual machines.

Looking at the generated header file for the IGVM library, I see
some quite bad namespace pollution. eg igvm_defs.h has:

typedef uint64_t u64_le;
typedef uint32_t u32_le;

#define UINT32_FLAGS_VALUE(x) *((uint32_t*)&(x))

#define MAKE_INVALID_IMPL(x, y) x##y
#define MAKE_INVALID(x, y) MAKE_INVALID_IMPL(x, y)
#define INVALID MAKE_INVALID(INVALID_, __COUNTER__)

enum IgvmPageDataType {
  NORMAL = 0,
  SECRETS = 1,
  CPUID_DATA = 2,
  CPUID_XF = 3,
};

enum IgvmPlatformType {
  NATIVE = 0,
  VSM_ISOLATION = 1,
  SEV_SNP = 2,
  TDX = 3,
};



enum IgvmVariableHeaderType {
  INVALID = 0,
  ...
}

There are soo many more examples in igvm_defs.h that I won't
list them all here.


These are all way too generic as names to be exposing in library
header file. We may be lucky right now that these definitions
don't clash with anything else defined in the compilation namespace
of the consuming application, but that's a bad bet to make long
term.

IMHO this really needs fixing before there's any use of this igvm
library, since fixing it will be a backwards-incompatible change.

Essentially everything in the header needs to have an 'IGVM/Igvm/igvm'
prefix on it.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 01/10] meson: Add optional dependency on IGVM library

[Roy Hopkins]

On Tue, 2024-04-16 at 15:13 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 03, 2024 at 12:11:32PM +0100, Roy Hopkins wrote:
> > The IGVM library allows Independent Guest Virtual Machine files to be
> > parsed and processed. IGVM files are used to configure guest memory
> > layout, initial processor state and other configuration pertaining to
> > secure virtual machines.
> 
> Looking at the generated header file for the IGVM library, I see
> some quite bad namespace pollution. eg igvm_defs.h has:
> 
> typedef uint64_t u64_le;
> typedef uint32_t u32_le;
> 
> #define UINT32_FLAGS_VALUE(x) *((uint32_t*)&(x))
> 
> #define MAKE_INVALID_IMPL(x, y) x##y
> #define MAKE_INVALID(x, y) MAKE_INVALID_IMPL(x, y)
> #define INVALID MAKE_INVALID(INVALID_, __COUNTER__)
> 
> enum IgvmPageDataType {
>   NORMAL = 0,
>   SECRETS = 1,
>   CPUID_DATA = 2,
>   CPUID_XF = 3,
> };
> 
> enum IgvmPlatformType {
>   NATIVE = 0,
>   VSM_ISOLATION = 1,
>   SEV_SNP = 2,
>   TDX = 3,
> };
> 
> 
> 
> enum IgvmVariableHeaderType {
>   INVALID = 0,
>   ...
> }
> 
> There are soo many more examples in igvm_defs.h that I won't
> list them all here.
> 
> 
> These are all way too generic as names to be exposing in library
> header file. We may be lucky right now that these definitions
> don't clash with anything else defined in the compilation namespace
> of the consuming application, but that's a bad bet to make long
> term.
> 
> IMHO this really needs fixing before there's any use of this igvm
> library, since fixing it will be a backwards-incompatible change.
> 
> Essentially everything in the header needs to have an 'IGVM/Igvm/igvm'
> prefix on it.
> 
> With regards,
> Daniel

Daniel,

I've just submitted a patch for the IGVM C library to fix this, ensuring
everything is now uniquely identified with an IGVM_/Igvm_ prefix and fixing some
of the other issues in the generated header file:
https://github.com/microsoft/igvm/pull/52

I'll update this with the changes once merged.

Thanks,
Roy

[PATCH v2 02/10] backends/confidential-guest-support: Add IGVM file parameter

[Roy Hopkins]

In order to add support for parsing IGVM files for secure virtual
machines, a the path to an IGVM file needs to be specified as
part of the guest configuration. It makes sense to add this to
the ConfidentialGuestSupport object as this is common to all secure
virtual machines that potentially could support IGVM based
configuration.

This patch allows the filename to be configured via the QEMU
object model in preparation for subsequent patches that will read and
parse the IGVM file.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 backends/confidential-guest-support.c     | 21 +++++++++++++++++++++
 include/exec/confidential-guest-support.h |  9 +++++++++
 qapi/qom.json                             | 13 +++++++++++++
 qemu-options.hx                           |  8 +++++++-
 4 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index 052fde8db0..da436fb736 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -20,8 +20,29 @@ OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
                             CONFIDENTIAL_GUEST_SUPPORT,
                             OBJECT)
 
+#if defined(CONFIG_IGVM)
+static char *get_igvm(Object *obj, Error **errp)
+{
+    ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
+    return g_strdup(cgs->igvm_filename);
+}
+
+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);
+}
+#endif
+
 static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
 {
+#if defined(CONFIG_IGVM)
+    object_class_property_add_str(oc, "igvm-file",
+        get_igvm, set_igvm);
+    object_class_property_set_description(oc, "igvm-file",
+        "Set the IGVM filename to use");
+#endif
 }
 
 static void confidential_guest_support_init(Object *obj)
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ba2dd4b5df..ec74da8877 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -51,6 +51,15 @@ struct ConfidentialGuestSupport {
      * so 'ready' is not set, we'll abort.
      */
     bool ready;
+
+#if defined(CONFIG_IGVM)
+    /*
+     * igvm_filename: Optional filename that specifies a file that contains
+     *                the configuration of the guest in Independent Guest
+     *                Virtual Machine (IGVM) format.
+     */
+    char *igvm_filename;
+#endif
 };
 
 typedef struct ConfidentialGuestSupportClass {
diff --git a/qapi/qom.json b/qapi/qom.json
index 85e6b4f84a..5935e1b7a6 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -874,6 +874,18 @@
   'base': 'RngProperties',
   'data': { '*filename': 'str' } }
 
+##
+# @ConfidentialGuestProperties:
+#
+# Properties common to objects that are derivatives of confidential-guest-support.
+#
+# @igvm-file: IGVM file to use to configure guest (default: none)
+#
+# Since: 9.1
+##
+{ 'struct': 'ConfidentialGuestProperties',
+  'data': { '*igvm-file': 'str' } }
+
 ##
 # @SevGuestProperties:
 #
@@ -901,6 +913,7 @@
 # Since: 2.12
 ##
 { 'struct': 'SevGuestProperties',
+  'base': 'ConfidentialGuestProperties',
   'data': { '*sev-device': 'str',
             '*dh-cert-file': 'str',
             '*session-file': 'str',
diff --git a/qemu-options.hx b/qemu-options.hx
index 7fd1713fa8..0466bb048b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5655,7 +5655,7 @@ SRST
                  -object secret,id=sec0,keyid=secmaster0,format=base64,\\
                      data=$SECRET,iv=$(<iv.b64)
 
-    ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off]``
+    ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off,igvm-file=file]``
         Create a Secure Encrypted Virtualization (SEV) guest object,
         which can be used to provide the guest memory encryption support
         on AMD processors.
@@ -5699,6 +5699,12 @@ SRST
         cmdline to a designated guest firmware page for measured Linux
         boot with -kernel. The default is off. (Since 6.2)
 
+        The ``igvm-file`` is an optional parameter that, when specified,
+        allows an Independent Guest Virtual Machine (IGVM) file to be
+        specified that configures the secure virtual machine and can
+        include, for example, an SVSM module, system firmware, initial
+        boot state, etc.
+
         e.g to launch a SEV guest
 
         .. parsed-literal::
-- 
2.43.0

Re: [PATCH v2 02/10] backends/confidential-guest-support: Add IGVM file parameter

[Daniel P. Berrangé]

On Wed, Apr 03, 2024 at 12:11:33PM +0100, Roy Hopkins wrote:
> In order to add support for parsing IGVM files for secure virtual
> machines, a the path to an IGVM file needs to be specified as
> part of the guest configuration. It makes sense to add this to
> the ConfidentialGuestSupport object as this is common to all secure
> virtual machines that potentially could support IGVM based
> configuration.
> 
> This patch allows the filename to be configured via the QEMU
> object model in preparation for subsequent patches that will read and
> parse the IGVM file.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  backends/confidential-guest-support.c     | 21 +++++++++++++++++++++
>  include/exec/confidential-guest-support.h |  9 +++++++++
>  qapi/qom.json                             | 13 +++++++++++++
>  qemu-options.hx                           |  8 +++++++-
>  4 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
> index 052fde8db0..da436fb736 100644
> --- a/backends/confidential-guest-support.c
> +++ b/backends/confidential-guest-support.c
> @@ -20,8 +20,29 @@ OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
>                              CONFIDENTIAL_GUEST_SUPPORT,
>                              OBJECT)
>  
> +#if defined(CONFIG_IGVM)
> +static char *get_igvm(Object *obj, Error **errp)
> +{
> +    ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
> +    return g_strdup(cgs->igvm_filename);
> +}
> +
> +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);
> +}
> +#endif
> +
>  static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
>  {
> +#if defined(CONFIG_IGVM)
> +    object_class_property_add_str(oc, "igvm-file",
> +        get_igvm, set_igvm);
> +    object_class_property_set_description(oc, "igvm-file",
> +        "Set the IGVM filename to use");
> +#endif
>  }
>  
>  static void confidential_guest_support_init(Object *obj)
> diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
> index ba2dd4b5df..ec74da8877 100644
> --- a/include/exec/confidential-guest-support.h
> +++ b/include/exec/confidential-guest-support.h
> @@ -51,6 +51,15 @@ struct ConfidentialGuestSupport {
>       * so 'ready' is not set, we'll abort.
>       */
>      bool ready;
> +
> +#if defined(CONFIG_IGVM)
> +    /*
> +     * igvm_filename: Optional filename that specifies a file that contains
> +     *                the configuration of the guest in Independent Guest
> +     *                Virtual Machine (IGVM) format.
> +     */
> +    char *igvm_filename;
> +#endif
>  };
>  
>  typedef struct ConfidentialGuestSupportClass {
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 85e6b4f84a..5935e1b7a6 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -874,6 +874,18 @@
>    'base': 'RngProperties',
>    'data': { '*filename': 'str' } }
>  
> +##
> +# @ConfidentialGuestProperties:
> +#
> +# Properties common to objects that are derivatives of confidential-guest-support.
> +#
> +# @igvm-file: IGVM file to use to configure guest (default: none)
> +#
> +# Since: 9.1
> +##
> +{ 'struct': 'ConfidentialGuestProperties',
> +  'data': { '*igvm-file': 'str' } }

Since the rest of this patch is conditional on CONFIG_IGVM,
this property should be too, so apps can probe for whether
QEMU is built with IGVM support or not.

> +
>  ##
>  # @SevGuestProperties:
>  #
> @@ -901,6 +913,7 @@
>  # Since: 2.12
>  ##
>  { 'struct': 'SevGuestProperties',
> +  'base': 'ConfidentialGuestProperties',
>    'data': { '*sev-device': 'str',
>              '*dh-cert-file': 'str',
>              '*session-file': 'str',

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 03/10] backends/confidential-guest-support: Add functions to support IGVM

[Roy Hopkins]

In preparation for supporting the processing of IGVM files to configure
guests, this adds a set of functions to ConfidentialGuestSupport
allowing configuration of secure virtual machines that can be
implemented for each supported isolation platform type such as Intel TDX
or AMD SEV-SNP. These functions will be called by IGVM processing code
in subsequent patches.

This commit provides a default implementation of the functions that
either perform no action or generate a warning or error when they are
called. Targets that support ConfidentalGuestSupport should override
these implementations.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 backends/confidential-guest-support.c     | 32 ++++++++++
 include/exec/confidential-guest-support.h | 74 +++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index da436fb736..cb0bc543c0 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -14,6 +14,8 @@
 #include "qemu/osdep.h"
 
 #include "exec/confidential-guest-support.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
 
 OBJECT_DEFINE_ABSTRACT_TYPE(ConfidentialGuestSupport,
                             confidential_guest_support,
@@ -45,8 +47,38 @@ static void confidential_guest_support_class_init(ObjectClass *oc, void *data)
 #endif
 }
 
+static int check_support(ConfidentialGuestPlatformType platform,
+                         uint16_t platform_version, uint8_t highest_vtl,
+                         uint64_t shared_gpa_boundary)
+{
+    /* Default: no support. */
+    return 0;
+}
+
+static int set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
+                           ConfidentialGuestPageType memory_type,
+                           uint16_t cpu_index, Error **errp)
+{
+    error_setg(errp,
+               "Setting confidential guest state is not supported for this platform");
+    return -1;
+}
+
+static int get_mem_map_entry(int index, ConfidentialGuestMemoryMapEntry *entry,
+                             Error **errp)
+{
+    error_setg(
+        errp,
+        "Obtaining the confidential guest memory map is not supported for this platform");
+    return -1;
+}
+
 static void confidential_guest_support_init(Object *obj)
 {
+    ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
+    cgs->check_support = check_support;
+    cgs->set_guest_state = set_guest_state;
+    cgs->get_mem_map_entry = get_mem_map_entry;
 }
 
 static void confidential_guest_support_finalize(Object *obj)
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index ec74da8877..a8ad84fa07 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -21,10 +21,44 @@
 #ifndef CONFIG_USER_ONLY
 
 #include "qom/object.h"
+#include "exec/hwaddr.h"
+
+#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)
 
+typedef enum ConfidentialGuestPlatformType {
+    CGS_PLATFORM_SEV,
+    CGS_PLATFORM_SEV_ES,
+} ConfidentialGuestPlatformType;
+
+typedef enum ConfidentialGuestMemoryType {
+    CGS_MEM_RAM,
+    CGS_MEM_RESERVED,
+    CGS_MEM_ACPI,
+    CGS_MEM_NVS,
+    CGS_MEM_UNUSABLE,
+} ConfidentialGuestMemoryType;
+
+typedef struct ConfidentialGuestMemoryMapEntry {
+    uint64_t gpa;
+    uint64_t size;
+    ConfidentialGuestMemoryType type;
+} ConfidentialGuestMemoryMapEntry;
+
+typedef enum ConfidentialGuestPageType {
+    CGS_PAGE_TYPE_NORMAL,
+    CGS_PAGE_TYPE_VMSA,
+    CGS_PAGE_TYPE_ZERO,
+    CGS_PAGE_TYPE_UNMEASURED,
+    CGS_PAGE_TYPE_SECRETS,
+    CGS_PAGE_TYPE_CPUID,
+    CGS_PAGE_TYPE_REQUIRED_MEMORY,
+} ConfidentialGuestPageType;
+
 struct ConfidentialGuestSupport {
     Object parent;
 
@@ -60,6 +94,46 @@ struct ConfidentialGuestSupport {
      */
     char *igvm_filename;
 #endif
+
+    /*
+     * The following virtual methods need to be implemented by systems that
+     * support confidential guests that can be configured with IGVM and are
+     * used during processing of the IGVM file with process_igvm().
+     */
+
+    /*
+     * Check for to see if this confidential guest supports a particular
+     * platform or configuration
+     */
+    int (*check_support)(ConfidentialGuestPlatformType platform,
+                         uint16_t platform_version, uint8_t highest_vtl,
+                         uint64_t shared_gpa_boundary);
+
+    /*
+     * Configure part of the state of a guest for a particular set of data, page
+     * type and gpa. This can be used for example to pre-populate and measure
+     * guest memory contents, define private ranges or set the initial CPU state
+     * for one or more CPUs.
+     *
+     * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial CPU
+     * context for a virtual CPU. The format of the data depends on the type of
+     * confidential virtual machine. For example, for SEV-ES ptr will point to a
+     * vmcb_save_area structure that should be copied into guest memory at the
+     * address specified in gpa. The cpu_index parameter contains the index of
+     * the CPU the VMSA applies to.
+     */
+    int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
+                           ConfidentialGuestPageType memory_type,
+                           uint16_t cpu_index, Error **errp);
+
+    /*
+     * Iterate the system memory map, getting the entry with the given index
+     * that can be populated into guest memory.
+     *
+     * Returns 0 for ok, 1 if the index is out of range and -1 on error.
+     */
+    int (*get_mem_map_entry)(int index, ConfidentialGuestMemoryMapEntry *entry,
+                             Error **errp);
 };
 
 typedef struct ConfidentialGuestSupportClass {
-- 
2.43.0

Re: [PATCH v2 03/10] backends/confidential-guest-support: Add functions to support IGVM

[Philippe Mathieu-Daudé]

Hi Roy,

On 3/4/24 13:11, Roy Hopkins wrote:
> In preparation for supporting the processing of IGVM files to configure
> guests, this adds a set of functions to ConfidentialGuestSupport
> allowing configuration of secure virtual machines that can be
> implemented for each supported isolation platform type such as Intel TDX
> or AMD SEV-SNP. These functions will be called by IGVM processing code
> in subsequent patches.
> 
> This commit provides a default implementation of the functions that
> either perform no action or generate a warning or error when they are
> called. Targets that support ConfidentalGuestSupport should override
> these implementations.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>   backends/confidential-guest-support.c     | 32 ++++++++++
>   include/exec/confidential-guest-support.h | 74 +++++++++++++++++++++++
>   2 files changed, 106 insertions(+)


>   struct ConfidentialGuestSupport {
>       Object parent;
>   
> @@ -60,6 +94,46 @@ struct ConfidentialGuestSupport {
>        */
>       char *igvm_filename;
>   #endif
> +
> +    /*
> +     * The following virtual methods need to be implemented by systems that
> +     * support confidential guests that can be configured with IGVM and are
> +     * used during processing of the IGVM file with process_igvm().
> +     */
> +
> +    /*
> +     * Check for to see if this confidential guest supports a particular
> +     * platform or configuration
> +     */
> +    int (*check_support)(ConfidentialGuestPlatformType platform,
> +                         uint16_t platform_version, uint8_t highest_vtl,
> +                         uint64_t shared_gpa_boundary);
> +
> +    /*
> +     * Configure part of the state of a guest for a particular set of data, page
> +     * type and gpa. This can be used for example to pre-populate and measure
> +     * guest memory contents, define private ranges or set the initial CPU state
> +     * for one or more CPUs.
> +     *
> +     * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial CPU
> +     * context for a virtual CPU. The format of the data depends on the type of
> +     * confidential virtual machine. For example, for SEV-ES ptr will point to a
> +     * vmcb_save_area structure that should be copied into guest memory at the
> +     * address specified in gpa. The cpu_index parameter contains the index of
> +     * the CPU the VMSA applies to.
> +     */
> +    int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
> +                           ConfidentialGuestPageType memory_type,
> +                           uint16_t cpu_index, Error **errp);
> +
> +    /*
> +     * Iterate the system memory map, getting the entry with the given index
> +     * that can be populated into guest memory.
> +     *
> +     * Returns 0 for ok, 1 if the index is out of range and -1 on error.
> +     */
> +    int (*get_mem_map_entry)(int index, ConfidentialGuestMemoryMapEntry *entry,
> +                             Error **errp);
>   };
>   
>   typedef struct ConfidentialGuestSupportClass {

Methods are usually a class field, not an instance one. Any
reason to diverge from this norm?

Regards,

Phil.

Re: [PATCH v2 03/10] backends/confidential-guest-support: Add functions to support IGVM

[Daniel P. Berrangé]

On Thu, Apr 04, 2024 at 10:00:53AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Roy,
> 
> On 3/4/24 13:11, Roy Hopkins wrote:
> > In preparation for supporting the processing of IGVM files to configure
> > guests, this adds a set of functions to ConfidentialGuestSupport
> > allowing configuration of secure virtual machines that can be
> > implemented for each supported isolation platform type such as Intel TDX
> > or AMD SEV-SNP. These functions will be called by IGVM processing code
> > in subsequent patches.
> > 
> > This commit provides a default implementation of the functions that
> > either perform no action or generate a warning or error when they are
> > called. Targets that support ConfidentalGuestSupport should override
> > these implementations.
> > 
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> >   backends/confidential-guest-support.c     | 32 ++++++++++
> >   include/exec/confidential-guest-support.h | 74 +++++++++++++++++++++++
> >   2 files changed, 106 insertions(+)
> 
> 
> >   struct ConfidentialGuestSupport {
> >       Object parent;
> > @@ -60,6 +94,46 @@ struct ConfidentialGuestSupport {
> >        */
> >       char *igvm_filename;
> >   #endif
> > +
> > +    /*
> > +     * The following virtual methods need to be implemented by systems that
> > +     * support confidential guests that can be configured with IGVM and are
> > +     * used during processing of the IGVM file with process_igvm().
> > +     */
> > +
> > +    /*
> > +     * Check for to see if this confidential guest supports a particular
> > +     * platform or configuration
> > +     */
> > +    int (*check_support)(ConfidentialGuestPlatformType platform,
> > +                         uint16_t platform_version, uint8_t highest_vtl,
> > +                         uint64_t shared_gpa_boundary);
> > +
> > +    /*
> > +     * Configure part of the state of a guest for a particular set of data, page
> > +     * type and gpa. This can be used for example to pre-populate and measure
> > +     * guest memory contents, define private ranges or set the initial CPU state
> > +     * for one or more CPUs.
> > +     *
> > +     * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the initial CPU
> > +     * context for a virtual CPU. The format of the data depends on the type of
> > +     * confidential virtual machine. For example, for SEV-ES ptr will point to a
> > +     * vmcb_save_area structure that should be copied into guest memory at the
> > +     * address specified in gpa. The cpu_index parameter contains the index of
> > +     * the CPU the VMSA applies to.
> > +     */
> > +    int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > +                           ConfidentialGuestPageType memory_type,
> > +                           uint16_t cpu_index, Error **errp);
> > +
> > +    /*
> > +     * Iterate the system memory map, getting the entry with the given index
> > +     * that can be populated into guest memory.
> > +     *
> > +     * Returns 0 for ok, 1 if the index is out of range and -1 on error.
> > +     */
> > +    int (*get_mem_map_entry)(int index, ConfidentialGuestMemoryMapEntry *entry,
> > +                             Error **errp);
> >   };
> >   typedef struct ConfidentialGuestSupportClass {
> 
> Methods are usually a class field, not an instance one. Any
> reason to diverge from this norm?

Agreed, this should all be against the Class.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 03/10] backends/confidential-guest-support: Add functions to support IGVM

[Roy Hopkins]

On Tue, 2024-04-16 at 14:31 +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 04, 2024 at 10:00:53AM +0200, Philippe Mathieu-Daudé wrote:
> > Hi Roy,
> > 
> > On 3/4/24 13:11, Roy Hopkins wrote:
> > > In preparation for supporting the processing of IGVM files to configure
> > > guests, this adds a set of functions to ConfidentialGuestSupport
> > > allowing configuration of secure virtual machines that can be
> > > implemented for each supported isolation platform type such as Intel TDX
> > > or AMD SEV-SNP. These functions will be called by IGVM processing code
> > > in subsequent patches.
> > > 
> > > This commit provides a default implementation of the functions that
> > > either perform no action or generate a warning or error when they are
> > > called. Targets that support ConfidentalGuestSupport should override
> > > these implementations.
> > > 
> > > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > > ---
> > >   backends/confidential-guest-support.c     | 32 ++++++++++
> > >   include/exec/confidential-guest-support.h | 74 +++++++++++++++++++++++
> > >   2 files changed, 106 insertions(+)
> > 
> > 
> > >   struct ConfidentialGuestSupport {
> > >       Object parent;
> > > @@ -60,6 +94,46 @@ struct ConfidentialGuestSupport {
> > >        */
> > >       char *igvm_filename;
> > >   #endif
> > > +
> > > +    /*
> > > +     * The following virtual methods need to be implemented by systems
> > > that
> > > +     * support confidential guests that can be configured with IGVM and
> > > are
> > > +     * used during processing of the IGVM file with process_igvm().
> > > +     */
> > > +
> > > +    /*
> > > +     * Check for to see if this confidential guest supports a particular
> > > +     * platform or configuration
> > > +     */
> > > +    int (*check_support)(ConfidentialGuestPlatformType platform,
> > > +                         uint16_t platform_version, uint8_t highest_vtl,
> > > +                         uint64_t shared_gpa_boundary);
> > > +
> > > +    /*
> > > +     * Configure part of the state of a guest for a particular set of
> > > data, page
> > > +     * type and gpa. This can be used for example to pre-populate and
> > > measure
> > > +     * guest memory contents, define private ranges or set the initial
> > > CPU state
> > > +     * for one or more CPUs.
> > > +     *
> > > +     * If memory_type is CGS_PAGE_TYPE_VMSA then ptr points to the
> > > initial CPU
> > > +     * context for a virtual CPU. The format of the data depends on the
> > > type of
> > > +     * confidential virtual machine. For example, for SEV-ES ptr will
> > > point to a
> > > +     * vmcb_save_area structure that should be copied into guest memory
> > > at the
> > > +     * address specified in gpa. The cpu_index parameter contains the
> > > index of
> > > +     * the CPU the VMSA applies to.
> > > +     */
> > > +    int (*set_guest_state)(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > > +                           ConfidentialGuestPageType memory_type,
> > > +                           uint16_t cpu_index, Error **errp);
> > > +
> > > +    /*
> > > +     * Iterate the system memory map, getting the entry with the given
> > > index
> > > +     * that can be populated into guest memory.
> > > +     *
> > > +     * Returns 0 for ok, 1 if the index is out of range and -1 on error.
> > > +     */
> > > +    int (*get_mem_map_entry)(int index, ConfidentialGuestMemoryMapEntry
> > > *entry,
> > > +                             Error **errp);
> > >   };
> > >   typedef struct ConfidentialGuestSupportClass {
> > 
> > Methods are usually a class field, not an instance one. Any
> > reason to diverge from this norm?
> 
> Agreed, this should all be against the Class.
> 
> 
> With regards,
> Daniel

Right, thanks. I'll make this change.

Regards,
Roy

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

[Roy Hopkins]

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;
+
+static 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[] = {
+    { 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) {
+        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;
+}
+
+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;
+    }
+    return 0;
+}
+
+static int process_mem_page(ConfidentialGuestSupport *cgs, int i,
+                            const IGVM_VHS_PAGE_DATA *page_data, Error **errp)
+{
+    ERRP_GUARD();
+    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) {
+            if (!*errp) {
+                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) {
+                if (!*errp) {
+                    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) {
+                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;
+    }
+
+    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;
+
+    /*
+     * 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) {
+        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'))
 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;
+}
+
+static inline int igvm_process(ConfidentialGuestSupport *cgs, Error **errp)
+{
+}
+
+#endif
+
+#endif
-- 
2.43.0

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

[Philippe Mathieu-Daudé]

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

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

[Roy Hopkins]

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.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.
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.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;
> };
> 

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(&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.
> 
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, &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])
> 
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.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
> 
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

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Roy Hopkins]

On Tue, 2024-04-16 at 15:05 +0100, Daniel P. Berrangé wrote:
> 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
> }
> 
Noted.

> 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 '<' ?
> 
It is valid as far as the IGVM library is concerned but makes no sense in the
context of the QEMU IGVM loader, so I've changed this to '<= 0' as suggested.

> > +        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

Thanks for the review. I've again revisited the error handling and made the
changes as you have suggested, using the assumption that `errp` is always set
correctly if a function that takes it as a parameter returns an error code.

I'll also be addressing all the other comments in the next version.

Regards,
Roy

[PATCH v2 05/10] i386/pc: Process IGVM file during PC initialization if present

[Roy Hopkins]

An IGVM file contains configuration of a guest that supports
confidential computing hardware. As part of the PC system
initialisation, the IGVM needs to be processed to apply this
configuration before the guest is started.

This patch introduces processing of a provided IGVM file at the end of
the current PC initialization steps. If an IGVM file has been provided
then the directives in the file are processed completing the
initialization of the target.

If no IGVM file has been specified by the user then no there is no
intended consequences in these changes.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 backends/confidential-guest-support.c     | 18 ++++++++++++++++++
 hw/i386/pc_piix.c                         |  4 ++++
 hw/i386/pc_q35.c                          |  4 ++++
 include/exec/confidential-guest-support.h | 17 +++++++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
index adfe447334..79c0f3fc56 100644
--- a/backends/confidential-guest-support.c
+++ b/backends/confidential-guest-support.c
@@ -88,3 +88,21 @@ static void confidential_guest_support_init(Object *obj)
 static void confidential_guest_support_finalize(Object *obj)
 {
 }
+
+bool cgs_is_igvm(ConfidentialGuestSupport *cgs)
+{
+#if defined(CONFIG_IGVM)
+    return cgs && cgs->igvm;
+#else
+    return false;
+#endif
+}
+
+void cgs_process_igvm(ConfidentialGuestSupport *cgs)
+{
+#if defined(CONFIG_IGVM)
+    if (cgs && cgs_is_igvm(cgs)) {
+        igvm_process(cgs, &error_fatal);
+    }
+#endif
+}
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 18ba076609..f63ddb8e83 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -68,6 +68,7 @@
 #include "hw/i386/acpi-build.h"
 #include "kvm/kvm-cpu.h"
 #include "target/i386/cpu.h"
+#include "exec/confidential-guest-support.h"
 
 #define XEN_IOAPIC_NUM_PIRQS 128ULL
 
@@ -366,6 +367,9 @@ static void pc_init1(MachineState *machine, const char *pci_type)
                                x86_nvdimm_acpi_dsmio,
                                x86ms->fw_cfg, OBJECT(pcms));
     }
+
+    /* Apply confidential guest state from IGVM if supplied */
+    cgs_process_igvm(machine->cgs);
 }
 
 typedef enum PCSouthBridgeOption {
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b5922b44af..3f24728cd3 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -60,6 +60,7 @@
 #include "hw/mem/nvdimm.h"
 #include "hw/i386/acpi-build.h"
 #include "target/i386/cpu.h"
+#include "exec/confidential-guest-support.h"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -327,6 +328,9 @@ static void pc_q35_init(MachineState *machine)
                                x86_nvdimm_acpi_dsmio,
                                x86ms->fw_cfg, OBJECT(pcms));
     }
+
+    /* Apply confidential guest state from IGVM if supplied */
+    cgs_process_igvm(machine->cgs);
 }
 
 #define DEFINE_Q35_MACHINE(suffix, name, compatfn, optionfn) \
diff --git a/include/exec/confidential-guest-support.h b/include/exec/confidential-guest-support.h
index 9419e91249..c380eee2c3 100644
--- a/include/exec/confidential-guest-support.h
+++ b/include/exec/confidential-guest-support.h
@@ -145,6 +145,23 @@ typedef struct ConfidentialGuestSupportClass {
     ObjectClass parent;
 } ConfidentialGuestSupportClass;
 
+/*
+ * Check whether the configuration of the confidential guest is provided
+ * using an IGVM file. IGVM configuration can include the system firmware,
+ * initial CPU state and other configuration that should override standard
+ * system initialization. This function should be used by platforms to
+ * determine which devices and configuration to include during system
+ * initialization.
+ */
+bool cgs_is_igvm(ConfidentialGuestSupport *cgs);
+/*
+ * If IGVM is supported and an IGVM file has been specified then the
+ * configuration described in the file is applied to the guest.
+ * Configuration of a confidential guest includes the layout of the
+ * guest memory, including firmware and initial CPU state.
+ */
+void cgs_process_igvm(ConfidentialGuestSupport *cgs);
+
 #endif /* !CONFIG_USER_ONLY */
 
 #endif /* QEMU_CONFIDENTIAL_GUEST_SUPPORT_H */
-- 
2.43.0

Re: [PATCH v2 05/10] i386/pc: Process IGVM file during PC initialization if present

[Daniel P. Berrangé]

On Wed, Apr 03, 2024 at 12:11:36PM +0100, Roy Hopkins wrote:
> An IGVM file contains configuration of a guest that supports
> confidential computing hardware. As part of the PC system
> initialisation, the IGVM needs to be processed to apply this
> configuration before the guest is started.
> 
> This patch introduces processing of a provided IGVM file at the end of
> the current PC initialization steps. If an IGVM file has been provided
> then the directives in the file are processed completing the
> initialization of the target.
> 
> If no IGVM file has been specified by the user then no there is no
> intended consequences in these changes.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  backends/confidential-guest-support.c     | 18 ++++++++++++++++++
>  hw/i386/pc_piix.c                         |  4 ++++
>  hw/i386/pc_q35.c                          |  4 ++++
>  include/exec/confidential-guest-support.h | 17 +++++++++++++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/backends/confidential-guest-support.c b/backends/confidential-guest-support.c
> index adfe447334..79c0f3fc56 100644
> --- a/backends/confidential-guest-support.c
> +++ b/backends/confidential-guest-support.c
> @@ -88,3 +88,21 @@ static void confidential_guest_support_init(Object *obj)
>  static void confidential_guest_support_finalize(Object *obj)
>  {
>  }
> +
> +bool cgs_is_igvm(ConfidentialGuestSupport *cgs)
> +{
> +#if defined(CONFIG_IGVM)
> +    return cgs && cgs->igvm;
> +#else
> +    return false;
> +#endif
> +}
> +
> +void cgs_process_igvm(ConfidentialGuestSupport *cgs)
> +{
> +#if defined(CONFIG_IGVM)
> +    if (cgs && cgs_is_igvm(cgs)) {

Either remove the 'cgs &&' check which cgs_is_igvm already
does, or fully inline 'cgs_is_igvm'.

> +        igvm_process(cgs, &error_fatal);
> +    }
> +#endif
> +}


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v2 06/10] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM

[Roy Hopkins]

When using an IGVM file the configuration of the system firmware is
defined by IGVM directives contained in the file. In this case the user
should not configure any pflash devices.

This commit skips initialization of the ROM mode when pflash0 is not set
then checks to ensure no pflash devices have been configured when using
IGVM, exiting with an error message if this is not the case.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 hw/i386/pc_sysfw.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 3efabbbab2..2412f26225 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -226,8 +226,13 @@ void pc_system_firmware_init(PCMachineState *pcms,
     }
 
     if (!pflash_blk[0]) {
-        /* Machine property pflash0 not set, use ROM mode */
-        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false);
+        /*
+         * Machine property pflash0 not set, use ROM mode unless using IGVM,
+         * in which case the firmware must be provided by the IGVM file.
+         */
+        if (!cgs_is_igvm(MACHINE(pcms)->cgs)) {
+            x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false);
+        }
     } else {
         if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
             /*
@@ -243,6 +248,20 @@ void pc_system_firmware_init(PCMachineState *pcms,
     }
 
     pc_system_flash_cleanup_unused(pcms);
+
+    /*
+     * The user should not have specified any pflash devices when using IGVM
+     * to configure the guest.
+     */
+    if (cgs_is_igvm(MACHINE(pcms)->cgs)) {
+        for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
+            if (pcms->flash[i]) {
+                error_report("pflash devices cannot be configured when "
+                             "using IGVM");
+                exit(1);
+            }
+        }
+    }
 }
 
 void x86_firmware_configure(void *ptr, int size)
-- 
2.43.0

Re: [PATCH v2 06/10] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM

[Ani Sinha]

> On 3 Apr 2024, at 16:41, Roy Hopkins <roy.hopkins@suse.com> wrote:
> 
> When using an IGVM file the configuration of the system firmware is
> defined by IGVM directives contained in the file. In this case the user
> should not configure any pflash devices.
> 
> This commit skips initialization of the ROM mode when pflash0 is not set
> then checks to ensure no pflash devices have been configured when using
> IGVM, exiting with an error message if this is not the case.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
> hw/i386/pc_sysfw.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 3efabbbab2..2412f26225 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -226,8 +226,13 @@ void pc_system_firmware_init(PCMachineState *pcms,
>     }
> 
>     if (!pflash_blk[0]) {
> -        /* Machine property pflash0 not set, use ROM mode */
> -        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false);
> +        /*
> +         * Machine property pflash0 not set, use ROM mode unless using IGVM,
> +         * in which case the firmware must be provided by the IGVM file.

What if the igvm file does not contain a firmware? Can we have a check for that case somewhere and assert if firmware is absent?

> +         */
> +        if (!cgs_is_igvm(MACHINE(pcms)->cgs)) {
> +            x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false);
> +        }
>     } else {
>         if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
>             /*
> @@ -243,6 +248,20 @@ void pc_system_firmware_init(PCMachineState *pcms,
>     }
> 
>     pc_system_flash_cleanup_unused(pcms);
> +
> +    /*
> +     * The user should not have specified any pflash devices when using IGVM
> +     * to configure the guest.
> +     */
> +    if (cgs_is_igvm(MACHINE(pcms)->cgs)) {
> +        for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> +            if (pcms->flash[i]) {
> +                error_report("pflash devices cannot be configured when "
> +                             "using IGVM");
> +                exit(1);
> +            }
> +        }
> +    }

I have not tested this change but looking at pc_system_flash_create() creates flash[0] and flash[1] for all cases (well except for isapc machines). So for igvm case, would this not cause an exit all the time?

> }
> 
> void x86_firmware_configure(void *ptr, int size)
> -- 
> 2.43.0
> 

Re: [PATCH v2 06/10] i386/pc_sysfw: Ensure sysfw flash configuration does not conflict with IGVM

[Roy Hopkins]

On Thu, 2024-04-04 at 18:06 +0530, Ani Sinha wrote:
> 
> 
> > On 3 Apr 2024, at 16:41, Roy Hopkins <roy.hopkins@suse.com> wrote:
> > 
> > When using an IGVM file the configuration of the system firmware is
> > defined by IGVM directives contained in the file. In this case the user
> > should not configure any pflash devices.
> > 
> > This commit skips initialization of the ROM mode when pflash0 is not set
> > then checks to ensure no pflash devices have been configured when using
> > IGVM, exiting with an error message if this is not the case.
> > 
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> > hw/i386/pc_sysfw.c | 23 +++++++++++++++++++++--
> > 1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index 3efabbbab2..2412f26225 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -226,8 +226,13 @@ void pc_system_firmware_init(PCMachineState *pcms,
> >     }
> > 
> >     if (!pflash_blk[0]) {
> > -        /* Machine property pflash0 not set, use ROM mode */
> > -        x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory, false);
> > +        /*
> > +         * Machine property pflash0 not set, use ROM mode unless using
> > IGVM,
> > +         * in which case the firmware must be provided by the IGVM file.
> 
> What if the igvm file does not contain a firmware? Can we have a check for
> that case somewhere and assert if firmware is absent?
> 
I don't think we can easily check for presence of firmware. In fact, using IGVM
means that you can effectively launch a guest without traditional firmware. A
good example of this is if you use IGVM to deploy an SVSM module that is used to
help with guest migration. In this case, the SVSM code would be placed in memory
(by the IGVM directives) and the initial CPU state configured to launch the SVSM
kernel directly.

> > +         */
> > +        if (!cgs_is_igvm(MACHINE(pcms)->cgs)) {
> > +            x86_bios_rom_init(MACHINE(pcms), "bios.bin", rom_memory,
> > false);
> > +        }
> >     } else {
> >         if (kvm_enabled() && !kvm_readonly_mem_enabled()) {
> >             /*
> > @@ -243,6 +248,20 @@ void pc_system_firmware_init(PCMachineState *pcms,
> >     }
> > 
> >     pc_system_flash_cleanup_unused(pcms);
> > +
> > +    /*
> > +     * The user should not have specified any pflash devices when using
> > IGVM
> > +     * to configure the guest.
> > +     */
> > +    if (cgs_is_igvm(MACHINE(pcms)->cgs)) {
> > +        for (i = 0; i < ARRAY_SIZE(pcms->flash); i++) {
> > +            if (pcms->flash[i]) {
> > +                error_report("pflash devices cannot be configured when "
> > +                             "using IGVM");
> > +                exit(1);
> > +            }
> > +        }
> > +    }
> 
> I have not tested this change but looking at pc_system_flash_create() creates
> flash[0] and flash[1] for all cases (well except for isapc machines). So for
> igvm case, would this not cause an exit all the time?
> 
Although the flash devices are created, if they are not configured then they are
removed by the call to pc_system_flash_cleanup_unused() above, meaning that this
check succeeds in the IGVM case.

> > }
> > 
> > void x86_firmware_configure(void *ptr, int size)
> > -- 
> > 2.43.0
> > 
> 

Regards,
Roy

[PATCH v2 07/10] i386/sev: Refactor setting of reset vector and initial CPU state

[Roy Hopkins]

When an SEV guest is started, the reset vector and state are
extracted from metadata that is contained in the firmware volume.

In preparation for using IGVM to setup the initial CPU state,
the code has been refactored to populate vmcb_save_area for each
CPU which is then applied during guest startup and CPU reset.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 target/i386/sev.c | 288 +++++++++++++++++++++++++++++++++++++++++-----
 target/i386/sev.h | 110 ++++++++++++++++++
 2 files changed, 369 insertions(+), 29 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 72930ff0dc..31dfdc3fe5 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -74,9 +74,7 @@ struct SevGuestState {
     SevState state;
     gchar *measurement;
 
-    uint32_t reset_cs;
-    uint32_t reset_ip;
-    bool reset_data_valid;
+    QTAILQ_HEAD(, SevLaunchVmsa) launch_vmsa;
 };
 
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
@@ -99,6 +97,12 @@ typedef struct QEMU_PACKED SevHashTableDescriptor {
 /* hard code sha256 digest size */
 #define HASH_SIZE 32
 
+/* Convert between SEV-ES VMSA and SegmentCache flags/attributes */
+#define FLAGS_VMSA_TO_SEGCACHE(flags) \
+    ((((flags) & 0xff00) << 12) | (((flags) & 0xff) << 8))
+#define FLAGS_SEGCACHE_TO_VMSA(flags) \
+    ((((flags) & 0xff00) >> 8) | (((flags) & 0xf00000) >> 12))
+
 typedef struct QEMU_PACKED SevHashTableEntry {
     QemuUUID guid;
     uint16_t len;
@@ -125,6 +129,15 @@ typedef struct QEMU_PACKED PaddedSevHashTable {
 QEMU_BUILD_BUG_ON(sizeof(PaddedSevHashTable) % 16 != 0);
 
 static SevGuestState *sev_guest;
+
+typedef struct SevLaunchVmsa {
+    QTAILQ_ENTRY(SevLaunchVmsa) next;
+
+    uint16_t cpu_index;
+    uint64_t gpa;
+    struct sev_es_save_area vmsa;
+} SevLaunchVmsa;
+
 static Error *sev_mig_blocker;
 
 static const char *const sev_fw_errlist[] = {
@@ -291,6 +304,148 @@ sev_guest_finalize(Object *obj)
 {
 }
 
+static void sev_apply_cpu_context(CPUState *cpu)
+{
+    SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
+    X86CPU *x86;
+    CPUX86State *env;
+    struct SevLaunchVmsa *launch_vmsa;
+
+    /* See if an initial VMSA has been provided for this CPU */
+    QTAILQ_FOREACH(launch_vmsa, &sev_guest->launch_vmsa, next)
+    {
+        if (cpu->cpu_index == launch_vmsa->cpu_index) {
+            x86 = X86_CPU(cpu);
+            env = &x86->env;
+
+            /*
+             * Ideally we would provide the VMSA directly to kvm which would
+             * ensure that the resulting initial VMSA measurement which is
+             * calculated during KVM_SEV_LAUNCH_UPDATE_VMSA is calculated from
+             * exactly what we provide here. Currently this is not possible so
+             * we need to copy the parts of the VMSA structure that we currently
+             * support into the CPU state.
+             */
+            cpu_load_efer(env, launch_vmsa->vmsa.efer);
+            cpu_x86_update_cr4(env, launch_vmsa->vmsa.cr4);
+            cpu_x86_update_cr0(env, launch_vmsa->vmsa.cr0);
+            cpu_x86_update_cr3(env, launch_vmsa->vmsa.cr3);
+            env->xcr0 = launch_vmsa->vmsa.xcr0;
+
+            cpu_x86_load_seg_cache(
+                env, R_CS, launch_vmsa->vmsa.cs.selector,
+                launch_vmsa->vmsa.cs.base, launch_vmsa->vmsa.cs.limit,
+                FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.cs.attrib));
+            cpu_x86_load_seg_cache(
+                env, R_DS, launch_vmsa->vmsa.ds.selector,
+                launch_vmsa->vmsa.ds.base, launch_vmsa->vmsa.ds.limit,
+                FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ds.attrib));
+            cpu_x86_load_seg_cache(
+                env, R_ES, launch_vmsa->vmsa.es.selector,
+                launch_vmsa->vmsa.es.base, launch_vmsa->vmsa.es.limit,
+                FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.es.attrib));
+            cpu_x86_load_seg_cache(
+                env, R_FS, launch_vmsa->vmsa.fs.selector,
+                launch_vmsa->vmsa.fs.base, launch_vmsa->vmsa.fs.limit,
+                FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.fs.attrib));
+            cpu_x86_load_seg_cache(
+                env, R_GS, launch_vmsa->vmsa.gs.selector,
+                launch_vmsa->vmsa.gs.base, launch_vmsa->vmsa.gs.limit,
+                FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.gs.attrib));
+            cpu_x86_load_seg_cache(
+                env, R_SS, launch_vmsa->vmsa.ss.selector,
+                launch_vmsa->vmsa.ss.base, launch_vmsa->vmsa.ss.limit,
+                FLAGS_VMSA_TO_SEGCACHE(launch_vmsa->vmsa.ss.attrib));
+
+            env->dr[6] = launch_vmsa->vmsa.dr6;
+            env->dr[7] = launch_vmsa->vmsa.dr7;
+
+            env->regs[R_EAX] = launch_vmsa->vmsa.rax;
+            env->regs[R_ECX] = launch_vmsa->vmsa.rcx;
+            env->regs[R_EDX] = launch_vmsa->vmsa.rdx;
+            env->regs[R_EBX] = launch_vmsa->vmsa.rbx;
+            env->regs[R_ESP] = launch_vmsa->vmsa.rsp;
+            env->regs[R_EBP] = launch_vmsa->vmsa.rbp;
+            env->regs[R_ESI] = launch_vmsa->vmsa.rsi;
+            env->regs[R_EDI] = launch_vmsa->vmsa.rdi;
+#ifdef TARGET_X86_64
+            env->regs[R_R8] = launch_vmsa->vmsa.r8;
+            env->regs[R_R9] = launch_vmsa->vmsa.r9;
+            env->regs[R_R10] = launch_vmsa->vmsa.r10;
+            env->regs[R_R11] = launch_vmsa->vmsa.r11;
+            env->regs[R_R12] = launch_vmsa->vmsa.r12;
+            env->regs[R_R13] = launch_vmsa->vmsa.r13;
+            env->regs[R_R14] = launch_vmsa->vmsa.r14;
+            env->regs[R_R15] = launch_vmsa->vmsa.r15;
+#endif
+            env->eip = launch_vmsa->vmsa.rip;
+            break;
+        }
+    }
+}
+
+static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
+                               uint32_t ctx_len, hwaddr gpa)
+{
+    SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
+    SevLaunchVmsa *launch_vmsa;
+    CPUState *cpu;
+    bool exists = false;
+
+    /*
+     * Setting the CPU context is only supported for SEV-ES. The context buffer
+     * will contain a sev_es_save_area from the Linux kernel which is defined by
+     * "Table B-4. VMSA Layout, State Save Area for SEV-ES" in the AMD64 APM,
+     * Volume 2.
+     */
+
+    if (!sev_es_enabled()) {
+        error_report("SEV: unable to set CPU context: Not supported");
+        return 1;
+    }
+
+    if (ctx_len < sizeof(struct sev_es_save_area)) {
+        error_report("SEV: unable to set CPU context: "
+                     "Invalid context provided");
+        return 1;
+    }
+
+    cpu = qemu_get_cpu(cpu_index);
+    if (!cpu) {
+        error_report("SEV: unable to set CPU context for out of bounds "
+                     "CPU index %d", cpu_index);
+        return 1;
+    }
+
+    /*
+     * If the context of this VP has already been set then replace it with the
+     * new context.
+     */
+    QTAILQ_FOREACH(launch_vmsa, &sev_guest->launch_vmsa, next)
+    {
+        if (cpu_index == launch_vmsa->cpu_index) {
+            launch_vmsa->gpa = gpa;
+            memcpy(&launch_vmsa->vmsa, ctx, sizeof(launch_vmsa->vmsa));
+            exists = true;
+            break;
+        }
+    }
+
+    if (!exists) {
+        /* New VP context */
+        launch_vmsa = g_new0(SevLaunchVmsa, 1);
+        memcpy(&launch_vmsa->vmsa, ctx, sizeof(launch_vmsa->vmsa));
+        launch_vmsa->cpu_index = cpu_index;
+        launch_vmsa->gpa = gpa;
+        QTAILQ_INSERT_TAIL(&sev_guest->launch_vmsa, launch_vmsa, next);
+    }
+
+    /* Synchronise the VMSA with the current CPU state */
+    sev_apply_cpu_context(cpu);
+
+    return 0;
+}
+
 static char *
 sev_guest_get_session_file(Object *obj, Error **errp)
 {
@@ -394,6 +549,7 @@ sev_guest_instance_init(Object *obj)
     object_property_add_uint32_ptr(obj, "reduced-phys-bits",
                                    &sev->reduced_phys_bits,
                                    OBJ_PROP_FLAG_READWRITE);
+    QTAILQ_INIT(&sev->launch_vmsa);
 }
 
 /* sev guest info */
@@ -784,6 +940,16 @@ static int
 sev_launch_update_vmsa(SevGuestState *sev)
 {
     int ret, fw_error;
+    CPUState *cpu;
+
+    /*
+     * The initial CPU state is measured as part of KVM_SEV_LAUNCH_UPDATE_VMSA.
+     * Synchronise the CPU state to any provided launch VMSA structures.
+     */
+    CPU_FOREACH(cpu) {
+        sev_apply_cpu_context(cpu);
+    }
+
 
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_UPDATE_VMSA, NULL, &fw_error);
     if (ret) {
@@ -1197,34 +1363,100 @@ sev_es_find_reset_vector(void *flash_ptr, uint64_t flash_size,
     return sev_es_parse_reset_block(info, addr);
 }
 
-void sev_es_set_reset_vector(CPUState *cpu)
-{
-    X86CPU *x86;
-    CPUX86State *env;
 
-    /* Only update if we have valid reset information */
-    if (!sev_guest || !sev_guest->reset_data_valid) {
-        return;
-    }
+static void seg_to_vmsa(const SegmentCache *cpu_seg, struct vmcb_seg *vmsa_seg)
+{
+    vmsa_seg->selector = cpu_seg->selector;
+    vmsa_seg->base = cpu_seg->base;
+    vmsa_seg->limit = cpu_seg->limit;
+    vmsa_seg->attrib = FLAGS_SEGCACHE_TO_VMSA(cpu_seg->flags);
+}
 
-    /* Do not update the BSP reset state */
-    if (cpu->cpu_index == 0) {
-        return;
-    }
+static void initialize_vmsa(const CPUState *cpu, struct sev_es_save_area *vmsa)
+{
+    const X86CPU *x86 = X86_CPU(cpu);
+    const CPUX86State *env = &x86->env;
 
-    x86 = X86_CPU(cpu);
-    env = &x86->env;
+    /*
+     * Initialize the SEV-ES save area from the current state of
+     * the CPU. The entire state does not need to be copied, only the state
+     * that is copied back to the CPUState in sev_apply_cpu_context.
+     */
+    memset(vmsa, 0, sizeof(struct sev_es_save_area));
+    vmsa->efer = env->efer;
+    vmsa->cr0 = env->cr[0];
+    vmsa->cr3 = env->cr[3];
+    vmsa->cr4 = env->cr[4];
+    vmsa->xcr0 = env->xcr0;
+
+    seg_to_vmsa(&env->segs[R_CS], &vmsa->cs);
+    seg_to_vmsa(&env->segs[R_DS], &vmsa->ds);
+    seg_to_vmsa(&env->segs[R_ES], &vmsa->es);
+    seg_to_vmsa(&env->segs[R_FS], &vmsa->fs);
+    seg_to_vmsa(&env->segs[R_GS], &vmsa->gs);
+    seg_to_vmsa(&env->segs[R_SS], &vmsa->ss);
+
+    vmsa->dr6 = env->dr[6];
+    vmsa->dr7 = env->dr[7];
+
+    vmsa->rax = env->regs[R_EAX];
+    vmsa->rcx = env->regs[R_ECX];
+    vmsa->rdx = env->regs[R_EDX];
+    vmsa->rbx = env->regs[R_EBX];
+    vmsa->rsp = env->regs[R_ESP];
+    vmsa->rbp = env->regs[R_EBP];
+    vmsa->rsi = env->regs[R_ESI];
+    vmsa->rdi = env->regs[R_EDI];
+
+#ifdef TARGET_X86_64
+    vmsa->r8 = env->regs[R_R8];
+    vmsa->r9 = env->regs[R_R9];
+    vmsa->r10 = env->regs[R_R10];
+    vmsa->r11 = env->regs[R_R11];
+    vmsa->r12 = env->regs[R_R12];
+    vmsa->r13 = env->regs[R_R13];
+    vmsa->r14 = env->regs[R_R14];
+    vmsa->r15 = env->regs[R_R15];
+#endif
+
+    vmsa->rip = env->eip;
+}
 
-    cpu_x86_load_seg_cache(env, R_CS, 0xf000, sev_guest->reset_cs, 0xffff,
-                           DESC_P_MASK | DESC_S_MASK | DESC_CS_MASK |
-                           DESC_R_MASK | DESC_A_MASK);
+static void sev_es_set_ap_context(uint32_t reset_addr)
+{
+    CPUState *cpu;
+    struct sev_es_save_area vmsa;
+    SegmentCache cs;
+
+    cs.selector = 0xf000;
+    cs.base = reset_addr & 0xffff0000;
+    cs.limit = 0xffff;
+    cs.flags = DESC_P_MASK | DESC_S_MASK | DESC_CS_MASK | DESC_R_MASK |
+               DESC_A_MASK;
+
+    CPU_FOREACH(cpu) {
+        if (cpu->cpu_index == 0) {
+            /* Do not update the BSP reset state */
+            continue;
+        }
+        initialize_vmsa(cpu, &vmsa);
+        seg_to_vmsa(&cs, &vmsa.cs);
+        vmsa.rip = reset_addr & 0x0000ffff;
+        sev_set_cpu_context(cpu->cpu_index, &vmsa,
+                            sizeof(struct sev_es_save_area), 0);
+        sev_apply_cpu_context(cpu);
+    }
+}
 
-    env->eip = sev_guest->reset_ip;
+void sev_es_set_reset_vector(CPUState *cpu)
+{
+    if (sev_enabled()) {
+        sev_apply_cpu_context(cpu);
+    }
 }
 
 int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
 {
-    CPUState *cpu;
     uint32_t addr;
     int ret;
 
@@ -1239,14 +1471,12 @@ int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
         return ret;
     }
 
+    /*
+     * The reset vector is saved into a CPU context for each AP but not for
+     * the BSP. This is applied during guest startup or when the CPU is reset.
+     */
     if (addr) {
-        sev_guest->reset_cs = addr & 0xffff0000;
-        sev_guest->reset_ip = addr & 0x0000ffff;
-        sev_guest->reset_data_valid = true;
-
-        CPU_FOREACH(cpu) {
-            sev_es_set_reset_vector(cpu);
-        }
+        sev_es_set_ap_context(addr);
     }
 
     return 0;
diff --git a/target/i386/sev.h b/target/i386/sev.h
index e7499c95b1..1fd896d896 100644
--- a/target/i386/sev.h
+++ b/target/i386/sev.h
@@ -38,6 +38,116 @@ typedef struct SevKernelLoaderContext {
     size_t cmdline_size;
 } SevKernelLoaderContext;
 
+/* Save area definition for SEV-ES and SEV-SNP guests */
+struct QEMU_PACKED sev_es_save_area {
+    struct vmcb_seg es;
+    struct vmcb_seg cs;
+    struct vmcb_seg ss;
+    struct vmcb_seg ds;
+    struct vmcb_seg fs;
+    struct vmcb_seg gs;
+    struct vmcb_seg gdtr;
+    struct vmcb_seg ldtr;
+    struct vmcb_seg idtr;
+    struct vmcb_seg tr;
+    uint64_t vmpl0_ssp;
+    uint64_t vmpl1_ssp;
+    uint64_t vmpl2_ssp;
+    uint64_t vmpl3_ssp;
+    uint64_t u_cet;
+    uint8_t reserved_0xc8[2];
+    uint8_t vmpl;
+    uint8_t cpl;
+    uint8_t reserved_0xcc[4];
+    uint64_t efer;
+    uint8_t reserved_0xd8[104];
+    uint64_t xss;
+    uint64_t cr4;
+    uint64_t cr3;
+    uint64_t cr0;
+    uint64_t dr7;
+    uint64_t dr6;
+    uint64_t rflags;
+    uint64_t rip;
+    uint64_t dr0;
+    uint64_t dr1;
+    uint64_t dr2;
+    uint64_t dr3;
+    uint64_t dr0_addr_mask;
+    uint64_t dr1_addr_mask;
+    uint64_t dr2_addr_mask;
+    uint64_t dr3_addr_mask;
+    uint8_t reserved_0x1c0[24];
+    uint64_t rsp;
+    uint64_t s_cet;
+    uint64_t ssp;
+    uint64_t isst_addr;
+    uint64_t rax;
+    uint64_t star;
+    uint64_t lstar;
+    uint64_t cstar;
+    uint64_t sfmask;
+    uint64_t kernel_gs_base;
+    uint64_t sysenter_cs;
+    uint64_t sysenter_esp;
+    uint64_t sysenter_eip;
+    uint64_t cr2;
+    uint8_t reserved_0x248[32];
+    uint64_t g_pat;
+    uint64_t dbgctl;
+    uint64_t br_from;
+    uint64_t br_to;
+    uint64_t last_excp_from;
+    uint64_t last_excp_to;
+    uint8_t reserved_0x298[80];
+    uint32_t pkru;
+    uint32_t tsc_aux;
+    uint8_t reserved_0x2f0[24];
+    uint64_t rcx;
+    uint64_t rdx;
+    uint64_t rbx;
+    uint64_t reserved_0x320; /* rsp already available at 0x01d8 */
+    uint64_t rbp;
+    uint64_t rsi;
+    uint64_t rdi;
+    uint64_t r8;
+    uint64_t r9;
+    uint64_t r10;
+    uint64_t r11;
+    uint64_t r12;
+    uint64_t r13;
+    uint64_t r14;
+    uint64_t r15;
+    uint8_t reserved_0x380[16];
+    uint64_t guest_exit_info_1;
+    uint64_t guest_exit_info_2;
+    uint64_t guest_exit_int_info;
+    uint64_t guest_nrip;
+    uint64_t sev_features;
+    uint64_t vintr_ctrl;
+    uint64_t guest_exit_code;
+    uint64_t virtual_tom;
+    uint64_t tlb_id;
+    uint64_t pcpu_id;
+    uint64_t event_inj;
+    uint64_t xcr0;
+    uint8_t reserved_0x3f0[16];
+
+    /* Floating point area */
+    uint64_t x87_dp;
+    uint32_t mxcsr;
+    uint16_t x87_ftw;
+    uint16_t x87_fsw;
+    uint16_t x87_fcw;
+    uint16_t x87_fop;
+    uint16_t x87_ds;
+    uint16_t x87_cs;
+    uint64_t x87_rip;
+    uint8_t fpreg_x87[80];
+    uint8_t fpreg_xmm[256];
+    uint8_t fpreg_ymm[256];
+};
+
 #ifdef CONFIG_SEV
 bool sev_enabled(void);
 bool sev_es_enabled(void);
-- 
2.43.0

[PATCH v2 08/10] i386/sev: Implement ConfidentialGuestSupport functions for SEV

[Roy Hopkins]

The ConfidentialGuestSupport object defines a number of virtual
functions that are called during processing of IGVM directives to query
or configure initial guest state. In order to support processing of IGVM
files, these functions need to be implemented by relevant isolation
hardware support code such as SEV.

This commit implements the required functions for SEV-ES and adds
support for processing IGVM files for configuring the guest.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 target/i386/sev.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 137 insertions(+)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 31dfdc3fe5..46313e7024 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -37,6 +37,7 @@
 #include "qapi/qapi-commands-misc-target.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
+#include "hw/i386/e820_memory_layout.h"
 #include "exec/address-spaces.h"
 
 #define TYPE_SEV_GUEST "sev-guest"
@@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = {
 
 #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
 
+static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr,
+                                  uint64_t len);
+
 static int
 sev_ioctl(int fd, int cmd, void *data, int *error)
 {
@@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj)
 {
 }
 
+static int cgs_check_support(ConfidentialGuestPlatformType platform,
+                             uint16_t platform_version, uint8_t highest_vtl,
+                             uint64_t shared_gpa_boundary)
+{
+    return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) ||
+            ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0;
+}
+
 static void sev_apply_cpu_context(CPUState *cpu)
 {
     SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
@@ -384,6 +396,54 @@ static void sev_apply_cpu_context(CPUState *cpu)
     }
 }
 
+static int check_vmsa_supported(const struct sev_es_save_area *vmsa)
+{
+    struct sev_es_save_area vmsa_check;
+    size_t i;
+    /*
+     * Clear all supported fields so we can then check the entire structure
+     * is zero.
+     */
+    memcpy(&vmsa_check, vmsa, sizeof(struct sev_es_save_area));
+    memset(&vmsa_check.es, 0, sizeof(vmsa_check.es));
+    memset(&vmsa_check.cs, 0, sizeof(vmsa_check.cs));
+    memset(&vmsa_check.ss, 0, sizeof(vmsa_check.ss));
+    memset(&vmsa_check.ds, 0, sizeof(vmsa_check.ds));
+    memset(&vmsa_check.fs, 0, sizeof(vmsa_check.fs));
+    memset(&vmsa_check.gs, 0, sizeof(vmsa_check.gs));
+    vmsa_check.efer = 0;
+    vmsa_check.cr0 = 0;
+    vmsa_check.cr3 = 0;
+    vmsa_check.cr4 = 0;
+    vmsa_check.xcr0 = 0;
+    vmsa_check.dr6 = 0;
+    vmsa_check.dr7 = 0;
+    vmsa_check.rax = 0;
+    vmsa_check.rcx = 0;
+    vmsa_check.rdx = 0;
+    vmsa_check.rbx = 0;
+    vmsa_check.rsp = 0;
+    vmsa_check.rbp = 0;
+    vmsa_check.rsi = 0;
+    vmsa_check.rdi = 0;
+    vmsa_check.r8 = 0;
+    vmsa_check.r9 = 0;
+    vmsa_check.r10 = 0;
+    vmsa_check.r11 = 0;
+    vmsa_check.r12 = 0;
+    vmsa_check.r13 = 0;
+    vmsa_check.r14 = 0;
+    vmsa_check.r15 = 0;
+    vmsa_check.rip = 0;
+
+    for (i = 0; i < sizeof(vmsa_check); ++i) {
+        if (((uint8_t *)&vmsa_check)[i]) {
+            return 0;
+        }
+    }
+    return 1;
+}
+
 static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
                                uint32_t ctx_len, hwaddr gpa)
 {
@@ -446,6 +506,77 @@ static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
     return 0;
 }
 
+static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
+                               ConfidentialGuestPageType memory_type,
+                               uint16_t cpu_index, Error **errp)
+{
+    SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
+    int ret = 1;
+
+    if (!sev_enabled()) {
+        error_setg(errp, "%s: attempt to configure guest memory, but SEV "
+                     "is not enabled",
+                     __func__);
+    } else if (memory_type == CGS_PAGE_TYPE_VMSA) {
+        if (!sev_es_enabled()) {
+            error_setg(errp,
+                       "%s: attempt to configure initial VMSA, but SEV-ES "
+                       "is not supported",
+                       __func__);
+        } else {
+            if (!check_vmsa_supported((const struct sev_es_save_area *)ptr)) {
+                error_setg(errp,
+                           "%s: The VMSA contains fields that are not "
+                           "synchronized with KVM. Continuing would result in "
+                           "either unpredictable guest behavior, or a "
+                           "mismatched launch measurement.",
+                           __func__);
+            } else {
+                ret = sev_set_cpu_context(cpu_index, ptr, len, gpa);
+            }
+        }
+    } else if ((memory_type == CGS_PAGE_TYPE_ZERO) ||
+               (memory_type == CGS_PAGE_TYPE_NORMAL)) {
+        ret = sev_launch_update_data(sev, ptr, len);
+    } else if (memory_type != CGS_PAGE_TYPE_UNMEASURED) {
+        error_setg(
+            errp,
+            "%s: attempted to configure guest memory to use memory_type %d, "
+            "but this type is not supported",
+            __func__, (int)memory_type);
+    }
+    return ret;
+}
+
+static int cgs_get_mem_map_entry(int index,
+                                 ConfidentialGuestMemoryMapEntry *entry,
+                                 Error **errp)
+{
+    if ((index < 0) || (index >= e820_get_num_entries())) {
+        return 1;
+    }
+    entry->gpa = e820_table[index].address;
+    entry->size = e820_table[index].length;
+    switch (e820_table[index].type) {
+    case E820_RAM:
+        entry->type = CGS_MEM_RAM;
+        break;
+    case E820_RESERVED:
+        entry->type = CGS_MEM_RESERVED;
+        break;
+    case E820_ACPI:
+        entry->type = CGS_MEM_ACPI;
+        break;
+    case E820_NVS:
+        entry->type = CGS_MEM_NVS;
+        break;
+    case E820_UNUSABLE:
+        entry->type = CGS_MEM_UNUSABLE;
+        break;
+    }
+    return 0;
+}
+
 static char *
 sev_guest_get_session_file(Object *obj, Error **errp)
 {
@@ -537,6 +668,7 @@ static void
 sev_guest_instance_init(Object *obj)
 {
     SevGuestState *sev = SEV_GUEST(obj);
+    ConfidentialGuestSupport *cgs = CONFIDENTIAL_GUEST_SUPPORT(obj);
 
     sev->sev_device = g_strdup(DEFAULT_SEV_DEVICE);
     sev->policy = DEFAULT_GUEST_POLICY;
@@ -549,6 +681,11 @@ sev_guest_instance_init(Object *obj)
     object_property_add_uint32_ptr(obj, "reduced-phys-bits",
                                    &sev->reduced_phys_bits,
                                    OBJ_PROP_FLAG_READWRITE);
+
+    cgs->check_support = cgs_check_support;
+    cgs->set_guest_state = cgs_set_guest_state;
+    cgs->get_mem_map_entry = cgs_get_mem_map_entry;
+
     QTAILQ_INIT(&sev->launch_vmsa);
 }
 
-- 
2.43.0

Re: [PATCH v2 08/10] i386/sev: Implement ConfidentialGuestSupport functions for SEV

[Daniel P. Berrangé]

On Wed, Apr 03, 2024 at 12:11:39PM +0100, Roy Hopkins wrote:
> The ConfidentialGuestSupport object defines a number of virtual
> functions that are called during processing of IGVM directives to query
> or configure initial guest state. In order to support processing of IGVM
> files, these functions need to be implemented by relevant isolation
> hardware support code such as SEV.
> 
> This commit implements the required functions for SEV-ES and adds
> support for processing IGVM files for configuring the guest.
> 
> Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> ---
>  target/i386/sev.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 137 insertions(+)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 31dfdc3fe5..46313e7024 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -37,6 +37,7 @@
>  #include "qapi/qapi-commands-misc-target.h"
>  #include "exec/confidential-guest-support.h"
>  #include "hw/i386/pc.h"
> +#include "hw/i386/e820_memory_layout.h"
>  #include "exec/address-spaces.h"
>  
>  #define TYPE_SEV_GUEST "sev-guest"
> @@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = {
>  
>  #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
>  
> +static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr,
> +                                  uint64_t len);
> +
>  static int
>  sev_ioctl(int fd, int cmd, void *data, int *error)
>  {
> @@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj)
>  {
>  }
>  
> +static int cgs_check_support(ConfidentialGuestPlatformType platform,
> +                             uint16_t platform_version, uint8_t highest_vtl,
> +                             uint64_t shared_gpa_boundary)
> +{
> +    return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) ||
> +            ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0;
> +}
> +
>  static void sev_apply_cpu_context(CPUState *cpu)
>  {
>      SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> @@ -384,6 +396,54 @@ static void sev_apply_cpu_context(CPUState *cpu)
>      }
>  }
>  
> +static int check_vmsa_supported(const struct sev_es_save_area *vmsa)
> +{
> +    struct sev_es_save_area vmsa_check;
> +    size_t i;
> +    /*
> +     * Clear all supported fields so we can then check the entire structure
> +     * is zero.
> +     */
> +    memcpy(&vmsa_check, vmsa, sizeof(struct sev_es_save_area));
> +    memset(&vmsa_check.es, 0, sizeof(vmsa_check.es));
> +    memset(&vmsa_check.cs, 0, sizeof(vmsa_check.cs));
> +    memset(&vmsa_check.ss, 0, sizeof(vmsa_check.ss));
> +    memset(&vmsa_check.ds, 0, sizeof(vmsa_check.ds));
> +    memset(&vmsa_check.fs, 0, sizeof(vmsa_check.fs));
> +    memset(&vmsa_check.gs, 0, sizeof(vmsa_check.gs));
> +    vmsa_check.efer = 0;
> +    vmsa_check.cr0 = 0;
> +    vmsa_check.cr3 = 0;
> +    vmsa_check.cr4 = 0;
> +    vmsa_check.xcr0 = 0;
> +    vmsa_check.dr6 = 0;
> +    vmsa_check.dr7 = 0;
> +    vmsa_check.rax = 0;
> +    vmsa_check.rcx = 0;
> +    vmsa_check.rdx = 0;
> +    vmsa_check.rbx = 0;
> +    vmsa_check.rsp = 0;
> +    vmsa_check.rbp = 0;
> +    vmsa_check.rsi = 0;
> +    vmsa_check.rdi = 0;
> +    vmsa_check.r8 = 0;
> +    vmsa_check.r9 = 0;
> +    vmsa_check.r10 = 0;
> +    vmsa_check.r11 = 0;
> +    vmsa_check.r12 = 0;
> +    vmsa_check.r13 = 0;
> +    vmsa_check.r14 = 0;
> +    vmsa_check.r15 = 0;
> +    vmsa_check.rip = 0;
> +
> +    for (i = 0; i < sizeof(vmsa_check); ++i) {
> +        if (((uint8_t *)&vmsa_check)[i]) {
> +            return 0;
> +        }
> +    }
> +    return 1;

Can this just be:

   return !buffer_is_zero(vmsa_check, sizeof(vmsa_check))


> +}
> +
>  static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
>                                 uint32_t ctx_len, hwaddr gpa)
>  {
> @@ -446,6 +506,77 @@ static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
>      return 0;
>  }
>  
> +static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
> +                               ConfidentialGuestPageType memory_type,
> +                               uint16_t cpu_index, Error **errp)
> +{
> +    SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> +    int ret = 1;

This results in a return '1' in some errors, but several of the callers
are checking '< 0' for the error condition. This variable is redundant
anyway, i'd suggest just putting 'return -1' calls after error_setg

> +
> +    if (!sev_enabled()) {
> +        error_setg(errp, "%s: attempt to configure guest memory, but SEV "
> +                     "is not enabled",
> +                     __func__);
> +    } else if (memory_type == CGS_PAGE_TYPE_VMSA) {
> +        if (!sev_es_enabled()) {
> +            error_setg(errp,
> +                       "%s: attempt to configure initial VMSA, but SEV-ES "
> +                       "is not supported",
> +                       __func__);
> +        } else {
> +            if (!check_vmsa_supported((const struct sev_es_save_area *)ptr)) {
> +                error_setg(errp,
> +                           "%s: The VMSA contains fields that are not "
> +                           "synchronized with KVM. Continuing would result in "
> +                           "either unpredictable guest behavior, or a "
> +                           "mismatched launch measurement.",
> +                           __func__);
> +            } else {
> +                ret = sev_set_cpu_context(cpu_index, ptr, len, gpa);

This needs to set 'errp' if 'ret' is non-zero, or assert
that it is always zer0.

> +            }
> +        }
> +    } else if ((memory_type == CGS_PAGE_TYPE_ZERO) ||
> +               (memory_type == CGS_PAGE_TYPE_NORMAL)) {
> +        ret = sev_launch_update_data(sev, ptr, len);

Likewise needs to set 'errp' or assert.

> +    } else if (memory_type != CGS_PAGE_TYPE_UNMEASURED) {
> +        error_setg(
> +            errp,
> +            "%s: attempted to configure guest memory to use memory_type %d, "
> +            "but this type is not supported",
> +            __func__, (int)memory_type);
> +    }
> +    return ret;
> +}

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 08/10] i386/sev: Implement ConfidentialGuestSupport functions for SEV

[Roy Hopkins]

On Tue, 2024-04-16 at 15:30 +0100, Daniel P. Berrangé wrote:
> On Wed, Apr 03, 2024 at 12:11:39PM +0100, Roy Hopkins wrote:
> > The ConfidentialGuestSupport object defines a number of virtual
> > functions that are called during processing of IGVM directives to query
> > or configure initial guest state. In order to support processing of IGVM
> > files, these functions need to be implemented by relevant isolation
> > hardware support code such as SEV.
> > 
> > This commit implements the required functions for SEV-ES and adds
> > support for processing IGVM files for configuring the guest.
> > 
> > Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
> > ---
> >  target/i386/sev.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 137 insertions(+)
> > 
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 31dfdc3fe5..46313e7024 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -37,6 +37,7 @@
> >  #include "qapi/qapi-commands-misc-target.h"
> >  #include "exec/confidential-guest-support.h"
> >  #include "hw/i386/pc.h"
> > +#include "hw/i386/e820_memory_layout.h"
> >  #include "exec/address-spaces.h"
> >  
> >  #define TYPE_SEV_GUEST "sev-guest"
> > @@ -170,6 +171,9 @@ static const char *const sev_fw_errlist[] = {
> >  
> >  #define SEV_FW_MAX_ERROR      ARRAY_SIZE(sev_fw_errlist)
> >  
> > +static int sev_launch_update_data(SevGuestState *sev_guest, uint8_t *addr,
> > +                                  uint64_t len);
> > +
> >  static int
> >  sev_ioctl(int fd, int cmd, void *data, int *error)
> >  {
> > @@ -304,6 +308,14 @@ sev_guest_finalize(Object *obj)
> >  {
> >  }
> >  
> > +static int cgs_check_support(ConfidentialGuestPlatformType platform,
> > +                             uint16_t platform_version, uint8_t
> > highest_vtl,
> > +                             uint64_t shared_gpa_boundary)
> > +{
> > +    return (((platform == CGS_PLATFORM_SEV_ES) && sev_es_enabled()) ||
> > +            ((platform == CGS_PLATFORM_SEV) && sev_enabled())) ? 1 : 0;
> > +}
> > +
> >  static void sev_apply_cpu_context(CPUState *cpu)
> >  {
> >      SevGuestState *sev_guest = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> > @@ -384,6 +396,54 @@ static void sev_apply_cpu_context(CPUState *cpu)
> >      }
> >  }
> >  
> > +static int check_vmsa_supported(const struct sev_es_save_area *vmsa)
> > +{
> > +    struct sev_es_save_area vmsa_check;
> > +    size_t i;
> > +    /*
> > +     * Clear all supported fields so we can then check the entire structure
> > +     * is zero.
> > +     */
> > +    memcpy(&vmsa_check, vmsa, sizeof(struct sev_es_save_area));
> > +    memset(&vmsa_check.es, 0, sizeof(vmsa_check.es));
> > +    memset(&vmsa_check.cs, 0, sizeof(vmsa_check.cs));
> > +    memset(&vmsa_check.ss, 0, sizeof(vmsa_check.ss));
> > +    memset(&vmsa_check.ds, 0, sizeof(vmsa_check.ds));
> > +    memset(&vmsa_check.fs, 0, sizeof(vmsa_check.fs));
> > +    memset(&vmsa_check.gs, 0, sizeof(vmsa_check.gs));
> > +    vmsa_check.efer = 0;
> > +    vmsa_check.cr0 = 0;
> > +    vmsa_check.cr3 = 0;
> > +    vmsa_check.cr4 = 0;
> > +    vmsa_check.xcr0 = 0;
> > +    vmsa_check.dr6 = 0;
> > +    vmsa_check.dr7 = 0;
> > +    vmsa_check.rax = 0;
> > +    vmsa_check.rcx = 0;
> > +    vmsa_check.rdx = 0;
> > +    vmsa_check.rbx = 0;
> > +    vmsa_check.rsp = 0;
> > +    vmsa_check.rbp = 0;
> > +    vmsa_check.rsi = 0;
> > +    vmsa_check.rdi = 0;
> > +    vmsa_check.r8 = 0;
> > +    vmsa_check.r9 = 0;
> > +    vmsa_check.r10 = 0;
> > +    vmsa_check.r11 = 0;
> > +    vmsa_check.r12 = 0;
> > +    vmsa_check.r13 = 0;
> > +    vmsa_check.r14 = 0;
> > +    vmsa_check.r15 = 0;
> > +    vmsa_check.rip = 0;
> > +
> > +    for (i = 0; i < sizeof(vmsa_check); ++i) {
> > +        if (((uint8_t *)&vmsa_check)[i]) {
> > +            return 0;
> > +        }
> > +    }
> > +    return 1;
> 
> Can this just be:
> 
>    return !buffer_is_zero(vmsa_check, sizeof(vmsa_check))
> 
> 
> > +}
> > +
> >  static int sev_set_cpu_context(uint16_t cpu_index, const void *ctx,
> >                                 uint32_t ctx_len, hwaddr gpa)
> >  {
> > @@ -446,6 +506,77 @@ static int sev_set_cpu_context(uint16_t cpu_index,
> > const void *ctx,
> >      return 0;
> >  }
> >  
> > +static int cgs_set_guest_state(hwaddr gpa, uint8_t *ptr, uint64_t len,
> > +                               ConfidentialGuestPageType memory_type,
> > +                               uint16_t cpu_index, Error **errp)
> > +{
> > +    SevGuestState *sev = SEV_GUEST(MACHINE(qdev_get_machine())->cgs);
> > +    int ret = 1;
> 
> This results in a return '1' in some errors, but several of the callers
> are checking '< 0' for the error condition. This variable is redundant
> anyway, i'd suggest just putting 'return -1' calls after error_setg
> 
> > +
> > +    if (!sev_enabled()) {
> > +        error_setg(errp, "%s: attempt to configure guest memory, but SEV "
> > +                     "is not enabled",
> > +                     __func__);
> > +    } else if (memory_type == CGS_PAGE_TYPE_VMSA) {
> > +        if (!sev_es_enabled()) {
> > +            error_setg(errp,
> > +                       "%s: attempt to configure initial VMSA, but SEV-ES "
> > +                       "is not supported",
> > +                       __func__);
> > +        } else {
> > +            if (!check_vmsa_supported((const struct sev_es_save_area
> > *)ptr)) {
> > +                error_setg(errp,
> > +                           "%s: The VMSA contains fields that are not "
> > +                           "synchronized with KVM. Continuing would result
> > in "
> > +                           "either unpredictable guest behavior, or a "
> > +                           "mismatched launch measurement.",
> > +                           __func__);
> > +            } else {
> > +                ret = sev_set_cpu_context(cpu_index, ptr, len, gpa);
> 
> This needs to set 'errp' if 'ret' is non-zero, or assert
> that it is always zer0.
> 
> > +            }
> > +        }
> > +    } else if ((memory_type == CGS_PAGE_TYPE_ZERO) ||
> > +               (memory_type == CGS_PAGE_TYPE_NORMAL)) {
> > +        ret = sev_launch_update_data(sev, ptr, len);
> 
> Likewise needs to set 'errp' or assert.
> 
> > +    } else if (memory_type != CGS_PAGE_TYPE_UNMEASURED) {
> > +        error_setg(
> > +            errp,
> > +            "%s: attempted to configure guest memory to use memory_type %d,
> > "
> > +            "but this type is not supported",
> > +            __func__, (int)memory_type);
> > +    }
> > +    return ret;
> > +}
> 
> With regards,
> Daniel

Thanks Daniel, I'll be addressing all of these in the next version.

Regards,
Roy

[PATCH v2 09/10] docs/system: Add documentation on support for IGVM

[Roy Hopkins]

IGVM support has been implemented for Confidential Guests that support
AMD SEV and AMD SEV-ES. Add some documentation that gives some
background on the IGVM format and how to use it to configure a
confidential guest.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 docs/system/i386/amd-memory-encryption.rst |   2 +
 docs/system/igvm.rst                       | 129 +++++++++++++++++++++
 docs/system/index.rst                      |   1 +
 3 files changed, 132 insertions(+)
 create mode 100644 docs/system/igvm.rst

diff --git a/docs/system/i386/amd-memory-encryption.rst b/docs/system/i386/amd-memory-encryption.rst
index e9bc142bc1..a253bf7db1 100644
--- a/docs/system/i386/amd-memory-encryption.rst
+++ b/docs/system/i386/amd-memory-encryption.rst
@@ -1,3 +1,5 @@
+.. _amd-sev:
+
 AMD Secure Encrypted Virtualization (SEV)
 =========================================
 
diff --git a/docs/system/igvm.rst b/docs/system/igvm.rst
new file mode 100644
index 0000000000..b07c11fa6e
--- /dev/null
+++ b/docs/system/igvm.rst
@@ -0,0 +1,129 @@
+Independent Guest Virtual Machine (IGVM) support
+================================================
+
+IGVM files are designed to encapsulate all the information required to launch a
+virtual machine on any given virtualization stack in a deterministic way. This
+allows the cryptographic measurement of initial guest state for Confidential
+Guests to be calculated when the IGVM file is built, allowing a relying party to
+verify the initial state of a guest via a remote attestation.
+
+QEMU supports IGVM files through the Confidential Guest Support object. An igvm
+filename can optionally be passed to the object which will subsequently be
+parsed and used to configure the guest state prior to launching the guest.
+
+Further Information on IGVM
+---------------------------
+
+Information about the IGVM format, including links to the format specification
+and documentation for the Rust and C libraries can be found at the project
+repository:
+
+https://github.com/microsoft/igvm
+
+
+Supported Platforms
+-------------------
+
+Currently, IGVM files can be provided for Confidential Guests on host systems
+that support AMD SEV and SEV-ES running under KVM.
+
+
+Limitations when using IGVM with AMD SEV and SEV-ES
+---------------------------------------------------
+
+IGVM files configure the initial state of the guest using a set of directives.
+Not every directive is supported by every Confidential Guest type. For example,
+AMD SEV does not support encrypted save state regions, therefore setting the
+initial CPU state using IGVM for SEV is not possible. When an IGVM file contains
+directives that are not supported for the active platform, an error is displayed
+and the guest launch is aborted.
+
+The table below describes the list of directives that are supported for SEV and
+SEV-ES.
+
+.. list-table:: SEV & SEV-ES Supported Directives
+   :widths: 35 65
+   :header-rows: 1
+
+   * - IGVM directive
+     - Notes
+   * - IGVM_VHT_PAGE_DATA
+     - ``NORMAL`` zero, measured and unmeasured page types are supported. Other
+       page types result in an error.
+   * - IGVM_VHT_PARAMETER_AREA
+     -
+   * - IGVM_VHT_PARAMETER_INSERT
+     -
+   * - IGVM_VHT_MEMORY_MAP
+     - The memory map page is populated using entries from the E820 table.
+   * - IGVM_VHT_VP_COUNT_PARAMETER
+     - The guest parameter page is populated with the CPU count.
+   * - IGVM_VHT_ENVIRONMENT_INFO_PARAMETER
+     - The ``memory_is_shared`` parameter is set to 1 in the guest parameter
+       page.
+
+.. list-table:: Additional SEV-ES Supported Directives
+   :widths: 25 75
+   :header-rows: 1
+
+   * - IGVM directive
+     - Notes
+   * - IGVM_VHT_VP_CONTEXT
+     - Setting of the initial CPU state for the boot CPU and additional CPUs is
+       supported with limitations on the fields that can be provided in the
+       VMSA. See below for details on which fields are supported.
+
+Initial CPU state with SEV-ES VMSA
+----------------------------------
+
+The initial state of guest CPUs can be defined in the IGVM file for AMD SEV-ES.
+The state data is provided as a VMSA structure as defined in Table B-4 in the
+AMD64 Architecture Programmer's Manual, Volume 2 [1].
+
+The IGVM VMSA is translated to CPU state in QEMU which is then synchronized
+by KVM to the guest VMSA during the launch process where it contributes to the
+launch measurement. See :ref:`amd-sev` for details on the launch process and
+guest launch measurement.
+
+It is important that no information is lost or changed when translating the
+VMSA provided by the IGVM file into the VSMA that is used to launch the guest.
+Therefore, QEMU restricts the VMSA fields that can be provided in the IGVM
+VMSA structure to the following registers:
+
+RAX, RCX, RDX, RBX, RBP, RSI, RDI, R8-R15, RSP, RIP, CS, DS, ES, FS, GS, SS,
+CR0, CR3, CR4, XCR0, EFER.
+
+When processing the IGVM file, QEMU will check if any fields other than the
+above are non-zero and generate an error if this is the case.
+
+Firmware Images with IGVM
+-------------------------
+
+When an IGVM filename is specified for a Confidential Guest Support object it
+overrides the default handling of system firmware: the firmware image, such as
+an OVMF binary should be contained as a payload of the IGVM file and not
+provided as a flash drive or via the ``-bios`` parameter. The default QEMU
+firmware is not automatically populated into the guest memory space.
+
+If an IGVM file is provided along with either the ``-bios`` parameter or pflash
+devices then an error is displayed and the guest startup is aborted.
+
+Running a Confidential Guest configured using IGVM
+--------------------------------------------------
+
+To run a confidential guest configured with IGVM you need to add the
+``igvm-file`` parameter to the "confidential guest support" object:
+
+Example (for AMD SEV)::
+
+    qemu-system-x86_64 \
+        <other parameters> \
+        -machine ...,confidential-guest-support=sev0 \
+        -object sev-guest,id=sev0,cbitpos=47,reduced-phys-bits=1,igvm-file=/path/to/guest.igvm
+
+References
+----------
+
+[1] AMD64 Architecture Programmer's Manual, Volume 2: System Programming
+  Rev 3.41
+  https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
\ No newline at end of file
diff --git a/docs/system/index.rst b/docs/system/index.rst
index c21065e519..6235dfab87 100644
--- a/docs/system/index.rst
+++ b/docs/system/index.rst
@@ -38,4 +38,5 @@ or Hypervisor.Framework.
    security
    multi-process
    confidential-guest-support
+   igvm
    vm-templating
-- 
2.43.0

[PATCH v2 10/10] docs/interop/firmware.json: Add igvm to FirmwareDevice

[Roy Hopkins]

Create an enum entry within FirmwareDevice for 'igvm' to describe that
an IGVM file can be used to map firmware into memory as an alternative
to pre-existing firmware devices.

Signed-off-by: Roy Hopkins <roy.hopkins@suse.com>
---
 docs/interop/firmware.json | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/docs/interop/firmware.json b/docs/interop/firmware.json
index 54a1fc6c10..9a9178606e 100644
--- a/docs/interop/firmware.json
+++ b/docs/interop/firmware.json
@@ -55,10 +55,17 @@
 #
 # @memory: The firmware is to be mapped into memory.
 #
+# @igvm: The firmware is defined by a file conforming to the IGVM
+#        specification and mapped into memory according to directives
+#        defined in the file. This is similar to @memory but may
+#        include additional processing defined by the IGVM file
+#        including initial CPU state or population of metadata into
+#        the guest address space.
+#
 # Since: 3.0
 ##
 { 'enum' : 'FirmwareDevice',
-  'data' : [ 'flash', 'kernel', 'memory' ] }
+  'data' : [ 'flash', 'kernel', 'memory', 'igvm' ] }
 
 ##
 # @FirmwareTarget:
-- 
2.43.0