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

[Shannon Zhao]

On 2015/5/8 0:09, Peter Maydell wrote:
> 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.
> 

Ok, will split this patch.

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

I didn't find a proper name. Maybe we should name it "CONFIG_ACPI_x86"
for "core.o piix4.o ich9.o pcihp.o" as these files are for x86.

>>  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.
> 

Ok, will fix this.

>> +};
>> +
>> +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, 

It's not generic. This structure is used to get the virt borad info.

> 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.
> 

Ok, will reuse MemMapEntry[] and irqmap[].

>> +    (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
> 
> 
> .
> 

-- 
Shannon