Re: [Qemu-devel] [PATCH v4 06/20] hw/arm/virt-acpi-build: Generation of DSDT table for virt devices

[Shannon Zhao <zhaoshenglong@huawei.com>]

On 2015/4/9 17:51, Alex Bennée wrote:
> 
> Shannon Zhao <zhaoshenglong@huawei.com> writes:
> 
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> DSDT consists of the usual common table header plus a definition
>> block in AML encoding which describes all devices in the platform.
>>
>> After initializing DSDT with header information the namespace is
>> created which is followed by the device encodings. The devices are
>> described using the Resource Template for the 32-Bit Fixed Memory
>> Range and the Extended Interrupt Descriptors.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/arm/virt-acpi-build.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 137 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 388838a..516c1d0 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -57,6 +57,139 @@
>>  #define ACPI_BUILD_DPRINTF(fmt, ...)
>>  #endif
>>  
>> +static void acpi_dsdt_add_cpus(Aml *scope, int max_cpus)
>> +{
>> +    Aml *dev, *crs;
>> +    int i;
>> +    char name[5];
> 
> name, dev and crs could be declared inside the for() loop.
> 
>> +    for (i = 0; i < max_cpus; i++) {
>> +        snprintf(name, 5, "CPU%u", i);
> 
> What happens here if you have 10 or 100 CPUs? 
> 
>> +        dev = aml_device("%s", name);
> 
> Also aml_device seems to take a format string so why not simply:
> 
>      dev = aml_device("CPU%u", i)
> 

Ok, it's better.

>> +        aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
>> +        aml_append(dev, aml_name_decl("_UID", aml_int(i)));
>> +        crs = aml_resource_template();
>> +        aml_append(dev, aml_name_decl("_CRS", crs));
>> +        aml_append(scope, dev);
>> +    }
>> +}
>> +
>> +static void acpi_dsdt_add_uart(Aml *scope, const hwaddr *uart_addr,
>> +                                           const int *uart_irq)
>> +{
>> +    Aml *dev, *crs;
>> +
>> +    dev = aml_device("COM0");
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0011")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>> +
>> +    crs = aml_resource_template();
>> +    aml_append(crs,
>> +               aml_memory32_fixed(uart_addr[0], uart_addr[1], 0x01));
>> +    aml_append(crs,
>> +               aml_interrupt(0x01, *uart_irq + 32));
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +    aml_append(scope, dev);
>> +}
>> +
>> +static void acpi_dsdt_add_rtc(Aml *scope, const hwaddr *rtc_addr,
>> +                                          const int *rtc_irq)
>> +{
>> +    Aml *dev, *crs;
>> +
>> +    dev = aml_device("RTC0");
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0013")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>> +
>> +    crs = aml_resource_template();
>> +    aml_append(crs,
>> +               aml_memory32_fixed(rtc_addr[0], rtc_addr[1], 0x01));
>> +    aml_append(crs,
>> +               aml_interrupt(0x01, *rtc_irq + 32));
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +    aml_append(scope, dev);
>> +}
>> +
>> +static void acpi_dsdt_add_flash(Aml *scope, const hwaddr *flash_addr)
>> +{
>> +    Aml *dev, *crs;
>> +    hwaddr base = flash_addr[0];
>> +    hwaddr size = flash_addr[1];
>> +
>> +    dev = aml_device("FLS0");
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0015")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>> +
>> +    crs = aml_resource_template();
>> +    aml_append(crs,
>> +               aml_memory32_fixed(base, size, 0x01));
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +    aml_append(scope, dev);
>> +
>> +    dev = aml_device("FLS1");
>> +    aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0015")));
>> +    aml_append(dev, aml_name_decl("_UID", aml_int(1)));
>> +    crs = aml_resource_template();
>> +    aml_append(crs,
>> +               aml_memory32_fixed(base + size, size, 0x01));
>> +    aml_append(dev, aml_name_decl("_CRS", crs));
>> +    aml_append(scope, dev);
>> +}
>> +
>> +static void acpi_dsdt_add_virtio(Aml *scope, const hwaddr *mmio_addrs,
>> +                                             const int *mmio_irq, int num)
>> +{
>> +    Aml *dev, *crs;
>> +    hwaddr base = mmio_addrs[0];
>> +    hwaddr size = mmio_addrs[1];
> 
> What ensures all these hw addresses are in 32 bit space on 64 bit platforms?
> 

As we're generating the ACPI tables for machine virt, from a15memmap[] we know the hw addresses are below 1GB.

>> +    int irq = *mmio_irq + 32;
>> +    int i;
>> +    char name[5];
>> +
>> +    for (i = 0; i < num; i++) {
>> +        snprintf(name, 5, "VR%02u", i);
>> +        dev = aml_device("%s", name);
> 
> Again why not call aml_device directly?
> 
>> +        aml_append(dev, aml_name_decl("_HID", aml_string("LNRO0005")));
>> +        aml_append(dev, aml_name_decl("_UID", aml_int(i)));
>> +
>> +        crs = aml_resource_template();
>> +        aml_append(crs,
>> +                   aml_memory32_fixed(base, size, 0x01));
>> +        aml_append(crs,
>> +                   aml_interrupt(0x01, irq + i));
>> +        aml_append(dev, aml_name_decl("_CRS", crs));
>> +        aml_append(scope, dev);
>> +        base += size;
>> +    }
>> +}
>> +
>> +/* DSDT */
>> +static void
>> +build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>> +{
>> +    Aml *scope, *dsdt;
>> +    acpi_dsdt_info *info = guest_info->dsdt_info;
>> +
>> +    dsdt = init_aml_allocator();
>> +    /* Reserve space for header */
>> +    acpi_data_push(dsdt->buf, sizeof(AcpiTableHeader));
>> +
>> +    scope = aml_scope("\\_SB");
>> +    acpi_dsdt_add_cpus(scope, guest_info->max_cpus);
>> +    acpi_dsdt_add_uart(scope, info->uart_addr, info->uart_irq);
>> +    acpi_dsdt_add_rtc(scope, info->rtc_addr, info->rtc_irq);
>> +    acpi_dsdt_add_flash(scope, info->flash_addr);
>> +    acpi_dsdt_add_virtio(scope, info->virtio_mmio_addr,
>> +             info->virtio_mmio_irq, info->virtio_mmio_num);
>> +
>> +    aml_append(dsdt, scope);
>> +    /* copy AML table into ACPI tables blob and patch header there */
>> +    g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
>> +    build_header(linker, table_data,
>> +        (void *)(table_data->data + table_data->len - dsdt->buf->len),
>> +        "DSDT", dsdt->buf->len, 1);
>> +    free_aml_allocator();
>> +}
>> +
>>  typedef
>>  struct AcpiBuildState {
>>      /* Copy of table in RAM (for patching). */
>> @@ -72,6 +205,7 @@ static
>>  void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>>  {
>>      GArray *table_offsets;
>> +    GArray *tables_blob = tables->table_data;
>>  
>>      table_offsets = g_array_new(false, true /* clear */,
>>                                          sizeof(uint32_t));
>> @@ -89,6 +223,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>>       * DSDT
>>       */
>>  
>> +    /* DSDT is pointed to by FADT */
>> +    build_dsdt(tables_blob, tables->linker, guest_info);
>> +
>>      /* Cleanup memory that's no longer used. */
>>      g_array_free(table_offsets, true);
>>  }
>