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
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
> -----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
> -----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
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
> -----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
© 2016 - 2024 Red Hat, Inc.