* [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR
@ 2024-01-16 1:09 Sia Jee Heng
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Sia Jee Heng @ 2024-01-16 1:09 UTC (permalink / raw)
To: qemu-arm, qemu-devel, qemu-riscv
Cc: mst, imammedo, anisinha, peter.maydell, jeeheng.sia,
shannon.zhaosl, sunilvl, palmer, alistair.francis, bin.meng,
liwei1518, dbarboza, zhiwei_liu
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
Changes in v2:
- Renamed the build_spcr_rev2() function to spcr_setup().
- SPCR table version is passed from spcr_setup() to the common
build_spcr() function.
- Added "Reviewed-by" from Daniel for patch 2.
- The term 'RFC' has been removed from this series, as the dependency code
from [1] has been merged into Alistair's riscv-to-apply.next branch. The
first series of this patch can be found at [2].
[1] https://lore.kernel.org/qemu-devel/20231218150247.466427-1-sunilvl@ventanamicro.com/
[2] https://lore.kernel.org/qemu-devel/20240105090608.5745-1-jeeheng.sia@starfivetech.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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-01-16 1:09 [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Sia Jee Heng
@ 2024-01-16 1:09 ` Sia Jee Heng
2024-02-15 3:26 ` Alistair Francis
2024-03-06 18:57 ` Daniel Henrique Barboza
2024-01-16 1:09 ` [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table Sia Jee Heng
2024-02-15 9:54 ` [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Alistair Francis
2 siblings, 2 replies; 13+ messages in thread
From: Sia Jee Heng @ 2024-01-16 1:09 UTC (permalink / raw)
To: qemu-arm, qemu-devel, qemu-riscv
Cc: mst, imammedo, anisinha, peter.maydell, jeeheng.sia,
shannon.zhaosl, sunilvl, palmer, alistair.francis, bin.meng,
liwei1518, dbarboza, zhiwei_liu
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..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, 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 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, 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, 2, 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);
+ spcr_setup(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..a3784155cb 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 uint8_t rev,
+ const char *oem_id, const char *oem_table_id);
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table
2024-01-16 1:09 [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Sia Jee Heng
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
@ 2024-01-16 1:09 ` Sia Jee Heng
2024-02-15 3:29 ` Alistair Francis
2024-02-15 9:54 ` [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Alistair Francis
2 siblings, 1 reply; 13+ messages in thread
From: Sia Jee Heng @ 2024-01-16 1:09 UTC (permalink / raw)
To: qemu-arm, qemu-devel, qemu-riscv
Cc: mst, imammedo, anisinha, peter.maydell, jeeheng.sia,
shannon.zhaosl, sunilvl, palmer, alistair.francis, bin.meng,
liwei1518, dbarboza, zhiwei_liu
Generate Serial Port Console Redirection Table (SPCR) for RISC-V
virtual machine.
Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.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 26c7e4482d..7fc5071c84 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
+spcr_setup(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, 2, 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);
+ spcr_setup(tables_blob, tables->linker, s);
+
acpi_add_table(table_offsets, tables_blob);
{
AcpiMcfgInfo mcfg = {
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
@ 2024-02-15 3:26 ` Alistair Francis
2024-03-06 18:57 ` Daniel Henrique Barboza
1 sibling, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2024-02-15 3:26 UTC (permalink / raw)
To: Sia Jee Heng
Cc: qemu-arm, qemu-devel, qemu-riscv, mst, imammedo, anisinha,
peter.maydell, shannon.zhaosl, sunilvl, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu
On Tue, Jan 16, 2024 at 11:11 AM Sia Jee Heng
<jeeheng.sia@starfivetech.com> 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>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> 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, 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 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, 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, 2, 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);
> + spcr_setup(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..a3784155cb 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 uint8_t rev,
> + const char *oem_id, const char *oem_table_id);
> #endif
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table
2024-01-16 1:09 ` [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table Sia Jee Heng
@ 2024-02-15 3:29 ` Alistair Francis
0 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2024-02-15 3:29 UTC (permalink / raw)
To: Sia Jee Heng
Cc: qemu-arm, qemu-devel, qemu-riscv, mst, imammedo, anisinha,
peter.maydell, shannon.zhaosl, sunilvl, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu
On Tue, Jan 16, 2024 at 11:11 AM Sia Jee Heng
<jeeheng.sia@starfivetech.com> wrote:
>
> Generate Serial Port Console Redirection Table (SPCR) for RISC-V
> virtual machine.
>
> Signed-off-by: Sia Jee Heng <jeeheng.sia@starfivetech.com>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> 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 26c7e4482d..7fc5071c84 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
> +spcr_setup(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, 2, 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);
> + spcr_setup(tables_blob, tables->linker, s);
> +
> acpi_add_table(table_offsets, tables_blob);
> {
> AcpiMcfgInfo mcfg = {
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR
2024-01-16 1:09 [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Sia Jee Heng
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
2024-01-16 1:09 ` [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table Sia Jee Heng
@ 2024-02-15 9:54 ` Alistair Francis
2 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2024-02-15 9:54 UTC (permalink / raw)
To: Sia Jee Heng
Cc: qemu-arm, qemu-devel, qemu-riscv, mst, imammedo, anisinha,
peter.maydell, shannon.zhaosl, sunilvl, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu
On Tue, Jan 16, 2024 at 11:11 AM Sia Jee Heng
<jeeheng.sia@starfivetech.com> wrote:
>
> 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
>
> Changes in v2:
> - Renamed the build_spcr_rev2() function to spcr_setup().
> - SPCR table version is passed from spcr_setup() to the common
> build_spcr() function.
> - Added "Reviewed-by" from Daniel for patch 2.
> - The term 'RFC' has been removed from this series, as the dependency code
> from [1] has been merged into Alistair's riscv-to-apply.next branch. The
> first series of this patch can be found at [2].
>
> [1] https://lore.kernel.org/qemu-devel/20231218150247.466427-1-sunilvl@ventanamicro.com/
> [2] https://lore.kernel.org/qemu-devel/20240105090608.5745-1-jeeheng.sia@starfivetech.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
Thanks!
Applied to riscv-to-apply.next
Alistair
>
> 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
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
2024-02-15 3:26 ` Alistair Francis
@ 2024-03-06 18:57 ` Daniel Henrique Barboza
2024-03-06 20:00 ` Daniel Henrique Barboza
2024-03-07 1:33 ` Alistair Francis
1 sibling, 2 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2024-03-06 18:57 UTC (permalink / raw)
To: Sia Jee Heng, qemu-arm, qemu-devel, qemu-riscv
Cc: mst, imammedo, anisinha, peter.maydell, shannon.zhaosl, sunilvl,
palmer, alistair.francis, bin.meng, liwei1518, zhiwei_liu
Hi,
This patch break check-qtest, most specifically 'bios-table'test', for aarch64.
I found this while running riscv-to-apply.next in the Gitlab pipeline.
Here's the output:
$ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 ./tests/qtest/bios-tables-test
TAP version 13
# random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
# starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine none -accel qtest
1..8
# Start of aarch64 tests
# Start of acpi tests
# starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
acpi-test: Warning! SPCR binary file mismatch. Actual [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, aml:tests/data/acpi/virt/SPCR].
The diff is here:
--- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
+++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
@@ -1,57 +1,49 @@
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20220331 (64-bit version)
* Copyright (c) 2000 - 2022 Intel Corporation
(...)
[000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection Table]
-[004h 0004 4] Table Length : 00000050
+[004h 0004 4] Table Length : 0000004F
[008h 0008 1] Revision : 02
-[009h 0009 1] Checksum : B1
+[009h 0009 1] Checksum : B2
[00Ah 0010 6] Oem ID : "BOCHS "
(...)
-[042h 0066 2] PCI Vendor ID : FFFF
+[042h 0066 2] PCI Vendor ID : 00FF
After inspecting the common helper and what the original ARM code was doing
I found out that we're missing something down there:
On 1/15/24 22:09, 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..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, 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);
Here. We're missing the "Language" entry.
This diff fixes the broken test:
$ git diff
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f3904650e4..6d4517cfbe 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
build_append_int_noprefix(table_data, f->stop_bits, 1);
/* Flow Control */
build_append_int_noprefix(table_data, f->flow_control, 1);
+ /* Language */
+ build_append_int_noprefix(table_data, f->language, 1);
/* Terminal Type */
build_append_int_noprefix(table_data, f->terminal_type, 1);
/* PCI Device ID */
As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
is intentional or a foresight.
Thanks,
Daniel
> + /* 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 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, 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, 2, 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);
> + spcr_setup(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..a3784155cb 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 uint8_t rev,
> + const char *oem_id, const char *oem_table_id);
> #endif
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-03-06 18:57 ` Daniel Henrique Barboza
@ 2024-03-06 20:00 ` Daniel Henrique Barboza
2024-03-07 1:33 ` Alistair Francis
1 sibling, 0 replies; 13+ messages in thread
From: Daniel Henrique Barboza @ 2024-03-06 20:00 UTC (permalink / raw)
To: Sia Jee Heng, qemu-arm, qemu-devel, qemu-riscv
Cc: mst, imammedo, anisinha, peter.maydell, shannon.zhaosl, sunilvl,
palmer, alistair.francis, bin.meng, liwei1518, zhiwei_liu
On 3/6/24 15:57, Daniel Henrique Barboza wrote:
> As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
> is intentional or a foresight.
s/foresight/hindsight
There's no 'make check' for what we want to say in the ML but hopefully there's
a way to enable 'bios-table-test' for RISC-V :D
Daniel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-03-06 18:57 ` Daniel Henrique Barboza
2024-03-06 20:00 ` Daniel Henrique Barboza
@ 2024-03-07 1:33 ` Alistair Francis
2024-03-07 1:42 ` JeeHeng Sia
2024-03-07 3:45 ` Sunil V L
1 sibling, 2 replies; 13+ messages in thread
From: Alistair Francis @ 2024-03-07 1:33 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Sia Jee Heng, qemu-arm, qemu-devel, qemu-riscv, mst, imammedo,
anisinha, peter.maydell, shannon.zhaosl, sunilvl, palmer,
alistair.francis, bin.meng, liwei1518, zhiwei_liu
On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> This patch break check-qtest, most specifically 'bios-table'test', for aarch64.
> I found this while running riscv-to-apply.next in the Gitlab pipeline.
>
>
> Here's the output:
>
> $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 ./tests/qtest/bios-tables-test
> TAP version 13
> # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine none -accel qtest
> 1..8
> # Start of aarch64 tests
> # Start of acpi tests
> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
> acpi-test: Warning! SPCR binary file mismatch. Actual [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, aml:tests/data/acpi/virt/SPCR].
>
> The diff is here:
>
> --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
> +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
> @@ -1,57 +1,49 @@
> /*
> * Intel ACPI Component Architecture
> * AML/ASL+ Disassembler version 20220331 (64-bit version)
> * Copyright (c) 2000 - 2022 Intel Corporation
>
> (...)
>
> [000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection Table]
> -[004h 0004 4] Table Length : 00000050
> +[004h 0004 4] Table Length : 0000004F
> [008h 0008 1] Revision : 02
> -[009h 0009 1] Checksum : B1
> +[009h 0009 1] Checksum : B2
> [00Ah 0010 6] Oem ID : "BOCHS "
>
> (...)
>
> -[042h 0066 2] PCI Vendor ID : FFFF
> +[042h 0066 2] PCI Vendor ID : 00FF
>
>
> After inspecting the common helper and what the original ARM code was doing
> I found out that we're missing something down there:
>
>
> On 1/15/24 22:09, 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..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, 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);
>
> Here. We're missing the "Language" entry.
>
>
> This diff fixes the broken test:
>
>
> $ git diff
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index f3904650e4..6d4517cfbe 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
> build_append_int_noprefix(table_data, f->stop_bits, 1);
> /* Flow Control */
> build_append_int_noprefix(table_data, f->flow_control, 1);
> + /* Language */
> + build_append_int_noprefix(table_data, f->language, 1);
> /* Terminal Type */
> build_append_int_noprefix(table_data, f->terminal_type, 1);
> /* PCI Device ID */
Thanks! I added this to the patch
>
>
>
> As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
> is intentional or a foresight.
I think you are looking for oversight :)
I think it's just an oversight, not intentional.
Alistair
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-03-07 1:33 ` Alistair Francis
@ 2024-03-07 1:42 ` JeeHeng Sia
2024-03-07 3:45 ` Sunil V L
1 sibling, 0 replies; 13+ messages in thread
From: JeeHeng Sia @ 2024-03-07 1:42 UTC (permalink / raw)
To: Alistair Francis, Daniel Henrique Barboza
Cc: 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,
sunilvl@ventanamicro.com, palmer@dabbelt.com,
alistair.francis@wdc.com, bin.meng@windriver.com,
liwei1518@gmail.com, zhiwei_liu@linux.alibaba.com
> -----Original Message-----
> From: Alistair Francis <alistair23@gmail.com>
> Sent: Thursday, March 7, 2024 9:33 AM
> 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;
> sunilvl@ventanamicro.com; palmer@dabbelt.com; alistair.francis@wdc.com; bin.meng@windriver.com; liwei1518@gmail.com;
> zhiwei_liu@linux.alibaba.com
> Subject: Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
>
> On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > Hi,
> >
> > This patch break check-qtest, most specifically 'bios-table'test', for aarch64.
> > I found this while running riscv-to-apply.next in the Gitlab pipeline.
> >
> >
> > Here's the output:
> >
> > $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 ./tests/qtest/bios-tables-test
> > TAP version 13
> > # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
> > # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev
> socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine none -accel
> qtest
> > 1..8
> > # Start of aarch64 tests
> > # Start of acpi tests
> > # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev
> socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg
> -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive
> if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-
> test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
> > acpi-test: Warning! SPCR binary file mismatch. Actual [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
> > See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> > acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl,
> aml:tests/data/acpi/virt/SPCR].
> >
> > The diff is here:
> >
> > --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
> > +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
> > @@ -1,57 +1,49 @@
> > /*
> > * Intel ACPI Component Architecture
> > * AML/ASL+ Disassembler version 20220331 (64-bit version)
> > * Copyright (c) 2000 - 2022 Intel Corporation
> >
> > (...)
> >
> > [000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection Table]
> > -[004h 0004 4] Table Length : 00000050
> > +[004h 0004 4] Table Length : 0000004F
> > [008h 0008 1] Revision : 02
> > -[009h 0009 1] Checksum : B1
> > +[009h 0009 1] Checksum : B2
> > [00Ah 0010 6] Oem ID : "BOCHS "
> >
> > (...)
> >
> > -[042h 0066 2] PCI Vendor ID : FFFF
> > +[042h 0066 2] PCI Vendor ID : 00FF
> >
> >
> > After inspecting the common helper and what the original ARM code was doing
> > I found out that we're missing something down there:
> >
> >
> > On 1/15/24 22:09, 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..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, 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);
> >
> > Here. We're missing the "Language" entry.
> >
> >
> > This diff fixes the broken test:
> >
> >
> > $ git diff
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index f3904650e4..6d4517cfbe 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
> > build_append_int_noprefix(table_data, f->stop_bits, 1);
> > /* Flow Control */
> > build_append_int_noprefix(table_data, f->flow_control, 1);
> > + /* Language */
> > + build_append_int_noprefix(table_data, f->language, 1);
> > /* Terminal Type */
> > build_append_int_noprefix(table_data, f->terminal_type, 1);
> > /* PCI Device ID */
>
> Thanks! I added this to the patch
>
> >
> >
> >
> > As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
> > is intentional or a foresight.
>
> I think you are looking for oversight :)
>
> I think it's just an oversight, not intentional.
I apologize for the oversight I've made that caused the trouble.
Thanks for the quick turnaround.
>
> Alistair
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-03-07 1:33 ` Alistair Francis
2024-03-07 1:42 ` JeeHeng Sia
@ 2024-03-07 3:45 ` Sunil V L
2024-03-07 9:22 ` Daniel Henrique Barboza
1 sibling, 1 reply; 13+ messages in thread
From: Sunil V L @ 2024-03-07 3:45 UTC (permalink / raw)
To: Alistair Francis
Cc: Daniel Henrique Barboza, Sia Jee Heng, qemu-arm, qemu-devel,
qemu-riscv, mst, imammedo, anisinha, peter.maydell,
shannon.zhaosl, palmer, alistair.francis, bin.meng, liwei1518,
zhiwei_liu
On Thu, Mar 07, 2024 at 11:33:25AM +1000, Alistair Francis wrote:
> On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
> >
> > Hi,
> >
> > This patch break check-qtest, most specifically 'bios-table'test', for aarch64.
> > I found this while running riscv-to-apply.next in the Gitlab pipeline.
> >
> >
> > Here's the output:
> >
> > $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 ./tests/qtest/bios-tables-test
> > TAP version 13
> > # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
> > # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine none -accel qtest
> > 1..8
> > # Start of aarch64 tests
> > # Start of acpi tests
> > # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
> > acpi-test: Warning! SPCR binary file mismatch. Actual [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
> > See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> > acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, aml:tests/data/acpi/virt/SPCR].
> >
> > The diff is here:
> >
> > --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
> > +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
> > @@ -1,57 +1,49 @@
> > /*
> > * Intel ACPI Component Architecture
> > * AML/ASL+ Disassembler version 20220331 (64-bit version)
> > * Copyright (c) 2000 - 2022 Intel Corporation
> >
> > (...)
> >
> > [000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection Table]
> > -[004h 0004 4] Table Length : 00000050
> > +[004h 0004 4] Table Length : 0000004F
> > [008h 0008 1] Revision : 02
> > -[009h 0009 1] Checksum : B1
> > +[009h 0009 1] Checksum : B2
> > [00Ah 0010 6] Oem ID : "BOCHS "
> >
> > (...)
> >
> > -[042h 0066 2] PCI Vendor ID : FFFF
> > +[042h 0066 2] PCI Vendor ID : 00FF
> >
> >
> > After inspecting the common helper and what the original ARM code was doing
> > I found out that we're missing something down there:
> >
> >
> > On 1/15/24 22:09, 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..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, 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);
> >
> > Here. We're missing the "Language" entry.
> >
> >
> > This diff fixes the broken test:
> >
> >
> > $ git diff
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index f3904650e4..6d4517cfbe 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
> > build_append_int_noprefix(table_data, f->stop_bits, 1);
> > /* Flow Control */
> > build_append_int_noprefix(table_data, f->flow_control, 1);
> > + /* Language */
> > + build_append_int_noprefix(table_data, f->language, 1);
> > /* Terminal Type */
> > build_append_int_noprefix(table_data, f->terminal_type, 1);
> > /* PCI Device ID */
>
> Thanks! I added this to the patch
>
> >
> >
> >
> > As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
> > is intentional or a foresight.
>
> I think you are looking for oversight :)
>
> I think it's just an oversight, not intentional.
>
Adding these test cases is WIP. Drew had already pointed this. Myself
and Haibo are looking at it. But I doubt patches can be sent before 9.0
deadlines. Can we add them post 9.0?
Thanks,
Sunil
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-03-07 3:45 ` Sunil V L
@ 2024-03-07 9:22 ` Daniel Henrique Barboza
2024-03-20 4:41 ` Alistair Francis
0 siblings, 1 reply; 13+ messages in thread
From: Daniel Henrique Barboza @ 2024-03-07 9:22 UTC (permalink / raw)
To: Sunil V L, Alistair Francis
Cc: Sia Jee Heng, qemu-arm, qemu-devel, qemu-riscv, mst, imammedo,
anisinha, peter.maydell, shannon.zhaosl, palmer, alistair.francis,
bin.meng, liwei1518, zhiwei_liu
On 3/7/24 00:45, Sunil V L wrote:
> On Thu, Mar 07, 2024 at 11:33:25AM +1000, Alistair Francis wrote:
>> On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza
>> <dbarboza@ventanamicro.com> wrote:
>>>
>>> Hi,
>>>
>>> This patch break check-qtest, most specifically 'bios-table'test', for aarch64.
>>> I found this while running riscv-to-apply.next in the Gitlab pipeline.
>>>
>>>
>>> Here's the output:
>>>
>>> $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 ./tests/qtest/bios-tables-test
>>> TAP version 13
>>> # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine none -accel qtest
>>> 1..8
>>> # Start of aarch64 tests
>>> # Start of acpi tests
>>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
>>> acpi-test: Warning! SPCR binary file mismatch. Actual [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
>>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
>>> acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, aml:tests/data/acpi/virt/SPCR].
>>>
>>> The diff is here:
>>>
>>> --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
>>> +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
>>> @@ -1,57 +1,49 @@
>>> /*
>>> * Intel ACPI Component Architecture
>>> * AML/ASL+ Disassembler version 20220331 (64-bit version)
>>> * Copyright (c) 2000 - 2022 Intel Corporation
>>>
>>> (...)
>>>
>>> [000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection Table]
>>> -[004h 0004 4] Table Length : 00000050
>>> +[004h 0004 4] Table Length : 0000004F
>>> [008h 0008 1] Revision : 02
>>> -[009h 0009 1] Checksum : B1
>>> +[009h 0009 1] Checksum : B2
>>> [00Ah 0010 6] Oem ID : "BOCHS "
>>>
>>> (...)
>>>
>>> -[042h 0066 2] PCI Vendor ID : FFFF
>>> +[042h 0066 2] PCI Vendor ID : 00FF
>>>
>>>
>>> After inspecting the common helper and what the original ARM code was doing
>>> I found out that we're missing something down there:
>>>
>>>
>>> On 1/15/24 22:09, 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..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, 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);
>>>
>>> Here. We're missing the "Language" entry.
>>>
>>>
>>> This diff fixes the broken test:
>>>
>>>
>>> $ git diff
>>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>>> index f3904650e4..6d4517cfbe 100644
>>> --- a/hw/acpi/aml-build.c
>>> +++ b/hw/acpi/aml-build.c
>>> @@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
>>> build_append_int_noprefix(table_data, f->stop_bits, 1);
>>> /* Flow Control */
>>> build_append_int_noprefix(table_data, f->flow_control, 1);
>>> + /* Language */
>>> + build_append_int_noprefix(table_data, f->language, 1);
>>> /* Terminal Type */
>>> build_append_int_noprefix(table_data, f->terminal_type, 1);
>>> /* PCI Device ID */
>>
>> Thanks! I added this to the patch
>>
>>>
>>>
>>>
>>> As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
>>> is intentional or a foresight.
>>
>> I think you are looking for oversight :)
>>
>> I think it's just an oversight, not intentional.
>>
> Adding these test cases is WIP. Drew had already pointed this. Myself
> and Haibo are looking at it. But I doubt patches can be sent before 9.0
> deadlines. Can we add them post 9.0?
I don't recall the last time I had to deal with a new test being added after the
code freeze. It's not a new feature, it increases test coverage and it might find
bugs, so my guess is that it's fine.
Alistair/Peter, any thoughts?
Thanks,
Daniel
>
> Thanks,
> Sunil
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
2024-03-07 9:22 ` Daniel Henrique Barboza
@ 2024-03-20 4:41 ` Alistair Francis
0 siblings, 0 replies; 13+ messages in thread
From: Alistair Francis @ 2024-03-20 4:41 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: Sunil V L, Sia Jee Heng, qemu-arm, qemu-devel, qemu-riscv, mst,
imammedo, anisinha, peter.maydell, shannon.zhaosl, palmer,
alistair.francis, bin.meng, liwei1518, zhiwei_liu
On Thu, Mar 7, 2024 at 7:22 PM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 3/7/24 00:45, Sunil V L wrote:
> > On Thu, Mar 07, 2024 at 11:33:25AM +1000, Alistair Francis wrote:
> >> On Thu, Mar 7, 2024 at 4:59 AM Daniel Henrique Barboza
> >> <dbarboza@ventanamicro.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> This patch break check-qtest, most specifically 'bios-table'test', for aarch64.
> >>> I found this while running riscv-to-apply.next in the Gitlab pipeline.
> >>>
> >>>
> >>> Here's the output:
> >>>
> >>> $ make -j && QTEST_QEMU_BINARY=./qemu-system-aarch64 V=1 ./tests/qtest/bios-tables-test
> >>> TAP version 13
> >>> # random seed: R02Sf0f2fa0a3fac5d540b1681c820621b7d
> >>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine none -accel qtest
> >>> 1..8
> >>> # Start of aarch64 tests
> >>> # Start of acpi tests
> >>> # starting QEMU: exec ./qemu-system-aarch64 -qtest unix:/tmp/qtest-591353.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-591353.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -machine virt -accel tcg -nodefaults -nographic -drive if=pflash,format=raw,file=pc-bios/edk2-aarch64-code.fd,readonly=on -drive if=pflash,format=raw,file=pc-bios/edk2-arm-vars.fd,snapshot=on -cdrom tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2 -cpu cortex-a57 -smbios type=4,max-speed=2900,current-speed=2700 -accel qtest
> >>> acpi-test: Warning! SPCR binary file mismatch. Actual [aml:/tmp/aml-9G53J2], Expected [aml:tests/data/acpi/virt/SPCR].
> >>> See source file tests/qtest/bios-tables-test.c for instructions on how to update expected files.
> >>> acpi-test: Warning! SPCR mismatch. Actual [asl:/tmp/asl-SR53J2.dsl, aml:/tmp/aml-9G53J2], Expected [asl:/tmp/asl-4Z33J2.dsl, aml:tests/data/acpi/virt/SPCR].
> >>>
> >>> The diff is here:
> >>>
> >>> --- /tmp/asl-4Z33J2.dsl 2024-03-06 15:40:24.879879348 -0300
> >>> +++ /tmp/asl-SR53J2.dsl 2024-03-06 15:40:24.877879347 -0300
> >>> @@ -1,57 +1,49 @@
> >>> /*
> >>> * Intel ACPI Component Architecture
> >>> * AML/ASL+ Disassembler version 20220331 (64-bit version)
> >>> * Copyright (c) 2000 - 2022 Intel Corporation
> >>>
> >>> (...)
> >>>
> >>> [000h 0000 4] Signature : "SPCR" [Serial Port Console Redirection Table]
> >>> -[004h 0004 4] Table Length : 00000050
> >>> +[004h 0004 4] Table Length : 0000004F
> >>> [008h 0008 1] Revision : 02
> >>> -[009h 0009 1] Checksum : B1
> >>> +[009h 0009 1] Checksum : B2
> >>> [00Ah 0010 6] Oem ID : "BOCHS "
> >>>
> >>> (...)
> >>>
> >>> -[042h 0066 2] PCI Vendor ID : FFFF
> >>> +[042h 0066 2] PCI Vendor ID : 00FF
> >>>
> >>>
> >>> After inspecting the common helper and what the original ARM code was doing
> >>> I found out that we're missing something down there:
> >>>
> >>>
> >>> On 1/15/24 22:09, 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..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, 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);
> >>>
> >>> Here. We're missing the "Language" entry.
> >>>
> >>>
> >>> This diff fixes the broken test:
> >>>
> >>>
> >>> $ git diff
> >>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> >>> index f3904650e4..6d4517cfbe 100644
> >>> --- a/hw/acpi/aml-build.c
> >>> +++ b/hw/acpi/aml-build.c
> >>> @@ -2024,6 +2024,8 @@ void build_spcr(GArray *table_data, BIOSLinker *linker,
> >>> build_append_int_noprefix(table_data, f->stop_bits, 1);
> >>> /* Flow Control */
> >>> build_append_int_noprefix(table_data, f->flow_control, 1);
> >>> + /* Language */
> >>> + build_append_int_noprefix(table_data, f->language, 1);
> >>> /* Terminal Type */
> >>> build_append_int_noprefix(table_data, f->terminal_type, 1);
> >>> /* PCI Device ID */
> >>
> >> Thanks! I added this to the patch
> >>
> >>>
> >>>
> >>>
> >>> As a side note, it seems like 'bios-table-test' isn't being run for RISC-V. Not sure if this
> >>> is intentional or a foresight.
> >>
> >> I think you are looking for oversight :)
> >>
> >> I think it's just an oversight, not intentional.
> >>
> > Adding these test cases is WIP. Drew had already pointed this. Myself
> > and Haibo are looking at it. But I doubt patches can be sent before 9.0
> > deadlines. Can we add them post 9.0?
>
> I don't recall the last time I had to deal with a new test being added after the
> code freeze. It's not a new feature, it increases test coverage and it might find
> bugs, so my guess is that it's fine.
>
> Alistair/Peter, any thoughts?
It's probably a post 9.0 thing, especially as I have been sick the
last week and am only just catching up.
Alistair
>
>
> Thanks,
>
> Daniel
>
>
>
>
> >
> > Thanks,
> > Sunil
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-03-20 4:43 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-16 1:09 [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Sia Jee Heng
2024-01-16 1:09 ` [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location Sia Jee Heng
2024-02-15 3:26 ` Alistair Francis
2024-03-06 18:57 ` Daniel Henrique Barboza
2024-03-06 20:00 ` Daniel Henrique Barboza
2024-03-07 1:33 ` Alistair Francis
2024-03-07 1:42 ` JeeHeng Sia
2024-03-07 3:45 ` Sunil V L
2024-03-07 9:22 ` Daniel Henrique Barboza
2024-03-20 4:41 ` Alistair Francis
2024-01-16 1:09 ` [PATCH v2 2/2] hw/riscv/virt-acpi-build.c: Generate SPCR table Sia Jee Heng
2024-02-15 3:29 ` Alistair Francis
2024-02-15 9:54 ` [PATCH v2 0/2] RISC-V: ACPI: Enable SPCR Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).