Re: [Qemu-devel] [PATCH v6 22/22] hw/arm/virt: Enable dynamic generation of ACPI v5.1 tables

[Peter Maydell]

On 7 May 2015 at 10:29, Shannon Zhao <address@hidden> wrote:
> From: Shannon Zhao <address@hidden>
>
> Expose the needed device information to the table generation
> insfrastructure and register a machine_init_done notify to

"infrastructure".

> call virt_acpi_build().
>
> Add CONFIG_ACPI to arm-softmmu.mak.
>
> Signed-off-by: Shannon Zhao <address@hidden>
> Signed-off-by: Shannon Zhao <address@hidden>
> ---
>  default-configs/arm-softmmu.mak      |  1 +
>  default-configs/i386-softmmu.mak     |  3 ++
>  default-configs/mips-softmmu.mak     |  3 ++
>  default-configs/mips64-softmmu.mak   |  3 ++
>  default-configs/mips64el-softmmu.mak |  3 ++
>  default-configs/mipsel-softmmu.mak   |  3 ++
>  default-configs/x86_64-softmmu.mak   |  3 ++
>  hw/acpi/Makefile.objs                |  5 ++-
>  hw/arm/virt.c                        | 78 
> ++++++++++++++++++++++++++++++++----
>  hw/i2c/Makefile.objs                 |  2 +-
>  10 files changed, 94 insertions(+), 10 deletions(-)
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index a767e4b..74f1db3 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -101,3 +101,4 @@ CONFIG_ALLWINNER_A10=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_ACPI=y
> diff --git a/default-configs/i386-softmmu.mak 
> b/default-configs/i386-softmmu.mak
> index 6a74e00..d2de500 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -15,6 +15,9 @@ CONFIG_PCSPK=y
>  CONFIG_PCKBD=y
>  CONFIG_FDC=y
>  CONFIG_ACPI=y
> +CONFIG_ACPI_CORE=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_CPU_HOTPLUG=y

This is splitting the basic CONFIG_ACPI into several pieces, right?
I think that deserves its own patch.

What's the difference now between "CONFIG_ACPI" and "CONFIG_ACPI_CORE" ?

>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_IDE_ISA=y
> diff --git a/default-configs/mips-softmmu.mak 
> b/default-configs/mips-softmmu.mak
> index cce2c81..c96d42d 100644
> --- a/default-configs/mips-softmmu.mak
> +++ b/default-configs/mips-softmmu.mak
> @@ -15,6 +15,9 @@ CONFIG_PCSPK=y
>  CONFIG_PCKBD=y
>  CONFIG_FDC=y
>  CONFIG_ACPI=y
> +CONFIG_ACPI_CORE=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_PIIX4=y
> diff --git a/default-configs/mips64-softmmu.mak 
> b/default-configs/mips64-softmmu.mak
> index 7a88a08..d229f9e 100644
> --- a/default-configs/mips64-softmmu.mak
> +++ b/default-configs/mips64-softmmu.mak
> @@ -15,6 +15,9 @@ CONFIG_PCSPK=y
>  CONFIG_PCKBD=y
>  CONFIG_FDC=y
>  CONFIG_ACPI=y
> +CONFIG_ACPI_CORE=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_PIIX4=y
> diff --git a/default-configs/mips64el-softmmu.mak 
> b/default-configs/mips64el-softmmu.mak
> index 095de43..ea31b8b 100644
> --- a/default-configs/mips64el-softmmu.mak
> +++ b/default-configs/mips64el-softmmu.mak
> @@ -15,6 +15,9 @@ CONFIG_PCSPK=y
>  CONFIG_PCKBD=y
>  CONFIG_FDC=y
>  CONFIG_ACPI=y
> +CONFIG_ACPI_CORE=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_PIIX4=y
> diff --git a/default-configs/mipsel-softmmu.mak 
> b/default-configs/mipsel-softmmu.mak
> index 0e25108..9a4462e 100644
> --- a/default-configs/mipsel-softmmu.mak
> +++ b/default-configs/mipsel-softmmu.mak
> @@ -15,6 +15,9 @@ CONFIG_PCSPK=y
>  CONFIG_PCKBD=y
>  CONFIG_FDC=y
>  CONFIG_ACPI=y
> +CONFIG_ACPI_CORE=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_PIIX4=y
> diff --git a/default-configs/x86_64-softmmu.mak 
> b/default-configs/x86_64-softmmu.mak
> index 46b87dd..11019b6 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -15,6 +15,9 @@ CONFIG_PCSPK=y
>  CONFIG_PCKBD=y
>  CONFIG_FDC=y
>  CONFIG_ACPI=y
> +CONFIG_ACPI_CORE=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_CPU_HOTPLUG=y
>  CONFIG_APM=y
>  CONFIG_I8257=y
>  CONFIG_IDE_ISA=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index b9fefa7..511771a 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -1,5 +1,6 @@
> -common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> -common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> +common-obj-$(CONFIG_ACPI_CORE) += core.o piix4.o ich9.o pcihp.o

Why is x86 specific stuff like piix4 in the CONFIG_ACPI_CORE set?

> +common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> +common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>  common-obj-$(CONFIG_ACPI) += acpi_interface.o
>  common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
>  common-obj-$(CONFIG_ACPI) += aml-build.o
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 565f573..9291045 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -43,6 +43,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/error-report.h"
>  #include "hw/pci-host/gpex.h"
> +#include "hw/arm/virt-acpi-build.h"
>
>  #define NUM_VIRTIO_TRANSPORTS 32
>
> @@ -60,6 +61,11 @@
>  #define GIC_FDT_IRQ_PPI_CPU_START 8
>  #define GIC_FDT_IRQ_PPI_CPU_WIDTH 8
>
> +#define ARCH_TIMER_VIRT_IRQ   11
> +#define ARCH_TIMER_S_EL1_IRQ  13
> +#define ARCH_TIMER_NS_EL1_IRQ 14
> +#define ARCH_TIMER_NS_EL2_IRQ 10
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> @@ -149,6 +155,29 @@ static const int a15irqmap[] = {
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>  };
>
> +static AcpiMadtInfo madt_info = {
> +    (MemMap *)&a15memmap[VIRT_GIC_CPU],
> +    (MemMap *)&a15memmap[VIRT_GIC_DIST]

These casts are really bad. You should just make sure the
types agree properly so we don't need a cast at all, rather
than having two different types which we implicitly require
to have identical structure.

> +};
> +
> +static AcpiDsdtInfo dsdt_info = {

"AcpiDsdtInfo" sounds like it ought to be a generic
ACPI structure, but in fact it's been defined in
include/hw/arm/virt-acpi-build.h. Is it actually
generic but in the wrong place? Or if it's not
generic, why can't the ACPI table building code use
our existing MemMapEntry[] and irqmap[] arrays to
get the information it wants, rather than having
its own data structures that we have to copy everything
across to? If there's missing info or unhelpful layout
in the current virt data structures we can always
improve them.

> +    (MemMap *)&a15memmap[VIRT_UART],
> +    .uart_irq = &a15irqmap[VIRT_UART],

Please don't mix named-initializer and non-named-initializer
syntax like this.

> +    (MemMap *)&a15memmap[VIRT_MMIO],
> +    .virtio_mmio_irq = &a15irqmap[VIRT_MMIO],
> +    .virtio_mmio_num = NUM_VIRTIO_TRANSPORTS,
> +    (MemMap *)&a15memmap[VIRT_RTC],
> +    .rtc_irq = &a15irqmap[VIRT_RTC],
> +    (MemMap *)&a15memmap[VIRT_FLASH],
> +};

thanks
-- PMM