Re: [PATCH v7 00/16] Introduce support for IGVM files

[Roy Hopkins <roy.hopkins@randomman.co.uk>]

On Wed, 2025-03-05 at 16:47 +0100, Stefano Garzarella wrote:
> Hi Roy,
> 
> I was testing this series with the IGVM file generated by COCONUT SVSM, 
> but QEMU was failing in this way:
> 
>    qemu-system-x86_64: KVM does not support guest_memfd
>    qemu-system-x86_64: failed to initialize kvm: Operation not permitted
> 
> After spending some time debugging, I found that IGVM is parsed in 
> kvm_arch_init(). One of the handler called during the parsing is 
> qigvm_prepare_memory(), which adds a new memory region calling 
> memory_region_init_ram_guest_memfd(), but it fails:
> 
> kvm_arch_init()
> -> qigvm_prepare_memory()
>    -> memory_region_init_ram_guest_memfd()
>       -> kvm_create_guest_memfd()
>            ...
>            if (!kvm_guest_memfd_supported) {
>                error_setg(errp, "KVM does not support guest_memfd");
>                return -1;
>            }
> 
> So, I applied the following change and SVSM booted!
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index f89568bfa3..840f36675e 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2722,17 +2722,17 @@ static int kvm_init(MachineState *ms)
>  
>      kvm_state = s;
>  
> -    ret = kvm_arch_init(ms, s);
> -    if (ret < 0) {
> -        goto err;
> -    }
> -
>      kvm_supported_memory_attributes = kvm_vm_check_extension(s, KVM_CAP_MEMORY_ATTRIBUTES);
>      kvm_guest_memfd_supported =
>          kvm_check_extension(s, KVM_CAP_GUEST_MEMFD) &&
>          kvm_check_extension(s, KVM_CAP_USER_MEMORY2) &&
>          (kvm_supported_memory_attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE);
>  
> +    ret = kvm_arch_init(ms, s);
> +    if (ret < 0) {
> +        goto err;
> +    }
> +
>      if (s->kernel_irqchip_split == ON_OFF_AUTO_AUTO) {
>          s->kernel_irqchip_split = mc->default_kernel_irqchip_split ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>      }
> 
> Checking, I discovered that it was done on purpose by Paolo, so not sure 
> if my fix is valid:
> 
> commit 586d708c1e3e5e29a0b3c05c347290aed9478854
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Fri Oct 11 10:39:58 2024 +0200
> 
>     accel/kvm: check for KVM_CAP_MEMORY_ATTRIBUTES on vm
>    
>     The exact set of available memory attributes can vary by VM.  In the
>     future it might vary depending on enabled capabilities, too.  Query the
>     extension on the VM level instead of on the KVM level, and only after
>     architecture-specific initialization.
>    
>     Inspired by an analogous patch by Tom Dohrmann.
>    
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> @Paolo any suggestion?
> 
> Thanks,
> Stefano
> 
Hi Stefano,

Thanks for testing this. The problem seems to be down to the fact that I
had to introduce an initial parsing of the IGVM file during initialization
to extract sev_features. I was parsing all directives in the file but it
appears this has some unwanted side effects.

Please could you try the patch below to see if it fixes the issue? If it
does I'll incorporate it into the patch series and resubmit.

From 3590460ec3945b02a679ad79735681a642596d60 Mon Sep 17 00:00:00 2001
From: Roy Hopkins <roy.hopkins@randomman.co.uk>
Date: Thu, 6 Mar 2025 11:25:07 +0000
Subject: [PATCH 1/1] backends/igvm: Add function to process only VP context

When initializing kvm for SEV, the sev_features need to be
passed to the initialization function. When using IGVM files,
sev_features is provided in the VP context definintions in
the file. Currently this is handled in sev.c by processing
the entire file to extract the VP context, however this has
unwanted side-effects. Therefore this commit adds a new
function that allows only the VP context definitions to
be parsed in the IGVM file.

Signed-off-by: Roy Hopkins <roy.hopkins@randomman.co.uk>
---
 backends/igvm-cfg.c       |  1 +
 backends/igvm.c           | 51 +++++++++++++++++++++++++++++++++++++++
 backends/igvm.h           |  3 +++
 include/system/igvm-cfg.h | 10 ++++++++
 target/i386/sev.c         | 10 ++++----
 5 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/backends/igvm-cfg.c b/backends/igvm-cfg.c
index 38f17dae44..25c4469768 100644
--- a/backends/igvm-cfg.c
+++ b/backends/igvm-cfg.c
@@ -41,6 +41,7 @@ static void igvm_cfg_class_init(ObjectClass *oc, void *data)
                                           "Set the IGVM filename to use");
 
     igvmc->process = qigvm_process_file;
+    igvmc->process_vp_context = qigvm_process_vp_context;
 }
 
 static void igvm_cfg_init(Object *obj)
diff --git a/backends/igvm.c b/backends/igvm.c
index 7673e4a882..aae83f8a77 100644
--- a/backends/igvm.c
+++ b/backends/igvm.c
@@ -965,3 +965,54 @@ cleanup:
 
     return retval;
 }
+
+int qigvm_process_vp_context(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
+                             Error **errp)
+{
+    int32_t header_count;
+    int retval = -1;
+    QIgvm ctx;
+
+    memset(&ctx, 0, sizeof(ctx));
+    ctx.file = qigvm_file_init(cfg->filename, errp);
+    if (ctx.file < 0) {
+        return -1;
+    }
+
+    ctx.cgs = cgs;
+    ctx.cgsc = cgs ? CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(cgs) : NULL;
+
+    /*
+     * Check that the IGVM file provides configuration for the current
+     * platform
+     */
+    if (qigvm_supported_platform_compat_mask(&ctx, errp) < 0) {
+        goto cleanup;
+    }
+
+    header_count = igvm_header_count(ctx.file, IGVM_HEADER_SECTION_DIRECTIVE);
+    if (header_count <= 0) {
+        error_setg(
+            errp, "Invalid directive header count in IGVM file. Error code: %X",
+            header_count);
+        goto cleanup;
+    }
+
+    for (ctx.current_header_index = 0;
+         ctx.current_header_index < (unsigned)header_count;
+         ctx.current_header_index++) {
+        IgvmVariableHeaderType type = igvm_get_header_type(
+            ctx.file, IGVM_HEADER_SECTION_DIRECTIVE, ctx.current_header_index);
+        if (type == IGVM_VHT_VP_CONTEXT) {
+            if (qigvm_handler(&ctx, type, errp) < 0) {
+                goto cleanup;
+            }
+        }
+    }
+    retval = 0;
+
+cleanup:
+    igvm_free(ctx.file);
+    
+    return retval;
+}
diff --git a/backends/igvm.h b/backends/igvm.h
index 269eb3a10e..a43b029d56 100644
--- a/backends/igvm.h
+++ b/backends/igvm.h
@@ -20,4 +20,7 @@
 int qigvm_process_file(IgvmCfg *igvm, ConfidentialGuestSupport *cgs,
                       Error **errp);
 
+int qigvm_process_vp_context(IgvmCfg *igvm, ConfidentialGuestSupport *cgs,
+                             Error **errp);
+
 #endif
diff --git a/include/system/igvm-cfg.h b/include/system/igvm-cfg.h
index 21fadfe5b7..0c1a7ef309 100644
--- a/include/system/igvm-cfg.h
+++ b/include/system/igvm-cfg.h
@@ -38,6 +38,16 @@ typedef struct IgvmCfgClass {
     int (*process)(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
                    Error **errp);
 
+    /*
+     * If an IGVM filename has been specified then only process 
+     * the VMSA sections in the IGVM file.
+     * Performs a no-op if no filename has been specified.
+     *
+     * Returns 0 for ok and -1 on error.
+     */
+    int (*process_vp_context)(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
+        Error **errp);
+
 } IgvmCfgClass;
 
 #define TYPE_IGVM_CFG "igvm-cfg"
diff --git a/target/i386/sev.c b/target/i386/sev.c
index ef25e64b14..d22e9870ea 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1893,14 +1893,14 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
          * each vcpu.
          *
          * The IGVM file is normally processed after initialization. Therefore
-         * we need to pre-process it here to extract sev_features in order to
-         * provide it to KVM_SEV_INIT2. Each cgs_* function that is called by
-         * the IGVM processor detects this pre-process by observing the state
-         * as SEV_STATE_UNINIT.
+         * we need to pre-process it here, just looking for the vp_context to
+         * extract sev_features in order to provide it to KVM_SEV_INIT2. Each
+         * cgs_* function that is called by the IGVM processor detects this 
+         * pre-process by observing the state as SEV_STATE_UNINIT.
          */
         if (x86machine->igvm) {
             if (IGVM_CFG_GET_CLASS(x86machine->igvm)
-                    ->process(x86machine->igvm, machine->cgs, errp) == -1) {
+                    ->process_vp_context(x86machine->igvm, machine->cgs, errp) == -1) {
                 return -1;
             }
             /*
-- 
2.43.0