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

["Alex Bennée" <alex.bennee@linaro.org>]

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)

> +        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?

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

-- 
Alex Bennée