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

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

v3:
- no enums, document struct and value in build_spcr [mst]
- document earliest spec version for table [mst]
- clarify _ADR comment [Igor]
v2:
- checkpatch style fixes [Shannon]
- table is revision 2, not 5 [Shannon]
- use AML_SYSTEM_MEMORY [Shannon]
- tested-by Shannon

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

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

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 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>
Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 include/hw/acpi/acpi-defs.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 59cf277434b37..7b4bfb7494717 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -197,6 +197,38 @@ enum {
 };
 
 /*
+ * Serial Port Console Redirection Table (SPCR), Rev. 1.02
+ *
+ * For .interface_type see Debug Port Table 2 (DBG2) serial port
+ * subtypes in Table 3, Rev. May 22, 2012
+ */
+struct AcpiSerialPortConsoleRedirection {
+    ACPI_TABLE_HEADER_DEF
+    uint8_t  interface_type;
+    uint8_t  reserved1[3];
+    struct AcpiGenericAddress base_address;
+    uint8_t  interrupt_types;
+    uint8_t  irq;
+    uint32_t gsi;
+    uint8_t  baud;
+    uint8_t  parity;
+    uint8_t  stopbits;
+    uint8_t  flowctrl;
+    uint8_t  term_type;
+    uint8_t  reserved2;
+    uint16_t pci_device_id;
+    uint16_t pci_vendor_id;
+    uint8_t  pci_bus;
+    uint8_t  pci_slot;
+    uint8_t  pci_func;
+    uint32_t pci_flags;
+    uint8_t  pci_seg;
+    uint32_t reserved3;
+} QEMU_PACKED;
+typedef struct AcpiSerialPortConsoleRedirection
+               AcpiSerialPortConsoleRedirection;
+
+/*
  * ACPI 1.0 Root System Description Table (RSDT)
  */
 struct AcpiRsdtDescriptorRev1
-- 
1.8.3.1

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

[Michael S. Tsirkin]

On Wed, Jun 10, 2015 at 05:52:38AM -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>
> Tested-by: Shannon Zhao <shannon.zhao@linaro.org>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  include/hw/acpi/acpi-defs.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 59cf277434b37..7b4bfb7494717 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -197,6 +197,38 @@ enum {
>  };
>  
>  /*
> + * Serial Port Console Redirection Table (SPCR), Rev. 1.02
> + *
> + * For .interface_type see Debug Port Table 2 (DBG2) serial port
> + * subtypes in Table 3, Rev. May 22, 2012
> + */
> +struct AcpiSerialPortConsoleRedirection {
> +    ACPI_TABLE_HEADER_DEF
> +    uint8_t  interface_type;
> +    uint8_t  reserved1[3];
> +    struct AcpiGenericAddress base_address;
> +    uint8_t  interrupt_types;
> +    uint8_t  irq;
> +    uint32_t gsi;
> +    uint8_t  baud;
> +    uint8_t  parity;
> +    uint8_t  stopbits;
> +    uint8_t  flowctrl;
> +    uint8_t  term_type;
> +    uint8_t  reserved2;
> +    uint16_t pci_device_id;
> +    uint16_t pci_vendor_id;
> +    uint8_t  pci_bus;
> +    uint8_t  pci_slot;
> +    uint8_t  pci_func;
> +    uint32_t pci_flags;
> +    uint8_t  pci_seg;
> +    uint32_t reserved3;
> +} QEMU_PACKED;
> +typedef struct AcpiSerialPortConsoleRedirection
> +               AcpiSerialPortConsoleRedirection;
> +
> +/*
>   * ACPI 1.0 Root System Description Table (RSDT)
>   */
>  struct AcpiRsdtDescriptorRev1
> -- 
> 1.8.3.1

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

[Igor Mammedov]

On Wed, 10 Jun 2015 05:52:38 -0400
Andrew Jones <drjones@redhat.com> 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>
> Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/hw/acpi/acpi-defs.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 59cf277434b37..7b4bfb7494717 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -197,6 +197,38 @@ enum {
>  };
>  
>  /*
> + * Serial Port Console Redirection Table (SPCR), Rev. 1.02
> + *
> + * For .interface_type see Debug Port Table 2 (DBG2) serial port
> + * subtypes in Table 3, Rev. May 22, 2012
> + */
> +struct AcpiSerialPortConsoleRedirection {
> +    ACPI_TABLE_HEADER_DEF
> +    uint8_t  interface_type;
> +    uint8_t  reserved1[3];
> +    struct AcpiGenericAddress base_address;
> +    uint8_t  interrupt_types;
> +    uint8_t  irq;
> +    uint32_t gsi;
> +    uint8_t  baud;
> +    uint8_t  parity;
> +    uint8_t  stopbits;
> +    uint8_t  flowctrl;
> +    uint8_t  term_type;
> +    uint8_t  reserved2;
> +    uint16_t pci_device_id;
> +    uint16_t pci_vendor_id;
> +    uint8_t  pci_bus;
> +    uint8_t  pci_slot;
> +    uint8_t  pci_func;
> +    uint32_t pci_flags;
> +    uint8_t  pci_seg;
> +    uint32_t reserved3;
> +} QEMU_PACKED;
> +typedef struct AcpiSerialPortConsoleRedirection
> +               AcpiSerialPortConsoleRedirection;
> +
> +/*
>   * ACPI 1.0 Root System Description Table (RSDT)
>   */
>  struct AcpiRsdtDescriptorRev1

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

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/arm/virt-acpi-build.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a9373ccaca6cb..d5a8b9c0178ea 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -84,6 +84,12 @@ 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, i.e. SPCR.base_address.address == _ADR.
+     */
+    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
+
     aml_append(scope, dev);
 }
 
@@ -334,6 +340,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 *uart_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 = 0x3;    /* ARM PL011 UART */
+
+    spcr->base_address.space_id = AML_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(uart_memmap->base);
+
+    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
+    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */
+
+    spcr->baud = 3;                /* Baud Rate: 3 = 9600 */
+    spcr->parity = 0;              /* No Parity */
+    spcr->stopbits = 1;            /* 1 Stop bit */
+    spcr->flowctrl = (1 << 1);     /* Bit[1] = RTS/CTS hardware flow control */
+    spcr->term_type = 0;           /* Terminal Type: 0 = VT100 */
+
+    spcr->pci_device_id = 0xffff;  /* PCI Device ID: not a PCI device */
+    spcr->pci_vendor_id = 0xffff;  /* PCI Vendor ID: not a PCI device */
+
+    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 2);
+}
+
+static void
 build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
 {
     AcpiTableMcfg *mcfg;
@@ -514,7 +552,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 +565,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 v3 2/2] hw/arm/virt-acpi-build: Add SPCR table

[Michael S. Tsirkin]

On Wed, Jun 10, 2015 at 05:52:39AM -0400, Andrew Jones wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Tested-by: Shannon Zhao <shannon.zhao@linaro.org>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a9373ccaca6cb..d5a8b9c0178ea 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,12 @@ 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, i.e. SPCR.base_address.address == _ADR.
> +     */
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> +
>      aml_append(scope, dev);
>  }
>  
> @@ -334,6 +340,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 *uart_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 = 0x3;    /* ARM PL011 UART */
> +
> +    spcr->base_address.space_id = AML_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(uart_memmap->base);
> +
> +    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
> +    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */
> +
> +    spcr->baud = 3;                /* Baud Rate: 3 = 9600 */
> +    spcr->parity = 0;              /* No Parity */
> +    spcr->stopbits = 1;            /* 1 Stop bit */
> +    spcr->flowctrl = (1 << 1);     /* Bit[1] = RTS/CTS hardware flow control */
> +    spcr->term_type = 0;           /* Terminal Type: 0 = VT100 */
> +
> +    spcr->pci_device_id = 0xffff;  /* PCI Device ID: not a PCI device */
> +    spcr->pci_vendor_id = 0xffff;  /* PCI Vendor ID: not a PCI device */
> +
> +    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 2);
> +}
> +
> +static void
>  build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      AcpiTableMcfg *mcfg;
> @@ -514,7 +552,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 +565,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 v3 2/2] hw/arm/virt-acpi-build: Add SPCR table

[Igor Mammedov]

On Wed, 10 Jun 2015 05:52:39 -0400
Andrew Jones <drjones@redhat.com> wrote:

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/arm/virt-acpi-build.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a9373ccaca6cb..d5a8b9c0178ea 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,12 @@ 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, i.e. SPCR.base_address.address == _ADR.
> +     */
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> +
>      aml_append(scope, dev);
>  }
>  
> @@ -334,6 +340,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 *uart_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 = 0x3;    /* ARM PL011 UART */
> +
> +    spcr->base_address.space_id = AML_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(uart_memmap->base);
> +
> +    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
> +    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */
> +
> +    spcr->baud = 3;                /* Baud Rate: 3 = 9600 */
> +    spcr->parity = 0;              /* No Parity */
> +    spcr->stopbits = 1;            /* 1 Stop bit */
> +    spcr->flowctrl = (1 << 1);     /* Bit[1] = RTS/CTS hardware flow control */
> +    spcr->term_type = 0;           /* Terminal Type: 0 = VT100 */
> +
> +    spcr->pci_device_id = 0xffff;  /* PCI Device ID: not a PCI device */
> +    spcr->pci_vendor_id = 0xffff;  /* PCI Vendor ID: not a PCI device */
> +
> +    build_header(linker, table_data, (void *)spcr, "SPCR", sizeof(*spcr), 2);
> +}
> +
> +static void
>  build_mcfg(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>  {
>      AcpiTableMcfg *mcfg;
> @@ -514,7 +552,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 +565,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);

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

[Peter Maydell]

On 10 June 2015 at 10:52, Andrew Jones <drjones@redhat.com> wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/arm/virt-acpi-build.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index a9373ccaca6cb..d5a8b9c0178ea 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -84,6 +84,12 @@ 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, i.e. SPCR.base_address.address == _ADR.
> +     */
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> +
>      aml_append(scope, dev);
>  }
>
> @@ -334,6 +340,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 *uart_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 = 0x3;    /* ARM PL011 UART */
> +
> +    spcr->base_address.space_id = AML_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(uart_memmap->base);
> +
> +    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
> +    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */

I'm still confused about when fields in these ACPI structs
need to be converted to little-endian, and when they don't.
Is there a rule-of-thumb I can use when I'm looking at patches?

thanks
-- PMM

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

[Michael S. Tsirkin]

On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> On 10 June 2015 at 10:52, Andrew Jones <drjones@redhat.com> wrote:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
> > ---
> >  hw/arm/virt-acpi-build.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index a9373ccaca6cb..d5a8b9c0178ea 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -84,6 +84,12 @@ 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, i.e. SPCR.base_address.address == _ADR.
> > +     */
> > +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > +
> >      aml_append(scope, dev);
> >  }
> >
> > @@ -334,6 +340,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 *uart_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 = 0x3;    /* ARM PL011 UART */
> > +
> > +    spcr->base_address.space_id = AML_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(uart_memmap->base);
> > +
> > +    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
> > +    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */
> 
> I'm still confused about when fields in these ACPI structs
> need to be converted to little-endian, and when they don't.
> Is there a rule-of-thumb I can use when I'm looking at patches?
> 
> thanks
> -- PMM

Normally it's all LE unless it's a single byte value.
Did not check this specific table.
We really need to add sparse support to check
endian-ness matches, or re-write it
all using byte_add so there's no duplication of info.


-- 
MST

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

[Andrew Jones]

On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> > On 10 June 2015 at 10:52, Andrew Jones <drjones@redhat.com> wrote:
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > Tested-by: Shannon Zhao <shannon.zhao@linaro.org>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 42 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index a9373ccaca6cb..d5a8b9c0178ea 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -84,6 +84,12 @@ 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, i.e. SPCR.base_address.address == _ADR.
> > > +     */
> > > +    aml_append(dev, aml_name_decl("_ADR", aml_int(uart_memmap->base)));
> > > +
> > >      aml_append(scope, dev);
> > >  }
> > >
> > > @@ -334,6 +340,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 *uart_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 = 0x3;    /* ARM PL011 UART */
> > > +
> > > +    spcr->base_address.space_id = AML_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(uart_memmap->base);
> > > +
> > > +    spcr->interrupt_types = (1 << 3); /* Bit[3] ARMH GIC interrupt */
> > > +    spcr->gsi = cpu_to_le32(irq);  /* Global System Interrupt */
> > 
> > I'm still confused about when fields in these ACPI structs
> > need to be converted to little-endian, and when they don't.
> > Is there a rule-of-thumb I can use when I'm looking at patches?
> > 
> > thanks
> > -- PMM
> 
> Normally it's all LE unless it's a single byte value.
> Did not check this specific table.
> We really need to add sparse support to check
> endian-ness matches, or re-write it
> all using byte_add so there's no duplication of info.
>

Everything used in the table is either a single byte, or I used le32,
Well, I didn't bother for the pci_{device,vendor}_id assignments, as
they're 0xffff anyway. I can change those two to make them more explicit,
if that's preferred.

Thanks,
drew

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

[Peter Maydell]

On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
>> > I'm still confused about when fields in these ACPI structs
>> > need to be converted to little-endian, and when they don't.
>> > Is there a rule-of-thumb I can use when I'm looking at patches?

>> Normally it's all LE unless it's a single byte value.
>> Did not check this specific table.
>> We really need to add sparse support to check
>> endian-ness matches, or re-write it
>> all using byte_add so there's no duplication of info.

> Everything used in the table is either a single byte, or I used le32,
> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> they're 0xffff anyway. I can change those two to make them more explicit,
> if that's preferred.

Yep, I just looked over the struct definition, so since this
has been reviewed I'll apply it to target-arm.next.

You could probably make it easier to review and write
code that has to do these endianness swaps with something
like

#define acpi_struct_assign(FIELD, VAL) \
  ((FIELD) = \
  __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
  __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
  __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
  __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
  abort))))

(untested, but based on some code in linux-user/qemu.h).

Then it's always

    acpi_struct_assign(spcr->field, value);

whether the field is 1, 2, 4 or 8 bytes.

Not my bit of the codebase though, so I'll leave it to the
ACPI maintainers to decide how much they like magic macros :-)

thanks
-- PMM

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

[Michael S. Tsirkin]

On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> > On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> >> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> >> > I'm still confused about when fields in these ACPI structs
> >> > need to be converted to little-endian, and when they don't.
> >> > Is there a rule-of-thumb I can use when I'm looking at patches?
> 
> >> Normally it's all LE unless it's a single byte value.
> >> Did not check this specific table.
> >> We really need to add sparse support to check
> >> endian-ness matches, or re-write it
> >> all using byte_add so there's no duplication of info.
> 
> > Everything used in the table is either a single byte, or I used le32,
> > Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> > they're 0xffff anyway. I can change those two to make them more explicit,
> > if that's preferred.
> 
> Yep, I just looked over the struct definition, so since this
> has been reviewed I'll apply it to target-arm.next.
> 
> You could probably make it easier to review and write
> code that has to do these endianness swaps with something
> like
> 
> #define acpi_struct_assign(FIELD, VAL) \
>   ((FIELD) = \
>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
>   abort))))
> 
> (untested, but based on some code in linux-user/qemu.h).
> 
> Then it's always
> 
>     acpi_struct_assign(spcr->field, value);
> 
> whether the field is 1, 2, 4 or 8 bytes.
> 
> Not my bit of the codebase though, so I'll leave it to the
> ACPI maintainers to decide how much they like magic macros :-)
> 
> thanks
> -- PMM


We don't much. One can use build_append_int_noprefix and just avoid
structs altogether.
We did this for some structures and I'm thinking it's a good direction
generally.

-- 
MST

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

[Shannon Zhao]

On 2015/6/16 2:13, Michael S. Tsirkin wrote:
> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
>>>>> I'm still confused about when fields in these ACPI structs
>>>>> need to be converted to little-endian, and when they don't.
>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
>>
>>>> Normally it's all LE unless it's a single byte value.
>>>> Did not check this specific table.
>>>> We really need to add sparse support to check
>>>> endian-ness matches, or re-write it
>>>> all using byte_add so there's no duplication of info.
>>
>>> Everything used in the table is either a single byte, or I used le32,
>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
>>> they're 0xffff anyway. I can change those two to make them more explicit,
>>> if that's preferred.
>>
>> Yep, I just looked over the struct definition, so since this
>> has been reviewed I'll apply it to target-arm.next.
>>
>> You could probably make it easier to review and write
>> code that has to do these endianness swaps with something
>> like
>>
>> #define acpi_struct_assign(FIELD, VAL) \
>>   ((FIELD) = \
>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
>>   abort))))
>>
>> (untested, but based on some code in linux-user/qemu.h).
>>
>> Then it's always
>>
>>     acpi_struct_assign(spcr->field, value);
>>
>> whether the field is 1, 2, 4 or 8 bytes.
>>
>> Not my bit of the codebase though, so I'll leave it to the
>> ACPI maintainers to decide how much they like magic macros :-)
>>
>> thanks
>> -- PMM
> 
> 
> We don't much. One can use build_append_int_noprefix and just avoid
> structs altogether.

But if we use build_append_int_noprefix, we have to bother about the
unused fields of the struct and have lots of
build_append_int_noprefix(table, 0, 1/2/4/8).

> We did this for some structures and I'm thinking it's a good direction
> generally.
> 

-- 
Shannon

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

[Igor Mammedov]

On Tue, 16 Jun 2015 09:33:19 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> 
> 
> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
> > On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
> >> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> >>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> >>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> >>>>> I'm still confused about when fields in these ACPI structs
> >>>>> need to be converted to little-endian, and when they don't.
> >>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
> >>
> >>>> Normally it's all LE unless it's a single byte value.
> >>>> Did not check this specific table.
> >>>> We really need to add sparse support to check
> >>>> endian-ness matches, or re-write it
> >>>> all using byte_add so there's no duplication of info.
> >>
> >>> Everything used in the table is either a single byte, or I used le32,
> >>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> >>> they're 0xffff anyway. I can change those two to make them more explicit,
> >>> if that's preferred.
> >>
> >> Yep, I just looked over the struct definition, so since this
> >> has been reviewed I'll apply it to target-arm.next.
> >>
> >> You could probably make it easier to review and write
> >> code that has to do these endianness swaps with something
> >> like
> >>
> >> #define acpi_struct_assign(FIELD, VAL) \
> >>   ((FIELD) = \
> >>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
> >>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
> >>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
> >>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
> >>   abort))))
> >>
> >> (untested, but based on some code in linux-user/qemu.h).
> >>
> >> Then it's always
> >>
> >>     acpi_struct_assign(spcr->field, value);
> >>
> >> whether the field is 1, 2, 4 or 8 bytes.
> >>
> >> Not my bit of the codebase though, so I'll leave it to the
> >> ACPI maintainers to decide how much they like magic macros :-)
> >>
> >> thanks
> >> -- PMM
> > 
> > 
> > We don't much. One can use build_append_int_noprefix and just avoid
> > structs altogether.
> 
> But if we use build_append_int_noprefix, we have to bother about the
> unused fields of the struct and have lots of
> build_append_int_noprefix(table, 0, 1/2/4/8).
that would be drop in replacement for struct
(i.e. you'll just use build_append_int_noprefix instead of struct)

It's easier to review either since it repeats table descriptions
from spec practically 1:1 and there is no need to invent names for
struct fields anymore.

this approach is used in aml_build.c and so far works well.

> 
> > We did this for some structures and I'm thinking it's a good direction
> > generally.
> > 
> 

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

[Michael S. Tsirkin]

On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
> > On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
> >> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> >>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> >>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> >>>>> I'm still confused about when fields in these ACPI structs
> >>>>> need to be converted to little-endian, and when they don't.
> >>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
> >>
> >>>> Normally it's all LE unless it's a single byte value.
> >>>> Did not check this specific table.
> >>>> We really need to add sparse support to check
> >>>> endian-ness matches, or re-write it
> >>>> all using byte_add so there's no duplication of info.
> >>
> >>> Everything used in the table is either a single byte, or I used le32,
> >>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> >>> they're 0xffff anyway. I can change those two to make them more explicit,
> >>> if that's preferred.
> >>
> >> Yep, I just looked over the struct definition, so since this
> >> has been reviewed I'll apply it to target-arm.next.
> >>
> >> You could probably make it easier to review and write
> >> code that has to do these endianness swaps with something
> >> like
> >>
> >> #define acpi_struct_assign(FIELD, VAL) \
> >>   ((FIELD) = \
> >>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
> >>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
> >>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
> >>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
> >>   abort))))
> >>
> >> (untested, but based on some code in linux-user/qemu.h).
> >>
> >> Then it's always
> >>
> >>     acpi_struct_assign(spcr->field, value);
> >>
> >> whether the field is 1, 2, 4 or 8 bytes.
> >>
> >> Not my bit of the codebase though, so I'll leave it to the
> >> ACPI maintainers to decide how much they like magic macros :-)
> >>
> >> thanks
> >> -- PMM
> > 
> > 
> > We don't much. One can use build_append_int_noprefix and just avoid
> > structs altogether.
> 
> But if we use build_append_int_noprefix, we have to bother about the
> unused fields of the struct and have lots of
> build_append_int_noprefix(table, 0, 1/2/4/8).

With a struct you have a bunch of reserved fields - is that very
different?

> > We did this for some structures and I'm thinking it's a good direction
> > generally.
> > 
> 
> -- 
> Shannon

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

[Shannon Zhao]

On 2015/6/16 22:19, Michael S. Tsirkin wrote:
> On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
>>
>>
>> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
>>> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
>>>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
>>>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
>>>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
>>>>>>> I'm still confused about when fields in these ACPI structs
>>>>>>> need to be converted to little-endian, and when they don't.
>>>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
>>>>
>>>>>> Normally it's all LE unless it's a single byte value.
>>>>>> Did not check this specific table.
>>>>>> We really need to add sparse support to check
>>>>>> endian-ness matches, or re-write it
>>>>>> all using byte_add so there's no duplication of info.
>>>>
>>>>> Everything used in the table is either a single byte, or I used le32,
>>>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
>>>>> they're 0xffff anyway. I can change those two to make them more explicit,
>>>>> if that's preferred.
>>>>
>>>> Yep, I just looked over the struct definition, so since this
>>>> has been reviewed I'll apply it to target-arm.next.
>>>>
>>>> You could probably make it easier to review and write
>>>> code that has to do these endianness swaps with something
>>>> like
>>>>
>>>> #define acpi_struct_assign(FIELD, VAL) \
>>>>   ((FIELD) = \
>>>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
>>>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
>>>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
>>>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
>>>>   abort))))
>>>>
>>>> (untested, but based on some code in linux-user/qemu.h).
>>>>
>>>> Then it's always
>>>>
>>>>     acpi_struct_assign(spcr->field, value);
>>>>
>>>> whether the field is 1, 2, 4 or 8 bytes.
>>>>
>>>> Not my bit of the codebase though, so I'll leave it to the
>>>> ACPI maintainers to decide how much they like magic macros :-)
>>>>
>>>> thanks
>>>> -- PMM
>>>
>>>
>>> We don't much. One can use build_append_int_noprefix and just avoid
>>> structs altogether.
>>
>> But if we use build_append_int_noprefix, we have to bother about the
>> unused fields of the struct and have lots of
>> build_append_int_noprefix(table, 0, 1/2/4/8).
> 
> With a struct you have a bunch of reserved fields - is that very
> different?
> 

Not only about the reserved fields, but also the fields which ARM
doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
build_append_int_noprefix, we should add lots of
build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
just need to care them when we define it, rather than every time we use.

>>> We did this for some structures and I'm thinking it's a good direction
>>> generally.
>>>
>>
>> -- 
>> Shannon

-- 
Shannon

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

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/6/16 22:19, Michael S. Tsirkin wrote:
> > On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
> >>
> >>
> >> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
> >>> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
> >>>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> >>>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> >>>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> >>>>>>> I'm still confused about when fields in these ACPI structs
> >>>>>>> need to be converted to little-endian, and when they don't.
> >>>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
> >>>>
> >>>>>> Normally it's all LE unless it's a single byte value.
> >>>>>> Did not check this specific table.
> >>>>>> We really need to add sparse support to check
> >>>>>> endian-ness matches, or re-write it
> >>>>>> all using byte_add so there's no duplication of info.
> >>>>
> >>>>> Everything used in the table is either a single byte, or I used le32,
> >>>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> >>>>> they're 0xffff anyway. I can change those two to make them more explicit,
> >>>>> if that's preferred.
> >>>>
> >>>> Yep, I just looked over the struct definition, so since this
> >>>> has been reviewed I'll apply it to target-arm.next.
> >>>>
> >>>> You could probably make it easier to review and write
> >>>> code that has to do these endianness swaps with something
> >>>> like
> >>>>
> >>>> #define acpi_struct_assign(FIELD, VAL) \
> >>>>   ((FIELD) = \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
> >>>>   abort))))
> >>>>
> >>>> (untested, but based on some code in linux-user/qemu.h).
> >>>>
> >>>> Then it's always
> >>>>
> >>>>     acpi_struct_assign(spcr->field, value);
> >>>>
> >>>> whether the field is 1, 2, 4 or 8 bytes.
> >>>>
> >>>> Not my bit of the codebase though, so I'll leave it to the
> >>>> ACPI maintainers to decide how much they like magic macros :-)
> >>>>
> >>>> thanks
> >>>> -- PMM
> >>>
> >>>
> >>> We don't much. One can use build_append_int_noprefix and just avoid
> >>> structs altogether.
> >>
> >> But if we use build_append_int_noprefix, we have to bother about the
> >> unused fields of the struct and have lots of
> >> build_append_int_noprefix(table, 0, 1/2/4/8).
> > 
> > With a struct you have a bunch of reserved fields - is that very
> > different?
> > 
> 
> Not only about the reserved fields, but also the fields which ARM
> doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
> AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
> build_append_int_noprefix, we should add lots of
> build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
> just need to care them when we define it, rather than every time we use.

So the advice above assumes that you have a wrapper function
for building each struct. Then you would just pass 0
as parameters as appropriate.

But I am not claiming we need to switch all code away
from structs. If you like it like this, keep it around.


> >>> We did this for some structures and I'm thinking it's a good direction
> >>> generally.
> >>>
> >>
> >> -- 
> >> Shannon
> 
> -- 
> Shannon

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

[Andrew Jones]

On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/6/16 22:19, Michael S. Tsirkin wrote:
> > On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
> >>
> >>
> >> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
> >>> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
> >>>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> >>>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> >>>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> >>>>>>> I'm still confused about when fields in these ACPI structs
> >>>>>>> need to be converted to little-endian, and when they don't.
> >>>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
> >>>>
> >>>>>> Normally it's all LE unless it's a single byte value.
> >>>>>> Did not check this specific table.
> >>>>>> We really need to add sparse support to check
> >>>>>> endian-ness matches, or re-write it
> >>>>>> all using byte_add so there's no duplication of info.
> >>>>
> >>>>> Everything used in the table is either a single byte, or I used le32,
> >>>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> >>>>> they're 0xffff anyway. I can change those two to make them more explicit,
> >>>>> if that's preferred.
> >>>>
> >>>> Yep, I just looked over the struct definition, so since this
> >>>> has been reviewed I'll apply it to target-arm.next.
> >>>>
> >>>> You could probably make it easier to review and write
> >>>> code that has to do these endianness swaps with something
> >>>> like
> >>>>
> >>>> #define acpi_struct_assign(FIELD, VAL) \
> >>>>   ((FIELD) = \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
> >>>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
> >>>>   abort))))
> >>>>
> >>>> (untested, but based on some code in linux-user/qemu.h).
> >>>>
> >>>> Then it's always
> >>>>
> >>>>     acpi_struct_assign(spcr->field, value);
> >>>>
> >>>> whether the field is 1, 2, 4 or 8 bytes.
> >>>>
> >>>> Not my bit of the codebase though, so I'll leave it to the
> >>>> ACPI maintainers to decide how much they like magic macros :-)
> >>>>
> >>>> thanks
> >>>> -- PMM
> >>>
> >>>
> >>> We don't much. One can use build_append_int_noprefix and just avoid
> >>> structs altogether.
> >>
> >> But if we use build_append_int_noprefix, we have to bother about the
> >> unused fields of the struct and have lots of
> >> build_append_int_noprefix(table, 0, 1/2/4/8).
> > 
> > With a struct you have a bunch of reserved fields - is that very
> > different?
> > 
> 
> Not only about the reserved fields, but also the fields which ARM
> doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
> AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
> build_append_int_noprefix, we should add lots of
> build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
> just need to care them when we define it, rather than every time we use.

I've had a chat with Igor about this, and thought about it, and I'm now
in the build_append camp. It took me a while to see that point of view,
as it isn't a pattern I'm familiar with. However, the pattern, which is

* table generation code is a sequence of build_appends, commented with
  strings matching strings in the spec
* table generation code has the minimal number of parameters necessary
  to be useful for all users, all other table values have default values
  filled (typically zero)
* the parameters to the table generation code can be packed in a param
  struct that has descriptive member names, allowing the caller to
  still use the struct initializing type pattern, and to more cleanly
  manage the parameters

with the benefits of

* structs (the param structs) stay small
* callers don't have to worry about endianness
* maximum code sharing across users
* no need to try and reproduce the spec as a struct definition, which
  can easily explode with comments/enums trying to describe each field
  accurately

So, I think it would be nice to convert ARM's ACPI generation over to
this type of pattern, which may allow more merging of ARM and x86.
However, that said, it'd be good to get the endianness fix patch and
the gicv2m patch in sooner than later. Maybe we should start with those
patches (just using cpu_to_le*), and then consider a rework of the
struct based table generation, before continuing to extend it.

Thanks,
drew

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

[Shannon Zhao]

On 2015/6/17 17:42, Andrew Jones wrote:
> On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
>>
>>
>> On 2015/6/16 22:19, Michael S. Tsirkin wrote:
>>> On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
>>>>
>>>>
>>>> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
>>>>> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
>>>>>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
>>>>>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
>>>>>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
>>>>>>>>> I'm still confused about when fields in these ACPI structs
>>>>>>>>> need to be converted to little-endian, and when they don't.
>>>>>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
>>>>>>
>>>>>>>> Normally it's all LE unless it's a single byte value.
>>>>>>>> Did not check this specific table.
>>>>>>>> We really need to add sparse support to check
>>>>>>>> endian-ness matches, or re-write it
>>>>>>>> all using byte_add so there's no duplication of info.
>>>>>>
>>>>>>> Everything used in the table is either a single byte, or I used le32,
>>>>>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
>>>>>>> they're 0xffff anyway. I can change those two to make them more explicit,
>>>>>>> if that's preferred.
>>>>>>
>>>>>> Yep, I just looked over the struct definition, so since this
>>>>>> has been reviewed I'll apply it to target-arm.next.
>>>>>>
>>>>>> You could probably make it easier to review and write
>>>>>> code that has to do these endianness swaps with something
>>>>>> like
>>>>>>
>>>>>> #define acpi_struct_assign(FIELD, VAL) \
>>>>>>   ((FIELD) = \
>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
>>>>>>   abort))))
>>>>>>
>>>>>> (untested, but based on some code in linux-user/qemu.h).
>>>>>>
>>>>>> Then it's always
>>>>>>
>>>>>>     acpi_struct_assign(spcr->field, value);
>>>>>>
>>>>>> whether the field is 1, 2, 4 or 8 bytes.
>>>>>>
>>>>>> Not my bit of the codebase though, so I'll leave it to the
>>>>>> ACPI maintainers to decide how much they like magic macros :-)
>>>>>>
>>>>>> thanks
>>>>>> -- PMM
>>>>>
>>>>>
>>>>> We don't much. One can use build_append_int_noprefix and just avoid
>>>>> structs altogether.
>>>>
>>>> But if we use build_append_int_noprefix, we have to bother about the
>>>> unused fields of the struct and have lots of
>>>> build_append_int_noprefix(table, 0, 1/2/4/8).
>>>
>>> With a struct you have a bunch of reserved fields - is that very
>>> different?
>>>
>>
>> Not only about the reserved fields, but also the fields which ARM
>> doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
>> AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
>> build_append_int_noprefix, we should add lots of
>> build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
>> just need to care them when we define it, rather than every time we use.
> 
> I've had a chat with Igor about this, and thought about it, and I'm now
> in the build_append camp. It took me a while to see that point of view,
> as it isn't a pattern I'm familiar with. However, the pattern, which is
> 
> * table generation code is a sequence of build_appends, commented with
>   strings matching strings in the spec
> * table generation code has the minimal number of parameters necessary
>   to be useful for all users, all other table values have default values
>   filled (typically zero)
> * the parameters to the table generation code can be packed in a param
>   struct that has descriptive member names, allowing the caller to
>   still use the struct initializing type pattern, and to more cleanly
>   manage the parameters
> 
> with the benefits of
> 
> * structs (the param structs) stay small

But the table generation codes will be huge and have lots of meaningless
build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
Maybe the readability is poor.

> * callers don't have to worry about endianness
> * maximum code sharing across users
> * no need to try and reproduce the spec as a struct definition, which
>   can easily explode with comments/enums trying to describe each field
>   accurately
> 
> So, I think it would be nice to convert ARM's ACPI generation over to
> this type of pattern, which may allow more merging of ARM and x86.
> However, that said, it'd be good to get the endianness fix patch and
> the gicv2m patch in sooner than later. Maybe we should start with those
> patches (just using cpu_to_le*), and then consider a rework of the
> struct based table generation, before continuing to extend it.
> 
> Thanks,
> drew
> 
> 
> .
> 

-- 
Shannon

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

[Andrew Jones]

On Thu, Jun 18, 2015 at 06:28:36PM +0800, Shannon Zhao wrote:
> 
> 
> On 2015/6/17 17:42, Andrew Jones wrote:
> > On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
> >>
> >>
> >> On 2015/6/16 22:19, Michael S. Tsirkin wrote:
> >>> On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
> >>>>
> >>>>
> >>>> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
> >>>>> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
> >>>>>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
> >>>>>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
> >>>>>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
> >>>>>>>>> I'm still confused about when fields in these ACPI structs
> >>>>>>>>> need to be converted to little-endian, and when they don't.
> >>>>>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
> >>>>>>
> >>>>>>>> Normally it's all LE unless it's a single byte value.
> >>>>>>>> Did not check this specific table.
> >>>>>>>> We really need to add sparse support to check
> >>>>>>>> endian-ness matches, or re-write it
> >>>>>>>> all using byte_add so there's no duplication of info.
> >>>>>>
> >>>>>>> Everything used in the table is either a single byte, or I used le32,
> >>>>>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
> >>>>>>> they're 0xffff anyway. I can change those two to make them more explicit,
> >>>>>>> if that's preferred.
> >>>>>>
> >>>>>> Yep, I just looked over the struct definition, so since this
> >>>>>> has been reviewed I'll apply it to target-arm.next.
> >>>>>>
> >>>>>> You could probably make it easier to review and write
> >>>>>> code that has to do these endianness swaps with something
> >>>>>> like
> >>>>>>
> >>>>>> #define acpi_struct_assign(FIELD, VAL) \
> >>>>>>   ((FIELD) = \
> >>>>>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
> >>>>>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
> >>>>>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
> >>>>>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
> >>>>>>   abort))))
> >>>>>>
> >>>>>> (untested, but based on some code in linux-user/qemu.h).
> >>>>>>
> >>>>>> Then it's always
> >>>>>>
> >>>>>>     acpi_struct_assign(spcr->field, value);
> >>>>>>
> >>>>>> whether the field is 1, 2, 4 or 8 bytes.
> >>>>>>
> >>>>>> Not my bit of the codebase though, so I'll leave it to the
> >>>>>> ACPI maintainers to decide how much they like magic macros :-)
> >>>>>>
> >>>>>> thanks
> >>>>>> -- PMM
> >>>>>
> >>>>>
> >>>>> We don't much. One can use build_append_int_noprefix and just avoid
> >>>>> structs altogether.
> >>>>
> >>>> But if we use build_append_int_noprefix, we have to bother about the
> >>>> unused fields of the struct and have lots of
> >>>> build_append_int_noprefix(table, 0, 1/2/4/8).
> >>>
> >>> With a struct you have a bunch of reserved fields - is that very
> >>> different?
> >>>
> >>
> >> Not only about the reserved fields, but also the fields which ARM
> >> doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
> >> AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
> >> build_append_int_noprefix, we should add lots of
> >> build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
> >> just need to care them when we define it, rather than every time we use.
> > 
> > I've had a chat with Igor about this, and thought about it, and I'm now
> > in the build_append camp. It took me a while to see that point of view,
> > as it isn't a pattern I'm familiar with. However, the pattern, which is
> > 
> > * table generation code is a sequence of build_appends, commented with
> >   strings matching strings in the spec
> > * table generation code has the minimal number of parameters necessary
> >   to be useful for all users, all other table values have default values
> >   filled (typically zero)
> > * the parameters to the table generation code can be packed in a param
> >   struct that has descriptive member names, allowing the caller to
> >   still use the struct initializing type pattern, and to more cleanly
> >   manage the parameters
> > 
> > with the benefits of
> > 
> > * structs (the param structs) stay small
> 
> But the table generation codes will be huge and have lots of meaningless
> build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
> Maybe the readability is poor.

It should be about the same number of lines as the struct definition,
one line per field, and it'll only appear in one place, an API call in
aml-build.c. Looking at aml-build.c for some examples where this is
already done, e.g. aml_memory32_fixed and aml_interrupt, ones you added
:-), I would agree that, at first look, it's not super easy to see what's
what. However the key to understanding it is to have the spec open.
Knowing that no field is skipped allows a 1:1 mapping of build_append
lines to spec table fields, and good comments help guide the eye for
that mapping as well, even though just counting lines should work.

Note, the build_append_int_noprefix(table, 0, 1/2/4/8) lines are no more
meaningless than the unused fields in a struct definition. The advantage
of dropping the struct in favor of the build_append lines is the benefit
of simplifying the use of the used fields, as there are no more concerns
with endianness, nor the details of a AcpiGenericAddress, etc.

drew

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

[Shannon Zhao]

On 2015/6/18 19:03, Andrew Jones wrote:
> On Thu, Jun 18, 2015 at 06:28:36PM +0800, Shannon Zhao wrote:
>>
>>
>> On 2015/6/17 17:42, Andrew Jones wrote:
>>> On Wed, Jun 17, 2015 at 09:06:47AM +0800, Shannon Zhao wrote:
>>>>
>>>>
>>>> On 2015/6/16 22:19, Michael S. Tsirkin wrote:
>>>>> On Tue, Jun 16, 2015 at 09:33:19AM +0800, Shannon Zhao wrote:
>>>>>>
>>>>>>
>>>>>> On 2015/6/16 2:13, Michael S. Tsirkin wrote:
>>>>>>> On Mon, Jun 15, 2015 at 05:59:06PM +0100, Peter Maydell wrote:
>>>>>>>> On 15 June 2015 at 17:32, Andrew Jones <drjones@redhat.com> wrote:
>>>>>>>>> On Mon, Jun 15, 2015 at 06:10:25PM +0200, Michael S. Tsirkin wrote:
>>>>>>>>>> On Mon, Jun 15, 2015 at 04:45:58PM +0100, Peter Maydell wrote:
>>>>>>>>>>> I'm still confused about when fields in these ACPI structs
>>>>>>>>>>> need to be converted to little-endian, and when they don't.
>>>>>>>>>>> Is there a rule-of-thumb I can use when I'm looking at patches?
>>>>>>>>
>>>>>>>>>> Normally it's all LE unless it's a single byte value.
>>>>>>>>>> Did not check this specific table.
>>>>>>>>>> We really need to add sparse support to check
>>>>>>>>>> endian-ness matches, or re-write it
>>>>>>>>>> all using byte_add so there's no duplication of info.
>>>>>>>>
>>>>>>>>> Everything used in the table is either a single byte, or I used le32,
>>>>>>>>> Well, I didn't bother for the pci_{device,vendor}_id assignments, as
>>>>>>>>> they're 0xffff anyway. I can change those two to make them more explicit,
>>>>>>>>> if that's preferred.
>>>>>>>>
>>>>>>>> Yep, I just looked over the struct definition, so since this
>>>>>>>> has been reviewed I'll apply it to target-arm.next.
>>>>>>>>
>>>>>>>> You could probably make it easier to review and write
>>>>>>>> code that has to do these endianness swaps with something
>>>>>>>> like
>>>>>>>>
>>>>>>>> #define acpi_struct_assign(FIELD, VAL) \
>>>>>>>>   ((FIELD) = \
>>>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 1, VAL, \
>>>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 2, cpu_to_le16(VAL), \
>>>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 4, cpu_to_le32(VAL), \
>>>>>>>>   __builtin_choose_expr(sizeof(FIELD) == 8, cpu_to_le64(VAL), \
>>>>>>>>   abort))))
>>>>>>>>
>>>>>>>> (untested, but based on some code in linux-user/qemu.h).
>>>>>>>>
>>>>>>>> Then it's always
>>>>>>>>
>>>>>>>>     acpi_struct_assign(spcr->field, value);
>>>>>>>>
>>>>>>>> whether the field is 1, 2, 4 or 8 bytes.
>>>>>>>>
>>>>>>>> Not my bit of the codebase though, so I'll leave it to the
>>>>>>>> ACPI maintainers to decide how much they like magic macros :-)
>>>>>>>>
>>>>>>>> thanks
>>>>>>>> -- PMM
>>>>>>>
>>>>>>>
>>>>>>> We don't much. One can use build_append_int_noprefix and just avoid
>>>>>>> structs altogether.
>>>>>>
>>>>>> But if we use build_append_int_noprefix, we have to bother about the
>>>>>> unused fields of the struct and have lots of
>>>>>> build_append_int_noprefix(table, 0, 1/2/4/8).
>>>>>
>>>>> With a struct you have a bunch of reserved fields - is that very
>>>>> different?
>>>>>
>>>>
>>>> Not only about the reserved fields, but also the fields which ARM
>>>> doesn't use or x86 doesn't use. For example, xpm1a_event_block in struct
>>>> AcpiFadtDescriptorRev5_1 is not used for ARM now, if we use
>>>> build_append_int_noprefix, we should add lots of
>>>> build_append_int_noprefix(table, 0, 1/2/4/8). But if we use struct, we
>>>> just need to care them when we define it, rather than every time we use.
>>>
>>> I've had a chat with Igor about this, and thought about it, and I'm now
>>> in the build_append camp. It took me a while to see that point of view,
>>> as it isn't a pattern I'm familiar with. However, the pattern, which is
>>>
>>> * table generation code is a sequence of build_appends, commented with
>>>   strings matching strings in the spec
>>> * table generation code has the minimal number of parameters necessary
>>>   to be useful for all users, all other table values have default values
>>>   filled (typically zero)
>>> * the parameters to the table generation code can be packed in a param
>>>   struct that has descriptive member names, allowing the caller to
>>>   still use the struct initializing type pattern, and to more cleanly
>>>   manage the parameters
>>>
>>> with the benefits of
>>>
>>> * structs (the param structs) stay small
>>
>> But the table generation codes will be huge and have lots of meaningless
>> build_append_int_noprefix(table, 0, 1/2/4/8) for the unused fields.
>> Maybe the readability is poor.
> 
> It should be about the same number of lines as the struct definition,
> one line per field, and it'll only appear in one place, an API call in
> aml-build.c. Looking at aml-build.c for some examples where this is
> already done, e.g. aml_memory32_fixed and aml_interrupt, ones you added

These are operators which don't have too many unused fields for
different platforms. Here we are discussing about the generation of ACPI
table.

If you want to rework these, maybe you can have a try to rework FACP
table first and compare the lines and readability.

> :-), I would agree that, at first look, it's not super easy to see what's
> what. However the key to understanding it is to have the spec open.
> Knowing that no field is skipped allows a 1:1 mapping of build_append
> lines to spec table fields, and good comments help guide the eye for
> that mapping as well, even though just counting lines should work.
> 
> Note, the build_append_int_noprefix(table, 0, 1/2/4/8) lines are no more
> meaningless than the unused fields in a struct definition. The advantage
> of dropping the struct in favor of the build_append lines is the benefit
> of simplifying the use of the used fields, as there are no more concerns
> with endianness, nor the details of a AcpiGenericAddress, etc.
> 
> drew
> 
> .
> 

-- 
Shannon

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

[Michael S. Tsirkin]

On Wed, Jun 10, 2015 at 05:52:37AM -0400, 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.

Looks good to me.
Feel free to merge through the arm tree.

> v3:
> - no enums, document struct and value in build_spcr [mst]
> - document earliest spec version for table [mst]
> - clarify _ADR comment [Igor]
> v2:
> - checkpatch style fixes [Shannon]
> - table is revision 2, not 5 [Shannon]
> - use AML_SYSTEM_MEMORY [Shannon]
> - tested-by Shannon
> 
> Andrew Jones (2):
>   ACPI: Add definitions for the SPCR table
>   hw/arm/virt-acpi-build: Add SPCR table
> 
>  hw/arm/virt-acpi-build.c    | 43 ++++++++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+), 1 deletion(-)
> 
> -- 
> 1.8.3.1