[Qemu-devel] [PATCH v3 0/2] ACPI/arm-virt: add DBG2

[Qemu-devel] [PATCH v3 0/2] ACPI/arm-virt: add DBG2

[Leif Lindholm]

The Debug Port Table 2 (DBG2) is mandated by the ARM Server Base Boot
Requirements specification. Add the DBG2 table definitions, and set up
an entry in the ARM virt machine for the pl011 UART.

Changes since v2:
- Typedef format cleanup.
- Restructured to use progressive acpi_data_push() calls for each new
  portion of the table.
  - Added some helper functions to reduce clutter.
  - Added some local variables to track state that could otherwise fall
    foul of shifting pointers in future.
  - Added some boiler plate for oem_data additions (but no actual
    oem_data).

Changes since v1:
- Static structure replaced with separate Header/Device structs.
- Missing cpu_to_le*() transforms added in table construction.
- Added missing setting of address_size_offset.
- Commit message modified to mention SPCR spec version bump.

Leif Lindholm (2):
  ACPI: Add definitions for the DBG2 table
  hw/arm/virt-acpi-build: Add DBG2 table

 hw/arm/virt-acpi-build.c    | 88 ++++++++++++++++++++++++++++++++++++++++++++-
 include/hw/acpi/acpi-defs.h | 33 +++++++++++++++--
 2 files changed, 118 insertions(+), 3 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH v3 1/2] ACPI: Add definitions for the DBG2 table

[Leif Lindholm]

The DBG2 table can be considered a "companion" to SPCR - it points out
debug consoles available in the system.

Also update SPCR comments to reflect DBG2 is now described in this file,
and update the supported SPCR specification revision (no functional
change).

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 include/hw/acpi/acpi-defs.h | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 2b431e6..3cfc5a6 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -197,10 +197,39 @@ enum {
 };
 
 /*
- * Serial Port Console Redirection Table (SPCR), Rev. 1.02
+ * Debug Port Table 2 (DBG2)
  *
  * For .interface_type see Debug Port Table 2 (DBG2) serial port
- * subtypes in Table 3, Rev. May 22, 2012
+ * subtypes in Table 3, Rev. Aug 10, 2015
+ *
+ */
+struct AcpiDebugPort2Header {
+    ACPI_TABLE_HEADER_DEF
+    uint32_t devices_offset;
+    uint32_t devices_count;
+} QEMU_PACKED;
+typedef struct AcpiDebugPort2Header AcpiDebugPort2Header;
+
+struct AcpiDebugPort2Device {
+    uint8_t  revision;
+    uint16_t length;
+    uint8_t  address_count;
+    uint16_t namepath_length;
+    uint16_t namepath_offset;
+    uint16_t oem_data_length;
+    uint16_t oem_data_offset;
+    uint16_t port_type;
+    uint16_t port_subtype;
+    uint8_t  reserved1[2];
+    uint16_t base_address_offset;
+    uint16_t address_size_offset;
+} QEMU_PACKED;
+typedef struct AcpiDebugPort2Device AcpiDebugPort2Device;
+
+/*
+ * Serial Port Console Redirection Table (SPCR), Rev. 1.03
+ *
+ * .interface_type format same as for DBG2.
  */
 struct AcpiSerialPortConsoleRedirection {
     ACPI_TABLE_HEADER_DEF
-- 
2.1.4

[Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add DBG2 table

[Leif Lindholm]

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

Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add DBG2 table

[Andrew Jones]

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 

Re: [Qemu-devel] [PATCH v3 2/2] hw/arm/virt-acpi-build: Add DBG2 table

[Peter Maydell]

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