Re: [Qemu-devel] [PATCH 18/26] hw: acpi: Initial hardware-reduced support

[Paolo Bonzini <pbonzini@redhat.com>]

On 22/10/2018 20:36, Samuel Ortiz wrote:
> +
> +static void acpi_reduced_build_update(void *build_opaque)
> +{
> +    MachineState *ms = MACHINE(build_opaque);
> +    AcpiBuildState *build_state = ms->firmware_build_state.acpi.state;
> +    AcpiConfiguration *conf = ms->firmware_build_state.acpi.conf;
> +    AcpiBuildTables tables;
> +
> +    /* No ACPI configuration? Nothing to do. */
> +    if (!conf) {
> +        return;
> +    }
> +
> +    /* No state to update or already patched? Nothing to do. */
> +    if (!build_state || build_state->patched) {
> +        return;
> +    }
> +    build_state->patched = true;
> +
> +    acpi_build_tables_init(&tables);
> +
> +    acpi_reduced_build(ms, &tables, conf);
> +
> +    acpi_ram_update(build_state->table_mr, tables.table_data);
> +    acpi_ram_update(build_state->rsdp_mr, tables.rsdp);
> +    acpi_ram_update(build_state->linker_mr, tables.linker->cmd_blob);
> +
> +    acpi_build_tables_cleanup(&tables, true);
> +}
> +

ms is not needed here; just pass the FirmwareBuildState as the opaque
value in rom_add_blob.

In fact, here:

> +    AcpiBuildState *build_state;
> +
> +    build_state = g_malloc0(sizeof(*build_state));
> +    machine->firmware_build_state.acpi.state = build_state;
> +    machine->firmware_build_state.acpi.conf = conf;
> +

I would say that you don't need FirmwareBuildState at all.  I cannot be
100% sure because I cannot see the caller of acpi_reduced_setup, but I
think you can add an AcpiConfiguration* field to AcpiBuildState and
encapsulate everything in AcpiBuildState.  In addition, the
AcpiBuildState need not be stored in the MachineState.

Instead, FirmwareBuildMethods should be a QOM interface (also please
refer explicitly to ACPI in the names, don't call it "firmware").  The
setup method of FirmwareBuildMethods can take that QOM interface, not
the MachineState, i.e. its prototype should be

void (*setup)(AcpiBuildMethods *acpibuild,
	      AcpiConfiguration *conf);

so that pc_machine_done does

    if (pcms->acpi_build_enabled) {
        acpi_conf_pc_init(pcms);
	/* This calls the ->setup method.  */
        acpi_builder_setup(ACPI_BUILD_METHODS(pcms),
			   &pcms->acpi_configuration);
    }

This is because MachineClass is used for dozens of machines that have
nothing to with ACPI.  Instead, machines that use ACPI (either reduced
or normal) can define the AcpiBuildMethods interface, and invoke the
entry point firmware_build_methods->setup (either acpi_setup or
acpi_reduced_setup) through acpi_builder_setup.

Paolo