| Message ID | 20181029170159.3801-20-sameo@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI reorganization for hardware-reduced support | expand |
On 29/10/2018 18:01, Samuel Ortiz wrote: > @@ -1556,6 +1557,7 @@ void acpi_build(AcpiBuildTables *tables, > GArray *tables_blob = tables->table_data; > AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL }; > Object *vmgenid_dev; > + AcpiBuilder *ab = ACPI_BUILDER(machine); > > acpi_get_pm_info(&pm); > acpi_get_misc_info(&misc); > @@ -1605,7 +1607,8 @@ void acpi_build(AcpiBuildTables *tables, > aml_len += tables_blob->len - fadt; > > acpi_add_table(table_offsets, tables_blob); > - build_madt(tables_blob, tables->linker, machine, conf); > + acpi_builder_madt(ab, tables_blob, tables->linker, > + machine, conf); > > vmgenid_dev = find_vmgenid_dev(); > if (vmgenid_dev) { Just a quick question before I go and actually apply the patches to look at the resulting code: is there any reason why you didn't add the MachineState and/or AcpiBuilder to AcpiBuildState, similar to how it was in v1? Thanks, Paolo
On 29/10/18 18:01, Samuel Ortiz wrote: > All PC machine type derivatives will use the same ACPI table build > methods. But with that change in place, any new x86 machine type will be > able to re-use the acpi-build API and customize part of it by defining > its own ACPI table build methods. > > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com> > --- > hw/i386/acpi-build.c | 15 ++++++++++----- > hw/i386/pc.c | 19 +++++++++++++++++++ > 2 files changed, 29 insertions(+), 5 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 5035dac556..d40599e6d1 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -35,6 +35,7 @@ > #include "hw/acpi/acpi-defs.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/cpu.h" > +#include "hw/acpi/builder.h" > #include "hw/nvram/fw_cfg.h" > #include "hw/acpi/bios-linker-loader.h" > #include "hw/loader.h" > @@ -1556,6 +1557,7 @@ void acpi_build(AcpiBuildTables *tables, > GArray *tables_blob = tables->table_data; > AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL }; > Object *vmgenid_dev; > + AcpiBuilder *ab = ACPI_BUILDER(machine); > > acpi_get_pm_info(&pm); > acpi_get_misc_info(&misc); > @@ -1605,7 +1607,8 @@ void acpi_build(AcpiBuildTables *tables, > aml_len += tables_blob->len - fadt; > > acpi_add_table(table_offsets, tables_blob); > - build_madt(tables_blob, tables->linker, machine, conf); > + acpi_builder_madt(ab, tables_blob, tables->linker, > + machine, conf); > > vmgenid_dev = find_vmgenid_dev(); > if (vmgenid_dev) { > @@ -1629,15 +1632,17 @@ void acpi_build(AcpiBuildTables *tables, > } > if (conf->numa_nodes) { > acpi_add_table(table_offsets, tables_blob); > - build_srat(tables_blob, tables->linker, machine, conf); > + > + acpi_builder_srat(ab, tables_blob, tables->linker, > + machine, conf); > if (have_numa_distance) { > acpi_add_table(table_offsets, tables_blob); > - build_slit(tables_blob, tables->linker); > + acpi_builder_slit(ab, tables_blob, tables->linker); > } > } > if (acpi_get_mcfg(&mcfg)) { > acpi_add_table(table_offsets, tables_blob); > - build_mcfg(tables_blob, tables->linker, &mcfg); > + acpi_builder_mcfg(ab, tables_blob, tables->linker, &mcfg); > } > if (x86_iommu_get_default()) { > IommuType IOMMUType = x86_iommu_get_type(); > @@ -1668,7 +1673,7 @@ void acpi_build(AcpiBuildTables *tables, > slic_oem.id, slic_oem.table_id); > > /* RSDP is in FSEG memory, so allocate it separately */ > - build_rsdp_rsdt(tables->rsdp, tables->linker, rsdt); > + acpi_builder_rsdp(ab, tables->rsdp, tables->linker, rsdt); > > /* We'll expose it all to Guest so we want to reduce > * chance of size changes. > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 1dcbbd5139..986ed0eabd 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -64,6 +64,7 @@ > #include "qemu/option.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/cpu_hotplug.h" > +#include "hw/acpi/builder.h" > #include "hw/boards.h" > #include "acpi-build.h" > #include "hw/mem/pc-dimm.h" > @@ -75,6 +76,7 @@ > #include "hw/nmi.h" > #include "hw/i386/intel_iommu.h" > #include "hw/net/ne2000-isa.h" > +#include "hw/i386/acpi.h" > > /* debug PC/ISA interrupts */ > //#define DEBUG_IRQ > @@ -2403,12 +2405,20 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp) > } > } > > +static AcpiConfiguration *pc_acpi_configuration(AcpiBuilder *builder) > +{ > + PCMachineState *pcms = PC_MACHINE(builder); > + > + return &pcms->acpi_configuration; > +} > + > static void pc_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); > HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); > NMIClass *nc = NMI_CLASS(oc); > + AcpiBuilderMethods *abm = ACPI_BUILDER_METHODS(oc); > > pcmc->pci_enabled = true; > pcmc->has_acpi_build = true; > @@ -2443,6 +2453,14 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > nc->nmi_monitor_handler = x86_nmi; > mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE; > > + /* ACPI building methods */ > + abm->madt = build_madt; > + abm->rsdp = build_rsdp_rsdt; Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com> > + abm->mcfg = build_mcfg; > + abm->srat = build_srat; > + abm->slit = build_slit; > + abm->configuration = pc_acpi_configuration; > + > object_class_property_add(oc, MEMORY_DEVICE_REGION_SIZE, "int", > pc_machine_get_device_memory_region_size, NULL, > NULL, NULL, &error_abort); > @@ -2494,6 +2512,7 @@ static const TypeInfo pc_machine_info = { > .interfaces = (InterfaceInfo[]) { > { TYPE_HOTPLUG_HANDLER }, > { TYPE_NMI }, > + { TYPE_ACPI_BUILDER }, > { } > }, > }; >
Hi Paolo, On Mon, Oct 29, 2018 at 06:28:39PM +0100, Paolo Bonzini wrote: > On 29/10/2018 18:01, Samuel Ortiz wrote: > > @@ -1556,6 +1557,7 @@ void acpi_build(AcpiBuildTables *tables, > > GArray *tables_blob = tables->table_data; > > AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL }; > > Object *vmgenid_dev; > > + AcpiBuilder *ab = ACPI_BUILDER(machine); > > > > acpi_get_pm_info(&pm); > > acpi_get_misc_info(&misc); > > @@ -1605,7 +1607,8 @@ void acpi_build(AcpiBuildTables *tables, > > aml_len += tables_blob->len - fadt; > > > > acpi_add_table(table_offsets, tables_blob); > > - build_madt(tables_blob, tables->linker, machine, conf); > > + acpi_builder_madt(ab, tables_blob, tables->linker, > > + machine, conf); > > > > vmgenid_dev = find_vmgenid_dev(); > > if (vmgenid_dev) { > > Just a quick question before I go and actually apply the patches to look > at the resulting code: is there any reason why you didn't add the > MachineState and/or AcpiBuilder to AcpiBuildState, similar to how it was > in v1? With v1 I was not adding MachineState to AcpiBuildState, I may be missing your point. Do you mean adding an AcpiBuilder pointer to AcpiConfiguration? Cheers, Samuel.
On 30/10/2018 15:13, Samuel Ortiz wrote: >> Just a quick question before I go and actually apply the patches to look >> at the resulting code: is there any reason why you didn't add the >> MachineState and/or AcpiBuilder to AcpiBuildState, similar to how it was >> in v1? > > With v1 I was not adding MachineState to AcpiBuildState, I may be > missing your point. > Do you mean adding an AcpiBuilder pointer to AcpiConfiguration? No, what I was remembering is the FirmwareBuildState, which you have removed according to my review. Sorry, KVM Forum was a bit exhausting. :) I think adding the MachineState or AcpiBuilder to AcpiBuildState instead of using qdev_get_machine() makes sense, but it is an independent cleanup. Paolo
On Tue, Oct 30, 2018 at 05:03:28PM +0100, Paolo Bonzini wrote: > On 30/10/2018 15:13, Samuel Ortiz wrote: > >> Just a quick question before I go and actually apply the patches to look > >> at the resulting code: is there any reason why you didn't add the > >> MachineState and/or AcpiBuilder to AcpiBuildState, similar to how it was > >> in v1? > > > > With v1 I was not adding MachineState to AcpiBuildState, I may be > > missing your point. > > Do you mean adding an AcpiBuilder pointer to AcpiConfiguration? > > No, what I was remembering is the FirmwareBuildState, which you have > removed according to my review. Sorry, KVM Forum was a bit exhausting. :) No worries, it was exhausting indeed :) Cheers, Samuel.
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 5035dac556..d40599e6d1 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -35,6 +35,7 @@ #include "hw/acpi/acpi-defs.h" #include "hw/acpi/acpi.h" #include "hw/acpi/cpu.h" +#include "hw/acpi/builder.h" #include "hw/nvram/fw_cfg.h" #include "hw/acpi/bios-linker-loader.h" #include "hw/loader.h" @@ -1556,6 +1557,7 @@ void acpi_build(AcpiBuildTables *tables, GArray *tables_blob = tables->table_data; AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL }; Object *vmgenid_dev; + AcpiBuilder *ab = ACPI_BUILDER(machine); acpi_get_pm_info(&pm); acpi_get_misc_info(&misc); @@ -1605,7 +1607,8 @@ void acpi_build(AcpiBuildTables *tables, aml_len += tables_blob->len - fadt; acpi_add_table(table_offsets, tables_blob); - build_madt(tables_blob, tables->linker, machine, conf); + acpi_builder_madt(ab, tables_blob, tables->linker, + machine, conf); vmgenid_dev = find_vmgenid_dev(); if (vmgenid_dev) { @@ -1629,15 +1632,17 @@ void acpi_build(AcpiBuildTables *tables, } if (conf->numa_nodes) { acpi_add_table(table_offsets, tables_blob); - build_srat(tables_blob, tables->linker, machine, conf); + + acpi_builder_srat(ab, tables_blob, tables->linker, + machine, conf); if (have_numa_distance) { acpi_add_table(table_offsets, tables_blob); - build_slit(tables_blob, tables->linker); + acpi_builder_slit(ab, tables_blob, tables->linker); } } if (acpi_get_mcfg(&mcfg)) { acpi_add_table(table_offsets, tables_blob); - build_mcfg(tables_blob, tables->linker, &mcfg); + acpi_builder_mcfg(ab, tables_blob, tables->linker, &mcfg); } if (x86_iommu_get_default()) { IommuType IOMMUType = x86_iommu_get_type(); @@ -1668,7 +1673,7 @@ void acpi_build(AcpiBuildTables *tables, slic_oem.id, slic_oem.table_id); /* RSDP is in FSEG memory, so allocate it separately */ - build_rsdp_rsdt(tables->rsdp, tables->linker, rsdt); + acpi_builder_rsdp(ab, tables->rsdp, tables->linker, rsdt); /* We'll expose it all to Guest so we want to reduce * chance of size changes. diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 1dcbbd5139..986ed0eabd 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -64,6 +64,7 @@ #include "qemu/option.h" #include "hw/acpi/acpi.h" #include "hw/acpi/cpu_hotplug.h" +#include "hw/acpi/builder.h" #include "hw/boards.h" #include "acpi-build.h" #include "hw/mem/pc-dimm.h" @@ -75,6 +76,7 @@ #include "hw/nmi.h" #include "hw/i386/intel_iommu.h" #include "hw/net/ne2000-isa.h" +#include "hw/i386/acpi.h" /* debug PC/ISA interrupts */ //#define DEBUG_IRQ @@ -2403,12 +2405,20 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp) } } +static AcpiConfiguration *pc_acpi_configuration(AcpiBuilder *builder) +{ + PCMachineState *pcms = PC_MACHINE(builder); + + return &pcms->acpi_configuration; +} + static void pc_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); PCMachineClass *pcmc = PC_MACHINE_CLASS(oc); HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc); NMIClass *nc = NMI_CLASS(oc); + AcpiBuilderMethods *abm = ACPI_BUILDER_METHODS(oc); pcmc->pci_enabled = true; pcmc->has_acpi_build = true; @@ -2443,6 +2453,14 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) nc->nmi_monitor_handler = x86_nmi; mc->default_cpu_type = TARGET_DEFAULT_CPU_TYPE; + /* ACPI building methods */ + abm->madt = build_madt; + abm->rsdp = build_rsdp_rsdt; + abm->mcfg = build_mcfg; + abm->srat = build_srat; + abm->slit = build_slit; + abm->configuration = pc_acpi_configuration; + object_class_property_add(oc, MEMORY_DEVICE_REGION_SIZE, "int", pc_machine_get_device_memory_region_size, NULL, NULL, NULL, &error_abort); @@ -2494,6 +2512,7 @@ static const TypeInfo pc_machine_info = { .interfaces = (InterfaceInfo[]) { { TYPE_HOTPLUG_HANDLER }, { TYPE_NMI }, + { TYPE_ACPI_BUILDER }, { } }, };
All PC machine type derivatives will use the same ACPI table build methods. But with that change in place, any new x86 machine type will be able to re-use the acpi-build API and customize part of it by defining its own ACPI table build methods. Signed-off-by: Samuel Ortiz <sameo@linux.intel.com> --- hw/i386/acpi-build.c | 15 ++++++++++----- hw/i386/pc.c | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-)