Re: [Qemu-devel] [PATCH 8/9] pc: ACPI: unify source of CPU hotplug IO base/len

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Dec 28, 2013 at 11:30:51PM +0100, Igor Mammedov wrote:
> use C headers defines as source of IO base/len for respective
> values in ASL code.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

That's cool.

It's a bit confusing that you add macro in one patch, then
drop it in another.
Could you reorder patches?
Add define and use it in host first, then use it directly.

It's not critical, nice to have.

> ---
>  hw/acpi/ich9.c                    | 4 +---
>  hw/acpi/piix4.c                   | 5 ++---
>  hw/i386/Makefile.objs             | 9 +++++++--
>  hw/i386/acpi-dsdt-cpu-hotplug.dsl | 7 ++++---
>  hw/i386/acpi-dsdt.dsl             | 3 ++-
>  hw/i386/q35-acpi-dsdt.dsl         | 3 ++-
>  include/hw/acpi/cpu_hotplug.h     | 6 +++++-
>  include/hw/acpi/ich9.h            | 5 +++++
>  include/hw/acpi/piix4.h           | 5 +++++
>  9 files changed, 33 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 03dee35..0afac42 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -44,8 +44,6 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
>  #define ICH9_DEBUG(fmt, ...)    do { } while (0)
>  #endif
>  
> -#define ICH9_PROC_BASE 0x0CD8
> -
>  static void ich9_pm_update_sci_fn(ACPIREGS *regs)
>  {
>      ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
> @@ -223,7 +221,7 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>  
>      AcpiCpuHotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> -                        &pm->gpe_cpu, ICH9_PROC_BASE);
> +                        &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>      pm->cpu_added_notifier.notify = ich9_cpu_added_req;
>      qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
>  }
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 9b7fe6b..4161d06 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -51,8 +51,6 @@
>  #define PCI_EJ_BASE 0xae08
>  #define PCI_RMV_BASE 0xae0c
>  
> -#define PIIX4_PROC_BASE 0xaf00
> -
>  #define PIIX4_PCI_HOTPLUG_STATUS 2
>  
>  struct pci_status {
> @@ -702,7 +700,8 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>          pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
>      }
>  
> -    AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu, PIIX4_PROC_BASE);
> +    AcpiCpuHotplug_init(parent, OBJECT(s), &s->gpe_cpu,
> +                        PIIX4_CPU_HOTPLUG_IO_BASE);
>      s->cpu_added_notifier.notify = piix4_cpu_added_req;
>      qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
>  }
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index edf5256..e9c49d5 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -19,10 +19,15 @@ ifdef IASL
>  DSL_DEPS = $(SRC_PATH)/hw/i386/%.dsl \
>  	$(SRC_PATH)/scripts/acpi_extract_preprocess.py \
>  	$(SRC_PATH)/scripts/acpi_extract.py \
> -	$(SRC_PATH)/hw/i386/acpi-dsdt-*.dsl
> +	$(SRC_PATH)/hw/i386/acpi-dsdt-*.dsl \
> +	$(SRC_PATH)/include/hw/acpi/ich9.h \
> +	$(SRC_PATH)/include/hw/acpi/piix4.h \
> +	$(SRC_PATH)/include/hw/acpi/cpu_hotplug.h
> +
> +ASL_FLAGS = $(QEMU_CFLAGS) -DACPI_ASL
>  
>  hw/i386/%.hex: $(DSL_DEPS)
> -	$(call quiet-command, cpp -P $< -o $*.dsl.i.orig, "  CPP $(TARGET_DIR)$*.dsl.i.orig")
> +	$(call quiet-command, cpp $(ASL_FLAGS) -P $< -o $*.dsl.i.orig, "  CPP $(TARGET_DIR)$*.dsl.i.orig")
>  	$(call quiet-command, $(PYTHON) $(SRC_PATH)/scripts/acpi_extract_preprocess.py $*.dsl.i.orig > $*.dsl.i, "  ACPI_PREPROCESS $(TARGET_DIR)$*.dsl.i")
>  	$(call quiet-command, $(IASL) $(call iasl-option,$(IASL),-Pn,) -vs -l -tc -p $* $*.dsl.i $(if $(V), , > /dev/null) 2>&1 ,"  IASL $(TARGET_DIR)$*.dsl.i")
>  	$(call quiet-command, $(PYTHON) $(SRC_PATH)/scripts/acpi_extract.py $*.lst > $*.off, "  ACPI_EXTRACT $(TARGET_DIR)$*.off")
> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> index 869aa00..942b119 100644
> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> @@ -17,6 +17,8 @@
>   * CPU hotplug
>   ****************************************************************/
>  
> +#include "hw/acpi/cpu_hotplug.h"
> +
>  Scope(\_SB) {
>      /* Objects filled in by run-time generated SSDT */
>      External(NTFY, MethodObj)
> @@ -52,8 +54,7 @@ Scope(\_SB) {
>          Sleep(200)
>      }
>  
> -#define CPU_STATUS_LEN 32
> -    OperationRegion(PRST, SystemIO, CPU_STATUS_BASE, CPU_STATUS_LEN)
> +    OperationRegion(PRST, SystemIO, CPU_STATUS_BASE, ACPI_GPE_PROC_LEN)
>      Field(PRST, ByteAcc, NoLock, Preserve) {
>          PRS, 256
>      }
> @@ -95,7 +96,7 @@ Scope(\_SB) {
>          Name(_HID, "ACPI0004")
>  
>          Name(_CRS, ResourceTemplate() {
> -            IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
> +            IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, ACPI_GPE_PROC_LEN)
>          })
>      }
>  }
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index f00b6ca..d98b019 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -326,7 +326,8 @@ DefinitionBlock (
>          }
>      }
>  
> -#define CPU_STATUS_BASE 0xaf00
> +#include "hw/acpi/piix4.h"
> +#define CPU_STATUS_BASE PIIX4_CPU_HOTPLUG_IO_BASE
>  #include "acpi-dsdt-cpu-hotplug.dsl"
>  
>  
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 9466714..ae74427 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -420,7 +420,8 @@ DefinitionBlock (
>          define_gsi_link(GSIH, 0, 0x17)
>      }
>  
> -#define CPU_STATUS_BASE 0x0CD8
> +#include "hw/acpi/ich9.h"
> +#define CPU_STATUS_BASE ICH9_CPU_HOTPLUG_IO_BASE
>  #include "acpi-dsdt-cpu-hotplug.dsl"
>  
>  
> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> index a0a9abc..00c0866 100644
> --- a/include/hw/acpi/cpu_hotplug.h
> +++ b/include/hw/acpi/cpu_hotplug.h
> @@ -12,11 +12,14 @@
>  #ifndef ACPI_HOTPLUG_H
>  #define ACPI_HOTPLUG_H
>  
> +#define ACPI_GPE_PROC_LEN 32
> +
> +#ifndef ACPI_ASL

I think you use this to split the header out - things for IASL
and things not for IASL.
Let's not do pre-processor tricks.  Just create a new header for IASL only.
cpu_hotplug_defs.h or something.

Put a comment there that only defines are allowed there.

> +
>  #include "hw/acpi/acpi.h"
>  
>  #define ACPI_CPU_HOTPLUG_STATUS 4
>  
> -#define ACPI_GPE_PROC_LEN 32
>  
>  typedef struct AcpiCpuHotplug {
>      MemoryRegion io;
> @@ -27,4 +30,5 @@ void AcpiCpuHotplug_add(ACPIGPE *gpe, AcpiCpuHotplug *g, CPUState *cpu);
>  
>  void AcpiCpuHotplug_init(MemoryRegion *parent, Object *owner,
>                           AcpiCpuHotplug *gpe_cpu, uint16_t base);
> +#endif /* !ACPI_ASL */
>  #endif
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 104f419..47baa92 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -21,6 +21,10 @@
>  #ifndef HW_ACPI_ICH9_H
>  #define HW_ACPI_ICH9_H
>  
> +#define ICH9_CPU_HOTPLUG_IO_BASE 0x0CD8
> +
> +#ifndef ACPI_ASL
> +
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu_hotplug.h"
>  
> @@ -55,4 +59,5 @@ extern const VMStateDescription vmstate_ich9_pm;
>  
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
>  
> +#endif /* !ACPI_ASL */
>  #endif /* HW_ACPI_ICH9_H */
> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
> index 65e6fd7..8606375 100644
> --- a/include/hw/acpi/piix4.h
> +++ b/include/hw/acpi/piix4.h
> @@ -1,8 +1,13 @@
>  #ifndef HW_ACPI_PIIX4_H
>  #define HW_ACPI_PIIX4_H
>  
> +#define PIIX4_CPU_HOTPLUG_IO_BASE 0xaf00
> +
> +#ifndef ACPI_ASL
> +
>  #include "qemu/typedefs.h"
>  
>  Object *piix4_pm_find(void);
>  
> +#endif /* !ACPI_ASL */
>  #endif
> -- 
> 1.8.4.2