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

2025-12-11 Thread Stefano Garzarella

On Thu, Dec 11, 2025 at 10:24:35AM +0100, Oliver Steffen wrote:

On Thu, Dec 11, 2025 at 9:46 AM Stefano Garzarella  wrote:

On Thu, Dec 11, 2025 at 09:15:17AM +0100, Oliver Steffen wrote:


[...]


>diff --git a/target/i386/sev.c b/target/i386/sev.c
>index fd2dada013..ffeb9f52a2 100644
>--- a/target/i386/sev.c
>+++ b/target/i386/sev.c
>@@ -1892,7 +1892,7 @@ static int sev_common_kvm_init(ConfidentialGuestSupport 
*cgs, Error **errp)
>  */
> if (x86machine->igvm) {
> if (IGVM_CFG_GET_CLASS(x86machine->igvm)
>-->process(x86machine->igvm, machine->cgs, true, errp) ==
>+->process(x86machine->igvm, machine->cgs, true, NULL, 
errp) ==

Why here we don't need to pass it?


Here we only read the IGVM to figure out the initial vcpu configuration
(the `onlyVpContext` parameter is true) to initialize kvm,
The actual IGVM processing is done later.


okay, I see, thanks!


Should I mention in the comment above why madt is NULL here ?


Yes, please :-)

Stefano




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

2025-12-11 Thread Oliver Steffen
On Thu, Dec 11, 2025 at 9:46 AM Stefano Garzarella  wrote:
>
> On Thu, Dec 11, 2025 at 09:15:17AM +0100, Oliver Steffen wrote:
> >Use the new acpi_build_madt_standalone() function to fill the MADT
> >parameter field.
>
> The cover letter will not usually be part of the git history, so IMO it
> is better to include also here the information that you have rightly
> written there, explaining why we are adding this change.

Will do.

> >
> >Signed-off-by: Oliver Steffen 
> >---
> > backends/igvm-cfg.c   |  8 +++-
> > backends/igvm.c   | 37 -
> > include/system/igvm-cfg.h |  4 ++--
> > include/system/igvm.h |  2 +-
> > target/i386/sev.c |  2 +-
> > 5 files changed, 47 insertions(+), 6 deletions(-)
> >
> >diff --git a/backends/igvm-cfg.c b/backends/igvm-cfg.c
> >index c1b45401f4..0a77f7b7a1 100644
> >--- a/backends/igvm-cfg.c
> >+++ b/backends/igvm-cfg.c
> >@@ -17,6 +17,7 @@
> > #include "qom/object_interfaces.h"
> > #include "hw/qdev-core.h"
> > #include "hw/boards.h"
> >+#include "hw/i386/acpi-build.h"
> >
> > #include "trace.h"
> >
> >@@ -48,10 +49,15 @@ static void igvm_reset_hold(Object *obj, ResetType type)
> > {
> > MachineState *ms = MACHINE(qdev_get_machine());
> > IgvmCfg *igvm = IGVM_CFG(obj);
> >+GArray *madt = NULL;
> >
> > trace_igvm_reset_hold(type);
> >
> >-qigvm_process_file(igvm, ms->cgs, false, &error_fatal);
> >+madt = acpi_build_madt_standalone(ms);
> >+
> >+qigvm_process_file(igvm, ms->cgs, false, madt, &error_fatal);
> >+
> >+g_array_free(madt, true);
> > }
> >
> > static void igvm_reset_exit(Object *obj, ResetType type)
> >diff --git a/backends/igvm.c b/backends/igvm.c
> >index a350c890cc..7e56b19b0a 100644
> >--- a/backends/igvm.c
> >+++ b/backends/igvm.c
> >@@ -93,6 +93,7 @@ typedef struct QIgvm {
> > unsigned region_start_index;
> > unsigned region_last_index;
> > unsigned region_page_count;
> >+GArray *madt;
> > } QIgvm;
> >
> > static int qigvm_directive_page_data(QIgvm *ctx, const uint8_t *header_data,
> >@@ -120,6 +121,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_initialization_madt(QIgvm *ctx,
> >+ const uint8_t *header_data, Error 
> >**errp);
> >
> > struct QIGVMHandler {
> > uint32_t type;
> >@@ -148,6 +151,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_initialization_madt },
> > };
> >
> > static int qigvm_handler(QIgvm *ctx, uint32_t type, Error **errp)
> >@@ -764,6 +769,34 @@ static int qigvm_initialization_guest_policy(QIgvm *ctx,
> > return 0;
> > }
> >
> >+static int qigvm_initialization_madt(QIgvm *ctx,
> >+ const uint8_t *header_data, Error 
> >**errp)
> >+{
> >+const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER 
> >*)header_data;
> >+QIgvmParameterData *param_entry;
> >+
> >+if (ctx->madt == NULL) {
> >+return 0;
> >+}
> >+
> >+/* Find the parameter area that should hold the device tree */
> >+QTAILQ_FOREACH(param_entry, &ctx->parameter_data, next)
> >+{
> >+if (param_entry->index == param->parameter_area_index) {
> >+
> >+if (ctx->madt->len > param_entry->size) {
> >+error_setg(
> >+errp,
> >+"IGVM: MADT size exceeds parameter area defined in IGVM 
> >file");
> >+return -1;
> >+}
> >+memcpy(param_entry->data, ctx->madt->data, ctx->madt->len);
> >+break;
> >+}
> >+}
> >+return 0;
> >+}
> >+
> > static int qigvm_supported_platform_compat_mask(QIgvm *ctx, Error **errp)
> > {
> > int32_t header_count;
> >@@ -892,7 +925,7 @@ IgvmHandle qigvm_file_init(char *filename, Error **errp)
> > }
> >
> > int qigvm_process_file(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
> >-   bool onlyVpContext, Error **errp)
> >+   bool onlyVpContext, GArray *madt, Error **errp)
> > {
> > int32_t header_count;
> > QIgvmParameterData *parameter;
> >@@ -915,6 +948,8 @@ int qigvm_process_file(IgvmCfg *cfg, 
> >ConfidentialGuestSupport *cgs,
> > ctx.cgs = cgs;
> > ctx.cgsc = cgs ? CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(cgs) : NULL;
> >
> >+ctx.madt = madt;
> >+
> > /*
> >  * Check that the IGVM file provides configuration for the current
> >  * platform
> >diff --git a/include/system/igvm-cfg.h b/include/system/igvm-cfg.h
> >index 7dc48677fd..1a04302beb 100644
> >--- a/include/s

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

2025-12-11 Thread Stefano Garzarella

On Thu, Dec 11, 2025 at 09:15:17AM +0100, Oliver Steffen wrote:

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


The cover letter will not usually be part of the git history, so IMO it 
is better to include also here the information that you have rightly 
written there, explaining why we are adding this change.




Signed-off-by: Oliver Steffen 
---
backends/igvm-cfg.c   |  8 +++-
backends/igvm.c   | 37 -
include/system/igvm-cfg.h |  4 ++--
include/system/igvm.h |  2 +-
target/i386/sev.c |  2 +-
5 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/backends/igvm-cfg.c b/backends/igvm-cfg.c
index c1b45401f4..0a77f7b7a1 100644
--- a/backends/igvm-cfg.c
+++ b/backends/igvm-cfg.c
@@ -17,6 +17,7 @@
#include "qom/object_interfaces.h"
#include "hw/qdev-core.h"
#include "hw/boards.h"
+#include "hw/i386/acpi-build.h"

#include "trace.h"

@@ -48,10 +49,15 @@ static void igvm_reset_hold(Object *obj, ResetType type)
{
MachineState *ms = MACHINE(qdev_get_machine());
IgvmCfg *igvm = IGVM_CFG(obj);
+GArray *madt = NULL;

trace_igvm_reset_hold(type);

-qigvm_process_file(igvm, ms->cgs, false, &error_fatal);
+madt = acpi_build_madt_standalone(ms);
+
+qigvm_process_file(igvm, ms->cgs, false, madt, &error_fatal);
+
+g_array_free(madt, true);
}

static void igvm_reset_exit(Object *obj, ResetType type)
diff --git a/backends/igvm.c b/backends/igvm.c
index a350c890cc..7e56b19b0a 100644
--- a/backends/igvm.c
+++ b/backends/igvm.c
@@ -93,6 +93,7 @@ typedef struct QIgvm {
unsigned region_start_index;
unsigned region_last_index;
unsigned region_page_count;
+GArray *madt;
} QIgvm;

static int qigvm_directive_page_data(QIgvm *ctx, const uint8_t *header_data,
@@ -120,6 +121,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_initialization_madt(QIgvm *ctx,
+ const uint8_t *header_data, Error **errp);

struct QIGVMHandler {
uint32_t type;
@@ -148,6 +151,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_initialization_madt },
};

static int qigvm_handler(QIgvm *ctx, uint32_t type, Error **errp)
@@ -764,6 +769,34 @@ static int qigvm_initialization_guest_policy(QIgvm *ctx,
return 0;
}

+static int qigvm_initialization_madt(QIgvm *ctx,
+ const uint8_t *header_data, Error **errp)
+{
+const IGVM_VHS_PARAMETER *param = (const IGVM_VHS_PARAMETER *)header_data;
+QIgvmParameterData *param_entry;
+
+if (ctx->madt == NULL) {
+return 0;
+}
+
+/* Find the parameter area that should hold the device tree */
+QTAILQ_FOREACH(param_entry, &ctx->parameter_data, next)
+{
+if (param_entry->index == param->parameter_area_index) {
+
+if (ctx->madt->len > param_entry->size) {
+error_setg(
+errp,
+"IGVM: MADT size exceeds parameter area defined in IGVM 
file");
+return -1;
+}
+memcpy(param_entry->data, ctx->madt->data, ctx->madt->len);
+break;
+}
+}
+return 0;
+}
+
static int qigvm_supported_platform_compat_mask(QIgvm *ctx, Error **errp)
{
int32_t header_count;
@@ -892,7 +925,7 @@ IgvmHandle qigvm_file_init(char *filename, Error **errp)
}

int qigvm_process_file(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
-   bool onlyVpContext, Error **errp)
+   bool onlyVpContext, GArray *madt, Error **errp)
{
int32_t header_count;
QIgvmParameterData *parameter;
@@ -915,6 +948,8 @@ int qigvm_process_file(IgvmCfg *cfg, 
ConfidentialGuestSupport *cgs,
ctx.cgs = cgs;
ctx.cgsc = cgs ? CONFIDENTIAL_GUEST_SUPPORT_GET_CLASS(cgs) : NULL;

+ctx.madt = madt;
+
/*
 * Check that the IGVM file provides configuration for the current
 * platform
diff --git a/include/system/igvm-cfg.h b/include/system/igvm-cfg.h
index 7dc48677fd..1a04302beb 100644
--- a/include/system/igvm-cfg.h
+++ b/include/system/igvm-cfg.h
@@ -42,8 +42,8 @@ typedef struct IgvmCfgClass {
 *
 * Returns 0 for ok and -1 on error.
 */


Should we update the documentation of this function now that we have a 
new parameter, also explaining that it's optional.



-int (*process)(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
-   bool onlyVpContext, Error **errp);
+int (*process)(IgvmCfg *cfg, ConfidentialGuestSupport *cgs,
+   bool onlyVpContext, GArray *madt, E