[v4] igvm: Supply MADT via IGVM parameter

[PATCH v4 5/5] igvm: Fill MADT IGVM parameter field

Posted by Oliver Steffen 3 weeks, 4 days ago

Use the new acpi_build_madt_standalone() function to fill the MADT
parameter field.

The IGVM parameter can be consumed by Coconut SVSM [1], instead of
relying on the fw_cfg interface, which has caused problems before due to
unexpected access [2,3]. Using IGVM parameters is the default way for
Coconut SVSM across hypervisors; switching over would allow removing
specialized code paths for QEMU in Coconut.

Coconut SVSM needs to know the SMP configuration, but does not look at
any other ACPI data, nor does it interact with the PCI bus settings.
Since the MADT is static and not linked with other ACPI tables, it can
be supplied stand-alone like this.

Generating the MADT twice (during ACPI table building and IGVM processing)
seems acceptable, since there is no infrastructure to obtain the MADT
out of the ACPI table memory area.

In any case OVMF, which runs after SVSM has already been initialized,
will continue reading all ACPI tables via fw_cfg and provide fixed up
ACPI data to the OS as before without any changes.

[1] https://github.com/coconut-svsm/svsm/pull/858
[2] https://gitlab.com/qemu-project/qemu/-/issues/2882
[3] https://github.com/coconut-svsm/svsm/issues/646

Signed-off-by: Oliver Steffen <osteffen@redhat.com>
---
 backends/igvm.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/backends/igvm.c b/backends/igvm.c
index cb2f997c87..980068fb58 100644
--- a/backends/igvm.c
+++ b/backends/igvm.c
@@ -18,6 +18,7 @@
 #include "system/memory.h"
 #include "system/address-spaces.h"
 #include "hw/core/cpu.h"
+#include "hw/i386/acpi-build.h"
 
 #include "trace.h"
 
@@ -134,6 +135,8 @@ static int qigvm_directive_snp_id_block(QIgvm *ctx, const uint8_t *header_data,
 static int qigvm_initialization_guest_policy(QIgvm *ctx,
                                        const uint8_t *header_data,
                                        Error **errp);
+static int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data,
+                                Error **errp);
 
 struct QIGVMHandler {
     uint32_t type;
@@ -162,6 +165,8 @@ static struct QIGVMHandler handlers[] = {
       qigvm_directive_snp_id_block },
     { IGVM_VHT_GUEST_POLICY, IGVM_HEADER_SECTION_INITIALIZATION,
       qigvm_initialization_guest_policy },
+    { IGVM_VHT_MADT, IGVM_HEADER_SECTION_DIRECTIVE,
+      qigvm_directive_madt },
 };
 
 static int qigvm_handler(QIgvm *ctx, uint32_t type, Error **errp)
@@ -771,6 +776,33 @@ static int qigvm_initialization_guest_policy(QIgvm *ctx,
     return 0;
 }
 
+static int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data,
+                                Error **errp)
+{
+    const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
+    QIgvmParameterData *param_entry;
+    int result = 0;
+
+    /* Find the parameter area that should hold the MADT data */
+    param_entry = qigvm_find_param_entry(ctx, param);
+    if (param_entry != NULL) {
+
+        GArray *madt = acpi_build_madt_standalone(ctx->machine_state);
+
+        if (madt->len <= param_entry->size) {
+            memcpy(param_entry->data, madt->data, madt->len);
+        } else {
+            error_setg(
+                errp,
+                "IGVM: MADT size exceeds parameter area defined in IGVM file");
+            result = -1;
+        }
+
+        g_array_free(madt, true);
+    }
+    return result;
+}
+
 static int qigvm_supported_platform_compat_mask(QIgvm *ctx, Error **errp)
 {
     int32_t header_count;
-- 
2.52.0

Re: [PATCH v4 5/5] igvm: Fill MADT IGVM parameter field

Posted by Luigi Leonardi 3 weeks, 4 days ago

On Wed, Jan 14, 2026 at 06:50:07PM +0100, Oliver Steffen wrote:
>Use the new acpi_build_madt_standalone() function to fill the MADT
>parameter field.
>
>The IGVM parameter can be consumed by Coconut SVSM [1], instead of
>relying on the fw_cfg interface, which has caused problems before due to
>unexpected access [2,3]. Using IGVM parameters is the default way for
>Coconut SVSM across hypervisors; switching over would allow removing
>specialized code paths for QEMU in Coconut.
>
>Coconut SVSM needs to know the SMP configuration, but does not look at
>any other ACPI data, nor does it interact with the PCI bus settings.
>Since the MADT is static and not linked with other ACPI tables, it can
>be supplied stand-alone like this.
>
>Generating the MADT twice (during ACPI table building and IGVM processing)
>seems acceptable, since there is no infrastructure to obtain the MADT
>out of the ACPI table memory area.
>
>In any case OVMF, which runs after SVSM has already been initialized,
>will continue reading all ACPI tables via fw_cfg and provide fixed up
>ACPI data to the OS as before without any changes.
>
>[1] https://github.com/coconut-svsm/svsm/pull/858
>[2] https://gitlab.com/qemu-project/qemu/-/issues/2882
>[3] https://github.com/coconut-svsm/svsm/issues/646
>
>Signed-off-by: Oliver Steffen <osteffen@redhat.com>
>---
> backends/igvm.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
>diff --git a/backends/igvm.c b/backends/igvm.c
>index cb2f997c87..980068fb58 100644
>--- a/backends/igvm.c
>+++ b/backends/igvm.c
>@@ -18,6 +18,7 @@
> #include "system/memory.h"
> #include "system/address-spaces.h"
> #include "hw/core/cpu.h"
>+#include "hw/i386/acpi-build.h"
>
> #include "trace.h"
>
>@@ -134,6 +135,8 @@ static int qigvm_directive_snp_id_block(QIgvm *ctx, const uint8_t *header_data,
> static int qigvm_initialization_guest_policy(QIgvm *ctx,
>                                        const uint8_t *header_data,
>                                        Error **errp);
>+static int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data,
>+                                Error **errp);
>
> struct QIGVMHandler {
>     uint32_t type;
>@@ -162,6 +165,8 @@ static struct QIGVMHandler handlers[] = {
>       qigvm_directive_snp_id_block },
>     { IGVM_VHT_GUEST_POLICY, IGVM_HEADER_SECTION_INITIALIZATION,
>       qigvm_initialization_guest_policy },
>+    { IGVM_VHT_MADT, IGVM_HEADER_SECTION_DIRECTIVE,
>+      qigvm_directive_madt },
> };
>
> static int qigvm_handler(QIgvm *ctx, uint32_t type, Error **errp)
>@@ -771,6 +776,33 @@ static int qigvm_initialization_guest_policy(QIgvm *ctx,
>     return 0;
> }
>
>+static int qigvm_directive_madt(QIgvm *ctx, const uint8_t *header_data,
>+                                Error **errp)
>+{
>+    const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
>+    QIgvmParameterData *param_entry;
>+    int result = 0;
>+
>+    /* Find the parameter area that should hold the MADT data */
>+    param_entry = qigvm_find_param_entry(ctx, param);
>+    if (param_entry != NULL) {

what about an early return here? I think it would make the code much 
cleaner.

On top of that, we return 0 even if we don't find the entry, is that 
correct?

Luigi

Re: [PATCH v4 5/5] igvm: Fill MADT IGVM parameter field

Posted by Gerd Hoffmann 3 weeks, 4 days ago

  Hi,

> > +    /* Find the parameter area that should hold the MADT data */
> > +    param_entry = qigvm_find_param_entry(ctx, param);
> > +    if (param_entry != NULL) {

> On top of that, we return 0 even if we don't find the entry, is that
> correct?

If we can't find the parameter area the IGVM file is not consistent.
I think it makes sense to warn about that.  Not sure if we should
treat that as fatal error.

take care,
  Gerd

[PATCH v4 1/5] hw/acpi: Make BIOS linker optional
[PATCH v4 2/5] hw/acpi: Add standalone function to build MADT
[PATCH v4 3/5] igvm: Add common function for finding parameter entries
[PATCH v4 4/5] igvm: Pass machine state to IGVM file processing
[PATCH v4 5/5] igvm: Fill MADT IGVM parameter field