[Qemu-devel] [PATCH 0/2] ACPI: ARM: add SPCR table

[Qemu-devel] [PATCH 0/2] ACPI: ARM: add SPCR table

[Andrew Jones]

Adding this table allows the guest to boot without the console=
parameter added to the kernel command line. And anyway, kernel doc
Documentation/arm64/acpi_object_usage.txt says it's a required
table for arm64.

Andrew Jones (2):
  ACPI: Add definitions for the SPCR table
  hw/arm/virt-acpi-build: Add SPCR table

 hw/arm/virt-acpi-build.c    | 44 ++++++++++++++++++++++++++-
 include/hw/acpi/acpi-defs.h | 72 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 1 deletion(-)

-- 
1.8.3.1

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

[Andrew Jones]

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,
+};
+
+struct AcpiSerialPortConsoleRedirection
+{
+    ACPI_TABLE_HEADER_DEF            /* ACPI common table header */
+    uint8_t  interface_type;         /* See ACPI_SERIAL_ enums */
+    uint8_t  reserved1[3];           /* Reserved, must be 0 */
+    struct AcpiGenericAddress base_address;
+    uint8_t  interrupt_types;        /* Interrupt type support mask,
+                                      * see ACPI_SERIAL_ITYPE_ enums */
+    uint8_t  irq;                    /* PC irq used. 0 if ACPI_SERIAL_ITYPE_PC
+                                      * is not set, else must be one of
+                                      * 2-7,9-12,14-15 */
+    uint32_t gsi;                    /* Not valid if only ACPI_SERIAL_ITYPE_PC
+                                      * is set in the interrupt_types mask */
+    uint8_t  baud;                   /* See ACPI_SERIAL_BAUD_ enums */
+    uint8_t  parity;                 /* 0 = no parity, 1-255 reserved */
+    uint8_t  stopbits;               /* Must be 1 */
+    uint8_t  flowctrl;               /* See ACPI_SERIAL_FLOW_ enums */
+    uint8_t  term_type;              /* See ACPI_SERIAL_TERM_ enums */
+    uint8_t  reserved2;              /* Reserved, must be 0 */
+    uint16_t pci_device_id;          /* 0xffff, if not a pci dev */
+    uint16_t pci_vendor_id;          /* 0xffff, if not a pci dev */
+    uint8_t  pci_bus;                /* PCI bus number, 0 if not a pci dev */
+    uint8_t  pci_slot;               /* PCI slot number, 0 if not a pci dev */
+    uint8_t  pci_func;               /* PCI function num, 0 if not a pci dev */
+    uint32_t pci_flags;              /* PCI compatibility mask,
+                                      *   bits 1-31 reserved
+                                      *   bit0 = 0, if not a pci dev
+                                      *   bit0 = 1, don't suppress PNP or
+                                      *             power management
+                                      */
+    uint8_t  pci_seg;                /* PCI segment number, 0 for systems
+                                        with less than 255 PCI busses */
+    uint32_t reserved3;              /* Reserved, must be 0 */
+} QEMU_PACKED;
+typedef struct AcpiSerialPortConsoleRedirection AcpiSerialPortConsoleRedirection;
+
+/*
  * ACPI 1.0 Root System Description Table (RSDT)
  */
 struct AcpiRsdtDescriptorRev1
-- 
1.8.3.1

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

[Shannon Zhao]

Hi Andrew,

Some minor comments below. Use scripts/checkpatch.pl before submitting.

On 2015/6/8 22:00, 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,
> +};
> +
> +struct AcpiSerialPortConsoleRedirection
> +{

open brace '{' following struct go on the same line

> +    ACPI_TABLE_HEADER_DEF            /* ACPI common table header */
> +    uint8_t  interface_type;         /* See ACPI_SERIAL_ enums */
> +    uint8_t  reserved1[3];           /* Reserved, must be 0 */
> +    struct AcpiGenericAddress base_address;
> +    uint8_t  interrupt_types;        /* Interrupt type support mask,
> +                                      * see ACPI_SERIAL_ITYPE_ enums */
> +    uint8_t  irq;                    /* PC irq used. 0 if ACPI_SERIAL_ITYPE_PC
> +                                      * is not set, else must be one of
> +                                      * 2-7,9-12,14-15 */
> +    uint32_t gsi;                    /* Not valid if only ACPI_SERIAL_ITYPE_PC
> +                                      * is set in the interrupt_types mask */
> +    uint8_t  baud;                   /* See ACPI_SERIAL_BAUD_ enums */
> +    uint8_t  parity;                 /* 0 = no parity, 1-255 reserved */
> +    uint8_t  stopbits;               /* Must be 1 */
> +    uint8_t  flowctrl;               /* See ACPI_SERIAL_FLOW_ enums */
> +    uint8_t  term_type;              /* See ACPI_SERIAL_TERM_ enums */
> +    uint8_t  reserved2;              /* Reserved, must be 0 */
> +    uint16_t pci_device_id;          /* 0xffff, if not a pci dev */
> +    uint16_t pci_vendor_id;          /* 0xffff, if not a pci dev */
> +    uint8_t  pci_bus;                /* PCI bus number, 0 if not a pci dev */
> +    uint8_t  pci_slot;               /* PCI slot number, 0 if not a pci dev */
> +    uint8_t  pci_func;               /* PCI function num, 0 if not a pci dev */
> +    uint32_t pci_flags;              /* PCI compatibility mask,
> +                                      *   bits 1-31 reserved
> +                                      *   bit0 = 0, if not a pci dev
> +                                      *   bit0 = 1, don't suppress PNP or
> +                                      *             power management
> +                                      */
> +    uint8_t  pci_seg;                /* PCI segment number, 0 for systems
> +                                        with less than 255 PCI busses */
> +    uint32_t reserved3;              /* Reserved, must be 0 */
> +} QEMU_PACKED;
> +typedef struct AcpiSerialPortConsoleRedirection AcpiSerialPortConsoleRedirection;

line over 80 characters

> +
> +/*
>   * ACPI 1.0 Root System Description Table (RSDT)
>   */
>  struct AcpiRsdtDescriptorRev1
> 

-- 
Shannon

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

[Michael S. Tsirkin]

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.
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.

> +struct AcpiSerialPortConsoleRedirection
> +{
> +    ACPI_TABLE_HEADER_DEF            /* ACPI common table header */
> +    uint8_t  interface_type;         /* See ACPI_SERIAL_ enums */
> +    uint8_t  reserved1[3];           /* Reserved, must be 0 */
> +    struct AcpiGenericAddress base_address;
> +    uint8_t  interrupt_types;        /* Interrupt type support mask,
> +                                      * see ACPI_SERIAL_ITYPE_ enums */
> +    uint8_t  irq;                    /* PC irq used. 0 if ACPI_SERIAL_ITYPE_PC
> +                                      * is not set, else must be one of
> +                                      * 2-7,9-12,14-15 */
> +    uint32_t gsi;                    /* Not valid if only ACPI_SERIAL_ITYPE_PC
> +                                      * is set in the interrupt_types mask */
> +    uint8_t  baud;                   /* See ACPI_SERIAL_BAUD_ enums */
> +    uint8_t  parity;                 /* 0 = no parity, 1-255 reserved */
> +    uint8_t  stopbits;               /* Must be 1 */
> +    uint8_t  flowctrl;               /* See ACPI_SERIAL_FLOW_ enums */
> +    uint8_t  term_type;              /* See ACPI_SERIAL_TERM_ enums */
> +    uint8_t  reserved2;              /* Reserved, must be 0 */
> +    uint16_t pci_device_id;          /* 0xffff, if not a pci dev */
> +    uint16_t pci_vendor_id;          /* 0xffff, if not a pci dev */
> +    uint8_t  pci_bus;                /* PCI bus number, 0 if not a pci dev */
> +    uint8_t  pci_slot;               /* PCI slot number, 0 if not a pci dev */
> +    uint8_t  pci_func;               /* PCI function num, 0 if not a pci dev */
> +    uint32_t pci_flags;              /* PCI compatibility mask,
> +                                      *   bits 1-31 reserved
> +                                      *   bit0 = 0, if not a pci dev
> +                                      *   bit0 = 1, don't suppress PNP or
> +                                      *             power management
> +                                      */
> +    uint8_t  pci_seg;                /* PCI segment number, 0 for systems
> +                                        with less than 255 PCI busses */
> +    uint32_t reserved3;              /* Reserved, must be 0 */
> +} QEMU_PACKED;
> +typedef struct AcpiSerialPortConsoleRedirection AcpiSerialPortConsoleRedirection;
> +
> +/*
>   * ACPI 1.0 Root System Description Table (RSDT)
>   */
>  struct AcpiRsdtDescriptorRev1
> -- 
> 1.8.3.1

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

[Andrew Jones]

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. 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 (and struct documentation?).

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? In both cases, I guess the actual value is
what would be compared on lookup.

> 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)
 */

assuming I understood your comment correctly.

Thanks for the review,
drew

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

[Michael S. Tsirkin]

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

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

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt-acpi-build.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..192bbecf6e573 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,13 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
                aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
                              AML_EXCLUSIVE, uart_irq));
     aml_append(dev, aml_name_decl("_CRS", crs));
+
+    /*
+     * The _ADR entry is used to link this device to the UART described
+     * in the SPCR table.
+     */
+    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
+
     aml_append(scope, dev);
 }
 
@@ -334,6 +341,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
 }
 
 static void
+build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
+{
+    AcpiSerialPortConsoleRedirection *spcr;
+    const MemMapEntry *memmap = &guest_info->memmap[VIRT_UART];
+    int irq = guest_info->irqmap[VIRT_UART] + ARM_SPI_BASE;
+
+    spcr = acpi_data_push(table_data, sizeof(*spcr));
+
+    spcr->interface_type = ACPI_SERIAL_ARM_PL011_UART;
+
+    spcr->base_address.space_id = 0;      /* System Memory */
+    spcr->base_address.bit_width = 8;
+    spcr->base_address.bit_offset = 0;
+    spcr->base_address.access_width = 1;
+    spcr->base_address.address = cpu_to_le64(memmap->base);
+
+    spcr->interrupt_types = ACPI_SERIAL_ITYPE_ARMH_GIC;
+    spcr->gsi = cpu_to_le32(irq);
+
+    spcr->baud = ACPI_SERIAL_BAUD_9600;
+    spcr->parity = 0;
+    spcr->stopbits = 1;
+    spcr->flowctrl = ACPI_SERIAL_FLOW_HW;
+    spcr->term_type = ACPI_SERIAL_TERM_VT100;
+
+    spcr->pci_device_id = 0xffff;
+    spcr->pci_vendor_id = 0xffff;
+
+    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 5);
+}
+
+static void
 build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
 {
     AcpiTableMcfg *mcfg;
@@ -514,7 +553,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 pointed to by RSDT */
+    /* FADT MADT GTDT SPCR pointed to by RSDT */
     acpi_add_table(table_offsets, tables_blob);
     build_fadt(tables_blob, tables->linker, dsdt);
 
@@ -527,6 +566,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
     acpi_add_table(table_offsets, tables_blob);
     build_mcfg(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 */
     rsdt = tables_blob->len;
     build_rsdt(tables_blob, tables->linker, table_offsets);
-- 
1.8.3.1

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

[Shannon Zhao]

On 2015/6/8 22:00, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a9373ccaca6cb..192bbecf6e573 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,13 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>                 aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
>                               AML_EXCLUSIVE, uart_irq));
>      aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +    /*
> +     * The _ADR entry is used to link this device to the UART described
> +     * in the SPCR table.
> +     */
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> +
>      aml_append(scope, dev);
>  }
>  
> @@ -334,6 +341,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>  }
>  
>  static void
> +build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> +    AcpiSerialPortConsoleRedirection *spcr;
> +    const MemMapEntry *memmap = &guest_info->memmap[VIRT_UART];
> +    int irq = guest_info->irqmap[VIRT_UART] + ARM_SPI_BASE;
> +
> +    spcr = acpi_data_push(table_data, sizeof(*spcr));
> +
> +    spcr->interface_type = ACPI_SERIAL_ARM_PL011_UART;
> +
> +    spcr->base_address.space_id = 0;      /* System Memory */
> +    spcr->base_address.bit_width = 8;
> +    spcr->base_address.bit_offset = 0;
> +    spcr->base_address.access_width = 1;
> +    spcr->base_address.address = cpu_to_le64(memmap->base);
> +
> +    spcr->interrupt_types = ACPI_SERIAL_ITYPE_ARMH_GIC;
> +    spcr->gsi = cpu_to_le32(irq);
> +
> +    spcr->baud = ACPI_SERIAL_BAUD_9600;
> +    spcr->parity = 0;
> +    spcr->stopbits = 1;
> +    spcr->flowctrl = ACPI_SERIAL_FLOW_HW;
> +    spcr->term_type = ACPI_SERIAL_TERM_VT100;
> +
> +    spcr->pci_device_id = 0xffff;
> +    spcr->pci_vendor_id = 0xffff;
> +
> +    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 5);

Look at the SPCR table spec, it says the revision is 2.

> +}
> +
> +static void
>  build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      AcpiTableMcfg *mcfg;
> @@ -514,7 +553,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 pointed to by RSDT */
> +    /* FADT MADT GTDT SPCR pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt(tables_blob, tables->linker, dsdt);
>  
> @@ -527,6 +566,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_mcfg(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 */
>      rsdt = tables_blob->len;
>      build_rsdt(tables_blob, tables->linker, table_offsets);
> 

-- 
Shannon

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

[Andrew Jones]

Hi Shannon,

Thanks for the review.

On Tue, Jun 09, 2015 at 10:54:44AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/6/8 22:00, Andrew Jones wrote:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 43 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index a9373ccaca6cb..192bbecf6e573 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -84,6 +84,13 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
> >                 aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
> >                               AML_EXCLUSIVE, uart_irq));
> >      aml_append(dev, aml_name_decl("_CRS", crs));
> > +
> > +    /*
> > +     * The _ADR entry is used to link this device to the UART described
> > +     * in the SPCR table.
> > +     */

(oh, I should QEMU-stylify this comment)

> > +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > +
> >      aml_append(scope, dev);
> >  }
> >  
> > @@ -334,6 +341,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
> >  }
> >  
> >  static void
> > +build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> > +{
> > +    AcpiSerialPortConsoleRedirection *spcr;
> > +    const MemMapEntry *memmap = &guest_info->memmap[VIRT_UART];
> > +    int irq = guest_info->irqmap[VIRT_UART] + ARM_SPI_BASE;
> > +
> > +    spcr = acpi_data_push(table_data, sizeof(*spcr));
> > +
> > +    spcr->interface_type = ACPI_SERIAL_ARM_PL011_UART;
> > +
> > +    spcr->base_address.space_id = 0;      /* System Memory */
> > +    spcr->base_address.bit_width = 8;
> > +    spcr->base_address.bit_offset = 0;
> > +    spcr->base_address.access_width = 1;
> > +    spcr->base_address.address = cpu_to_le64(memmap->base);
> > +
> > +    spcr->interrupt_types = ACPI_SERIAL_ITYPE_ARMH_GIC;
> > +    spcr->gsi = cpu_to_le32(irq);
> > +
> > +    spcr->baud = ACPI_SERIAL_BAUD_9600;
> > +    spcr->parity = 0;
> > +    spcr->stopbits = 1;
> > +    spcr->flowctrl = ACPI_SERIAL_FLOW_HW;
> > +    spcr->term_type = ACPI_SERIAL_TERM_VT100;
> > +
> > +    spcr->pci_device_id = 0xffff;
> > +    spcr->pci_vendor_id = 0xffff;
> > +
> > +    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 5);
> 
> Look at the SPCR table spec, it says the revision is 2.

grr... forgot to change that after copy+paste.

> 
> > +}
> > +
> > +static void
> >  build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >  {
> >      AcpiTableMcfg *mcfg;
> > @@ -514,7 +553,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 pointed to by RSDT */
> > +    /* FADT MADT GTDT SPCR pointed to by RSDT */
> >      acpi_add_table(table_offsets, tables_blob);
> >      build_fadt(tables_blob, tables->linker, dsdt);
> >  
> > @@ -527,6 +566,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> >      acpi_add_table(table_offsets, tables_blob);
> >      build_mcfg(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 */
> >      rsdt = tables_blob->len;
> >      build_rsdt(tables_blob, tables->linker, table_offsets);
> > 
> 
> -- 
> Shannon
> 
>

Any more comments on the series (from anyone) before I send a v2?

drew 

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

[Shannon Zhao]

On 2015/6/8 22:00, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a9373ccaca6cb..192bbecf6e573 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,13 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>                 aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
>                               AML_EXCLUSIVE, uart_irq));
>      aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +    /*
> +     * The _ADR entry is used to link this device to the UART described
> +     * in the SPCR table.
> +     */
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> +
>      aml_append(scope, dev);
>  }
>  
> @@ -334,6 +341,38 @@ build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
>  }
>  
>  static void
> +build_spcr(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> +{
> +    AcpiSerialPortConsoleRedirection *spcr;
> +    const MemMapEntry *memmap = &guest_info->memmap[VIRT_UART];
> +    int irq = guest_info->irqmap[VIRT_UART] + ARM_SPI_BASE;
> +
> +    spcr = acpi_data_push(table_data, sizeof(*spcr));
> +
> +    spcr->interface_type = ACPI_SERIAL_ARM_PL011_UART;
> +
> +    spcr->base_address.space_id = 0;      /* System Memory */

use enum AML_SYSTEM_MEMORY for space_id.

> +    spcr->base_address.bit_width = 8;
> +    spcr->base_address.bit_offset = 0;
> +    spcr->base_address.access_width = 1;
> +    spcr->base_address.address = cpu_to_le64(memmap->base);
> +
> +    spcr->interrupt_types = ACPI_SERIAL_ITYPE_ARMH_GIC;
> +    spcr->gsi = cpu_to_le32(irq);
> +
> +    spcr->baud = ACPI_SERIAL_BAUD_9600;
> +    spcr->parity = 0;
> +    spcr->stopbits = 1;
> +    spcr->flowctrl = ACPI_SERIAL_FLOW_HW;
> +    spcr->term_type = ACPI_SERIAL_TERM_VT100;
> +
> +    spcr->pci_device_id = 0xffff;
> +    spcr->pci_vendor_id = 0xffff;
> +
> +    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 5);
> +}
> +
> +static void
>  build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      AcpiTableMcfg *mcfg;
> @@ -514,7 +553,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 pointed to by RSDT */
> +    /* FADT MADT GTDT SPCR pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt(tables_blob, tables->linker, dsdt);
>  
> @@ -527,6 +566,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_mcfg(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 */
>      rsdt = tables_blob->len;
>      build_rsdt(tables_blob, tables->linker, table_offsets);
> 

-- 
Shannon

Re: [Qemu-devel] [PATCH 0/2] ACPI: ARM: add SPCR table

[Shannon Zhao]

On 2015/6/8 22:00, Andrew Jones wrote:
> Adding this table allows the guest to boot without the console=
> parameter added to the kernel command line. And anyway, kernel doc
> Documentation/arm64/acpi_object_usage.txt says it's a required
> table for arm64.
> 
> Andrew Jones (2):
>   ACPI: Add definitions for the SPCR table
>   hw/arm/virt-acpi-build: Add SPCR table
> 
Test this patchset on ARM without appending "console=".

Tested-by: Shannon Zhao <shannon.zhao@linaro.org>

>  hw/arm/virt-acpi-build.c    | 44 ++++++++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+), 1 deletion(-)
> 

-- 
Shannon