qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to co

From: Daniel Henrique Barboza
Subject: Re: [PATCH v2 1/2] hw/arm/virt-acpi-build.c: Migrate SPCR creation to common location
Date: Wed, 6 Mar 2024 15:57:47 -0300
User-agent: Mozilla Thunderbird 
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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]