Re: [PATCH v2 3/3] igvm: Fill MADT IGVM parameter field
Hi, > > > (b) pass MachineState instead of ConfidentialGuestSupport, so > > > we can use the MachineState here to generate the madt. > > > > > >Luigi, any opinion? I think device tree support will need access to > > >MachineState too, and I think both madt and dt should take the same > > >approach here. > > > > I have a slight preference over MachineState as it's more generic and we > > don't need to add more fields in IgvmCfg for new features. > > > Passing in MachineState would be easy, but do we really want to add machine > related logic (building of ACPI tables, and later maybe device trees) > into the igvm backend? Good point. That clearly speaks for (b). There already is MachineState->fdt, filled in by machine code. We can let machine code store the madt in MachineState too, and the have the igvm code simply pick it up from there. take care, Gerd
Re: [PATCH v2 3/3] igvm: Fill MADT IGVM parameter field
On Thu, Dec 11, 2025 at 12:30 PM Luigi Leonardi wrote:
>
> Hi,
>
> On Thu, Dec 11, 2025 at 12:15:59PM +0100, Gerd Hoffmann wrote:
> > Hi,
> >
> >> +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 */
> >
> >cut+paste error in the comment.
Will do.
> >
> >> +QTAILQ_FOREACH(param_entry, &ctx->parameter_data, next)
> >> +{
> >> +if (param_entry->index == param->parameter_area_index) {
> >
> >Hmm, that is a pattern repeated a number of times already in the igvm
> >code. Should we factor that out into a helper function?
>
> +1
Will do.
>
> >
> >> 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)
> >
> >I'd like to see less parameters for this function, not more.
> >
> >I think sensible options here are:
> >
> > (a) store the madt pointer in IgvmCfg, or
> > (b) pass MachineState instead of ConfidentialGuestSupport, so
> > we can use the MachineState here to generate the madt.
> >
> >Luigi, any opinion? I think device tree support will need access to
> >MachineState too, and I think both madt and dt should take the same
> >approach here.
>
> I have a slight preference over MachineState as it's more generic and we
> don't need to add more fields in IgvmCfg for new features.
>
Passing in MachineState would be easy, but do we really want to add machine
related logic (building of ACPI tables, and later maybe device trees)
into the igvm backend?
> Luigi
>
Re: [PATCH v2 3/3] igvm: Fill MADT IGVM parameter field
Hi,
On Thu, Dec 11, 2025 at 12:15:59PM +0100, Gerd Hoffmann wrote:
Hi,
+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 */
cut+paste error in the comment.
+QTAILQ_FOREACH(param_entry, &ctx->parameter_data, next)
+{
+if (param_entry->index == param->parameter_area_index) {
Hmm, that is a pattern repeated a number of times already in the igvm
code. Should we factor that out into a helper function?
+1
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)
I'd like to see less parameters for this function, not more.
I think sensible options here are:
(a) store the madt pointer in IgvmCfg, or
(b) pass MachineState instead of ConfidentialGuestSupport, so
we can use the MachineState here to generate the madt.
Luigi, any opinion? I think device tree support will need access to
MachineState too, and I think both madt and dt should take the same
approach here.
I have a slight preference over MachineState as it's more generic and we
don't need to add more fields in IgvmCfg for new features.
Luigi
Re: [PATCH v2 3/3] igvm: Fill MADT IGVM parameter field
Hi,
> +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 */
cut+paste error in the comment.
> +QTAILQ_FOREACH(param_entry, &ctx->parameter_data, next)
> +{
> +if (param_entry->index == param->parameter_area_index) {
Hmm, that is a pattern repeated a number of times already in the igvm
code. Should we factor that out into a helper function?
> 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)
I'd like to see less parameters for this function, not more.
I think sensible options here are:
(a) store the madt pointer in IgvmCfg, or
(b) pass MachineState instead of ConfidentialGuestSupport, so
we can use the MachineState here to generate the madt.
Luigi, any opinion? I think device tree support will need access to
MachineState too, and I think both madt and dt should take the same
approach here.
Long-term I'd like to also get rid of the onlyVpContext parameter.
That cleanup is something for another patch series though.
take care,
Gerd
