[v3,2/2] hw/arm/virt-acpi-build: Add DBG2 table

Add a DBG2 table, describing the pl011 UART.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 hw/arm/virt-acpi-build.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

On Tue, Sep 15, 2015 at 03:44:41PM +0100, Leif Lindholm wrote:
> Add a DBG2 table, describing the pl011 UART.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9088248..763d531 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -351,6 +351,89 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>      return rsdp_table;
>  }
>  
> +static int
> +dbg2_addresses_size(int num_addr)
> +{
> +    return num_addr * (sizeof(struct AcpiGenericAddress) + sizeof(uint32_t));
> +}
> +
> +static int
> +dbg2_dev_length(int num_addr, const char *namepath, int oemdata_length)
> +{
> +    int size;
> +
> +    size = sizeof(AcpiDebugPort2Device);
> +    size += dbg2_addresses_size(num_addr);
> +    size += strlen(namepath) + 1;
> +    size += oemdata_length;
> +
> +    return size;
> +}

I think macros should suffice for the above helpers.

> +
> +static void
> +build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> +    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
> +    AcpiDebugPort2Header *dbg2;
> +    AcpiDebugPort2Device *dev;
> +    struct AcpiGenericAddress *addr;
> +    uint32_t *addr_size;
> +    char *data;
> +    const char namepath[] = ".";
> +    int address_count, oem_length, table_revision, table_size;
> +
> +    address_count = 1;
> +    oem_length = 0;
> +    table_revision = 0;
> +    table_size = sizeof(*dbg2) + dbg2_dev_length(address_count, namepath,
> +                                                 oem_length);
> +
> +    dbg2 = acpi_data_push(table_data, sizeof(*dbg2));
> +    dbg2->devices_offset = sizeof(*dbg2);
> +    dbg2->devices_count = 1;
> +
> +    dev = acpi_data_push(table_data, sizeof(*dev));
> +    dev->revision = table_revision;

dev->revision and table_revision are presumably independent. I think
they should both get their own explicit '= 0'. Doing so allows us to get
rid of the local variable. I actually find the local variables, which
are constants, a bit crufty, and would prefer to just see the numbers.

> +    dev->length = cpu_to_le16(dbg2_dev_length(address_count, namepath,
> +                                              oem_length));
> +    dev->address_count = address_count;
> +    dev->namepath_length = cpu_to_le16(strlen(namepath));

what happened to the strlen + 1

> +    dev->namepath_offset = cpu_to_le16(sizeof(*dev) +
> +                                       dbg2_addresses_size(address_count));
> +    dev->oem_data_length = cpu_to_le16(oem_length);
> +    if (oem_length) {
> +        dev->oem_data_offset = cpu_to_le16(dbg2_dev_length(address_count,
> +                                                           namepath, 0));
> +    } else {
> +        dev->oem_data_offset = 0;
> +    }

I wouldn't bother with the special oem_data handling now, since we don't
plan to use it. If somebody extends this function for nonzero oem_length
sometime, then they can deal with it.

> +    dev->port_type = cpu_to_le16(0x8000);    /* Serial */
> +    dev->port_subtype = cpu_to_le16(0x3);    /* ARM PL011 UART */
> +    dev->base_address_offset = cpu_to_le16(sizeof(*dev));
> +    dev->address_size_offset = cpu_to_le16(sizeof(*dev) +
> +                                           address_count * sizeof(*addr));

Could create another macro for this offset calculation. Actually could add
a macro for each for consistency.

> +
> +    addr = acpi_data_push(table_data, sizeof(*addr) * address_count);
> +    addr->space_id = AML_SYSTEM_MEMORY;
> +    addr->bit_width = 8;
> +    addr->bit_offset = 0;
> +    addr->access_width = 1;
> +    addr->address = cpu_to_le64(uart_memmap->base);
> +
> +    addr_size = acpi_data_push(table_data, sizeof(*addr_size) * address_count);
> +    *addr_size = cpu_to_le32(sizeof(*addr));
> +
> +    data = acpi_data_push(table_data, strlen(namepath) + 1);

After dropping the oem data handling code, then we can use a better name
than 'data' for this.

> +    strcpy(data, namepath);
> +
> +    if (oem_length) {
> +        data = acpi_data_push(table_data, oem_length);
> +    }
> +
> +    build_header(linker, table_data, (void *)dbg2, "DBG2", table_size,
> +                 table_revision);
> +}
> +
>  static void
>  build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
> @@ -577,7 +660,7 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>      dsdt = tables_blob->len;
>      build_dsdt(tables_blob, tables->linker, guest_info);
>  
> -    /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
> +    /* FADT MADT GTDT MCFG DBG2 SPCR pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt(tables_blob, tables->linker, dsdt);
>  
> @@ -591,6 +674,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>      build_mcfg(tables_blob, tables->linker, guest_info);
>  
>      acpi_add_table(table_offsets, tables_blob);
> +    build_dbg2(tables_blob, tables->linker, guest_info);
> +
> +    acpi_add_table(table_offsets, tables_blob);
>      build_spcr(tables_blob, tables->linker, guest_info);
>  
>      /* RSDT is pointed to by RSDP */
> -- 
> 2.1.4
> 
>

I liked Shannon's suggestion to use more acpi_data_pushing, which this
version provides, but, as we're making assumptions (only one device
and no OEM data), then I think we can further simplify this version.

Thanks,
drew

On 15 September 2015 at 17:42, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Sep 15, 2015 at 03:44:41PM +0100, Leif Lindholm wrote:
>> Add a DBG2 table, describing the pl011 UART.
>>
>> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
>> ---
>>  hw/arm/virt-acpi-build.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9088248..763d531 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -351,6 +351,89 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>>      return rsdp_table;
>>  }
>>
>> +static int
>> +dbg2_addresses_size(int num_addr)
>> +{
>> +    return num_addr * (sizeof(struct AcpiGenericAddress) + sizeof(uint32_t));
>> +}
>> +
>> +static int
>> +dbg2_dev_length(int num_addr, const char *namepath, int oemdata_length)
>> +{
>> +    int size;
>> +
>> +    size = sizeof(AcpiDebugPort2Device);
>> +    size += dbg2_addresses_size(num_addr);
>> +    size += strlen(namepath) + 1;
>> +    size += oemdata_length;
>> +
>> +    return size;
>> +}
>
> I think macros should suffice for the above helpers.

...but if you can write them as functions then why not do so?
The compiler's smart enough to inline as appropriate, and it's
not like performance is critical with one-time ACPI table
building anyhow.

Incidentally putting a linebreak before the function name is
not the usual QEMU style for function definitions.

thanks
-- PMM