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

[Leif Lindholm]

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 <address@hidden>
> >> ---
> >>  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 <address@hidden>
> > 
> > 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