| Message ID | 1441635826-4866-2-git-send-email-leif.lindholm@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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
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 > >
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 > > >
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
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(-)