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

[Qemu-devel] [PATCH 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.

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

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

-- 
2.1.4

[Qemu-devel] [PATCH 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.

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

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 2b431e6..af062a7 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -197,10 +197,43 @@ 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
+ *
+ * The specification permits multiple ports with multiple addresses, but this
+ * implementation is limited to one port with one address.
+ */
+struct AcpiDebugPort2 {
+    ACPI_TABLE_HEADER_DEF
+    uint32_t debug_devices_offset;
+    uint32_t number_debug_devices;
+    struct {
+      uint8_t  revision;
+      uint16_t length;
+      uint8_t  number_generic_address_registers;
+      uint16_t namespace_string_length;
+      uint16_t namespace_string_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_register_offset;
+      uint16_t address_size_offset;
+      struct AcpiGenericAddress base_address[1];
+      uint32_t address_size[1];
+      uint8_t  namespace_string[2];
+    } QEMU_PACKED debug_devices[1];
+} QEMU_PACKED;
+typedef struct AcpiDebugPort2
+               AcpiDebugPort2;
+
+/*
+ * 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

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

[Andrew Jones]

On Mon, Sep 07, 2015 at 03:23:45PM +0100, Leif Lindholm wrote:
> The DBG2 table can be considered a "companion" to SPCR - it points out
> debug consoles available in the system.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 2b431e6..af062a7 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -197,10 +197,43 @@ 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
> + *
> + * The specification permits multiple ports with multiple addresses, but this
> + * implementation is limited to one port with one address.
> + */
> +struct AcpiDebugPort2 {
> +    ACPI_TABLE_HEADER_DEF
> +    uint32_t debug_devices_offset;
> +    uint32_t number_debug_devices;
> +    struct {
> +      uint8_t  revision;
> +      uint16_t length;
> +      uint8_t  number_generic_address_registers;
> +      uint16_t namespace_string_length;
> +      uint16_t namespace_string_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_register_offset;
> +      uint16_t address_size_offset;
> +      struct AcpiGenericAddress base_address[1];

Not sure we want to limit the number of addresses. Non ARM (non PL011)
users of this table may not find one address sufficient.

> +      uint32_t address_size[1];
> +      uint8_t  namespace_string[2];
> +    } QEMU_PACKED debug_devices[1];

I'm guessing it's unlikely we'll ever want more than one debug port. So
can we just drop the debug_devices array, flatting the table? OTOH, this
is generic acpi table generation code here, and maybe x86 will want to
use more than one port? In that case we should pull the debug device
structure definition out, and then properly handle the variable length
array. But this is where Igor and mst will suggest just using aml_appends,
instead of defining these structures at all :-)

> +} QEMU_PACKED;
> +typedef struct AcpiDebugPort2
> +               AcpiDebugPort2;
> +
> +/*
> + * 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
> 

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

[Michael S. Tsirkin]

On Mon, Sep 07, 2015 at 05:51:50PM +0200, Andrew Jones wrote:
> On Mon, Sep 07, 2015 at 03:23:45PM +0100, Leif Lindholm wrote:
> > The DBG2 table can be considered a "companion" to SPCR - it points out
> > debug consoles available in the system.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 2b431e6..af062a7 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -197,10 +197,43 @@ 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
> > + *
> > + * The specification permits multiple ports with multiple addresses, but this
> > + * implementation is limited to one port with one address.
> > + */
> > +struct AcpiDebugPort2 {
> > +    ACPI_TABLE_HEADER_DEF
> > +    uint32_t debug_devices_offset;
> > +    uint32_t number_debug_devices;
> > +    struct {
> > +      uint8_t  revision;
> > +      uint16_t length;
> > +      uint8_t  number_generic_address_registers;
> > +      uint16_t namespace_string_length;
> > +      uint16_t namespace_string_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_register_offset;
> > +      uint16_t address_size_offset;
> > +      struct AcpiGenericAddress base_address[1];
> 
> Not sure we want to limit the number of addresses. Non ARM (non PL011)
> users of this table may not find one address sufficient.
> 
> > +      uint32_t address_size[1];
> > +      uint8_t  namespace_string[2];
> > +    } QEMU_PACKED debug_devices[1];
> 
> I'm guessing it's unlikely we'll ever want more than one debug port. So
> can we just drop the debug_devices array, flatting the table? OTOH, this
> is generic acpi table generation code here, and maybe x86 will want to
> use more than one port? In that case we should pull the debug device
> structure definition out, and then properly handle the variable length
> array. But this is where Igor and mst will suggest just using aml_appends,
> instead of defining these structures at all :-)

Yes - structures are fine when they are static, but for dynamic
stuff, aml_append wins hands down.
You simply add comments in code documenting each field as
it's added.




Igor actually likes aml_append for static things as well
but it's a matter of taste.

> > +} QEMU_PACKED;
> > +typedef struct AcpiDebugPort2
> > +               AcpiDebugPort2;
> > +
> > +/*
> > + * 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
> > 

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

[Igor Mammedov]

On Thu, 10 Sep 2015 11:19:09 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Sep 07, 2015 at 05:51:50PM +0200, Andrew Jones wrote:
> > On Mon, Sep 07, 2015 at 03:23:45PM +0100, Leif Lindholm wrote:
> > > The DBG2 table can be considered a "companion" to SPCR - it points out
> > > debug consoles available in the system.
> > > 
> > > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > > ---
> > >  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 35 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index 2b431e6..af062a7 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -197,10 +197,43 @@ 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
> > > + *
> > > + * The specification permits multiple ports with multiple addresses, but this
> > > + * implementation is limited to one port with one address.
> > > + */
> > > +struct AcpiDebugPort2 {
> > > +    ACPI_TABLE_HEADER_DEF
> > > +    uint32_t debug_devices_offset;
> > > +    uint32_t number_debug_devices;
> > > +    struct {
> > > +      uint8_t  revision;
> > > +      uint16_t length;
> > > +      uint8_t  number_generic_address_registers;
> > > +      uint16_t namespace_string_length;
> > > +      uint16_t namespace_string_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_register_offset;
> > > +      uint16_t address_size_offset;
> > > +      struct AcpiGenericAddress base_address[1];
> > 
> > Not sure we want to limit the number of addresses. Non ARM (non PL011)
> > users of this table may not find one address sufficient.
> > 
> > > +      uint32_t address_size[1];
> > > +      uint8_t  namespace_string[2];
> > > +    } QEMU_PACKED debug_devices[1];
> > 
> > I'm guessing it's unlikely we'll ever want more than one debug port. So
> > can we just drop the debug_devices array, flatting the table? OTOH, this
> > is generic acpi table generation code here, and maybe x86 will want to
> > use more than one port? In that case we should pull the debug device
> > structure definition out, and then properly handle the variable length
> > array. But this is where Igor and mst will suggest just using aml_appends,
> > instead of defining these structures at all :-)
> 
> Yes - structures are fine when they are static, but for dynamic
> stuff, aml_append wins hands down.
> You simply add comments in code documenting each field as
> it's added.
> 
> 
> 
> 
> Igor actually likes aml_append for static things as well
> but it's a matter of taste.
Yep, I won't fight over static tables described as structs,
only we have to take a special care so that field would be
in right endiannes which is broken in this patch as Drew noticed.
With aml_append() it's done automatically for user +
a better table description including explicit fields size.

> 
> > > +} QEMU_PACKED;
> > > +typedef struct AcpiDebugPort2
> > > +               AcpiDebugPort2;
> > > +
> > > +/*
> > > + * 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
> > > 

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

[Shannon Zhao]

On 2015/9/7 22:23, Leif Lindholm wrote:
> The DBG2 table can be considered a "companion" to SPCR - it points out
> debug consoles available in the system.
> 
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 2b431e6..af062a7 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -197,10 +197,43 @@ 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
> + *
> + * The specification permits multiple ports with multiple addresses, but this
> + * implementation is limited to one port with one address.
> + */
> +struct AcpiDebugPort2 {
> +    ACPI_TABLE_HEADER_DEF
> +    uint32_t debug_devices_offset;
> +    uint32_t number_debug_devices;
> +    struct {
> +      uint8_t  revision;
> +      uint16_t length;
> +      uint8_t  number_generic_address_registers;
> +      uint16_t namespace_string_length;
> +      uint16_t namespace_string_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_register_offset;
> +      uint16_t address_size_offset;
> +      struct AcpiGenericAddress base_address[1];
> +      uint32_t address_size[1];
> +      uint8_t  namespace_string[2];
> +    } QEMU_PACKED debug_devices[1];

This debug_device should be defined alone. And use acpi_data_push to add
multiple debug_devices structures according to the number of debug device.

> +} QEMU_PACKED;
> +typedef struct AcpiDebugPort2
> +               AcpiDebugPort2;
> +
> +/*
> + * Serial Port Console Redirection Table (SPCR), Rev. 1.03
> + *

Why do you change the revision of SPCR from 1.02 to 1.03?

> + * .interface_type format same as for DBG2.
>   */
>  struct AcpiSerialPortConsoleRedirection {
>      ACPI_TABLE_HEADER_DEF
> 

-- 
Shannon

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

[Leif Lindholm]

On Tue, Sep 08, 2015 at 11:27:08AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/9/7 22:23, Leif Lindholm wrote:
> > The DBG2 table can be considered a "companion" to SPCR - it points out
> > debug consoles available in the system.
> > 
> > Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> > ---
> >  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 2b431e6..af062a7 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -197,10 +197,43 @@ 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
> > + *
> > + * The specification permits multiple ports with multiple addresses, but this
> > + * implementation is limited to one port with one address.
> > + */
> > +struct AcpiDebugPort2 {
> > +    ACPI_TABLE_HEADER_DEF
> > +    uint32_t debug_devices_offset;
> > +    uint32_t number_debug_devices;
> > +    struct {
> > +      uint8_t  revision;
> > +      uint16_t length;
> > +      uint8_t  number_generic_address_registers;
> > +      uint16_t namespace_string_length;
> > +      uint16_t namespace_string_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_register_offset;
> > +      uint16_t address_size_offset;
> > +      struct AcpiGenericAddress base_address[1];
> > +      uint32_t address_size[1];
> > +      uint8_t  namespace_string[2];
> > +    } QEMU_PACKED debug_devices[1];
> 
> This debug_device should be defined alone. And use acpi_data_push to add
> multiple debug_devices structures according to the number of debug device.

Sure. That sounds a lot less ugly :)
I didn't quite get the interfaces, but with what you say above I think
I do. (And I think Drew suggested similarly.)

Will update and resend.

> > +} QEMU_PACKED;
> > +typedef struct AcpiDebugPort2
> > +               AcpiDebugPort2;
> > +
> > +/*
> > + * Serial Port Console Redirection Table (SPCR), Rev. 1.03
> > + *
> 
> Why do you change the revision of SPCR from 1.02 to 1.03?

Mainly because 1.03 has been released, with no other modifications
than a license update (which has hopefully unblocked the inclusion of
DBG2/SPCR support in Linux kernel). Sorry, should have stated.

> > + * .interface_type format same as for DBG2.
> >   */
> >  struct AcpiSerialPortConsoleRedirection {
> >      ACPI_TABLE_HEADER_DEF
> > 

Thanks!

/
    Leif

[Qemu-devel] [PATCH 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 | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9088248..6d53dbe 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -352,6 +352,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
 }
 
 static void
+build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+    AcpiDebugPort2 *dbg2;
+    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
+
+    dbg2 = acpi_data_push(table_data, sizeof(*dbg2));
+
+    dbg2->debug_devices_offset = 44;
+    dbg2->number_debug_devices = 1;
+    dbg2->debug_devices[0].revision = 0;
+    dbg2->debug_devices[0].length = 40;
+    dbg2->debug_devices[0].number_generic_address_registers = 1;
+    dbg2->debug_devices[0].namespace_string_length = 2;
+    dbg2->debug_devices[0].namespace_string_offset = 38;
+    dbg2->debug_devices[0].oem_data_length = 0;
+    dbg2->debug_devices[0].oem_data_offset = 0;
+    dbg2->debug_devices[0].port_type = 0x8000;    /* Serial */
+    dbg2->debug_devices[0].port_subtype = 0x3;    /* ARM PL011 UART */
+    dbg2->debug_devices[0].base_address_register_offset = 22;
+
+    dbg2->debug_devices[0].base_address[0].space_id = AML_SYSTEM_MEMORY;
+    dbg2->debug_devices[0].base_address[0].bit_width = 8;
+    dbg2->debug_devices[0].base_address[0].bit_offset = 0;
+    dbg2->debug_devices[0].base_address[0].access_width = 1;
+    dbg2->debug_devices[0].base_address[0].address = cpu_to_le64(uart_memmap->base);
+
+    strcpy((char *)dbg2->debug_devices[0].namespace_string, ".");
+
+    build_header(linker, table_data, (void *)dbg2, "DBG2", sizeof(*dbg2), 0);
+}
+
+static void
 build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
 {
     AcpiSerialPortConsoleRedirection *spcr;
@@ -577,7 +609,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 +623,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 2/2] hw/arm/virt-acpi-build: Add DBG2 table

[Andrew Jones]

On Mon, Sep 07, 2015 at 03:23:46PM +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 | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 9088248..6d53dbe 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -352,6 +352,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>  }
>  
>  static void
> +build_dbg2(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> +    AcpiDebugPort2 *dbg2;
> +    const MemMapEntry *uart_memmap = &guest_info->memmap[VIRT_UART];
> +
> +    dbg2 = acpi_data_push(table_data, sizeof(*dbg2));
> +
> +    dbg2->debug_devices_offset = 44;

I'd prefer using offsetof(), even though it's unlikely to ever change.

> +    dbg2->number_debug_devices = 1;
> +    dbg2->debug_devices[0].revision = 0;
> +    dbg2->debug_devices[0].length = 40;

If we keep the debug device structure definition, then this can be
sizeof()

> +    dbg2->debug_devices[0].number_generic_address_registers = 1;
> +    dbg2->debug_devices[0].namespace_string_length = 2;
> +    dbg2->debug_devices[0].namespace_string_offset = 38;

Could use offsetof() if we have the debug device struct

> +    dbg2->debug_devices[0].oem_data_length = 0;
> +    dbg2->debug_devices[0].oem_data_offset = 0;
> +    dbg2->debug_devices[0].port_type = 0x8000;    /* Serial */
> +    dbg2->debug_devices[0].port_subtype = 0x3;    /* ARM PL011 UART */
> +    dbg2->debug_devices[0].base_address_register_offset = 22;

offsetof()

> +
> +    dbg2->debug_devices[0].base_address[0].space_id = AML_SYSTEM_MEMORY;
> +    dbg2->debug_devices[0].base_address[0].bit_width = 8;
> +    dbg2->debug_devices[0].base_address[0].bit_offset = 0;
> +    dbg2->debug_devices[0].base_address[0].access_width = 1;

We're missing a bunch of cpu_to_le*'s for the >1 byte, non-zero assignments
above.

> +    dbg2->debug_devices[0].base_address[0].address = cpu_to_le64(uart_memmap->base);
> +
> +    strcpy((char *)dbg2->debug_devices[0].namespace_string, ".");
> +
> +    build_header(linker, table_data, (void *)dbg2, "DBG2", sizeof(*dbg2), 0);
> +}
> +
> +static void
>  build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      AcpiSerialPortConsoleRedirection *spcr;
> @@ -577,7 +609,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 +623,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
> 
>

Thanks,
drew 

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

[Shannon Zhao]

On 2015/9/7 22:23, Leif Lindholm wrote:
> 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.
> 

Looking at Documentation/arm64/acpi_object_usage.txt in Linux kernel, it
says
"
DBG2   Signature Reserved (signature == "DBG2")
       == DeBuG port table 2 ==
       Microsoft only table, will not be supported.
"
It seems that Linux kernel doesn't support or need it, but Windows
requires it. So does it need to test this on Windows?

> Leif Lindholm (2):
>   ACPI: Add definitions for the DBG2 table
>   hw/arm/virt-acpi-build: Add DBG2 table
> 
>  hw/arm/virt-acpi-build.c    | 37 ++++++++++++++++++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
>  2 files changed, 71 insertions(+), 3 deletions(-)
> 

-- 
Shannon

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

[Leif Lindholm]

On Tue, Sep 08, 2015 at 11:18:27AM +0800, Shannon Zhao wrote:
> On 2015/9/7 22:23, Leif Lindholm wrote:
> > 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.
> 
> Looking at Documentation/arm64/acpi_object_usage.txt in Linux kernel, it
> says
> "
> DBG2   Signature Reserved (signature == "DBG2")
>        == DeBuG port table 2 ==
>        Microsoft only table, will not be supported.
> "
> It seems that Linux kernel doesn't support or need it, but Windows
> requires it. So does it need to test this on Windows?

No, it can be tested under Linux with the set I just sent out:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368614.html
But I needed this set for QEMU for the kernel people to have something
to test :)

That said, I would be overjoyed if someone _could_ test it on Windows.

/
    Leif

> > Leif Lindholm (2):
> >   ACPI: Add definitions for the DBG2 table
> >   hw/arm/virt-acpi-build: Add DBG2 table
> > 
> >  hw/arm/virt-acpi-build.c    | 37 ++++++++++++++++++++++++++++++++++++-
> >  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
> >  2 files changed, 71 insertions(+), 3 deletions(-)
> > 
> 
> -- 
> Shannon
> 

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

[Shannon Zhao]

On 2015/9/8 21:04, Leif Lindholm wrote:
> On Tue, Sep 08, 2015 at 11:18:27AM +0800, Shannon Zhao wrote:
>> On 2015/9/7 22:23, Leif Lindholm wrote:
>>> 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.
>>
>> Looking at Documentation/arm64/acpi_object_usage.txt in Linux kernel, it
>> says
>> "
>> DBG2   Signature Reserved (signature == "DBG2")
>>        == DeBuG port table 2 ==
>>        Microsoft only table, will not be supported.
>> "
>> It seems that Linux kernel doesn't support or need it, but Windows
>> requires it. So does it need to test this on Windows?
> 
> No, it can be tested under Linux with the set I just sent out:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368614.html

So you need to change Documentation/arm64/acpi_object_usage.txt as well.
But that is kernel side.

> But I needed this set for QEMU for the kernel people to have something
> to test :)
> 
> That said, I would be overjoyed if someone _could_ test it on Windows.
> 
> /
>     Leif
> 
>>> Leif Lindholm (2):
>>>   ACPI: Add definitions for the DBG2 table
>>>   hw/arm/virt-acpi-build: Add DBG2 table
>>>
>>>  hw/arm/virt-acpi-build.c    | 37 ++++++++++++++++++++++++++++++++++++-
>>>  include/hw/acpi/acpi-defs.h | 37 +++++++++++++++++++++++++++++++++++--
>>>  2 files changed, 71 insertions(+), 3 deletions(-)
>>>
>>
>> -- 
>> Shannon
>>
> 
> 

-- 
Shannon

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

[Andrew Jones]

On Wed, Sep 09, 2015 at 12:25:45PM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/9/8 21:04, Leif Lindholm wrote:
> > On Tue, Sep 08, 2015 at 11:18:27AM +0800, Shannon Zhao wrote:
> >> On 2015/9/7 22:23, Leif Lindholm wrote:
> >>> 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.
> >>
> >> Looking at Documentation/arm64/acpi_object_usage.txt in Linux kernel, it
> >> says
> >> "
> >> DBG2   Signature Reserved (signature == "DBG2")
> >>        == DeBuG port table 2 ==
> >>        Microsoft only table, will not be supported.
> >> "
> >> It seems that Linux kernel doesn't support or need it, but Windows
> >> requires it. So does it need to test this on Windows?
> > 
> > No, it can be tested under Linux with the set I just sent out:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368614.html
> 
> So you need to change Documentation/arm64/acpi_object_usage.txt as well.
> But that is kernel side.

I also wonder if Documentation/arm64/arm-acpi.txt should have its
"Booting using ACPI tables" section updated to reference (describe even?)
the use of SPCR. Maybe Leif's already doing that with the
this-time-really-for-upstream kernel spcr patches.

drew

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

[Al Stone]

On 09/08/2015 11:52 PM, Andrew Jones wrote:
> On Wed, Sep 09, 2015 at 12:25:45PM +0800, Shannon Zhao wrote:
>>
>>
>> On 2015/9/8 21:04, Leif Lindholm wrote:
>>> On Tue, Sep 08, 2015 at 11:18:27AM +0800, Shannon Zhao wrote:
>>>> On 2015/9/7 22:23, Leif Lindholm wrote:
>>>>> 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.
>>>>
>>>> Looking at Documentation/arm64/acpi_object_usage.txt in Linux kernel, it
>>>> says
>>>> "
>>>> DBG2   Signature Reserved (signature == "DBG2")
>>>>        == DeBuG port table 2 ==
>>>>        Microsoft only table, will not be supported.
>>>> "
>>>> It seems that Linux kernel doesn't support or need it, but Windows
>>>> requires it. So does it need to test this on Windows?
>>>
>>> No, it can be tested under Linux with the set I just sent out:
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/368614.html
>>
>> So you need to change Documentation/arm64/acpi_object_usage.txt as well.
>> But that is kernel side.
> 
> I also wonder if Documentation/arm64/arm-acpi.txt should have its
> "Booting using ACPI tables" section updated to reference (describe even?)
> the use of SPCR. Maybe Leif's already doing that with the
> this-time-really-for-upstream kernel spcr patches.
> 
> drew
> 

It's on my TODO list to update this documentation properly, to
include updates on DBG2 and SPCR along with a bunch of other
ACPI 6.0 changes.  My bad for the delays on this...

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------