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

Message ID 1442156793-30708-3-git-send-email-leif.lindholm@linaro.org
State New
Headers show

Commit Message

Leif Lindholm Sept. 13, 2015, 3:06 p.m.
Add a DBG2 table, describing the pl011 UART.

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

Comments

Andrew Jones Sept. 14, 2015, 4:35 p.m. | #1
On Sun, Sep 13, 2015 at 04:06:33PM +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 | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9088248..0ea7023 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -352,6 +352,61 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>  }
>  
>  static void
> +build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> +    AcpiDebugPort2Header *dbg2;
> +    AcpiDebugPort2Device *dev;
> +    struct AcpiGenericAddress *addr;
> +    uint32_t *addr_size;
> +    char *name;
> +    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
> +    int table_size, dev_size, namepath_length;
> +    const char namepath[] = ".";
> +
> +    namepath_length = strlen(namepath) + 1;
> +    dev_size = sizeof(*dev) + sizeof(*addr) * 1 + sizeof(uint32_t) * 1 +
> +        namepath_length;
> +    table_size = dev_size + sizeof(AcpiDebugPort2Header);
> +
> +    dbg2 = acpi_data_push(table_data, table_size);
> +    dev = (void *)dbg2 + sizeof(*dbg2);
> +    addr = (void *)dev + sizeof(*dev);
> +    addr_size = (void *)addr + sizeof(*addr);
> +    name = (void *)addr_size + sizeof(*addr_size);
> +
> +    dbg2->devices_offset = sizeof(*dbg2);
> +    dbg2->devices_count = 1;
> +
> +    /* First (only) debug device */
> +    dev->revision = 0;
> +    dev->length = cpu_to_le16(dev_size);
> +    dev->address_count = 1;
> +    dev->namepath_length = cpu_to_le16(namepath_length);
> +    dev->namepath_offset = cpu_to_le16((void *)name - (void *)dev);
> +    dev->oem_data_length = 0;
> +    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((void *)addr - (void *)dev);
> +    dev->address_size_offset = cpu_to_le16((void *)addr_size - (void *)dev);
> +
> +    /* First (only) address */
> +    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);
> +
> +    /* Size of first (only) address */
> +    *addr_size = cpu_to_le32(sizeof(*addr));
> +
> +    /* Namespace String for first (only) device */
> +    strcpy(name, namepath);
> +
> +    build_header(linker, table_data, (void *)dbg2, "DBG2", table_size, 0);
> +}
> +
> +static void
>  build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      AcpiSerialPortConsoleRedirection *spcr;
> @@ -577,7 +632,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 +646,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
> 
> 

This looks good to me, since it's pretty unlikely we'll ever want
more than one device, so

Reviewed-by: Andrew Jones <drjones@redhat.com>

But, when I read that the table generation had become dynamic, I was sort
of expecting something like

void dbg2_add_device(...)
{
...
}

void build_dbg2(...)
{
   do_top_level_table_stuff...

   for_each_debug_device
      dbg2_add_device()

   finalize_table...
}

drew
Shannon Zhao Sept. 15, 2015, 1:20 a.m. | #2
On 2015/9/15 0:35, Andrew Jones wrote:
> On Sun, Sep 13, 2015 at 04:06:33PM +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 | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 9088248..0ea7023 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -352,6 +352,61 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>>  }
>>  
>>  static void
>> +build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>> +{
>> +    AcpiDebugPort2Header *dbg2;
>> +    AcpiDebugPort2Device *dev;
>> +    struct AcpiGenericAddress *addr;
>> +    uint32_t *addr_size;
>> +    char *name;
>> +    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
>> +    int table_size, dev_size, namepath_length;
>> +    const char namepath[] = ".";
>> +
>> +    namepath_length = strlen(namepath) + 1;
>> +    dev_size = sizeof(*dev) + sizeof(*addr) * 1 + sizeof(uint32_t) * 1 +
>> +        namepath_length;
>> +    table_size = dev_size + sizeof(AcpiDebugPort2Header);
>> +
>> +    dbg2 = acpi_data_push(table_data, table_size);
>> +    dev = (void *)dbg2 + sizeof(*dbg2);
>> +    addr = (void *)dev + sizeof(*dev);
>> +    addr_size = (void *)addr + sizeof(*addr);
>> +    name = (void *)addr_size + sizeof(*addr_size);
>> +

This looks hard to read.

>> +    dbg2->devices_offset = sizeof(*dbg2);
>> +    dbg2->devices_count = 1;
>> +
>> +    /* First (only) debug device */
>> +    dev->revision = 0;
>> +    dev->length = cpu_to_le16(dev_size);
>> +    dev->address_count = 1;
>> +    dev->namepath_length = cpu_to_le16(namepath_length);
>> +    dev->namepath_offset = cpu_to_le16((void *)name - (void *)dev);
>> +    dev->oem_data_length = 0;
>> +    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((void *)addr - (void *)dev);
>> +    dev->address_size_offset = cpu_to_le16((void *)addr_size - (void *)dev);
>> +
>> +    /* First (only) address */
>> +    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);
>> +
>> +    /* Size of first (only) address */
>> +    *addr_size = cpu_to_le32(sizeof(*addr));
>> +
>> +    /* Namespace String for first (only) device */
>> +    strcpy(name, namepath);
>> +
>> +    build_header(linker, table_data, (void *)dbg2, "DBG2", table_size, 0);
>> +}
>> +
>> +static void
>>  build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>>  {
>>      AcpiSerialPortConsoleRedirection *spcr;
>> @@ -577,7 +632,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 +646,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
>>
>>
> 
> This looks good to me, since it's pretty unlikely we'll ever want
> more than one device, so
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> But, when I read that the table generation had become dynamic, I was sort
> of expecting something like
> 

Leif, you can have a look at build_madt.
Leif Lindholm Sept. 15, 2015, 2:30 p.m. | #3
On Tue, Sep 15, 2015 at 09:20:40AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/9/15 0:35, Andrew Jones wrote:
> > On Sun, Sep 13, 2015 at 04:06:33PM +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 | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 59 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 9088248..0ea7023 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -352,6 +352,61 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
> >>  }
> >>  
> >>  static void
> >> +build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >> +{
> >> +    AcpiDebugPort2Header *dbg2;
> >> +    AcpiDebugPort2Device *dev;
> >> +    struct AcpiGenericAddress *addr;
> >> +    uint32_t *addr_size;
> >> +    char *name;
> >> +    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
> >> +    int table_size, dev_size, namepath_length;
> >> +    const char namepath[] = ".";
> >> +
> >> +    namepath_length = strlen(namepath) + 1;
> >> +    dev_size = sizeof(*dev) + sizeof(*addr) * 1 + sizeof(uint32_t) * 1 +
> >> +        namepath_length;
> >> +    table_size = dev_size + sizeof(AcpiDebugPort2Header);
> >> +
> >> +    dbg2 = acpi_data_push(table_data, table_size);
> >> +    dev = (void *)dbg2 + sizeof(*dbg2);
> >> +    addr = (void *)dev + sizeof(*dev);
> >> +    addr_size = (void *)addr + sizeof(*addr);
> >> +    name = (void *)addr_size + sizeof(*addr_size);
> >> +
> 
> This looks hard to read.
> 
> >> +    dbg2->devices_offset = sizeof(*dbg2);
> >> +    dbg2->devices_count = 1;
> >> +
> >> +    /* First (only) debug device */
> >> +    dev->revision = 0;
> >> +    dev->length = cpu_to_le16(dev_size);
> >> +    dev->address_count = 1;
> >> +    dev->namepath_length = cpu_to_le16(namepath_length);
> >> +    dev->namepath_offset = cpu_to_le16((void *)name - (void *)dev);
> >> +    dev->oem_data_length = 0;
> >> +    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((void *)addr - (void *)dev);
> >> +    dev->address_size_offset = cpu_to_le16((void *)addr_size - (void *)dev);
> >> +
> >> +    /* First (only) address */
> >> +    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);
> >> +
> >> +    /* Size of first (only) address */
> >> +    *addr_size = cpu_to_le32(sizeof(*addr));
> >> +
> >> +    /* Namespace String for first (only) device */
> >> +    strcpy(name, namepath);
> >> +
> >> +    build_header(linker, table_data, (void *)dbg2, "DBG2", table_size, 0);
> >> +}
> >> +
> >> +static void
> >>  build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >>  {
> >>      AcpiSerialPortConsoleRedirection *spcr;
> >> @@ -577,7 +632,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 +646,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
> >>
> >>
> > 
> > This looks good to me, since it's pretty unlikely we'll ever want
> > more than one device, so
> > 
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > 
> > But, when I read that the table generation had become dynamic, I was sort
> > of expecting something like
> > 
> 
> Leif, you can have a look at build_madt.

That is actually one of the more confusing functions for me, what with
all the pointers that may silently become invalid during the execution
of the function.

But yeah, comparing that one with the i386 one, and perhaps the brain
somewhat more engaged than during the weekend, I have a version a bit
cleaner than the one I sent out over the weekend, and hopefully not
too likely to trigger spurious failures if edited by others in future.

Coming up.

/
    Leif

Patch

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9088248..0ea7023 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -352,6 +352,61 @@  build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
 }
 
 static void
+build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+    AcpiDebugPort2Header *dbg2;
+    AcpiDebugPort2Device *dev;
+    struct AcpiGenericAddress *addr;
+    uint32_t *addr_size;
+    char *name;
+    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
+    int table_size, dev_size, namepath_length;
+    const char namepath[] = ".";
+
+    namepath_length = strlen(namepath) + 1;
+    dev_size = sizeof(*dev) + sizeof(*addr) * 1 + sizeof(uint32_t) * 1 +
+        namepath_length;
+    table_size = dev_size + sizeof(AcpiDebugPort2Header);
+
+    dbg2 = acpi_data_push(table_data, table_size);
+    dev = (void *)dbg2 + sizeof(*dbg2);
+    addr = (void *)dev + sizeof(*dev);
+    addr_size = (void *)addr + sizeof(*addr);
+    name = (void *)addr_size + sizeof(*addr_size);
+
+    dbg2->devices_offset = sizeof(*dbg2);
+    dbg2->devices_count = 1;
+
+    /* First (only) debug device */
+    dev->revision = 0;
+    dev->length = cpu_to_le16(dev_size);
+    dev->address_count = 1;
+    dev->namepath_length = cpu_to_le16(namepath_length);
+    dev->namepath_offset = cpu_to_le16((void *)name - (void *)dev);
+    dev->oem_data_length = 0;
+    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((void *)addr - (void *)dev);
+    dev->address_size_offset = cpu_to_le16((void *)addr_size - (void *)dev);
+
+    /* First (only) address */
+    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);
+
+    /* Size of first (only) address */
+    *addr_size = cpu_to_le32(sizeof(*addr));
+
+    /* Namespace String for first (only) device */
+    strcpy(name, namepath);
+
+    build_header(linker, table_data, (void *)dbg2, "DBG2", table_size, 0);
+}
+
+static void
 build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
 {
     AcpiSerialPortConsoleRedirection *spcr;
@@ -577,7 +632,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 +646,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 */