Re: [Qemu-devel] [PATCH v3 19/19] hw: i386: Implement the ACPI builder interface for PC

2018-10-30 Thread Samuel Ortiz
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.



Re: [Qemu-devel] [PATCH v3 19/19] hw: i386: Implement the ACPI builder interface for PC

2018-10-30 Thread Paolo Bonzini
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



Re: [Qemu-devel] [PATCH v3 19/19] hw: i386: Implement the ACPI builder interface for PC

2018-10-30 Thread Samuel Ortiz
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();
> >  acpi_get_misc_info();
> > @@ -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.



Re: [Qemu-devel] [PATCH v3 19/19] hw: i386: Implement the ACPI builder interface for PC

2018-10-29 Thread Philippe Mathieu-Daudé

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 
---
  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();

  acpi_get_misc_info();
@@ -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()) {
  acpi_add_table(table_offsets, tables_blob);
-build_mcfg(tables_blob, tables->linker, );
+acpi_builder_mcfg(ab, tables_blob, tables->linker, );
  }
  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 >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é 
Tested-by: Philippe Mathieu-Daudé 


+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, _abort);
@@ -2494,6 +2512,7 @@ static const TypeInfo pc_machine_info = {
  .interfaces = (InterfaceInfo[]) {
   { TYPE_HOTPLUG_HANDLER },
   { TYPE_NMI },
+ { TYPE_ACPI_BUILDER },
   { }
  },
  };





Re: [Qemu-devel] [PATCH v3 19/19] hw: i386: Implement the ACPI builder interface for PC

2018-10-29 Thread Paolo Bonzini
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();
>  acpi_get_misc_info();
> @@ -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