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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 09 Apr 2015 10:51:33 +0100
Alex Bennée <alex.bennee@linaro.org> 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, ...)
I wonder if all this DPRINTF crap should be replaced by tracepoints.
Oo at least it shouldn't be used in new code.

> >  #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)
On top of that, ACPI name is limited to 4 characters so
CPU%u will allow to specify only 10 cpus, to support more
shrink CPU part and do something like:

Scope(CPUS) {
	Device(C000) {}
        ...
	Device(CFFF) {}
}

that would give us upto max 4096 CPU,
I have on TODO list to convert bitmap based x86 cpu hotplug to
memory hotplug based interface so that we could easily
switch to more CPUs when KVM starts support it.
And for ARM there is no point to have/maintain 2 interfaces
as we would have to do on x86.

> 
> > +        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));
uart_addr[0], uart_addr[1] doesn't tell anything about what they are.
and doing array of it when it's not dynamic sized is confusing not to
mention passing addresses and sizes in it.
The same goes for the rest of foo_addr[] used in this patch/series,
if it's _addr then it shouldn't contain sizes.

how about:
 acpi_dsdt_add_uart(scope, uint32_t uart_addr, uint32_t uart_size, ...)

 
> > +    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));
join it with line above it if it doesn't exceed 80 chr
same goes for other places.

> > +    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);
> > +
and drop this newline

> > +    aml_append(dsdt, scope);
a newline here pls

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