[RESEND RFC v1 0/2] RISC-V: ACPI: Enable SPCR

[RESEND RFC v1 0/2] RISC-V: ACPI: Enable SPCR

[Sia Jee Heng]

This series focuses on enabling the Serial Port Console Redirection (SPCR)
table for the RISC-V virt platform. Considering that ARM utilizes the same
function, the initial patch involves migrating the build_spcr function to
common code. This consolidation ensures that RISC-V avoids duplicating the
function.

The patch set is built upon Alistair's riscv-to-apply.next branch and
relies on Sunil's patches at [1].

[1] https://lore.kernel.org/qemu-devel/20231218150247.466427-1-sunilvl@ventanamicro.com/

Sia Jee Heng (2):
  hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
  hw/riscv/virt-acpi-build.c: Generate SPCR table

 hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
 hw/riscv/virt-acpi-build.c  | 39 +++++++++++++++++++++
 include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
 include/hw/acpi/aml-build.h |  4 +++
 5 files changed, 154 insertions(+), 41 deletions(-)

-- 
2.34.1

[RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[Sia Jee Heng]

RISC-V should also generate the SPCR in a manner similar to ARM.
Therefore, instead of replicating the code, relocate this function
to the common AML build.

Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
---
 hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
 include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
 include/hw/acpi/aml-build.h |  4 +++
 4 files changed, 115 insertions(+), 41 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index af66bde0f5..1efa534aa8 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
     }
 }
 
+void build_spcr(GArray *table_data, BIOSLinker *linker,
+                const AcpiSpcrData *f, const char *oem_id,
+                const char *oem_table_id)
+{
+    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
+                        .oem_table_id = oem_table_id };
+
+    acpi_table_begin(&table, table_data);
+    /* Interface type */
+    build_append_int_noprefix(table_data, f->interface_type, 1);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 3);
+    /* Base Address */
+    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
+                     f->base_addr.offset, f->base_addr.size,
+                     f->base_addr.addr);
+    /* Interrupt type */
+    build_append_int_noprefix(table_data, f->interrupt_type, 1);
+    /* IRQ */
+    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
+    /* Global System Interrupt */
+    build_append_int_noprefix(table_data, f->interrupt, 4);
+    /* Baud Rate */
+    build_append_int_noprefix(table_data, f->baud_rate, 1);
+    /* Parity */
+    build_append_int_noprefix(table_data, f->parity, 1);
+    /* Stop Bits */
+    build_append_int_noprefix(table_data, f->stop_bits, 1);
+    /* Flow Control */
+    build_append_int_noprefix(table_data, f->flow_control, 1);
+    /* Terminal Type */
+    build_append_int_noprefix(table_data, f->terminal_type, 1);
+    /* PCI Device ID  */
+    build_append_int_noprefix(table_data, f->pci_device_id, 2);
+    /* PCI Vendor ID */
+    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
+    /* PCI Bus Number */
+    build_append_int_noprefix(table_data, f->pci_bus, 1);
+    /* PCI Device Number */
+    build_append_int_noprefix(table_data, f->pci_device, 1);
+    /* PCI Function Number */
+    build_append_int_noprefix(table_data, f->pci_function, 1);
+    /* PCI Flags */
+    build_append_int_noprefix(table_data, f->pci_flags, 4);
+    /* PCI Segment */
+    build_append_int_noprefix(table_data, f->pci_segment, 1);
+    /* Reserved */
+    build_append_int_noprefix(table_data, 0, 4);
+
+    acpi_table_end(linker, &table);
+}
 /*
  * ACPI spec, Revision 6.3
  * 5.2.29 Processor Properties Topology Table (PPTT)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 510ab0dcca..a31f736d1a 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
  * Rev: 1.07
  */
 static void
-build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
+build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
-    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
-                        .oem_table_id = vms->oem_table_id };
-
-    acpi_table_begin(&table, table_data);
-
-    /* Interface Type */
-    build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
-    build_append_int_noprefix(table_data, 0, 3); /* Reserved */
-    /* Base Address */
-    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 3,
-                     vms->memmap[VIRT_UART].base);
-    /* Interrupt Type */
-    build_append_int_noprefix(table_data,
-        (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
-    build_append_int_noprefix(table_data, 0, 1); /* IRQ */
-    /* Global System Interrupt */
-    build_append_int_noprefix(table_data,
-                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
-    build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
-    build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
-    /* Stop Bits */
-    build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
-    /* Flow Control */
-    build_append_int_noprefix(table_data,
-        (1 << 1) /* RTS/CTS hardware flow control */, 1);
-    /* Terminal Type */
-    build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
-    build_append_int_noprefix(table_data, 0, 1); /* Language */
-    /* PCI Device ID  */
-    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
-    /* PCI Vendor ID */
-    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
-    build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
-    build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */
-    build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */
-    build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */
-    build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */
-    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
+    AcpiSpcrData serial = {
+        .interface_type = 3,       /* ARM PL011 UART */
+        .base_addr.id = AML_AS_SYSTEM_MEMORY,
+        .base_addr.width = 32,
+        .base_addr.offset = 0,
+        .base_addr.size = 3,
+        .base_addr.addr = vms->memmap[VIRT_UART].base,
+        .interrupt_type = (1 << 3),/* Bit[3] ARMH GIC interrupt*/
+        .pc_interrupt = 0,         /* IRQ */
+        .interrupt = (vms->irqmap[VIRT_UART] + ARM_SPI_BASE),
+        .baud_rate = 3,            /* 9600 */
+        .parity = 0,               /* No Parity */
+        .stop_bits = 1,            /* 1 Stop bit */
+        .flow_control = 1 << 1,    /* RTS/CTS hardware flow control */
+        .terminal_type = 0,        /* VT100 */
+        .language = 0,             /* Language */
+        .pci_device_id = 0xffff,   /* not a PCI device*/
+        .pci_vendor_id = 0xffff,   /* not a PCI device*/
+        .pci_bus = 0,
+        .pci_device = 0,
+        .pci_function = 0,
+        .pci_flags = 0,
+        .pci_segment = 0,
+    };
 
-    acpi_table_end(linker, &table);
+    build_spcr(table_data, linker, &serial, vms->oem_id, vms->oem_table_id);
 }
 
 /*
@@ -930,7 +916,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     }
 
     acpi_add_table(table_offsets, tables_blob);
-    build_spcr(tables_blob, tables->linker, vms);
+    build_spcr_v2(tables_blob, tables->linker, vms);
 
     acpi_add_table(table_offsets, tables_blob);
     build_dbg2(tables_blob, tables->linker, vms);
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 2b42e4192b..0e6e82b339 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -90,6 +90,39 @@ typedef struct AcpiFadtData {
     unsigned *xdsdt_tbl_offset;
 } AcpiFadtData;
 
+typedef struct AcpiGas {
+    uint8_t id;                /* Address space ID */
+    uint8_t width;             /* Register bit width */
+    uint8_t offset;            /* Register bit offset */
+    uint8_t size;              /* Access size */
+    uint64_t addr;             /* Address */
+} AcpiGas;
+
+/* SPCR (Serial Port Console Redirection table) */
+typedef struct AcpiSpcrData {
+    uint8_t interface_type;
+    uint8_t reserved[3];
+    struct AcpiGas base_addr;
+    uint8_t interrupt_type;
+    uint8_t pc_interrupt;
+    uint32_t interrupt;        /* Global system interrupt */
+    uint8_t baud_rate;
+    uint8_t parity;
+    uint8_t stop_bits;
+    uint8_t flow_control;
+    uint8_t terminal_type;
+    uint8_t language;
+    uint8_t reserved1;
+    uint16_t pci_device_id;    /* Must be 0xffff if not PCI device */
+    uint16_t pci_vendor_id;    /* Must be 0xffff if not PCI device */
+    uint8_t pci_bus;
+    uint8_t pci_device;
+    uint8_t pci_function;
+    uint32_t pci_flags;
+    uint8_t pci_segment;
+    uint32_t reserved2;
+} AcpiSpcrData;
+
 #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
 #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
 
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index ff2a310270..44e03e76bd 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -497,4 +497,8 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
 
 void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
                 const char *oem_id, const char *oem_table_id);
+
+void build_spcr(GArray *table_data, BIOSLinker *linker,
+                const AcpiSpcrData *f, const char *oem_id,
+                const char *oem_table_id);
 #endif
-- 
2.34.1

[RESEND RFC v1 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table

[Sia Jee Heng]

Generate Serial Port Console Redirection Table (SPCR) for RISC-V
virtual machine.

Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
---
 hw/riscv/virt-acpi-build.c | 39 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index d4a02579d6..388b3d1a84 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -174,6 +174,42 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
     aml_append(scope, dev);
 }
 
+/*
+ * Serial Port Console Redirection Table (SPCR)
+ * Rev: 1.07
+ */
+
+static void
+build_spcr_rev2(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
+{
+    AcpiSpcrData serial = {
+        .interface_type = 0,       /* 16550 compatible */
+        .base_addr.id = AML_AS_SYSTEM_MEMORY,
+        .base_addr.width = 32,
+        .base_addr.offset = 0,
+        .base_addr.size = 1,
+        .base_addr.addr = s->memmap[VIRT_UART0].base,
+        .interrupt_type = (1 << 4),/* Bit[4] RISC-V PLIC/APLIC */
+        .pc_interrupt = 0,
+        .interrupt = UART0_IRQ,
+        .baud_rate = 7,            /* 15200 */
+        .parity = 0,
+        .stop_bits = 1,
+        .flow_control = 0,
+        .terminal_type = 3,        /* ANSI */
+        .language = 0,             /* Language */
+        .pci_device_id = 0xffff,   /* not a PCI device*/
+        .pci_vendor_id = 0xffff,   /* not a PCI device*/
+        .pci_bus = 0,
+        .pci_device = 0,
+        .pci_function = 0,
+        .pci_flags = 0,
+        .pci_segment = 0,
+    };
+
+    build_spcr(table_data, linker, &serial, s->oem_id, s->oem_table_id);
+}
+
 /* RHCT Node[N] starts at offset 56 */
 #define RHCT_NODE_ARRAY_OFFSET 56
 
@@ -555,6 +591,9 @@ static void virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
     acpi_add_table(table_offsets, tables_blob);
     build_rhct(tables_blob, tables->linker, s);
 
+    acpi_add_table(table_offsets, tables_blob);
+    build_spcr_rev2(tables_blob, tables->linker, s);
+
     acpi_add_table(table_offsets, tables_blob);
     {
         AcpiMcfgInfo mcfg = {
-- 
2.34.1

Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[Daniel Henrique Barboza]

On 1/5/24 06:06, Sia Jee Heng wrote:
> RISC-V should also generate the SPCR in a manner similar to ARM.
> Therefore, instead of replicating the code, relocate this function
> to the common AML build.
> 
> Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> ---
>   hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
>   hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
>   include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
>   include/hw/acpi/aml-build.h |  4 +++
>   4 files changed, 115 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index af66bde0f5..1efa534aa8 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
>       }
>   }
>   
> +void build_spcr(GArray *table_data, BIOSLinker *linker,
> +                const AcpiSpcrData *f, const char *oem_id,
> +                const char *oem_table_id)
> +{
> +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> +                        .oem_table_id = oem_table_id };
> +
> +    acpi_table_begin(&table, table_data);
> +    /* Interface type */
> +    build_append_int_noprefix(table_data, f->interface_type, 1);
> +    /* Reserved */
> +    build_append_int_noprefix(table_data, 0, 3);
> +    /* Base Address */
> +    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> +                     f->base_addr.offset, f->base_addr.size,
> +                     f->base_addr.addr);
> +    /* Interrupt type */
> +    build_append_int_noprefix(table_data, f->interrupt_type, 1);
> +    /* IRQ */
> +    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
> +    /* Global System Interrupt */
> +    build_append_int_noprefix(table_data, f->interrupt, 4);
> +    /* Baud Rate */
> +    build_append_int_noprefix(table_data, f->baud_rate, 1);
> +    /* Parity */
> +    build_append_int_noprefix(table_data, f->parity, 1);
> +    /* Stop Bits */
> +    build_append_int_noprefix(table_data, f->stop_bits, 1);
> +    /* Flow Control */
> +    build_append_int_noprefix(table_data, f->flow_control, 1);
> +    /* Terminal Type */
> +    build_append_int_noprefix(table_data, f->terminal_type, 1);
> +    /* PCI Device ID  */
> +    build_append_int_noprefix(table_data, f->pci_device_id, 2);
> +    /* PCI Vendor ID */
> +    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> +    /* PCI Bus Number */
> +    build_append_int_noprefix(table_data, f->pci_bus, 1);
> +    /* PCI Device Number */
> +    build_append_int_noprefix(table_data, f->pci_device, 1);
> +    /* PCI Function Number */
> +    build_append_int_noprefix(table_data, f->pci_function, 1);
> +    /* PCI Flags */
> +    build_append_int_noprefix(table_data, f->pci_flags, 4);
> +    /* PCI Segment */
> +    build_append_int_noprefix(table_data, f->pci_segment, 1);
> +    /* Reserved */
> +    build_append_int_noprefix(table_data, 0, 4);
> +
> +    acpi_table_end(linker, &table);
> +}
>   /*
>    * ACPI spec, Revision 6.3
>    * 5.2.29 Processor Properties Topology Table (PPTT)
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 510ab0dcca..a31f736d1a 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>    * Rev: 1.07
>    */
>   static void
> -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> +build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)

Nit: I don't understand the '_v2' in the name of this function. Is it just to not collide
with the now public build_spcr()? Or does it have to do with the SPCR table being
'.rev = 2'? Because if it's the latter, you can name the common helper 'build_spcr_rev2'
(since both ARM and RISC-V use SPCR rev 2), keep this local build_spcr() initializing
the AcpiSpcrData struct with ARM attributes and then call the common build_spcr_rev2().


Code looks good othewise. Thanks,


Daniel



>   {
> -    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
> -                        .oem_table_id = vms->oem_table_id };
> -
> -    acpi_table_begin(&table, table_data);
> -
> -    /* Interface Type */
> -    build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
> -    build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> -    /* Base Address */
> -    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 3,
> -                     vms->memmap[VIRT_UART].base);
> -    /* Interrupt Type */
> -    build_append_int_noprefix(table_data,
> -        (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
> -    build_append_int_noprefix(table_data, 0, 1); /* IRQ */
> -    /* Global System Interrupt */
> -    build_append_int_noprefix(table_data,
> -                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
> -    build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
> -    build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
> -    /* Stop Bits */
> -    build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
> -    /* Flow Control */
> -    build_append_int_noprefix(table_data,
> -        (1 << 1) /* RTS/CTS hardware flow control */, 1);
> -    /* Terminal Type */
> -    build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
> -    build_append_int_noprefix(table_data, 0, 1); /* Language */
> -    /* PCI Device ID  */
> -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> -    /* PCI Vendor ID */
> -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> -    build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
> -    build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */
> -    build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */
> -    build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */
> -    build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */
> -    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> +    AcpiSpcrData serial = {
> +        .interface_type = 3,       /* ARM PL011 UART */
> +        .base_addr.id = AML_AS_SYSTEM_MEMORY,
> +        .base_addr.width = 32,
> +        .base_addr.offset = 0,
> +        .base_addr.size = 3,
> +        .base_addr.addr = vms->memmap[VIRT_UART].base,
> +        .interrupt_type = (1 << 3),/* Bit[3] ARMH GIC interrupt*/
> +        .pc_interrupt = 0,         /* IRQ */
> +        .interrupt = (vms->irqmap[VIRT_UART] + ARM_SPI_BASE),
> +        .baud_rate = 3,            /* 9600 */
> +        .parity = 0,               /* No Parity */
> +        .stop_bits = 1,            /* 1 Stop bit */
> +        .flow_control = 1 << 1,    /* RTS/CTS hardware flow control */
> +        .terminal_type = 0,        /* VT100 */
> +        .language = 0,             /* Language */
> +        .pci_device_id = 0xffff,   /* not a PCI device*/
> +        .pci_vendor_id = 0xffff,   /* not a PCI device*/
> +        .pci_bus = 0,
> +        .pci_device = 0,
> +        .pci_function = 0,
> +        .pci_flags = 0,
> +        .pci_segment = 0,
> +    };
>   
> -    acpi_table_end(linker, &table);
> +    build_spcr(table_data, linker, &serial, vms->oem_id, vms->oem_table_id);
>   }
>   
>   /*
> @@ -930,7 +916,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>       }
>   
>       acpi_add_table(table_offsets, tables_blob);
> -    build_spcr(tables_blob, tables->linker, vms);
> +    build_spcr_v2(tables_blob, tables->linker, vms);
>   
>       acpi_add_table(table_offsets, tables_blob);
>       build_dbg2(tables_blob, tables->linker, vms);
> diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> index 2b42e4192b..0e6e82b339 100644
> --- a/include/hw/acpi/acpi-defs.h
> +++ b/include/hw/acpi/acpi-defs.h
> @@ -90,6 +90,39 @@ typedef struct AcpiFadtData {
>       unsigned *xdsdt_tbl_offset;
>   } AcpiFadtData;
>   
> +typedef struct AcpiGas {
> +    uint8_t id;                /* Address space ID */
> +    uint8_t width;             /* Register bit width */
> +    uint8_t offset;            /* Register bit offset */
> +    uint8_t size;              /* Access size */
> +    uint64_t addr;             /* Address */
> +} AcpiGas;
> +
> +/* SPCR (Serial Port Console Redirection table) */
> +typedef struct AcpiSpcrData {
> +    uint8_t interface_type;
> +    uint8_t reserved[3];
> +    struct AcpiGas base_addr;
> +    uint8_t interrupt_type;
> +    uint8_t pc_interrupt;
> +    uint32_t interrupt;        /* Global system interrupt */
> +    uint8_t baud_rate;
> +    uint8_t parity;
> +    uint8_t stop_bits;
> +    uint8_t flow_control;
> +    uint8_t terminal_type;
> +    uint8_t language;
> +    uint8_t reserved1;
> +    uint16_t pci_device_id;    /* Must be 0xffff if not PCI device */
> +    uint16_t pci_vendor_id;    /* Must be 0xffff if not PCI device */
> +    uint8_t pci_bus;
> +    uint8_t pci_device;
> +    uint8_t pci_function;
> +    uint32_t pci_flags;
> +    uint8_t pci_segment;
> +    uint32_t reserved2;
> +} AcpiSpcrData;
> +
>   #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
>   #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
>   
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index ff2a310270..44e03e76bd 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -497,4 +497,8 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>   
>   void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
>                   const char *oem_id, const char *oem_table_id);
> +
> +void build_spcr(GArray *table_data, BIOSLinker *linker,
> +                const AcpiSpcrData *f, const char *oem_id,
> +                const char *oem_table_id);
>   #endif

Re: [RESEND RFC v1 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table

[Daniel Henrique Barboza]

On 1/5/24 06:06, Sia Jee Heng wrote:
> Generate Serial Port Console Redirection Table (SPCR) for RISC-V
> virtual machine.
> 
> Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> ---
>   hw/riscv/virt-acpi-build.c | 39 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index d4a02579d6..388b3d1a84 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -174,6 +174,42 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
>       aml_append(scope, dev);
>   }
>   
> +/*
> + * Serial Port Console Redirection Table (SPCR)
> + * Rev: 1.07

Shouldn't it be "Rev: 2.0"? The function is calling the common build_spcr() that
specifies

+    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
+                        .oem_table_id = oem_table_id };



Code LGTM regardless of the "Rev: " comment value.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>





> + */
> +
> +static void
> +build_spcr_rev2(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
> +{
> +    AcpiSpcrData serial = {
> +        .interface_type = 0,       /* 16550 compatible */
> +        .base_addr.id = AML_AS_SYSTEM_MEMORY,
> +        .base_addr.width = 32,
> +        .base_addr.offset = 0,
> +        .base_addr.size = 1,
> +        .base_addr.addr = s->memmap[VIRT_UART0].base,
> +        .interrupt_type = (1 << 4),/* Bit[4] RISC-V PLIC/APLIC */
> +        .pc_interrupt = 0,
> +        .interrupt = UART0_IRQ,
> +        .baud_rate = 7,            /* 15200 */
> +        .parity = 0,
> +        .stop_bits = 1,
> +        .flow_control = 0,
> +        .terminal_type = 3,        /* ANSI */
> +        .language = 0,             /* Language */
> +        .pci_device_id = 0xffff,   /* not a PCI device*/
> +        .pci_vendor_id = 0xffff,   /* not a PCI device*/
> +        .pci_bus = 0,
> +        .pci_device = 0,
> +        .pci_function = 0,
> +        .pci_flags = 0,
> +        .pci_segment = 0,
> +    };
> +
> +    build_spcr(table_data, linker, &serial, s->oem_id, s->oem_table_id);
> +}
> +
>   /* RHCT Node[N] starts at offset 56 */
>   #define RHCT_NODE_ARRAY_OFFSET 56
>   
> @@ -555,6 +591,9 @@ static void virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
>       acpi_add_table(table_offsets, tables_blob);
>       build_rhct(tables_blob, tables->linker, s);
>   
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_spcr_rev2(tables_blob, tables->linker, s);
> +
>       acpi_add_table(table_offsets, tables_blob);
>       {
>           AcpiMcfgInfo mcfg = {

Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[Sunil V L]

On Fri, Jan 05, 2024 at 09:19:14AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 1/5/24 06:06, Sia Jee Heng wrote:
> > RISC-V should also generate the SPCR in a manner similar to ARM.
> > Therefore, instead of replicating the code, relocate this function
> > to the common AML build.
> > 
> > Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> > ---
> >   hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
> >   hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
> >   include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
> >   include/hw/acpi/aml-build.h |  4 +++
> >   4 files changed, 115 insertions(+), 41 deletions(-)
> > 
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index af66bde0f5..1efa534aa8 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> >       }
> >   }
> > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > +                const AcpiSpcrData *f, const char *oem_id,
> > +                const char *oem_table_id)
> > +{
> > +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> > +                        .oem_table_id = oem_table_id };
> > +
> > +    acpi_table_begin(&table, table_data);
> > +    /* Interface type */
> > +    build_append_int_noprefix(table_data, f->interface_type, 1);
> > +    /* Reserved */
> > +    build_append_int_noprefix(table_data, 0, 3);
> > +    /* Base Address */
> > +    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> > +                     f->base_addr.offset, f->base_addr.size,
> > +                     f->base_addr.addr);
> > +    /* Interrupt type */
> > +    build_append_int_noprefix(table_data, f->interrupt_type, 1);
> > +    /* IRQ */
> > +    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
> > +    /* Global System Interrupt */
> > +    build_append_int_noprefix(table_data, f->interrupt, 4);
> > +    /* Baud Rate */
> > +    build_append_int_noprefix(table_data, f->baud_rate, 1);
> > +    /* Parity */
> > +    build_append_int_noprefix(table_data, f->parity, 1);
> > +    /* Stop Bits */
> > +    build_append_int_noprefix(table_data, f->stop_bits, 1);
> > +    /* Flow Control */
> > +    build_append_int_noprefix(table_data, f->flow_control, 1);
> > +    /* Terminal Type */
> > +    build_append_int_noprefix(table_data, f->terminal_type, 1);
> > +    /* PCI Device ID  */
> > +    build_append_int_noprefix(table_data, f->pci_device_id, 2);
> > +    /* PCI Vendor ID */
> > +    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> > +    /* PCI Bus Number */
> > +    build_append_int_noprefix(table_data, f->pci_bus, 1);
> > +    /* PCI Device Number */
> > +    build_append_int_noprefix(table_data, f->pci_device, 1);
> > +    /* PCI Function Number */
> > +    build_append_int_noprefix(table_data, f->pci_function, 1);
> > +    /* PCI Flags */
> > +    build_append_int_noprefix(table_data, f->pci_flags, 4);
> > +    /* PCI Segment */
> > +    build_append_int_noprefix(table_data, f->pci_segment, 1);
> > +    /* Reserved */
> > +    build_append_int_noprefix(table_data, 0, 4);
> > +
> > +    acpi_table_end(linker, &table);
> > +}
> >   /*
> >    * ACPI spec, Revision 6.3
> >    * 5.2.29 Processor Properties Topology Table (PPTT)
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 510ab0dcca..a31f736d1a 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >    * Rev: 1.07
> >    */
> >   static void
> > -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > +build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> 
> Nit: I don't understand the '_v2' in the name of this function. Is it just to not collide
> with the now public build_spcr()? Or does it have to do with the SPCR table being
> '.rev = 2'? Because if it's the latter, you can name the common helper 'build_spcr_rev2'
> (since both ARM and RISC-V use SPCR rev 2), keep this local build_spcr() initializing
> the AcpiSpcrData struct with ARM attributes and then call the common build_spcr_rev2().
> 
My suggestion is, keep the build_spcr() generic and take version as the
parameter.

Thanks,
Sunil

RE: [RESEND RFC v1 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table

[JeeHeng Sia]

> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Friday, January 5, 2024 8:27 PM
> To: JeeHeng Sia <jeeheng.sia@starfivetech.com>; qemu-arm@nongnu.org; qemu-devel@nongnu.org; qemu-riscv@nongnu.org
> Cc: mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sunilvl@ventanamicro.com; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com;
> zhiwei_liu@linux.alibaba.com
> Subject: Re: [RESEND RFC v1 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table
> 
> 
> 
> On 1/5/24 06:06, Sia Jee Heng wrote:
> > Generate Serial Port Console Redirection Table (SPCR) for RISC-V
> > virtual machine.
> >
> > Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> > ---
> >   hw/riscv/virt-acpi-build.c | 39 ++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 39 insertions(+)
> >
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> > index d4a02579d6..388b3d1a84 100644
> > --- a/hw/riscv/virt-acpi-build.c
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -174,6 +174,42 @@ acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap,
> >       aml_append(scope, dev);
> >   }
> >
> > +/*
> > + * Serial Port Console Redirection Table (SPCR)
> > + * Rev: 1.07
> 
> Shouldn't it be "Rev: 2.0"? The function is calling the common build_spcr() that
> specifies
I will give them a generic name for both the arch build_spcr() and the
common build_spcr(). The revision info should be passed to the common
build_spcr().
> 
> +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> +                        .oem_table_id = oem_table_id };
> 
> 
> 
> Code LGTM regardless of the "Rev: " comment value.
> 
> 
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
> 
> 
> 
> 
> > + */
> > +
> > +static void
> > +build_spcr_rev2(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
> > +{
> > +    AcpiSpcrData serial = {
> > +        .interface_type = 0,       /* 16550 compatible */
> > +        .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > +        .base_addr.width = 32,
> > +        .base_addr.offset = 0,
> > +        .base_addr.size = 1,
> > +        .base_addr.addr = s->memmap[VIRT_UART0].base,
> > +        .interrupt_type = (1 << 4),/* Bit[4] RISC-V PLIC/APLIC */
> > +        .pc_interrupt = 0,
> > +        .interrupt = UART0_IRQ,
> > +        .baud_rate = 7,            /* 15200 */
> > +        .parity = 0,
> > +        .stop_bits = 1,
> > +        .flow_control = 0,
> > +        .terminal_type = 3,        /* ANSI */
> > +        .language = 0,             /* Language */
> > +        .pci_device_id = 0xffff,   /* not a PCI device*/
> > +        .pci_vendor_id = 0xffff,   /* not a PCI device*/
> > +        .pci_bus = 0,
> > +        .pci_device = 0,
> > +        .pci_function = 0,
> > +        .pci_flags = 0,
> > +        .pci_segment = 0,
> > +    };
> > +
> > +    build_spcr(table_data, linker, &serial, s->oem_id, s->oem_table_id);
> > +}
> > +
> >   /* RHCT Node[N] starts at offset 56 */
> >   #define RHCT_NODE_ARRAY_OFFSET 56
> >
> > @@ -555,6 +591,9 @@ static void virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
> >       acpi_add_table(table_offsets, tables_blob);
> >       build_rhct(tables_blob, tables->linker, s);
> >
> > +    acpi_add_table(table_offsets, tables_blob);
> > +    build_spcr_rev2(tables_blob, tables->linker, s);
> > +
> >       acpi_add_table(table_offsets, tables_blob);
> >       {
> >           AcpiMcfgInfo mcfg = {

RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[JeeHeng Sia]

> -----Original Message-----
> From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Sent: Friday, January 5, 2024 8:19 PM
> To: JeeHeng Sia <jeeheng.sia@starfivetech.com>; qemu-arm@nongnu.org; qemu-devel@nongnu.org; qemu-riscv@nongnu.org
> Cc: mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sunilvl@ventanamicro.com; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com;
> zhiwei_liu@linux.alibaba.com
> Subject: Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> 
> 
> 
> On 1/5/24 06:06, Sia Jee Heng wrote:
> > RISC-V should also generate the SPCR in a manner similar to ARM.
> > Therefore, instead of replicating the code, relocate this function
> > to the common AML build.
> >
> > Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> > ---
> >   hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
> >   hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
> >   include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
> >   include/hw/acpi/aml-build.h |  4 +++
> >   4 files changed, 115 insertions(+), 41 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index af66bde0f5..1efa534aa8 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> >       }
> >   }
> >
> > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > +                const AcpiSpcrData *f, const char *oem_id,
> > +                const char *oem_table_id)
> > +{
> > +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> > +                        .oem_table_id = oem_table_id };
> > +
> > +    acpi_table_begin(&table, table_data);
> > +    /* Interface type */
> > +    build_append_int_noprefix(table_data, f->interface_type, 1);
> > +    /* Reserved */
> > +    build_append_int_noprefix(table_data, 0, 3);
> > +    /* Base Address */
> > +    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> > +                     f->base_addr.offset, f->base_addr.size,
> > +                     f->base_addr.addr);
> > +    /* Interrupt type */
> > +    build_append_int_noprefix(table_data, f->interrupt_type, 1);
> > +    /* IRQ */
> > +    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
> > +    /* Global System Interrupt */
> > +    build_append_int_noprefix(table_data, f->interrupt, 4);
> > +    /* Baud Rate */
> > +    build_append_int_noprefix(table_data, f->baud_rate, 1);
> > +    /* Parity */
> > +    build_append_int_noprefix(table_data, f->parity, 1);
> > +    /* Stop Bits */
> > +    build_append_int_noprefix(table_data, f->stop_bits, 1);
> > +    /* Flow Control */
> > +    build_append_int_noprefix(table_data, f->flow_control, 1);
> > +    /* Terminal Type */
> > +    build_append_int_noprefix(table_data, f->terminal_type, 1);
> > +    /* PCI Device ID  */
> > +    build_append_int_noprefix(table_data, f->pci_device_id, 2);
> > +    /* PCI Vendor ID */
> > +    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> > +    /* PCI Bus Number */
> > +    build_append_int_noprefix(table_data, f->pci_bus, 1);
> > +    /* PCI Device Number */
> > +    build_append_int_noprefix(table_data, f->pci_device, 1);
> > +    /* PCI Function Number */
> > +    build_append_int_noprefix(table_data, f->pci_function, 1);
> > +    /* PCI Flags */
> > +    build_append_int_noprefix(table_data, f->pci_flags, 4);
> > +    /* PCI Segment */
> > +    build_append_int_noprefix(table_data, f->pci_segment, 1);
> > +    /* Reserved */
> > +    build_append_int_noprefix(table_data, 0, 4);
> > +
> > +    acpi_table_end(linker, &table);
> > +}
> >   /*
> >    * ACPI spec, Revision 6.3
> >    * 5.2.29 Processor Properties Topology Table (PPTT)
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 510ab0dcca..a31f736d1a 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >    * Rev: 1.07
> >    */
> >   static void
> > -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > +build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> 
> Nit: I don't understand the '_v2' in the name of this function. Is it just to not collide
> with the now public build_spcr()? Or does it have to do with the SPCR table being
> '.rev = 2'? Because if it's the latter, you can name the common helper 'build_spcr_rev2'
> (since both ARM and RISC-V use SPCR rev 2), keep this local build_spcr() initializing
> the AcpiSpcrData struct with ARM attributes and then call the common build_spcr_rev2().
Sorry for the confusion. I will give them a generic name for both the arch build_spcr()
and the common build_spcr(). The revision info should be passed to the common
build_spcr().
> 
> 
> Code looks good othewise. Thanks,
> 
> 
> Daniel
> 
> 
> 
> >   {
> > -    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
> > -                        .oem_table_id = vms->oem_table_id };
> > -
> > -    acpi_table_begin(&table, table_data);
> > -
> > -    /* Interface Type */
> > -    build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
> > -    build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> > -    /* Base Address */
> > -    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 3,
> > -                     vms->memmap[VIRT_UART].base);
> > -    /* Interrupt Type */
> > -    build_append_int_noprefix(table_data,
> > -        (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
> > -    build_append_int_noprefix(table_data, 0, 1); /* IRQ */
> > -    /* Global System Interrupt */
> > -    build_append_int_noprefix(table_data,
> > -                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
> > -    build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
> > -    build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
> > -    /* Stop Bits */
> > -    build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
> > -    /* Flow Control */
> > -    build_append_int_noprefix(table_data,
> > -        (1 << 1) /* RTS/CTS hardware flow control */, 1);
> > -    /* Terminal Type */
> > -    build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
> > -    build_append_int_noprefix(table_data, 0, 1); /* Language */
> > -    /* PCI Device ID  */
> > -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> > -    /* PCI Vendor ID */
> > -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */
> > -    build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */
> > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */
> > -    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> > +    AcpiSpcrData serial = {
> > +        .interface_type = 3,       /* ARM PL011 UART */
> > +        .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > +        .base_addr.width = 32,
> > +        .base_addr.offset = 0,
> > +        .base_addr.size = 3,
> > +        .base_addr.addr = vms->memmap[VIRT_UART].base,
> > +        .interrupt_type = (1 << 3),/* Bit[3] ARMH GIC interrupt*/
> > +        .pc_interrupt = 0,         /* IRQ */
> > +        .interrupt = (vms->irqmap[VIRT_UART] + ARM_SPI_BASE),
> > +        .baud_rate = 3,            /* 9600 */
> > +        .parity = 0,               /* No Parity */
> > +        .stop_bits = 1,            /* 1 Stop bit */
> > +        .flow_control = 1 << 1,    /* RTS/CTS hardware flow control */
> > +        .terminal_type = 0,        /* VT100 */
> > +        .language = 0,             /* Language */
> > +        .pci_device_id = 0xffff,   /* not a PCI device*/
> > +        .pci_vendor_id = 0xffff,   /* not a PCI device*/
> > +        .pci_bus = 0,
> > +        .pci_device = 0,
> > +        .pci_function = 0,
> > +        .pci_flags = 0,
> > +        .pci_segment = 0,
> > +    };
> >
> > -    acpi_table_end(linker, &table);
> > +    build_spcr(table_data, linker, &serial, vms->oem_id, vms->oem_table_id);
> >   }
> >
> >   /*
> > @@ -930,7 +916,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> >       }
> >
> >       acpi_add_table(table_offsets, tables_blob);
> > -    build_spcr(tables_blob, tables->linker, vms);
> > +    build_spcr_v2(tables_blob, tables->linker, vms);
> >
> >       acpi_add_table(table_offsets, tables_blob);
> >       build_dbg2(tables_blob, tables->linker, vms);
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 2b42e4192b..0e6e82b339 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -90,6 +90,39 @@ typedef struct AcpiFadtData {
> >       unsigned *xdsdt_tbl_offset;
> >   } AcpiFadtData;
> >
> > +typedef struct AcpiGas {
> > +    uint8_t id;                /* Address space ID */
> > +    uint8_t width;             /* Register bit width */
> > +    uint8_t offset;            /* Register bit offset */
> > +    uint8_t size;              /* Access size */
> > +    uint64_t addr;             /* Address */
> > +} AcpiGas;
> > +
> > +/* SPCR (Serial Port Console Redirection table) */
> > +typedef struct AcpiSpcrData {
> > +    uint8_t interface_type;
> > +    uint8_t reserved[3];
> > +    struct AcpiGas base_addr;
> > +    uint8_t interrupt_type;
> > +    uint8_t pc_interrupt;
> > +    uint32_t interrupt;        /* Global system interrupt */
> > +    uint8_t baud_rate;
> > +    uint8_t parity;
> > +    uint8_t stop_bits;
> > +    uint8_t flow_control;
> > +    uint8_t terminal_type;
> > +    uint8_t language;
> > +    uint8_t reserved1;
> > +    uint16_t pci_device_id;    /* Must be 0xffff if not PCI device */
> > +    uint16_t pci_vendor_id;    /* Must be 0xffff if not PCI device */
> > +    uint8_t pci_bus;
> > +    uint8_t pci_device;
> > +    uint8_t pci_function;
> > +    uint32_t pci_flags;
> > +    uint8_t pci_segment;
> > +    uint32_t reserved2;
> > +} AcpiSpcrData;
> > +
> >   #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
> >   #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
> >
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index ff2a310270..44e03e76bd 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -497,4 +497,8 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> >
> >   void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
> >                   const char *oem_id, const char *oem_table_id);
> > +
> > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > +                const AcpiSpcrData *f, const char *oem_id,
> > +                const char *oem_table_id);
> >   #endif

RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[JeeHeng Sia]

> -----Original Message-----
> From: Sunil V L <sunilvl@ventanamicro.com>
> Sent: Monday, January 8, 2024 7:35 PM
> To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> Cc: JeeHeng Sia <jeeheng.sia@starfivetech.com>; qemu-arm@nongnu.org; qemu-devel@nongnu.org; qemu-riscv@nongnu.org;
> mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com; zhiwei_liu@linux.alibaba.com
> Subject: Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> 
> On Fri, Jan 05, 2024 at 09:19:14AM -0300, Daniel Henrique Barboza wrote:
> >
> >
> > On 1/5/24 06:06, Sia Jee Heng wrote:
> > > RISC-V should also generate the SPCR in a manner similar to ARM.
> > > Therefore, instead of replicating the code, relocate this function
> > > to the common AML build.
> > >
> > > Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> > > ---
> > >   hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
> > >   hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
> > >   include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
> > >   include/hw/acpi/aml-build.h |  4 +++
> > >   4 files changed, 115 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index af66bde0f5..1efa534aa8 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> > >       }
> > >   }
> > > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > > +                const AcpiSpcrData *f, const char *oem_id,
> > > +                const char *oem_table_id)
> > > +{
> > > +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> > > +                        .oem_table_id = oem_table_id };
> > > +
> > > +    acpi_table_begin(&table, table_data);
> > > +    /* Interface type */
> > > +    build_append_int_noprefix(table_data, f->interface_type, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data, 0, 3);
> > > +    /* Base Address */
> > > +    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> > > +                     f->base_addr.offset, f->base_addr.size,
> > > +                     f->base_addr.addr);
> > > +    /* Interrupt type */
> > > +    build_append_int_noprefix(table_data, f->interrupt_type, 1);
> > > +    /* IRQ */
> > > +    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
> > > +    /* Global System Interrupt */
> > > +    build_append_int_noprefix(table_data, f->interrupt, 4);
> > > +    /* Baud Rate */
> > > +    build_append_int_noprefix(table_data, f->baud_rate, 1);
> > > +    /* Parity */
> > > +    build_append_int_noprefix(table_data, f->parity, 1);
> > > +    /* Stop Bits */
> > > +    build_append_int_noprefix(table_data, f->stop_bits, 1);
> > > +    /* Flow Control */
> > > +    build_append_int_noprefix(table_data, f->flow_control, 1);
> > > +    /* Terminal Type */
> > > +    build_append_int_noprefix(table_data, f->terminal_type, 1);
> > > +    /* PCI Device ID  */
> > > +    build_append_int_noprefix(table_data, f->pci_device_id, 2);
> > > +    /* PCI Vendor ID */
> > > +    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> > > +    /* PCI Bus Number */
> > > +    build_append_int_noprefix(table_data, f->pci_bus, 1);
> > > +    /* PCI Device Number */
> > > +    build_append_int_noprefix(table_data, f->pci_device, 1);
> > > +    /* PCI Function Number */
> > > +    build_append_int_noprefix(table_data, f->pci_function, 1);
> > > +    /* PCI Flags */
> > > +    build_append_int_noprefix(table_data, f->pci_flags, 4);
> > > +    /* PCI Segment */
> > > +    build_append_int_noprefix(table_data, f->pci_segment, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data, 0, 4);
> > > +
> > > +    acpi_table_end(linker, &table);
> > > +}
> > >   /*
> > >    * ACPI spec, Revision 6.3
> > >    * 5.2.29 Processor Properties Topology Table (PPTT)
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 510ab0dcca..a31f736d1a 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >    * Rev: 1.07
> > >    */
> > >   static void
> > > -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > +build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >
> > Nit: I don't understand the '_v2' in the name of this function. Is it just to not collide
> > with the now public build_spcr()? Or does it have to do with the SPCR table being
> > '.rev = 2'? Because if it's the latter, you can name the common helper 'build_spcr_rev2'
> > (since both ARM and RISC-V use SPCR rev 2), keep this local build_spcr() initializing
> > the AcpiSpcrData struct with ARM attributes and then call the common build_spcr_rev2().
> >
> My suggestion is, keep the build_spcr() generic and take version as the
> parameter.
Alright. I will give them a generic name for both the arch build_spcr() and
the common build_spcr(). The revision info should be passed to the common
build_spcr().
> 
> Thanks,
> Sunil

RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location

[JeeHeng Sia]

> -----Original Message-----
> From: JeeHeng Sia
> Sent: Wednesday, January 10, 2024 4:02 PM
> To: Daniel Henrique Barboza <dbarboza@ventanamicro.com>; qemu-arm@nongnu.org; qemu-devel@nongnu.org; qemu-
> riscv@nongnu.org
> Cc: mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sunilvl@ventanamicro.com; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com;
> zhiwei_liu@linux.alibaba.com
> Subject: RE: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> 
> 
> 
> > -----Original Message-----
> > From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> > Sent: Friday, January 5, 2024 8:19 PM
> > To: JeeHeng Sia <jeeheng.sia@starfivetech.com>; qemu-arm@nongnu.org; qemu-devel@nongnu.org; qemu-riscv@nongnu.org
> > Cc: mst@redhat.com; imammedo@redhat.com; anisinha@redhat.com; peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> > sunilvl@ventanamicro.com; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com;
> > zhiwei_liu@linux.alibaba.com
> > Subject: Re: [RESEND RFC v1 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> >
> >
> >
> > On 1/5/24 06:06, Sia Jee Heng wrote:
> > > RISC-V should also generate the SPCR in a manner similar to ARM.
> > > Therefore, instead of replicating the code, relocate this function
> > > to the common AML build.
> > >
> > > Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> > > ---
> > >   hw/acpi/aml-build.c         | 51 ++++++++++++++++++++++++++++
> > >   hw/arm/virt-acpi-build.c    | 68 +++++++++++++++----------------------
> > >   include/hw/acpi/acpi-defs.h | 33 ++++++++++++++++++
> > >   include/hw/acpi/aml-build.h |  4 +++
> > >   4 files changed, 115 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index af66bde0f5..1efa534aa8 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.c
> > > @@ -1994,6 +1994,57 @@ static void build_processor_hierarchy_node(GArray *tbl, uint32_t flags,
> > >       }
> > >   }
> > >
> > > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > > +                const AcpiSpcrData *f, const char *oem_id,
> > > +                const char *oem_table_id)
> > > +{
> > > +    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = oem_id,
> > > +                        .oem_table_id = oem_table_id };
> > > +
> > > +    acpi_table_begin(&table, table_data);
> > > +    /* Interface type */
> > > +    build_append_int_noprefix(table_data, f->interface_type, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data, 0, 3);
> > > +    /* Base Address */
> > > +    build_append_gas(table_data, f->base_addr.id, f->base_addr.width,
> > > +                     f->base_addr.offset, f->base_addr.size,
> > > +                     f->base_addr.addr);
> > > +    /* Interrupt type */
> > > +    build_append_int_noprefix(table_data, f->interrupt_type, 1);
> > > +    /* IRQ */
> > > +    build_append_int_noprefix(table_data, f->pc_interrupt, 1);
> > > +    /* Global System Interrupt */
> > > +    build_append_int_noprefix(table_data, f->interrupt, 4);
> > > +    /* Baud Rate */
> > > +    build_append_int_noprefix(table_data, f->baud_rate, 1);
> > > +    /* Parity */
> > > +    build_append_int_noprefix(table_data, f->parity, 1);
> > > +    /* Stop Bits */
> > > +    build_append_int_noprefix(table_data, f->stop_bits, 1);
> > > +    /* Flow Control */
> > > +    build_append_int_noprefix(table_data, f->flow_control, 1);
> > > +    /* Terminal Type */
> > > +    build_append_int_noprefix(table_data, f->terminal_type, 1);
> > > +    /* PCI Device ID  */
> > > +    build_append_int_noprefix(table_data, f->pci_device_id, 2);
> > > +    /* PCI Vendor ID */
> > > +    build_append_int_noprefix(table_data, f->pci_vendor_id, 2);
> > > +    /* PCI Bus Number */
> > > +    build_append_int_noprefix(table_data, f->pci_bus, 1);
> > > +    /* PCI Device Number */
> > > +    build_append_int_noprefix(table_data, f->pci_device, 1);
> > > +    /* PCI Function Number */
> > > +    build_append_int_noprefix(table_data, f->pci_function, 1);
> > > +    /* PCI Flags */
> > > +    build_append_int_noprefix(table_data, f->pci_flags, 4);
> > > +    /* PCI Segment */
> > > +    build_append_int_noprefix(table_data, f->pci_segment, 1);
> > > +    /* Reserved */
> > > +    build_append_int_noprefix(table_data, 0, 4);
> > > +
> > > +    acpi_table_end(linker, &table);
> > > +}
> > >   /*
> > >    * ACPI spec, Revision 6.3
> > >    * 5.2.29 Processor Properties Topology Table (PPTT)
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 510ab0dcca..a31f736d1a 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -431,48 +431,34 @@ build_iort(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > >    * Rev: 1.07
> > >    */
> > >   static void
> > > -build_spcr(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> > > +build_spcr_v2(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >
> > Nit: I don't understand the '_v2' in the name of this function. Is it just to not collide
> > with the now public build_spcr()? Or does it have to do with the SPCR table being
> > '.rev = 2'? Because if it's the latter, you can name the common helper 'build_spcr_rev2'
> > (since both ARM and RISC-V use SPCR rev 2), keep this local build_spcr() initializing
> > the AcpiSpcrData struct with ARM attributes and then call the common build_spcr_rev2().
> Sorry for the confusion. I will give them a generic name for both the arch build_spcr()
> and the common build_spcr(). The revision info should be passed to the common
> build_spcr().
> >
> >
> > Code looks good othewise. Thanks,
Btw, as Sunil's code [1] has been merged into Alistair's riscv-to-apply.next,
I will remove the word "RFC" from the next submission.
Hope you guys are okay.
> >
> >
> > Daniel
> >
> >
> >
> > >   {
> > > -    AcpiTable table = { .sig = "SPCR", .rev = 2, .oem_id = vms->oem_id,
> > > -                        .oem_table_id = vms->oem_table_id };
> > > -
> > > -    acpi_table_begin(&table, table_data);
> > > -
> > > -    /* Interface Type */
> > > -    build_append_int_noprefix(table_data, 3, 1); /* ARM PL011 UART */
> > > -    build_append_int_noprefix(table_data, 0, 3); /* Reserved */
> > > -    /* Base Address */
> > > -    build_append_gas(table_data, AML_AS_SYSTEM_MEMORY, 32, 0, 3,
> > > -                     vms->memmap[VIRT_UART].base);
> > > -    /* Interrupt Type */
> > > -    build_append_int_noprefix(table_data,
> > > -        (1 << 3) /* Bit[3] ARMH GIC interrupt */, 1);
> > > -    build_append_int_noprefix(table_data, 0, 1); /* IRQ */
> > > -    /* Global System Interrupt */
> > > -    build_append_int_noprefix(table_data,
> > > -                              vms->irqmap[VIRT_UART] + ARM_SPI_BASE, 4);
> > > -    build_append_int_noprefix(table_data, 3 /* 9600 */, 1); /* Baud Rate */
> > > -    build_append_int_noprefix(table_data, 0 /* No Parity */, 1); /* Parity */
> > > -    /* Stop Bits */
> > > -    build_append_int_noprefix(table_data, 1 /* 1 Stop bit */, 1);
> > > -    /* Flow Control */
> > > -    build_append_int_noprefix(table_data,
> > > -        (1 << 1) /* RTS/CTS hardware flow control */, 1);
> > > -    /* Terminal Type */
> > > -    build_append_int_noprefix(table_data, 0 /* VT100 */, 1);
> > > -    build_append_int_noprefix(table_data, 0, 1); /* Language */
> > > -    /* PCI Device ID  */
> > > -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> > > -    /* PCI Vendor ID */
> > > -    build_append_int_noprefix(table_data, 0xffff /* not a PCI device*/, 2);
> > > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
> > > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Device Number */
> > > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Function Number */
> > > -    build_append_int_noprefix(table_data, 0, 4); /* PCI Flags */
> > > -    build_append_int_noprefix(table_data, 0, 1); /* PCI Segment */
> > > -    build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> > > +    AcpiSpcrData serial = {
> > > +        .interface_type = 3,       /* ARM PL011 UART */
> > > +        .base_addr.id = AML_AS_SYSTEM_MEMORY,
> > > +        .base_addr.width = 32,
> > > +        .base_addr.offset = 0,
> > > +        .base_addr.size = 3,
> > > +        .base_addr.addr = vms->memmap[VIRT_UART].base,
> > > +        .interrupt_type = (1 << 3),/* Bit[3] ARMH GIC interrupt*/
> > > +        .pc_interrupt = 0,         /* IRQ */
> > > +        .interrupt = (vms->irqmap[VIRT_UART] + ARM_SPI_BASE),
> > > +        .baud_rate = 3,            /* 9600 */
> > > +        .parity = 0,               /* No Parity */
> > > +        .stop_bits = 1,            /* 1 Stop bit */
> > > +        .flow_control = 1 << 1,    /* RTS/CTS hardware flow control */
> > > +        .terminal_type = 0,        /* VT100 */
> > > +        .language = 0,             /* Language */
> > > +        .pci_device_id = 0xffff,   /* not a PCI device*/
> > > +        .pci_vendor_id = 0xffff,   /* not a PCI device*/
> > > +        .pci_bus = 0,
> > > +        .pci_device = 0,
> > > +        .pci_function = 0,
> > > +        .pci_flags = 0,
> > > +        .pci_segment = 0,
> > > +    };
> > >
> > > -    acpi_table_end(linker, &table);
> > > +    build_spcr(table_data, linker, &serial, vms->oem_id, vms->oem_table_id);
> > >   }
> > >
> > >   /*
> > > @@ -930,7 +916,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> > >       }
> > >
> > >       acpi_add_table(table_offsets, tables_blob);
> > > -    build_spcr(tables_blob, tables->linker, vms);
> > > +    build_spcr_v2(tables_blob, tables->linker, vms);
> > >
> > >       acpi_add_table(table_offsets, tables_blob);
> > >       build_dbg2(tables_blob, tables->linker, vms);
> > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > > index 2b42e4192b..0e6e82b339 100644
> > > --- a/include/hw/acpi/acpi-defs.h
> > > +++ b/include/hw/acpi/acpi-defs.h
> > > @@ -90,6 +90,39 @@ typedef struct AcpiFadtData {
> > >       unsigned *xdsdt_tbl_offset;
> > >   } AcpiFadtData;
> > >
> > > +typedef struct AcpiGas {
> > > +    uint8_t id;                /* Address space ID */
> > > +    uint8_t width;             /* Register bit width */
> > > +    uint8_t offset;            /* Register bit offset */
> > > +    uint8_t size;              /* Access size */
> > > +    uint64_t addr;             /* Address */
> > > +} AcpiGas;
> > > +
> > > +/* SPCR (Serial Port Console Redirection table) */
> > > +typedef struct AcpiSpcrData {
> > > +    uint8_t interface_type;
> > > +    uint8_t reserved[3];
> > > +    struct AcpiGas base_addr;
> > > +    uint8_t interrupt_type;
> > > +    uint8_t pc_interrupt;
> > > +    uint32_t interrupt;        /* Global system interrupt */
> > > +    uint8_t baud_rate;
> > > +    uint8_t parity;
> > > +    uint8_t stop_bits;
> > > +    uint8_t flow_control;
> > > +    uint8_t terminal_type;
> > > +    uint8_t language;
> > > +    uint8_t reserved1;
> > > +    uint16_t pci_device_id;    /* Must be 0xffff if not PCI device */
> > > +    uint16_t pci_vendor_id;    /* Must be 0xffff if not PCI device */
> > > +    uint8_t pci_bus;
> > > +    uint8_t pci_device;
> > > +    uint8_t pci_function;
> > > +    uint32_t pci_flags;
> > > +    uint8_t pci_segment;
> > > +    uint32_t reserved2;
> > > +} AcpiSpcrData;
> > > +
> > >   #define ACPI_FADT_ARM_PSCI_COMPLIANT  (1 << 0)
> > >   #define ACPI_FADT_ARM_PSCI_USE_HVC    (1 << 1)
> > >
> > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > > index ff2a310270..44e03e76bd 100644
> > > --- a/include/hw/acpi/aml-build.h
> > > +++ b/include/hw/acpi/aml-build.h
> > > @@ -497,4 +497,8 @@ void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
> > >
> > >   void build_tpm2(GArray *table_data, BIOSLinker *linker, GArray *tcpalog,
> > >                   const char *oem_id, const char *oem_table_id);
> > > +
> > > +void build_spcr(GArray *table_data, BIOSLinker *linker,
> > > +                const AcpiSpcrData *f, const char *oem_id,
> > > +                const char *oem_table_id);
> > >   #endif