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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 09, 2015 at 07:03:04PM +0200, Andrew Jones wrote:
> On Tue, Jun 09, 2015 at 04:52:39PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2015 at 10:00:49AM -0400, Andrew Jones wrote:
> > > SPCR is the Serial Port Console Redirection table. See the document
> > > linked from http://uefi.org/acpi. For serial port types, "Interface
> > > Type", see the documentation for the Debug Port Table 2 (DBG2).
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  include/hw/acpi/acpi-defs.h | 72 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > > 
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index 59cf277434b37..e579d4c509fc8 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -197,6 +197,78 @@ enum {
> > >  };
> > >  
> > >  /*
> > > + * ACPI Serial Port Console Redirection Table
> > > + */
> > > +enum {
> > > +    ACPI_SERIAL_16550_COMPAT        = 0,
> > > +    ACPI_SERIAL_16550_SUBSET_COMPAT = 1,
> > > +    ACPI_SERIAL_ARM_PL011_UART      = 3,
> > > +};
> > > +
> > > +enum {
> > > +    ACPI_SERIAL_ITYPE_PC            = 1,
> > > +    ACPI_SERIAL_ITYPE_APIC          = 2,
> > > +    ACPI_SERIAL_ITYPE_SAPIC         = 4,
> > > +    ACPI_SERIAL_ITYPE_ARMH_GIC      = 8,
> > > +};
> > > +
> > > +enum {
> > > +    ACPI_SERIAL_BAUD_9600           = 3,
> > > +    ACPI_SERIAL_BAUD_19200          = 4,
> > > +    ACPI_SERIAL_BAUD_57600          = 6,
> > > +    ACPI_SERIAL_BAUD_115200         = 7,
> > > +};
> > > +
> > > +enum {
> > > +    ACPI_SERIAL_FLOW_DCD_REQUIRED   = 1,
> > > +    ACPI_SERIAL_FLOW_HW             = 2,
> > > +    ACPI_SERIAL_FLOW_SW             = 4,
> > > +};
> > > +
> > > +enum {
> > > +    ACPI_SERIAL_TERM_VT100          = 0,
> > > +    ACPI_SERIAL_TERM_VT100_PLUS     = 1,
> > > +    ACPI_SERIAL_TERM_VT_UTF8        = 2,
> > > +    ACPI_SERIAL_TERM_ANSI           = 3,
> > > +};
> > > +
> > 
> > Please don't do these single-use enums, this more than doubles the
> > amount of code required and makes it hard to look up things in spec.
> 
> They also serve to document the structure though. Without them
> we either need to add several lines to the comments for the struct
> members, or to simply not document the structure at all.

The idea is to document the code, that has both values
and struct names together.

> But, if
> the general preference is less lines of code, at the expense of
> always needing the spec open, then I won't resist removing the
> enums

In some cases, we've already given up on structs too, using
things like build_append_int_noprefix instead.

> (and struct documentation?).

Move the documentation to build_spcr, yes.

> I don't understand how they make looking things up in the spec
> more difficult. Is it because the naming doesn't exactly match
> the spec verbiage?

Yes.

> In both cases, I guess the actual value is
> what would be compared on lookup.

It's very convenient to locate things quickly using
full text search.
Try doing a full text search for 0x0 in a spec :)



> > Do this instead
> > 	sprc->interface_type = 0x0; /* full 16550 interface */
> > 
> > you should also list earliest spec version which has the data
> > since spec text changes with time.
> 
> I'll change the header to
> 
> /*
>  * ACPI 2.0 Serial Port Console Redirection Table (SPCR)
>  */

Isn't it actually
	Microsoft Serial Port Console Redirection Table
and documented in a couple of other specs?
ACPI spec just lists the signatures.

> assuming I understood your comment correctly.
> 
> Thanks for the review,
> drew