Re: [Qemu-devel] [PATCH v5 08/20] hw/arm/virt-acpi-build: Generate MADT table

[Shannon Zhao <shannon.zhao@linaro.org>]

On 2015/5/4 18:21, Igor Mammedov wrote:
> On Wed, 15 Apr 2015 21:24:57 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> 
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> MADT describes GIC enabled ARM platforms. The GICC and GICD
>> subtables are used to define the GIC regions.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  hw/arm/virt-acpi-build.c         | 61 ++++++++++++++++++++++++++++++++++++++++
>>  include/hw/acpi/acpi-defs.h      | 38 ++++++++++++++++++++++++-
>>  include/hw/arm/virt-acpi-build.h |  2 ++
>>  3 files changed, 100 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index c72a9c8..94cced0 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -50,6 +50,20 @@
>>  #include "qom/qom-qobject.h"
>>  #include "exec/ram_addr.h"
>>  
>> +typedef struct VirtAcpiCpuInfo {
>> +    DECLARE_BITMAP(found_cpus, VIRT_ACPI_CPU_ID_LIMIT);
>> +} VirtAcpiCpuInfo;
>> +
>> +static void virt_acpi_get_cpu_info(VirtAcpiCpuInfo *cpuinfo)
>> +{
>> +    CPUState *cpu;
>> +
>> +    memset(cpuinfo->found_cpus, 0, sizeof cpuinfo->found_cpus);
>> +    CPU_FOREACH(cpu) {
>> +        set_bit(cpu->cpu_index, cpuinfo->found_cpus);
>> +    }
>> +}
>> +
>>  static void acpi_dsdt_add_cpus(Aml *scope, int max_cpus)
>>  {
>>      uint16_t i;
>> @@ -146,6 +160,47 @@ static void acpi_dsdt_add_virtio(Aml *scope, const MemMap *virtio_mmio_memmap,
>>      }
>>  }
>>  
>> +/* MADT */
>> +static void
>> +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>> +           VirtAcpiCpuInfo *cpuinfo)
>> +{
>> +    int madt_start = table_data->len;
>> +    const struct AcpiMadtInfo *info = guest_info->madt_info;
>> +    AcpiMultipleApicTable *madt;
>> +    AcpiMadtGenericDistributor *gicd;
>> +    int i;
>> +
>> +    madt = acpi_data_push(table_data, sizeof *madt);
>> +    madt->local_apic_address = info->gic_cpu_memmap->addr;
> should be safe to drop this since OSPM must ignore this field if
> gicc provides base_address.
> 

Ok.
>> +    madt->flags = cpu_to_le32(1);
> is it correct?
> Looking at 5.1 spec it's x86 specific PCAT_COMPAT flag,
> why do you use it for ARM?
> 

Oh, will fix. Thanks.

>> +
>> +    for (i = 0; i < guest_info->max_cpus; i++) {
>> +        AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
>> +                                                     sizeof *gicc);
>> +        gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
>> +        gicc->length = sizeof(*gicc);
>> +        gicc->base_address = info->gic_cpu_memmap->addr;
>> +        gicc->cpu_interface_number = i;
>> +        gicc->arm_mpidr = i;
>> +        gicc->uid = i;
>> +        if (test_bit(i, cpuinfo->found_cpus)) {
>> +            gicc->flags = cpu_to_le32(1);
> #define ACPI_GICC_ENABLED 1
> 
>> +        } else {
>> +            gicc->flags = cpu_to_le32(0);
>> +        }
> not necessary else clause, field is zeroed by default.
> 

Ok, will remove.

>> +    }
>> +
>> +    gicd = acpi_data_push(table_data, sizeof *gicd);
>> +    gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
>> +    gicd->length = sizeof(*gicd);
>> +    gicd->base_address = info->gic_dist_memmap->addr;
>> +
>> +    build_header(linker, table_data,
>> +                 (void *)(table_data->data + madt_start), "APIC",
>> +                 table_data->len - madt_start, 1);
> wrong revision?
> 

Thanks, will fix this including other patches.

>> +}
>> +
>>  /* FADT */
>>  static void
>>  build_fadt(GArray *table_data, GArray *linker, unsigned dsdt)
>> @@ -215,8 +270,11 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>>  {
>>      GArray *table_offsets;
>>      unsigned dsdt;
>> +    VirtAcpiCpuInfo cpuinfo;
>>      GArray *tables_blob = tables->table_data;
>>  
>> +    virt_acpi_get_cpu_info(&cpuinfo);
>> +
>>      table_offsets = g_array_new(false, true /* clear */,
>>                                          sizeof(uint32_t));
>>  
>> @@ -241,6 +299,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>>      acpi_add_table(table_offsets, tables_blob);
>>      build_fadt(tables_blob, tables->linker, dsdt);
>>  
>> +    acpi_add_table(table_offsets, tables_blob);
>> +    build_madt(tables_blob, tables->linker, guest_info, &cpuinfo);
>> +
>>      /* Cleanup memory that's no longer used. */
>>      g_array_free(table_offsets, true);
>>  }
>> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
>> index bac981d..4092dc3 100644
>> --- a/include/hw/acpi/acpi-defs.h
>> +++ b/include/hw/acpi/acpi-defs.h
>> @@ -236,7 +236,13 @@ typedef struct AcpiMultipleApicTable AcpiMultipleApicTable;
>>  #define ACPI_APIC_IO_SAPIC           6
>>  #define ACPI_APIC_LOCAL_SAPIC        7
>>  #define ACPI_APIC_XRUPT_SOURCE       8
>> -#define ACPI_APIC_RESERVED           9           /* 9 and greater are reserved */
>> +#define ACPI_APIC_LOCAL_X2APIC       9
>> +#define ACPI_APIC_LOCAL_X2APIC_NMI      10
>> +#define ACPI_APIC_GENERIC_INTERRUPT     11
>> +#define ACPI_APIC_GENERIC_DISTRIBUTOR   12
>> +#define ACPI_APIC_GENERIC_MSI_FRAME     13
>> +#define ACPI_APIC_GENERIC_REDISTRIBUTOR 14
>> +#define ACPI_APIC_RESERVED              15   /* 15 and greater are reserved */
>>  
>>  /*
>>   * MADT sub-structures (Follow MULTIPLE_APIC_DESCRIPTION_TABLE)
>> @@ -284,6 +290,36 @@ struct AcpiMadtLocalNmi {
>>  } QEMU_PACKED;
>>  typedef struct AcpiMadtLocalNmi AcpiMadtLocalNmi;
>>  
>> +struct AcpiMadtGenericInterrupt {
>> +    ACPI_SUB_HEADER_DEF
>> +    uint16_t reserved;
>> +    uint32_t cpu_interface_number;
>> +    uint32_t uid;
>> +    uint32_t flags;
>> +    uint32_t parking_version;
>> +    uint32_t performance_interrupt;
>> +    uint64_t parked_address;
>> +    uint64_t base_address;
>> +    uint64_t gicv_base_address;
>> +    uint64_t gich_base_address;
>> +    uint32_t vgic_interrupt;
>> +    uint64_t gicr_base_address;
>> +    uint64_t arm_mpidr;
>> +} QEMU_PACKED;
>> +
>> +typedef struct AcpiMadtGenericInterrupt AcpiMadtGenericInterrupt;
>> +
>> +struct AcpiMadtGenericDistributor {
>> +    ACPI_SUB_HEADER_DEF
>> +    uint16_t reserved;
>> +    uint32_t gic_id;
>> +    uint64_t base_address;
>> +    uint32_t global_irq_base;
>> +    uint32_t reserved2;
>> +} QEMU_PACKED;
>> +
>> +typedef struct AcpiMadtGenericDistributor AcpiMadtGenericDistributor;
>> +
>>  /*
>>   * HPET Description Table
>>   */
>> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
>> index ece67a2..8f0b4a7 100644
>> --- a/include/hw/arm/virt-acpi-build.h
>> +++ b/include/hw/arm/virt-acpi-build.h
>> @@ -22,6 +22,8 @@
>>  
>>  #include "qemu-common.h"
>>  
>> +#define VIRT_ACPI_CPU_ID_LIMIT 8
>> +
>>  typedef struct MemMap {
>>      hwaddr addr;
>>      hwaddr size;
>