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