[v9,09/24] hw/arm/virt-acpi-build: Generate MADT table

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         | 57 ++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h      | 38 ++++++++++++++++++++++++++-
 include/hw/arm/virt-acpi-build.h |  3 +++
 3 files changed, 97 insertions(+), 1 deletion(-)

On 2015/5/27 17:34, Igor Mammedov wrote:
> On Wed, 27 May 2015 14:18:44 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
>
>>
>>
>> On 2015/5/27 0:00, Igor Mammedov wrote:
>>> On Mon, 25 May 2015 10:55:05 +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         | 57 ++++++++++++++++++++++++++++++++++++++++
>>>>   include/hw/acpi/acpi-defs.h      | 38 ++++++++++++++++++++++++++-
>>>>   include/hw/arm/virt-acpi-build.h |  3 +++
>>>>   3 files changed, 97 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index 0791501..29ad535 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -42,6 +42,20 @@
>>>>
>>>>   #define ARM_SPI_BASE 32
>>>>
>>>> +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 smp_cpus)
>>>>   {
>>>>       uint16_t i;
>>>> @@ -137,6 +151,43 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>>>>       }
>>>>   }
>>>>
>>>> +/* MADT */
>>>> +static void
>>>> +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info,
>>>> +           VirtAcpiCpuInfo *cpuinfo)
>>>> +{
>>>> +    int madt_start = table_data->len;
>>>> +    const MemMapEntry *memmap = guest_info->memmap;
>>>> +    AcpiMultipleApicTable *madt;
>>>> +    AcpiMadtGenericDistributor *gicd;
>>>> +    int i;
>>>> +
>>>> +    madt = acpi_data_push(table_data, sizeof *madt);
>>>> +
>>>> +    for (i = 0; i < guest_info->smp_cpus; i++) {
>>>> +        AcpiMadtGenericInterrupt *gicc = acpi_data_push(table_data,
>>>> +                                                     sizeof *gicc);
>>>> +        gicc->type = ACPI_APIC_GENERIC_INTERRUPT;
>>>> +        gicc->length = sizeof(*gicc);
>>>> +        gicc->base_address = memmap[VIRT_GIC_CPU].base;
>>>> +        gicc->cpu_interface_number = i;
>>>> +        gicc->arm_mpidr = i;
>>> this value is CPU type dependent, It would be more robust to
>>> have CPU specific callback to return aff[0-3] values
>>> and pack it here as described by spec.
>>>
>>> Also virt board uses a57 and according to ARM spec MPIDR register
>>> bits 2:7 are reserved and with smp_cpus > 4 value wouldn't follow
>>> spec using reserved bits 2:7.
>>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0488g/way1382037549036.html
>>>
>>> Also looking at KVM, it has its own  notion on how to encode MPIDR
>>> packing flat vcpu_id (which is cpu_index) into affinity fields so it
>>> won't match MPIDR in QEMU.
>>> Perhaps we should use MPIDR as vcpu_id so that KVM wouldn't have to
>>> reinvent it and just use QEMU supplied value or pack MPIDR into vcpu_id
>>> the way KVM expects it.
>>>
>>
>> KVM limits the number of VCPUs in aff0 to 16 while a57 limits aff0 to 4
>> according to the spec.
>> On the condition that host uses a57 CPU and start a VM with >4 cpus,
>> according to a57 spec KVM will use the reserve bits aff0[2:7] and this
>> is not right. But why does KVM work well? While the ARMv8 spec does not
>> reserve bits of aff0[2:7], I guess kernel doesn't mask bit[2:7] since it
>> must be compatible for all v8 CPUs.
>>
>> Since virt supports 8 vcpus now, so the MPIDR value of QEMU and KVM are
>> same, both are cpu_index. So I think we can stay current way. When the
>> GICv3 patchset goes into QEMU and virt supports more vcpus, we can use
>> the same way to encode MPIDR as KVM in QEMU.
> It's bound to break once CPU count goes over 16 and it's better to adhere
> CPU spec in regards to mipdr while we don't have a legacy stuff to support yet.
> Yes, it's CPU type depended but that's one more reason to let QEMU deal
> with its encoding and force KVM to use QEMU supplied mpidr rather than
> reinventing its own interpretation of it.
> In addition if we look at base a53 spec where aff0 is [0:7] bits wide,
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500f/BEIEGIGI.html
> it says that valid values for aff0 is 0-3, i.e. actually using only [0:1] bits
>

And from Shlomo's patchset [1] which adds GICv3 support in QEMU I know 
that GIC-500 supports 8 cpus per cluster and he encodes mpidr like below:

+    aff0 = cpuid % 8;
+    aff1 = cpuid / 8;
+    mpidr = (aff1 << 8) | aff0;

[1] [PATCH RFC V2 0/4] Implement GIC-500 from GICv3 family for arm64

So there are so many different ways to encode mpidr and every one has 
its reason. KVM does that as GICv3 can be able to address each CPU 
directly when sending IPIs, Shlomo refers to the GIC-500 spec and here 
we want to be consistent with A57 and A53 spec.

> All that said its not the reason to block this series over mpidr
> re-factoring,
Yes, it should not block this series.

> but it would be better to use here CPU provided affX values
> and encode them as per ACPI spec, then we won't have to redo this part
> of code in the future.
>
When adding GICv3 support, we have to touch these code in the future. At 
that moment or after this series, we could discuss how to encode mpidr 
both in QEMU and KVM.

> As for UID value we can pack all aff[0-3] values inside 32 UID since
> each is 8 bits wide max.
>

Will aff3 be used? If it's used, there are at least 4*256*256*1 VCPUs.

>>
>>>> +        gicc->uid = i;
>>> it looks like it's upto vendor how to encode topology in affX, so if
>>> we could avoid using aff3, i.e. limit us only to 32bit MPIDR register
>>> I'd would rather use it for this field as well and fix DSDT to use it
>>> also.
>>>
>>>> +        if (test_bit(i, cpuinfo->found_cpus)) {
>>>> +            gicc->flags = cpu_to_le32(ACPI_GICC_ENABLED);
>>>> +        }
>>> smp_cpus currently represents present cpus number so you
>>> can unconditionally set ACPI_GICC_ENABLED flag.
>>>
>>>> +    }
>>>> +
>>>> +    gicd = acpi_data_push(table_data, sizeof *gicd);
>>>> +    gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
>>>> +    gicd->length = sizeof(*gicd);
>>>> +    gicd->base_address = memmap[VIRT_GIC_DIST].base;
>>>> +
>>>> +    build_header(linker, table_data,
>>>> +                 (void *)(table_data->data + madt_start), "APIC",
>>>> +                 table_data->len - madt_start, 5);
>>>> +}
>>>> +
>>>>   /* FADT */
>>>>   static void
>>>>   build_fadt(GArray *table_data, GArray *linker, unsigned dsdt)
>>>> @@ -209,8 +260,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));
>>>>
>>>> @@ -235,6 +289,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 fadcf84..1e9dbe7 100644
>>>> --- a/include/hw/acpi/acpi-defs.h
>>>> +++ b/include/hw/acpi/acpi-defs.h
>>>> @@ -256,7 +256,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)
>>>> @@ -304,6 +310,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 ff00121..04f174d 100644
>>>> --- a/include/hw/arm/virt-acpi-build.h
>>>> +++ b/include/hw/arm/virt-acpi-build.h
>>>> @@ -23,6 +23,9 @@
>>>>   #include "qemu-common.h"
>>>>   #include "hw/arm/virt.h"
>>>>
>>>> +#define VIRT_ACPI_CPU_ID_LIMIT 8
>>>> +#define ACPI_GICC_ENABLED 1
>>>> +
>>>>   typedef struct VirtGuestInfo {
>>>>       int smp_cpus;
>>>>       FWCfgState *fw_cfg;
>>>
>>>
>>> .
>>>
>>
>