Re: RE: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On Tue, Jan 30, 2024 at 03:30:13AM +, JeeHeng Sia wrote: ... > > Sharing code is good, but if we have to parametrize the entire table, then > > we might as well keep Arm and RISCV separate. Building the table first > > with this struct, just to have it built again with the build_append API, > > doesn't make much sense to me. Do Arm and riscv really diverge on all > > these parameters? If not, then just add the parameters which do diverge > > build_scpr's arguments. > It is kind of chicken and egg thing, I would suggest let the arch code to > fill in the value. It doesn't make sense to change again in the future when > both riscv and arm realized the parameters were different. > Can arm confirm that these values wouldn't change in the future? > > We can't be sure that arm nor riscv will change in the future, but we (arm/riscv/etc. QEMU developers) control the code for both, so I don't see a problem with only parametrizing what's necessary today and then extending that, or completely separating, later if necessary. In any case, I'd rather see the two completely separate from the start, than to see the structure with all the parameters get added. There's no difference to me when reading a list of 's->param_name = value' or a list of build_append_int(table, value, size) /* param_name */. And, given we need the later eventually anyway, then there's no reason for the former at all. Thanks, drew
Re: RE: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On Tue, Jan 30, 2024 at 03:16:34AM +, JeeHeng Sia wrote: ... > > I think either there should be a comment that this supports only v2 of > > SPCR spec or it should be able to create SPCR of any version. IMO, I > > think it is better to add support till v4 (latest). Since consumers like > > Linux probably doesn't support v4 yet, ARM/RISC-V can continue to create > > v2 itself for the time being but the generic build_spcr() should be able > > to create v4 also if the arch requires it. > A v4 table depends on the updated acpica. I am not aware if there is a > request from ARM to update to v4. Anyway, RISC-V BRS Spec did mentioned > on poll-based sbi console. I can check with acpica community if updating > table to v4 is the go otherwise I would suggest we cont stick to v2 because > there is no compatible ACPI guest to test the code. > > Generally we want to produce the oldest version which meets the requirements in order to support the widest set of consumers. Thanks, drew
RE: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: Andrew Jones
> Sent: Monday, January 29, 2024 5:19 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> suni...@ventanamicro.com; pal...@dabbelt.com;
> alistair.fran...@wdc.com; bin.m...@windriver.com; liwei1...@gmail.com;
> dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation
> to common location
>
> On Sun, Jan 28, 2024 at 06:14:39PM -0800, 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
> > ---
> > 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..f3904650e4 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 uint8_t rev,
> > +const char *oem_id, const char *oem_table_id)
> > +{
> > +AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> > +.oem_table_id = oem_table_id };
> > +
> > +acpi_table_begin(, 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, );
> > +}
> > /*
> > * 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 a22a2f43a5..195767c0f0 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)
> > +spcr_setup(GArray *table_data, BIOSLinker *linker, VirtMachineState
RE: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
> -Original Message-
> From: Sunil V L
> Sent: Monday, January 29, 2024 1:13 PM
> To: JeeHeng Sia
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org; qemu-ri...@nongnu.org;
> m...@redhat.com; imamm...@redhat.com;
> anisi...@redhat.com; peter.mayd...@linaro.org; shannon.zha...@gmail.com;
> pal...@dabbelt.com; alistair.fran...@wdc.com;
> bin.m...@windriver.com; liwei1...@gmail.com; dbarb...@ventanamicro.com;
> zhiwei_...@linux.alibaba.com
> Subject: Re: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation
> to common location
>
> Hi Jee Heng,
>
> On Sun, Jan 28, 2024 at 06:14:39PM -0800, 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
> > ---
> > 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..f3904650e4 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 uint8_t rev,
> > +const char *oem_id, const char *oem_table_id)
> > +{
> > +AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> > +.oem_table_id = oem_table_id };
> > +
> > +acpi_table_begin(, 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);
> > +
> I think either there should be a comment that this supports only v2 of
> SPCR spec or it should be able to create SPCR of any version. IMO, I
> think it is better to add support till v4 (latest). Since consumers like
> Linux probably doesn't support v4 yet, ARM/RISC-V can continue to create
> v2 itself for the time being but the generic build_spcr() should be able
> to create v4 also if the arch requires it.
A v4 table depends on the updated acpica. I am not aware if there is a
request from ARM to update to v4. Anyway, RISC-V BRS Spec did mentioned
on poll-based sbi console. I can check with acpica community if updating
table to v4 is the go otherwise I would suggest we cont stick to v2 because
ther
Re: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
On Sun, Jan 28, 2024 at 06:14:39PM -0800, 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
> ---
> 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..f3904650e4 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 uint8_t rev,
> +const char *oem_id, const char *oem_table_id)
> +{
> +AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> +.oem_table_id = oem_table_id };
> +
> +acpi_table_begin(, 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, );
> +}
> /*
> * 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 a22a2f43a5..195767c0f0 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)
> +spcr_setup(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_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); /*
Re: [RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
Hi Jee Heng,
On Sun, Jan 28, 2024 at 06:14:39PM -0800, 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
> ---
> 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..f3904650e4 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 uint8_t rev,
> +const char *oem_id, const char *oem_table_id)
> +{
> +AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
> +.oem_table_id = oem_table_id };
> +
> +acpi_table_begin(, 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);
> +
I think either there should be a comment that this supports only v2 of
SPCR spec or it should be able to create SPCR of any version. IMO, I
think it is better to add support till v4 (latest). Since consumers like
Linux probably doesn't support v4 yet, ARM/RISC-V can continue to create
v2 itself for the time being but the generic build_spcr() should be able
to create v4 also if the arch requires it.
Thanks,
Sunil
> +acpi_table_end(linker, );
> +}
> /*
> * 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 a22a2f43a5..195767c0f0 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)
> +spcr_setup(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_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 */
> -
[RESEND v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
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
---
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..f3904650e4 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 uint8_t rev,
+const char *oem_id, const char *oem_table_id)
+{
+AcpiTable table = { .sig = "SPCR", .rev = rev, .oem_id = oem_id,
+.oem_table_id = oem_table_id };
+
+acpi_table_begin(, 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, );
+}
/*
* 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 a22a2f43a5..195767c0f0 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)
+spcr_setup(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_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, 0x /* not a PCI device*/, 2);
-/* PCI Vendor ID */
-build_append_int_noprefix(table_data, 0x /* not a PCI device*/, 2);
-build_append_int_noprefix(table_data, 0, 1); /* PCI Bus Number */
-
