* [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU
@ 2022-01-26 16:28 Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 01/10] ACPI ERST: bios-tables-test.c steps 1 and 2 Eric DeVolder
` (9 more replies)
0 siblings, 10 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This patchset introduces support for the ACPI Error Record
Serialization Table, ERST.
For background and implementation information, please see
docs/specs/acpi_erst.rst, which is patch 2/10.
Suggested-by: Konrad Wilk <konrad.wilk@oracle.com>
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
v14: 26jan2022
- Changed build_erst() to utilize a context structure for
generating accesses to ACTION and VALUE, per Michael Tsirkin.
- Other simplification per Ani Sinha.
v13: 24jan2022
- v12 erroneously omitted step 6 of bios-tables-test.c, this
has step 6 included.
- No other changes to v12.
v12: 10jan2022
- Converted macros in build_erst() to uppert to follow coding
style, as pointed out by Michael Tsirkin.
- And few items to help further simplify build_erst().
v11: 15dec2021
- Simplified build_erst() via feedback from Michael Tsirkin
- Addressed additional feedback from Ani Sinha
v10: 9dec2021
- Addressed additional feedback from Ani Sinha
v9: 2dec2021
- Addressed feedback from Ani Sinha
v8: 15oct2021
- Added Kconfig option for ERST, per Ani Sinha
- Fixed patch ordering, per Ani
v7: 7oct2021
- style improvements, per Igor
- use of endian accessors for storage header, per Igor
- a number of optimizations and improvements, per Igor
- updated spec for header, per Igor
- updated spec for rst format, per Michael Tsirkin
- updated spec for new record_size parameter
Due to changes in the spec, I am not carrying the
Acked-by from Ani Sinha.
- changes for and testing of migration to systems with
differing ERST_RECORD_SIZE
v6: 5aug2021
- Fixed compile warning/error, per Michael Tsirkin
- Fixed mingw32 build error, per Michael
- Converted exchange buffer to MemoryBackend, per Igor
- Migrated test to PCI, per Igor
- Significantly reduced amount of copying, per Igor
- Corrections/enhancements to acpi_erst.txt, per Igor
- Many misc/other small items, per Igor
v5: 30jun2021
- Create docs/specs/acpi_erst.txt, per Igor
- Separate PCI BARs for registers and memory, per Igor
- Convert debugging to use trace infrastructure, per Igor
- Various other fixups, per Igor
v4: 11jun2021
- Converted to a PCI device, per Igor.
- Updated qtest.
- Rearranged patches, per Igor.
v3: 28may2021
- Converted to using a TYPE_MEMORY_BACKEND_FILE object rather than
internal array with explicit file operations, per Igor.
- Changed the way the qdev and base address are handled, allowing
ERST to be disabled at run-time. Also aligns better with other
existing code.
v2: 8feb2021
- Added qtest/smoke test per Paolo Bonzini
- Split patch into smaller chunks, per Igor Mammedov
- Did away with use of ACPI packed structures, per Igor Mammedov
v1: 26oct2020
- initial post
---
Eric DeVolder (10):
ACPI ERST: bios-tables-test.c steps 1 and 2
ACPI ERST: specification for ERST support
ACPI ERST: PCI device_id for ERST
ACPI ERST: header file for ERST
ACPI ERST: support for ACPI ERST feature
ACPI ERST: build the ACPI ERST table
ACPI ERST: create ACPI ERST table for pc/x86 machines
ACPI ERST: qtest for ERST
ACPI ERST: bios-tables-test testcase
ACPI ERST: step 6 of bios-tables-test.c
docs/specs/acpi_erst.rst | 200 +++++++
hw/acpi/Kconfig | 6 +
hw/acpi/erst.c | 1069 +++++++++++++++++++++++++++++++++++++
hw/acpi/meson.build | 1 +
hw/acpi/trace-events | 15 +
hw/i386/acpi-build.c | 15 +
hw/i386/acpi-microvm.c | 15 +
include/hw/acpi/erst.h | 24 +
include/hw/pci/pci.h | 1 +
tests/data/acpi/microvm/ERST.pcie | Bin 0 -> 912 bytes
tests/data/acpi/pc/DSDT.acpierst | Bin 0 -> 5969 bytes
tests/data/acpi/pc/ERST.acpierst | Bin 0 -> 912 bytes
tests/data/acpi/q35/DSDT.acpierst | Bin 0 -> 8306 bytes
tests/data/acpi/q35/ERST.acpierst | Bin 0 -> 912 bytes
tests/qtest/bios-tables-test.c | 54 ++
tests/qtest/erst-test.c | 172 ++++++
tests/qtest/meson.build | 2 +
17 files changed, 1574 insertions(+)
create mode 100644 docs/specs/acpi_erst.rst
create mode 100644 hw/acpi/erst.c
create mode 100644 include/hw/acpi/erst.h
create mode 100644 tests/data/acpi/microvm/ERST.pcie
create mode 100644 tests/data/acpi/pc/DSDT.acpierst
create mode 100644 tests/data/acpi/pc/ERST.acpierst
create mode 100644 tests/data/acpi/q35/DSDT.acpierst
create mode 100644 tests/data/acpi/q35/ERST.acpierst
create mode 100644 tests/qtest/erst-test.c
--
1.8.3.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v14 01/10] ACPI ERST: bios-tables-test.c steps 1 and 2
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 02/10] ACPI ERST: specification for ERST support Eric DeVolder
` (8 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
Following the guidelines in tests/qtest/bios-tables-test.c, this
change adds empty placeholder files per step 1 for the new ERST
table, and excludes resulting changed files in bios-tables-test-allowed-diff.h
per step 2.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Igor Mammedov <imammedo@redhat.com>
---
tests/data/acpi/microvm/ERST.pcie | 0
tests/data/acpi/pc/DSDT.acpierst | 0
tests/data/acpi/pc/ERST.acpierst | 0
tests/data/acpi/q35/DSDT.acpierst | 0
tests/data/acpi/q35/ERST.acpierst | 0
tests/qtest/bios-tables-test-allowed-diff.h | 5 +++++
6 files changed, 5 insertions(+)
create mode 100644 tests/data/acpi/microvm/ERST.pcie
create mode 100644 tests/data/acpi/pc/DSDT.acpierst
create mode 100644 tests/data/acpi/pc/ERST.acpierst
create mode 100644 tests/data/acpi/q35/DSDT.acpierst
create mode 100644 tests/data/acpi/q35/ERST.acpierst
diff --git a/tests/data/acpi/microvm/ERST.pcie b/tests/data/acpi/microvm/ERST.pcie
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/acpi/pc/DSDT.acpierst b/tests/data/acpi/pc/DSDT.acpierst
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/acpi/pc/ERST.acpierst b/tests/data/acpi/pc/ERST.acpierst
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/acpi/q35/DSDT.acpierst b/tests/data/acpi/q35/DSDT.acpierst
new file mode 100644
index 0000000..e69de29
diff --git a/tests/data/acpi/q35/ERST.acpierst b/tests/data/acpi/q35/ERST.acpierst
new file mode 100644
index 0000000..e69de29
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523..603db07 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,6 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/DSDT.acpierst",
+"tests/data/acpi/pc/ERST.acpierst",
+"tests/data/acpi/q35/DSDT.acpierst",
+"tests/data/acpi/q35/ERST.acpierst",
+"tests/data/acpi/microvm/ERST.pcie",
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 02/10] ACPI ERST: specification for ERST support
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 01/10] ACPI ERST: bios-tables-test.c steps 1 and 2 Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 03/10] ACPI ERST: PCI device_id for ERST Eric DeVolder
` (7 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
Information on the implementation of the ACPI ERST support.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
---
docs/specs/acpi_erst.rst | 200 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 200 insertions(+)
create mode 100644 docs/specs/acpi_erst.rst
diff --git a/docs/specs/acpi_erst.rst b/docs/specs/acpi_erst.rst
new file mode 100644
index 0000000..a8a9d22
--- /dev/null
+++ b/docs/specs/acpi_erst.rst
@@ -0,0 +1,200 @@
+ACPI ERST DEVICE
+================
+
+The ACPI ERST device is utilized to support the ACPI Error Record
+Serialization Table, ERST, functionality. This feature is designed for
+storing error records in persistent storage for future reference
+and/or debugging.
+
+The ACPI specification[1], in Chapter "ACPI Platform Error Interfaces
+(APEI)", and specifically subsection "Error Serialization", outlines a
+method for storing error records into persistent storage.
+
+The format of error records is described in the UEFI specification[2],
+in Appendix N "Common Platform Error Record".
+
+While the ACPI specification allows for an NVRAM "mode" (see
+GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES) where non-volatile RAM is
+directly exposed for direct access by the OS/guest, this device
+implements the non-NVRAM "mode". This non-NVRAM "mode" is what is
+implemented by most BIOS (since flash memory requires programming
+operations in order to update its contents). Furthermore, as of the
+time of this writing, Linux only supports the non-NVRAM "mode".
+
+
+Background/Motivation
+---------------------
+
+Linux uses the persistent storage filesystem, pstore, to record
+information (eg. dmesg tail) upon panics and shutdowns. Pstore is
+independent of, and runs before, kdump. In certain scenarios (ie.
+hosts/guests with root filesystems on NFS/iSCSI where networking
+software and/or hardware fails, and thus kdump fails), pstore may
+contain information available for post-mortem debugging.
+
+Two common storage backends for the pstore filesystem are ACPI ERST
+and UEFI. Most BIOS implement ACPI ERST. UEFI is not utilized in all
+guests. With QEMU supporting ACPI ERST, it becomes a viable pstore
+storage backend for virtual machines (as it is now for bare metal
+machines).
+
+Enabling support for ACPI ERST facilitates a consistent method to
+capture kernel panic information in a wide range of guests: from
+resource-constrained microvms to very large guests, and in particular,
+in direct-boot environments (which would lack UEFI run-time services).
+
+Note that Microsoft Windows also utilizes the ACPI ERST for certain
+crash information, if available[3].
+
+
+Configuration|Usage
+-------------------
+
+To use ACPI ERST, a memory-backend-file object and acpi-erst device
+can be created, for example:
+
+ qemu ...
+ -object memory-backend-file,id=erstnvram,mem-path=acpi-erst.backing,size=0x10000,share=on \
+ -device acpi-erst,memdev=erstnvram
+
+For proper operation, the ACPI ERST device needs a memory-backend-file
+object with the following parameters:
+
+ - id: The id of the memory-backend-file object is used to associate
+ this memory with the acpi-erst device.
+ - size: The size of the ACPI ERST backing storage. This parameter is
+ required.
+ - mem-path: The location of the ACPI ERST backing storage file. This
+ parameter is also required.
+ - share: The share=on parameter is required so that updates to the
+ ERST backing store are written to the file.
+
+and ERST device:
+
+ - memdev: Is the object id of the memory-backend-file.
+ - record_size: Specifies the size of the records (or slots) in the
+ backend storage. Must be a power of two value greater than or
+ equal to 4096 (PAGE_SIZE).
+
+
+PCI Interface
+-------------
+
+The ERST device is a PCI device with two BARs, one for accessing the
+programming registers, and the other for accessing the record exchange
+buffer.
+
+BAR0 contains the programming interface consisting of ACTION and VALUE
+64-bit registers. All ERST actions/operations/side effects happen on
+the write to the ACTION, by design. Any data needed by the action must
+be placed into VALUE prior to writing ACTION. Reading the VALUE
+simply returns the register contents, which can be updated by a
+previous ACTION.
+
+BAR1 contains the 8KiB record exchange buffer, which is the
+implemented maximum record size.
+
+
+Backend Storage Format
+----------------------
+
+The backend storage is divided into fixed size "slots", 8KiB in
+length, with each slot storing a single record. Not all slots need to
+be occupied, and they need not be occupied in a contiguous fashion.
+The ability to clear/erase specific records allows for the formation
+of unoccupied slots.
+
+Slot 0 contains a backend storage header that identifies the contents
+as ERST and also facilitates efficient access to the records.
+Depending upon the size of the backend storage, additional slots will
+be designated to be a part of the slot 0 header. For example, at 8KiB,
+the slot 0 header can accomodate 1021 records. Thus a storage size
+of 8MiB (8KiB * 1024) requires an additional slot for use by the
+header. In this scenario, slot 0 and slot 1 form the backend storage
+header, and records can be stored starting at slot 2.
+
+Below is an example layout of the backend storage format (for storage
+size less than 8MiB). The size of the storage is a multiple of 8KiB,
+and contains N number of slots to store records. The example below
+shows two records (in CPER format) in the backend storage, while the
+remaining slots are empty/available.
+
+::
+
+ Slot Record
+ <------------------ 8KiB -------------------->
+ +--------------------------------------------+
+ 0 | storage header |
+ +--------------------------------------------+
+ 1 | empty/available |
+ +--------------------------------------------+
+ 2 | CPER |
+ +--------------------------------------------+
+ 3 | CPER |
+ +--------------------------------------------+
+ ... | |
+ +--------------------------------------------+
+ N | empty/available |
+ +--------------------------------------------+
+
+The storage header consists of some basic information and an array
+of CPER record_id's to efficiently access records in the backend
+storage.
+
+All fields in the header are stored in little endian format.
+
+::
+
+ +--------------------------------------------+
+ | magic | 0x0000
+ +--------------------------------------------+
+ | record_offset | record_size | 0x0008
+ +--------------------------------------------+
+ | record_count | reserved | version | 0x0010
+ +--------------------------------------------+
+ | record_id[0] | 0x0018
+ +--------------------------------------------+
+ | record_id[1] | 0x0020
+ +--------------------------------------------+
+ | record_id[...] |
+ +--------------------------------------------+
+ | record_id[N] | 0x1FF8
+ +--------------------------------------------+
+
+The 'magic' field contains the value 0x524F545354535245.
+
+The 'record_size' field contains the value 0x2000, 8KiB.
+
+The 'record_offset' field points to the first record_id in the array,
+0x0018.
+
+The 'version' field contains 0x0100, the first version.
+
+The 'record_count' field contains the number of valid records in the
+backend storage.
+
+The 'record_id' array fields are the 64-bit record identifiers of the
+CPER record in the corresponding slot. Stated differently, the
+location of a CPER record_id in the record_id[] array provides the
+slot index for the corresponding record in the backend storage.
+
+Note that, for example, with a backend storage less than 8MiB, slot 0
+contains the header, so the record_id[0] will never contain a valid
+CPER record_id. Instead slot 1 is the first available slot and thus
+record_id_[1] may contain a CPER.
+
+A 'record_id' of all 0s or all 1s indicates an invalid record (ie. the
+slot is available).
+
+
+References
+----------
+
+[1] "Advanced Configuration and Power Interface Specification",
+ version 4.0, June 2009.
+
+[2] "Unified Extensible Firmware Interface Specification",
+ version 2.1, October 2008.
+
+[3] "Windows Hardware Error Architecture", specfically
+ "Error Record Persistence Mechanism".
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 03/10] ACPI ERST: PCI device_id for ERST
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 01/10] ACPI ERST: bios-tables-test.c steps 1 and 2 Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 02/10] ACPI ERST: specification for ERST support Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 04/10] ACPI ERST: header file " Eric DeVolder
` (6 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This change reserves the PCI device_id for the new ACPI ERST
device.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Igor Mammedov <imammedo@redhat.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
---
include/hw/pci/pci.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 023abc0..c3f3c90 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -108,6 +108,7 @@ extern bool pci_available;
#define PCI_DEVICE_ID_REDHAT_MDPY 0x000f
#define PCI_DEVICE_ID_REDHAT_NVME 0x0010
#define PCI_DEVICE_ID_REDHAT_PVPANIC 0x0011
+#define PCI_DEVICE_ID_REDHAT_ACPI_ERST 0x0012
#define PCI_DEVICE_ID_REDHAT_QXL 0x0100
#define FMT_PCIBUS PRIx64
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 04/10] ACPI ERST: header file for ERST
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (2 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 03/10] ACPI ERST: PCI device_id for ERST Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 05/10] ACPI ERST: support for ACPI ERST feature Eric DeVolder
` (5 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This change introduces the public defintions for ACPI ERST.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
---
include/hw/acpi/erst.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 include/hw/acpi/erst.h
diff --git a/include/hw/acpi/erst.h b/include/hw/acpi/erst.h
new file mode 100644
index 0000000..9d63717
--- /dev/null
+++ b/include/hw/acpi/erst.h
@@ -0,0 +1,19 @@
+/*
+ * ACPI Error Record Serialization Table, ERST, Implementation
+ *
+ * ACPI ERST introduced in ACPI 4.0, June 16, 2009.
+ * ACPI Platform Error Interfaces : Error Serialization
+ *
+ * Copyright (c) 2021 Oracle and/or its affiliates.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#ifndef HW_ACPI_ERST_H
+#define HW_ACPI_ERST_H
+
+void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
+ const char *oem_id, const char *oem_table_id);
+
+#define TYPE_ACPI_ERST "acpi-erst"
+
+#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 05/10] ACPI ERST: support for ACPI ERST feature
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (3 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 04/10] ACPI ERST: header file " Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table Eric DeVolder
` (4 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This implements a PCI device for ACPI ERST. This implements the
non-NVRAM "mode" of operation for ERST as it is supported by
Linux and Windows.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
---
hw/acpi/Kconfig | 6 +
hw/acpi/erst.c | 844 +++++++++++++++++++++++++++++++++++++++++++++++++++
hw/acpi/meson.build | 1 +
hw/acpi/trace-events | 15 +
4 files changed, 866 insertions(+)
create mode 100644 hw/acpi/erst.c
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 622b0b5..19caebd 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -10,6 +10,7 @@ config ACPI_X86
select ACPI_HMAT
select ACPI_PIIX4
select ACPI_PCIHP
+ select ACPI_ERST
config ACPI_X86_ICH
bool
@@ -60,3 +61,8 @@ config ACPI_HW_REDUCED
select ACPI
select ACPI_MEMORY_HOTPLUG
select ACPI_NVDIMM
+
+config ACPI_ERST
+ bool
+ default y
+ depends on ACPI && PCI
diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
new file mode 100644
index 0000000..fe9ba51
--- /dev/null
+++ b/hw/acpi/erst.c
@@ -0,0 +1,844 @@
+/*
+ * ACPI Error Record Serialization Table, ERST, Implementation
+ *
+ * ACPI ERST introduced in ACPI 4.0, June 16, 2009.
+ * ACPI Platform Error Interfaces : Error Serialization
+ *
+ * Copyright (c) 2021 Oracle and/or its affiliates.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-core.h"
+#include "exec/memory.h"
+#include "qom/object.h"
+#include "hw/pci/pci.h"
+#include "qom/object_interfaces.h"
+#include "qemu/error-report.h"
+#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi-defs.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/bios-linker-loader.h"
+#include "exec/address-spaces.h"
+#include "sysemu/hostmem.h"
+#include "hw/acpi/erst.h"
+#include "trace.h"
+
+/* ACPI 4.0: Table 17-16 Serialization Actions */
+#define ACTION_BEGIN_WRITE_OPERATION 0x0
+#define ACTION_BEGIN_READ_OPERATION 0x1
+#define ACTION_BEGIN_CLEAR_OPERATION 0x2
+#define ACTION_END_OPERATION 0x3
+#define ACTION_SET_RECORD_OFFSET 0x4
+#define ACTION_EXECUTE_OPERATION 0x5
+#define ACTION_CHECK_BUSY_STATUS 0x6
+#define ACTION_GET_COMMAND_STATUS 0x7
+#define ACTION_GET_RECORD_IDENTIFIER 0x8
+#define ACTION_SET_RECORD_IDENTIFIER 0x9
+#define ACTION_GET_RECORD_COUNT 0xA
+#define ACTION_BEGIN_DUMMY_WRITE_OPERATION 0xB
+#define ACTION_RESERVED 0xC
+#define ACTION_GET_ERROR_LOG_ADDRESS_RANGE 0xD
+#define ACTION_GET_ERROR_LOG_ADDRESS_LENGTH 0xE
+#define ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES 0xF
+#define ACTION_GET_EXECUTE_OPERATION_TIMINGS 0x10 /* ACPI 6.3 */
+
+/* ACPI 4.0: Table 17-17 Command Status Definitions */
+#define STATUS_SUCCESS 0x00
+#define STATUS_NOT_ENOUGH_SPACE 0x01
+#define STATUS_HARDWARE_NOT_AVAILABLE 0x02
+#define STATUS_FAILED 0x03
+#define STATUS_RECORD_STORE_EMPTY 0x04
+#define STATUS_RECORD_NOT_FOUND 0x05
+
+/* UEFI 2.1: Appendix N Common Platform Error Record */
+#define UEFI_CPER_RECORD_MIN_SIZE 128U
+#define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
+#define UEFI_CPER_RECORD_ID_OFFSET 96U
+#define IS_UEFI_CPER_RECORD(ptr) \
+ (((ptr)[0] == 'C') && \
+ ((ptr)[1] == 'P') && \
+ ((ptr)[2] == 'E') && \
+ ((ptr)[3] == 'R'))
+
+/*
+ * NOTE that when accessing CPER fields within a record, memcpy()
+ * is utilized to avoid a possible misaligned access on the host.
+ */
+
+/*
+ * This implementation is an ACTION (cmd) and VALUE (data)
+ * interface consisting of just two 64-bit registers.
+ */
+#define ERST_REG_SIZE (16UL)
+#define ERST_ACTION_OFFSET (0UL) /* action (cmd) */
+#define ERST_VALUE_OFFSET (8UL) /* argument/value (data) */
+
+/*
+ * ERST_RECORD_SIZE is the buffer size for exchanging ERST
+ * record contents. Thus, it defines the maximum record size.
+ * As this is mapped through a PCI BAR, it must be a power of
+ * two and larger than UEFI_CPER_RECORD_MIN_SIZE.
+ * The backing storage is divided into fixed size "slots",
+ * each ERST_RECORD_SIZE in length, and each "slot"
+ * storing a single record. No attempt at optimizing storage
+ * through compression, compaction, etc is attempted.
+ * NOTE that slot 0 is reserved for the backing storage header.
+ * Depending upon the size of the backing storage, additional
+ * slots will be part of the slot 0 header in order to account
+ * for a record_id for each available remaining slot.
+ */
+/* 8KiB records, not too small, not too big */
+#define ERST_RECORD_SIZE (8192UL)
+
+#define ACPI_ERST_MEMDEV_PROP "memdev"
+#define ACPI_ERST_RECORD_SIZE_PROP "record_size"
+
+/*
+ * From the ACPI ERST spec sections:
+ * A record id of all 0s is used to indicate 'unspecified' record id.
+ * A record id of all 1s is used to indicate empty or end.
+ */
+#define ERST_UNSPECIFIED_RECORD_ID (0UL)
+#define ERST_EMPTY_END_RECORD_ID (~0UL)
+
+#define ERST_IS_VALID_RECORD_ID(rid) \
+ ((rid != ERST_UNSPECIFIED_RECORD_ID) && \
+ (rid != ERST_EMPTY_END_RECORD_ID))
+
+/*
+ * Implementation-specific definitions and types.
+ * Values are arbitrary and chosen for this implementation.
+ * See erst.rst documentation for details.
+ */
+#define ERST_EXECUTE_OPERATION_MAGIC 0x9CUL
+#define ERST_STORE_MAGIC 0x524F545354535245UL /* ERSTSTOR */
+typedef struct {
+ uint64_t magic;
+ uint32_t record_size;
+ uint32_t storage_offset; /* offset to record storage beyond header */
+ uint16_t version;
+ uint16_t reserved;
+ uint32_t record_count;
+ uint64_t map[]; /* contains record_ids, and position indicates index */
+} __attribute__((packed)) ERSTStorageHeader;
+
+/*
+ * Object cast macro
+ */
+#define ACPIERST(obj) \
+ OBJECT_CHECK(ERSTDeviceState, (obj), TYPE_ACPI_ERST)
+
+/*
+ * Main ERST device state structure
+ */
+typedef struct {
+ PCIDevice parent_obj;
+
+ /* Backend storage */
+ HostMemoryBackend *hostmem;
+ MemoryRegion *hostmem_mr;
+ uint32_t storage_size;
+ uint32_t default_record_size;
+
+ /* Programming registers */
+ MemoryRegion iomem_mr;
+
+ /* Exchange buffer */
+ MemoryRegion exchange_mr;
+
+ /* Interface state */
+ uint8_t operation;
+ uint8_t busy_status;
+ uint8_t command_status;
+ uint32_t record_offset;
+ uint64_t reg_action;
+ uint64_t reg_value;
+ uint64_t record_identifier;
+ ERSTStorageHeader *header;
+ unsigned first_record_index;
+ unsigned last_record_index;
+ unsigned next_record_index;
+
+} ERSTDeviceState;
+
+/*******************************************************************/
+/*******************************************************************/
+static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
+{
+ uint8_t *rc = NULL;
+ off_t offset = (index * le32_to_cpu(s->header->record_size));
+
+ g_assert(offset < s->storage_size);
+
+ rc = memory_region_get_ram_ptr(s->hostmem_mr);
+ rc += offset;
+
+ return rc;
+}
+
+static void make_erst_storage_header(ERSTDeviceState *s)
+{
+ ERSTStorageHeader *header = s->header;
+ unsigned mapsz, headersz;
+
+ header->magic = cpu_to_le64(ERST_STORE_MAGIC);
+ header->record_size = cpu_to_le32(s->default_record_size);
+ header->version = cpu_to_le16(0x0100);
+ header->reserved = cpu_to_le16(0x0000);
+
+ /* Compute mapsize */
+ mapsz = s->storage_size / s->default_record_size;
+ mapsz *= sizeof(uint64_t);
+ /* Compute header+map size */
+ headersz = sizeof(ERSTStorageHeader) + mapsz;
+ /* Round up to nearest integer multiple of ERST_RECORD_SIZE */
+ headersz = QEMU_ALIGN_UP(headersz, s->default_record_size);
+ header->storage_offset = cpu_to_le32(headersz);
+
+ /*
+ * The HostMemoryBackend initializes contents to zero,
+ * so all record_ids stashed in the map are zero'd.
+ * As well the record_count is zero. Properly initialized.
+ */
+}
+
+static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp)
+{
+ ERSTStorageHeader *header;
+ uint32_t record_size;
+
+ header = memory_region_get_ram_ptr(s->hostmem_mr);
+ s->header = header;
+
+ /* Ensure pointer to header is 64-bit aligned */
+ g_assert(QEMU_PTR_IS_ALIGNED(header, sizeof(uint64_t)));
+
+ /*
+ * Check if header is uninitialized; HostMemoryBackend inits to 0
+ */
+ if (le64_to_cpu(header->magic) == 0UL) {
+ make_erst_storage_header(s);
+ }
+
+ /* Validity check record_size */
+ record_size = le32_to_cpu(header->record_size);
+ if (!(
+ (record_size) && /* non zero */
+ (record_size >= UEFI_CPER_RECORD_MIN_SIZE) &&
+ (((record_size - 1) & record_size) == 0) && /* is power of 2 */
+ (record_size >= 4096) /* PAGE_SIZE */
+ )) {
+ error_setg(errp, "ERST record_size %u is invalid", record_size);
+ }
+
+ /* Validity check header */
+ if (!(
+ (le64_to_cpu(header->magic) == ERST_STORE_MAGIC) &&
+ ((le32_to_cpu(header->storage_offset) % record_size) == 0) &&
+ (le16_to_cpu(header->version) == 0x0100) &&
+ (le16_to_cpu(header->reserved) == 0)
+ )) {
+ error_setg(errp, "ERST backend storage header is invalid");
+ }
+
+ /* Check storage_size against record_size */
+ if (((s->storage_size % record_size) != 0) ||
+ (record_size > s->storage_size)) {
+ error_setg(errp, "ACPI ERST requires storage size be multiple of "
+ "record size (%uKiB)", record_size);
+ }
+
+ /* Compute offset of first and last record storage slot */
+ s->first_record_index = le32_to_cpu(header->storage_offset)
+ / record_size;
+ s->last_record_index = (s->storage_size / record_size);
+}
+
+static void update_map_entry(ERSTDeviceState *s, unsigned index,
+ uint64_t record_id)
+{
+ if (index < s->last_record_index) {
+ s->header->map[index] = cpu_to_le64(record_id);
+ }
+}
+
+static unsigned find_next_empty_record_index(ERSTDeviceState *s)
+{
+ unsigned rc = 0; /* 0 not a valid index */
+ unsigned index = s->first_record_index;
+
+ for (; index < s->last_record_index; ++index) {
+ if (le64_to_cpu(s->header->map[index]) == ERST_UNSPECIFIED_RECORD_ID) {
+ rc = index;
+ break;
+ }
+ }
+
+ return rc;
+}
+
+static unsigned lookup_erst_record(ERSTDeviceState *s,
+ uint64_t record_identifier)
+{
+ unsigned rc = 0; /* 0 not a valid index */
+
+ /* Find the record_identifier in the map */
+ if (record_identifier != ERST_UNSPECIFIED_RECORD_ID) {
+ /*
+ * Count number of valid records encountered, and
+ * short-circuit the loop if identifier not found
+ */
+ uint32_t record_count = le32_to_cpu(s->header->record_count);
+ unsigned count = 0;
+ unsigned index;
+ for (index = s->first_record_index; index < s->last_record_index &&
+ count < record_count; ++index) {
+ if (le64_to_cpu(s->header->map[index]) == record_identifier) {
+ rc = index;
+ break;
+ }
+ if (le64_to_cpu(s->header->map[index]) !=
+ ERST_UNSPECIFIED_RECORD_ID) {
+ ++count;
+ }
+ }
+ }
+
+ return rc;
+}
+
+/*
+ * ACPI 4.0: 17.4.1.1 Serialization Actions, also see
+ * ACPI 4.0: 17.4.2.2 Operations - Reading 6.c and 2.c
+ */
+static unsigned get_next_record_identifier(ERSTDeviceState *s,
+ uint64_t *record_identifier, bool first)
+{
+ unsigned found = 0;
+ unsigned index;
+
+ /* For operations needing to return 'first' record identifier */
+ if (first) {
+ /* Reset initial index to beginning */
+ s->next_record_index = s->first_record_index;
+ }
+ index = s->next_record_index;
+
+ *record_identifier = ERST_EMPTY_END_RECORD_ID;
+
+ if (le32_to_cpu(s->header->record_count)) {
+ for (; index < s->last_record_index; ++index) {
+ if (le64_to_cpu(s->header->map[index]) !=
+ ERST_UNSPECIFIED_RECORD_ID) {
+ /* where to start next time */
+ s->next_record_index = index + 1;
+ *record_identifier = le64_to_cpu(s->header->map[index]);
+ found = 1;
+ break;
+ }
+ }
+ }
+ if (!found) {
+ /* at end (ie scan complete), reset */
+ s->next_record_index = s->first_record_index;
+ }
+
+ return STATUS_SUCCESS;
+}
+
+/* ACPI 4.0: 17.4.2.3 Operations - Clearing */
+static unsigned clear_erst_record(ERSTDeviceState *s)
+{
+ unsigned rc = STATUS_RECORD_NOT_FOUND;
+ unsigned index;
+
+ /* Check for valid record identifier */
+ if (!ERST_IS_VALID_RECORD_ID(s->record_identifier)) {
+ return STATUS_FAILED;
+ }
+
+ index = lookup_erst_record(s, s->record_identifier);
+ if (index) {
+ /* No need to wipe record, just invalidate its map entry */
+ uint32_t record_count;
+ update_map_entry(s, index, ERST_UNSPECIFIED_RECORD_ID);
+ record_count = le32_to_cpu(s->header->record_count);
+ record_count -= 1;
+ s->header->record_count = cpu_to_le32(record_count);
+ rc = STATUS_SUCCESS;
+ }
+
+ return rc;
+}
+
+/* ACPI 4.0: 17.4.2.2 Operations - Reading */
+static unsigned read_erst_record(ERSTDeviceState *s)
+{
+ unsigned rc = STATUS_RECORD_NOT_FOUND;
+ unsigned exchange_length;
+ unsigned index;
+
+ /* Check if backend storage is empty */
+ if (le32_to_cpu(s->header->record_count) == 0) {
+ return STATUS_RECORD_STORE_EMPTY;
+ }
+
+ exchange_length = memory_region_size(&s->exchange_mr);
+
+ /* Check for record identifier of all 0s */
+ if (s->record_identifier == ERST_UNSPECIFIED_RECORD_ID) {
+ /* Set to 'first' record in storage */
+ get_next_record_identifier(s, &s->record_identifier, true);
+ /* record_identifier is now a valid id, or all 1s */
+ }
+
+ /* Check for record identifier of all 1s */
+ if (s->record_identifier == ERST_EMPTY_END_RECORD_ID) {
+ return STATUS_FAILED;
+ }
+
+ /* Validate record_offset */
+ if (s->record_offset > (exchange_length - UEFI_CPER_RECORD_MIN_SIZE)) {
+ return STATUS_FAILED;
+ }
+
+ index = lookup_erst_record(s, s->record_identifier);
+ if (index) {
+ uint8_t *nvram;
+ uint8_t *exchange;
+ uint32_t record_length;
+
+ /* Obtain pointer to the exchange buffer */
+ exchange = memory_region_get_ram_ptr(&s->exchange_mr);
+ exchange += s->record_offset;
+ /* Obtain pointer to slot in storage */
+ nvram = get_nvram_ptr_by_index(s, index);
+ /* Validate CPER record_length */
+ memcpy((uint8_t *)&record_length,
+ &nvram[UEFI_CPER_RECORD_LENGTH_OFFSET],
+ sizeof(uint32_t));
+ record_length = le32_to_cpu(record_length);
+ if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
+ rc = STATUS_FAILED;
+ }
+ if ((s->record_offset + record_length) > exchange_length) {
+ rc = STATUS_FAILED;
+ }
+ /* If all is ok, copy the record to the exchange buffer */
+ if (rc != STATUS_FAILED) {
+ memcpy(exchange, nvram, record_length);
+ rc = STATUS_SUCCESS;
+ }
+ } else {
+ /*
+ * See "Reading : 'The steps performed by the platform ...' 2.c"
+ * Set to 'first' record in storage
+ */
+ get_next_record_identifier(s, &s->record_identifier, true);
+ }
+
+ return rc;
+}
+
+/* ACPI 4.0: 17.4.2.1 Operations - Writing */
+static unsigned write_erst_record(ERSTDeviceState *s)
+{
+ unsigned rc = STATUS_FAILED;
+ unsigned exchange_length;
+ unsigned index;
+ uint64_t record_identifier;
+ uint32_t record_length;
+ uint8_t *exchange;
+ uint8_t *nvram = NULL;
+ bool record_found = false;
+
+ exchange_length = memory_region_size(&s->exchange_mr);
+
+ /* Validate record_offset */
+ if (s->record_offset > (exchange_length - UEFI_CPER_RECORD_MIN_SIZE)) {
+ return STATUS_FAILED;
+ }
+
+ /* Obtain pointer to record in the exchange buffer */
+ exchange = memory_region_get_ram_ptr(&s->exchange_mr);
+ exchange += s->record_offset;
+
+ /* Validate CPER record_length */
+ memcpy((uint8_t *)&record_length, &exchange[UEFI_CPER_RECORD_LENGTH_OFFSET],
+ sizeof(uint32_t));
+ record_length = le32_to_cpu(record_length);
+ if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
+ return STATUS_FAILED;
+ }
+ if ((s->record_offset + record_length) > exchange_length) {
+ return STATUS_FAILED;
+ }
+
+ /* Extract record identifier */
+ memcpy((uint8_t *)&record_identifier, &exchange[UEFI_CPER_RECORD_ID_OFFSET],
+ sizeof(uint64_t));
+ record_identifier = le64_to_cpu(record_identifier);
+
+ /* Check for valid record identifier */
+ if (!ERST_IS_VALID_RECORD_ID(record_identifier)) {
+ return STATUS_FAILED;
+ }
+
+ index = lookup_erst_record(s, record_identifier);
+ if (index) {
+ /* Record found, overwrite existing record */
+ nvram = get_nvram_ptr_by_index(s, index);
+ record_found = true;
+ } else {
+ /* Record not found, not an overwrite, allocate for write */
+ index = find_next_empty_record_index(s);
+ if (index) {
+ nvram = get_nvram_ptr_by_index(s, index);
+ } else {
+ /* All slots are occupied */
+ rc = STATUS_NOT_ENOUGH_SPACE;
+ }
+ }
+ if (nvram) {
+ /* Write the record into the slot */
+ memcpy(nvram, exchange, record_length);
+ memset(nvram + record_length, exchange_length - record_length, 0xFF);
+ /* If a new record, increment the record_count */
+ if (!record_found) {
+ uint32_t record_count;
+ record_count = le32_to_cpu(s->header->record_count);
+ record_count += 1; /* writing new record */
+ s->header->record_count = cpu_to_le32(record_count);
+ }
+ update_map_entry(s, index, record_identifier);
+ rc = STATUS_SUCCESS;
+ }
+
+ return rc;
+}
+
+/*******************************************************************/
+
+static uint64_t erst_rd_reg64(hwaddr addr,
+ uint64_t reg, unsigned size)
+{
+ uint64_t rdval;
+ uint64_t mask;
+ unsigned shift;
+
+ if (size == sizeof(uint64_t)) {
+ /* 64b access */
+ mask = 0xFFFFFFFFFFFFFFFFUL;
+ shift = 0;
+ } else {
+ /* 32b access */
+ mask = 0x00000000FFFFFFFFUL;
+ shift = ((addr & 0x4) == 0x4) ? 32 : 0;
+ }
+
+ rdval = reg;
+ rdval >>= shift;
+ rdval &= mask;
+
+ return rdval;
+}
+
+static uint64_t erst_wr_reg64(hwaddr addr,
+ uint64_t reg, uint64_t val, unsigned size)
+{
+ uint64_t wrval;
+ uint64_t mask;
+ unsigned shift;
+
+ if (size == sizeof(uint64_t)) {
+ /* 64b access */
+ mask = 0xFFFFFFFFFFFFFFFFUL;
+ shift = 0;
+ } else {
+ /* 32b access */
+ mask = 0x00000000FFFFFFFFUL;
+ shift = ((addr & 0x4) == 0x4) ? 32 : 0;
+ }
+
+ val &= mask;
+ val <<= shift;
+ mask <<= shift;
+ wrval = reg;
+ wrval &= ~mask;
+ wrval |= val;
+
+ return wrval;
+}
+
+static void erst_reg_write(void *opaque, hwaddr addr,
+ uint64_t val, unsigned size)
+{
+ ERSTDeviceState *s = (ERSTDeviceState *)opaque;
+
+ /*
+ * NOTE: All actions/operations/side effects happen on the WRITE,
+ * by this implementation's design. The READs simply return the
+ * reg_value contents.
+ */
+ trace_acpi_erst_reg_write(addr, val, size);
+
+ switch (addr) {
+ case ERST_VALUE_OFFSET + 0:
+ case ERST_VALUE_OFFSET + 4:
+ s->reg_value = erst_wr_reg64(addr, s->reg_value, val, size);
+ break;
+ case ERST_ACTION_OFFSET + 0:
+ /*
+ * NOTE: all valid values written to this register are of the
+ * ACTION_* variety. Thus there is no need to make this a 64-bit
+ * register, 32-bits is appropriate. As such ERST_ACTION_OFFSET+4
+ * is not needed.
+ */
+ switch (val) {
+ case ACTION_BEGIN_WRITE_OPERATION:
+ case ACTION_BEGIN_READ_OPERATION:
+ case ACTION_BEGIN_CLEAR_OPERATION:
+ case ACTION_BEGIN_DUMMY_WRITE_OPERATION:
+ case ACTION_END_OPERATION:
+ s->operation = val;
+ break;
+ case ACTION_SET_RECORD_OFFSET:
+ s->record_offset = s->reg_value;
+ break;
+ case ACTION_EXECUTE_OPERATION:
+ if ((uint8_t)s->reg_value == ERST_EXECUTE_OPERATION_MAGIC) {
+ s->busy_status = 1;
+ switch (s->operation) {
+ case ACTION_BEGIN_WRITE_OPERATION:
+ s->command_status = write_erst_record(s);
+ break;
+ case ACTION_BEGIN_READ_OPERATION:
+ s->command_status = read_erst_record(s);
+ break;
+ case ACTION_BEGIN_CLEAR_OPERATION:
+ s->command_status = clear_erst_record(s);
+ break;
+ case ACTION_BEGIN_DUMMY_WRITE_OPERATION:
+ s->command_status = STATUS_SUCCESS;
+ break;
+ case ACTION_END_OPERATION:
+ s->command_status = STATUS_SUCCESS;
+ break;
+ default:
+ s->command_status = STATUS_FAILED;
+ break;
+ }
+ s->busy_status = 0;
+ }
+ break;
+ case ACTION_CHECK_BUSY_STATUS:
+ s->reg_value = s->busy_status;
+ break;
+ case ACTION_GET_COMMAND_STATUS:
+ s->reg_value = s->command_status;
+ break;
+ case ACTION_GET_RECORD_IDENTIFIER:
+ s->command_status = get_next_record_identifier(s,
+ &s->reg_value, false);
+ break;
+ case ACTION_SET_RECORD_IDENTIFIER:
+ s->record_identifier = s->reg_value;
+ break;
+ case ACTION_GET_RECORD_COUNT:
+ s->reg_value = le32_to_cpu(s->header->record_count);
+ break;
+ case ACTION_GET_ERROR_LOG_ADDRESS_RANGE:
+ s->reg_value = (hwaddr)pci_get_bar_addr(PCI_DEVICE(s), 1);
+ break;
+ case ACTION_GET_ERROR_LOG_ADDRESS_LENGTH:
+ s->reg_value = le32_to_cpu(s->header->record_size);
+ break;
+ case ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES:
+ s->reg_value = 0x0; /* intentional, not NVRAM mode */
+ break;
+ case ACTION_GET_EXECUTE_OPERATION_TIMINGS:
+ s->reg_value =
+ (100ULL << 32) | /* 100us max time */
+ (10ULL << 0) ; /* 10us min time */
+ break;
+ default:
+ /* Unknown action/command, NOP */
+ break;
+ }
+ break;
+ default:
+ /* This should not happen, but if it does, NOP */
+ break;
+ }
+}
+
+static uint64_t erst_reg_read(void *opaque, hwaddr addr,
+ unsigned size)
+{
+ ERSTDeviceState *s = (ERSTDeviceState *)opaque;
+ uint64_t val = 0;
+
+ switch (addr) {
+ case ERST_ACTION_OFFSET + 0:
+ case ERST_ACTION_OFFSET + 4:
+ val = erst_rd_reg64(addr, s->reg_action, size);
+ break;
+ case ERST_VALUE_OFFSET + 0:
+ case ERST_VALUE_OFFSET + 4:
+ val = erst_rd_reg64(addr, s->reg_value, size);
+ break;
+ default:
+ break;
+ }
+ trace_acpi_erst_reg_read(addr, val, size);
+ return val;
+}
+
+static const MemoryRegionOps erst_reg_ops = {
+ .read = erst_reg_read,
+ .write = erst_reg_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+/*******************************************************************/
+/*******************************************************************/
+static int erst_post_load(void *opaque, int version_id)
+{
+ ERSTDeviceState *s = opaque;
+
+ /* Recompute pointer to header */
+ s->header = (ERSTStorageHeader *)get_nvram_ptr_by_index(s, 0);
+ trace_acpi_erst_post_load(s->header, le32_to_cpu(s->header->record_size));
+
+ return 0;
+}
+
+static const VMStateDescription erst_vmstate = {
+ .name = "acpi-erst",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .post_load = erst_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8(operation, ERSTDeviceState),
+ VMSTATE_UINT8(busy_status, ERSTDeviceState),
+ VMSTATE_UINT8(command_status, ERSTDeviceState),
+ VMSTATE_UINT32(record_offset, ERSTDeviceState),
+ VMSTATE_UINT64(reg_action, ERSTDeviceState),
+ VMSTATE_UINT64(reg_value, ERSTDeviceState),
+ VMSTATE_UINT64(record_identifier, ERSTDeviceState),
+ VMSTATE_UINT32(next_record_index, ERSTDeviceState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void erst_realizefn(PCIDevice *pci_dev, Error **errp)
+{
+ ERSTDeviceState *s = ACPIERST(pci_dev);
+
+ trace_acpi_erst_realizefn_in();
+
+ if (!s->hostmem) {
+ error_setg(errp, "'" ACPI_ERST_MEMDEV_PROP "' property is not set");
+ return;
+ } else if (host_memory_backend_is_mapped(s->hostmem)) {
+ error_setg(errp, "can't use already busy memdev: %s",
+ object_get_canonical_path_component(OBJECT(s->hostmem)));
+ return;
+ }
+
+ s->hostmem_mr = host_memory_backend_get_memory(s->hostmem);
+
+ /* HostMemoryBackend size will be multiple of PAGE_SIZE */
+ s->storage_size = object_property_get_int(OBJECT(s->hostmem), "size", errp);
+
+ /* Initialize backend storage and record_count */
+ check_erst_backend_storage(s, errp);
+
+ /* BAR 0: Programming registers */
+ memory_region_init_io(&s->iomem_mr, OBJECT(pci_dev), &erst_reg_ops, s,
+ TYPE_ACPI_ERST, ERST_REG_SIZE);
+ pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->iomem_mr);
+
+ /* BAR 1: Exchange buffer memory */
+ memory_region_init_ram(&s->exchange_mr, OBJECT(pci_dev),
+ "erst.exchange",
+ le32_to_cpu(s->header->record_size), errp);
+ pci_register_bar(pci_dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY,
+ &s->exchange_mr);
+
+ /* Include the backend storage in the migration stream */
+ vmstate_register_ram_global(s->hostmem_mr);
+
+ trace_acpi_erst_realizefn_out(s->storage_size);
+}
+
+static void erst_reset(DeviceState *dev)
+{
+ ERSTDeviceState *s = ACPIERST(dev);
+
+ trace_acpi_erst_reset_in(le32_to_cpu(s->header->record_count));
+ s->operation = 0;
+ s->busy_status = 0;
+ s->command_status = STATUS_SUCCESS;
+ s->record_identifier = ERST_UNSPECIFIED_RECORD_ID;
+ s->record_offset = 0;
+ s->next_record_index = s->first_record_index;
+ /* NOTE: first/last_record_index are computed only once */
+ trace_acpi_erst_reset_out(le32_to_cpu(s->header->record_count));
+}
+
+static Property erst_properties[] = {
+ DEFINE_PROP_LINK(ACPI_ERST_MEMDEV_PROP, ERSTDeviceState, hostmem,
+ TYPE_MEMORY_BACKEND, HostMemoryBackend *),
+ DEFINE_PROP_UINT32(ACPI_ERST_RECORD_SIZE_PROP, ERSTDeviceState,
+ default_record_size, ERST_RECORD_SIZE),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void erst_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ trace_acpi_erst_class_init_in();
+ k->realize = erst_realizefn;
+ k->vendor_id = PCI_VENDOR_ID_REDHAT;
+ k->device_id = PCI_DEVICE_ID_REDHAT_ACPI_ERST;
+ k->revision = 0x00;
+ k->class_id = PCI_CLASS_OTHERS;
+ dc->reset = erst_reset;
+ dc->vmsd = &erst_vmstate;
+ dc->user_creatable = true;
+ dc->hotpluggable = false;
+ device_class_set_props(dc, erst_properties);
+ dc->desc = "ACPI Error Record Serialization Table (ERST) device";
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ trace_acpi_erst_class_init_out();
+}
+
+static const TypeInfo erst_type_info = {
+ .name = TYPE_ACPI_ERST,
+ .parent = TYPE_PCI_DEVICE,
+ .class_init = erst_class_init,
+ .instance_size = sizeof(ERSTDeviceState),
+ .interfaces = (InterfaceInfo[]) {
+ { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+ { }
+ }
+};
+
+static void erst_register_types(void)
+{
+ type_register_static(&erst_type_info);
+}
+
+type_init(erst_register_types)
diff --git a/hw/acpi/meson.build b/hw/acpi/meson.build
index adf6347..f5b2298 100644
--- a/hw/acpi/meson.build
+++ b/hw/acpi/meson.build
@@ -22,6 +22,7 @@ acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_true: files('pcihp.c'))
acpi_ss.add(when: 'CONFIG_ACPI_PCIHP', if_false: files('acpi-pci-hotplug-stub.c'))
acpi_ss.add(when: 'CONFIG_ACPI_VIOT', if_true: files('viot.c'))
acpi_ss.add(when: 'CONFIG_ACPI_X86_ICH', if_true: files('ich9.c', 'tco.c'))
+acpi_ss.add(when: 'CONFIG_ACPI_ERST', if_true: files('erst.c'))
acpi_ss.add(when: 'CONFIG_IPMI', if_true: files('ipmi.c'), if_false: files('ipmi-stub.c'))
acpi_ss.add(when: 'CONFIG_PC', if_false: files('acpi-x86-stub.c'))
acpi_ss.add(when: 'CONFIG_TPM', if_true: files('tpm.c'))
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index 974d770..2250126 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -55,3 +55,18 @@ piix4_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64
# tco.c
tco_timer_reload(int ticks, int msec) "ticks=%d (%d ms)"
tco_timer_expired(int timeouts_no, bool strap, bool no_reboot) "timeouts_no=%d no_reboot=%d/%d"
+
+# erst.c
+acpi_erst_reg_write(uint64_t addr, uint64_t val, unsigned size) "addr: 0x%04" PRIx64 " <== 0x%016" PRIx64 " (size: %u)"
+acpi_erst_reg_read(uint64_t addr, uint64_t val, unsigned size) " addr: 0x%04" PRIx64 " ==> 0x%016" PRIx64 " (size: %u)"
+acpi_erst_mem_write(uint64_t addr, uint64_t val, unsigned size) "addr: 0x%06" PRIx64 " <== 0x%016" PRIx64 " (size: %u)"
+acpi_erst_mem_read(uint64_t addr, uint64_t val, unsigned size) " addr: 0x%06" PRIx64 " ==> 0x%016" PRIx64 " (size: %u)"
+acpi_erst_pci_bar_0(uint64_t addr) "BAR0: 0x%016" PRIx64
+acpi_erst_pci_bar_1(uint64_t addr) "BAR1: 0x%016" PRIx64
+acpi_erst_realizefn_in(void)
+acpi_erst_realizefn_out(unsigned size) "total nvram size %u bytes"
+acpi_erst_reset_in(unsigned record_count) "record_count %u"
+acpi_erst_reset_out(unsigned record_count) "record_count %u"
+acpi_erst_post_load(void *header, unsigned slot_size) "header: 0x%p slot_size %u"
+acpi_erst_class_init_in(void)
+acpi_erst_class_init_out(void)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (4 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 05/10] ACPI ERST: support for ACPI ERST feature Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-27 8:36 ` Ani Sinha
2022-01-26 16:28 ` [PATCH v14 07/10] ACPI ERST: create ACPI ERST table for pc/x86 machines Eric DeVolder
` (3 subsequent siblings)
9 siblings, 1 reply; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This builds the ACPI ERST table to inform OSPM how to communicate
with the acpi-erst device.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 225 insertions(+)
diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
index fe9ba51..5d5a639 100644
--- a/hw/acpi/erst.c
+++ b/hw/acpi/erst.c
@@ -59,6 +59,27 @@
#define STATUS_RECORD_STORE_EMPTY 0x04
#define STATUS_RECORD_NOT_FOUND 0x05
+/* ACPI 4.0: Table 17-19 Serialization Instructions */
+#define INST_READ_REGISTER 0x00
+#define INST_READ_REGISTER_VALUE 0x01
+#define INST_WRITE_REGISTER 0x02
+#define INST_WRITE_REGISTER_VALUE 0x03
+#define INST_NOOP 0x04
+#define INST_LOAD_VAR1 0x05
+#define INST_LOAD_VAR2 0x06
+#define INST_STORE_VAR1 0x07
+#define INST_ADD 0x08
+#define INST_SUBTRACT 0x09
+#define INST_ADD_VALUE 0x0A
+#define INST_SUBTRACT_VALUE 0x0B
+#define INST_STALL 0x0C
+#define INST_STALL_WHILE_TRUE 0x0D
+#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
+#define INST_GOTO 0x0F
+#define INST_SET_SRC_ADDRESS_BASE 0x10
+#define INST_SET_DST_ADDRESS_BASE 0x11
+#define INST_MOVE_DATA 0x12
+
/* UEFI 2.1: Appendix N Common Platform Error Record */
#define UEFI_CPER_RECORD_MIN_SIZE 128U
#define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
@@ -172,6 +193,210 @@ typedef struct {
/*******************************************************************/
/*******************************************************************/
+typedef struct {
+ GArray *table_data;
+ pcibus_t bar;
+ uint8_t instruction;
+ uint8_t flags;
+ uint8_t register_bit_width;
+ pcibus_t register_offset;
+} BuildSerializationInstructionEntry;
+
+/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
+static void build_serialization_instruction(
+ BuildSerializationInstructionEntry *e,
+ uint8_t serialization_action,
+ uint64_t value)
+{
+ /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
+ struct AcpiGenericAddress gas;
+ uint64_t mask;
+
+ /* Serialization Action */
+ build_append_int_noprefix(e->table_data, serialization_action, 1);
+ /* Instruction */
+ build_append_int_noprefix(e->table_data, e->instruction, 1);
+ /* Flags */
+ build_append_int_noprefix(e->table_data, e->flags, 1);
+ /* Reserved */
+ build_append_int_noprefix(e->table_data, 0, 1);
+ /* Register Region */
+ gas.space_id = AML_SYSTEM_MEMORY;
+ gas.bit_width = e->register_bit_width;
+ gas.bit_offset = 0;
+ gas.access_width = ctz32(e->register_bit_width) - 2;
+ gas.address = (uint64_t)(e->bar + e->register_offset);
+ build_append_gas_from_struct(e->table_data, &gas);
+ /* Value */
+ build_append_int_noprefix(e->table_data, value, 8);
+ /* Mask */
+ mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
+ build_append_int_noprefix(e->table_data, mask, 8);
+}
+
+/* ACPI 4.0: 17.4.1 Serialization Action Table */
+void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
+ const char *oem_id, const char *oem_table_id)
+{
+ /*
+ * Serialization Action Table
+ * The serialization action table must be generated first
+ * so that its size can be known in order to populate the
+ * Instruction Entry Count field.
+ */
+ GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
+ pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
+ AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
+ .oem_table_id = oem_table_id };
+ /* Contexts for the different ways ACTION and VALUE are accessed */
+ BuildSerializationInstructionEntry rd_value_32_val = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_READ_REGISTER_VALUE,
+ .flags = 0,
+ .register_bit_width = 32,
+ .register_offset = ERST_VALUE_OFFSET,
+ };
+ BuildSerializationInstructionEntry rd_value_32 = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_READ_REGISTER,
+ .flags = 0,
+ .register_bit_width = 32,
+ .register_offset = ERST_VALUE_OFFSET,
+ };
+ BuildSerializationInstructionEntry rd_value_64 = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_READ_REGISTER,
+ .flags = 0,
+ .register_bit_width = 64,
+ .register_offset = ERST_VALUE_OFFSET,
+ };
+ BuildSerializationInstructionEntry wr_value_32_val = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_WRITE_REGISTER_VALUE,
+ .flags = 0,
+ .register_bit_width = 32,
+ .register_offset = ERST_VALUE_OFFSET,
+ };
+ BuildSerializationInstructionEntry wr_value_32 = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_WRITE_REGISTER,
+ .flags = 0,
+ .register_bit_width = 32,
+ .register_offset = ERST_VALUE_OFFSET,
+ };
+ BuildSerializationInstructionEntry wr_value_64 = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_WRITE_REGISTER,
+ .flags = 0,
+ .register_bit_width = 64,
+ .register_offset = ERST_VALUE_OFFSET,
+ };
+ BuildSerializationInstructionEntry wr_action = {
+ .table_data = table_instruction_data,
+ .bar = bar0,
+ .instruction = INST_WRITE_REGISTER_VALUE,
+ .flags = 0,
+ .register_bit_width = 32,
+ .register_offset = ERST_ACTION_OFFSET,
+ };
+ unsigned action;
+
+ trace_acpi_erst_pci_bar_0(bar0);
+
+ /* Serialization Instruction Entries */
+ action = ACTION_BEGIN_WRITE_OPERATION;
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_BEGIN_READ_OPERATION;
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_BEGIN_CLEAR_OPERATION;
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_END_OPERATION;
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_SET_RECORD_OFFSET;
+ build_serialization_instruction(&wr_value_32, action, 0);
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_EXECUTE_OPERATION;
+ build_serialization_instruction(&wr_value_32_val, action,
+ ERST_EXECUTE_OPERATION_MAGIC);
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_CHECK_BUSY_STATUS;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_32_val, action, 0x01);
+
+ action = ACTION_GET_COMMAND_STATUS;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_32, action, 0);
+
+ action = ACTION_GET_RECORD_IDENTIFIER;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_64, action, 0);
+
+ action = ACTION_SET_RECORD_IDENTIFIER;
+ build_serialization_instruction(&wr_value_64, action, 0);
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_GET_RECORD_COUNT;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_32, action, 0);
+
+ action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
+ build_serialization_instruction(&wr_action, action, action);
+
+ action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_64, action, 0);
+
+ action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_64, action, 0);
+
+ action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_32, action, 0);
+
+ action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
+ build_serialization_instruction(&wr_action, action, action);
+ build_serialization_instruction(&rd_value_64, action, 0);
+
+ /* Serialization Header */
+ acpi_table_begin(&table, table_data);
+
+ /* Serialization Header Size */
+ build_append_int_noprefix(table_data, 48, 4);
+
+ /* Reserved */
+ build_append_int_noprefix(table_data, 0, 4);
+
+ /*
+ * Instruction Entry Count
+ * Each instruction entry is 32 bytes
+ */
+ g_assert((table_instruction_data->len) % 32 == 0);
+ build_append_int_noprefix(table_data,
+ (table_instruction_data->len / 32), 4);
+
+ /* Serialization Instruction Entries */
+ g_array_append_vals(table_data, table_instruction_data->data,
+ table_instruction_data->len);
+ g_array_free(table_instruction_data, TRUE);
+
+ acpi_table_end(linker, &table);
+}
+
+/*******************************************************************/
+/*******************************************************************/
static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
{
uint8_t *rc = NULL;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 07/10] ACPI ERST: create ACPI ERST table for pc/x86 machines
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (5 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 08/10] ACPI ERST: qtest for ERST Eric DeVolder
` (2 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This change exposes ACPI ERST support for x86 guests.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
---
hw/i386/acpi-build.c | 15 +++++++++++++++
hw/i386/acpi-microvm.c | 15 +++++++++++++++
include/hw/acpi/erst.h | 5 +++++
3 files changed, 35 insertions(+)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index ce823e8..ebd47aa 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -43,6 +43,7 @@
#include "sysemu/tpm.h"
#include "hw/acpi/tpm.h"
#include "hw/acpi/vmgenid.h"
+#include "hw/acpi/erst.h"
#include "sysemu/tpm_backend.h"
#include "hw/rtc/mc146818rtc_regs.h"
#include "migration/vmstate.h"
@@ -74,6 +75,8 @@
#include "hw/acpi/hmat.h"
#include "hw/acpi/viot.h"
+#include CONFIG_DEVICES
+
/* These are used to size the ACPI tables for -M pc-i440fx-1.7 and
* -M pc-i440fx-2.0. Even if the actual amount of AML generated grows
* a little bit, there should be plenty of free space since the DSDT
@@ -2575,6 +2578,18 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
x86ms->oem_table_id);
+#ifdef CONFIG_ACPI_ERST
+ {
+ Object *erst_dev;
+ erst_dev = find_erst_dev();
+ if (erst_dev) {
+ acpi_add_table(table_offsets, tables_blob);
+ build_erst(tables_blob, tables->linker, erst_dev,
+ x86ms->oem_id, x86ms->oem_table_id);
+ }
+ }
+#endif
+
vmgenid_dev = find_vmgenid_dev();
if (vmgenid_dev) {
acpi_add_table(table_offsets, tables_blob);
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index 196d318..68ca7e7 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -30,6 +30,7 @@
#include "hw/acpi/bios-linker-loader.h"
#include "hw/acpi/generic_event_device.h"
#include "hw/acpi/utils.h"
+#include "hw/acpi/erst.h"
#include "hw/i386/fw_cfg.h"
#include "hw/i386/microvm.h"
#include "hw/pci/pci.h"
@@ -40,6 +41,8 @@
#include "acpi-common.h"
#include "acpi-microvm.h"
+#include CONFIG_DEVICES
+
static void acpi_dsdt_add_virtio(Aml *scope,
MicrovmMachineState *mms)
{
@@ -207,6 +210,18 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
x86ms->oem_table_id);
+#ifdef CONFIG_ACPI_ERST
+ {
+ Object *erst_dev;
+ erst_dev = find_erst_dev();
+ if (erst_dev) {
+ acpi_add_table(table_offsets, tables_blob);
+ build_erst(tables_blob, tables->linker, erst_dev,
+ x86ms->oem_id, x86ms->oem_table_id);
+ }
+ }
+#endif
+
xsdt = tables_blob->len;
build_xsdt(tables_blob, tables->linker, table_offsets, x86ms->oem_id,
x86ms->oem_table_id);
diff --git a/include/hw/acpi/erst.h b/include/hw/acpi/erst.h
index 9d63717..b747fe7 100644
--- a/include/hw/acpi/erst.h
+++ b/include/hw/acpi/erst.h
@@ -16,4 +16,9 @@ void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
#define TYPE_ACPI_ERST "acpi-erst"
+/* returns NULL unless there is exactly one device */
+static inline Object *find_erst_dev(void)
+{
+ return object_resolve_path_type("", TYPE_ACPI_ERST, NULL);
+}
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 08/10] ACPI ERST: qtest for ERST
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (6 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 07/10] ACPI ERST: create ACPI ERST table for pc/x86 machines Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 09/10] ACPI ERST: bios-tables-test testcase Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 10/10] ACPI ERST: step 6 of bios-tables-test.c Eric DeVolder
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This change provides a qtest that locates and then does a simple
interrogation of the ERST feature within the guest.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
---
tests/qtest/erst-test.c | 172 ++++++++++++++++++++++++++++++++++++++++++++++++
tests/qtest/meson.build | 2 +
2 files changed, 174 insertions(+)
create mode 100644 tests/qtest/erst-test.c
diff --git a/tests/qtest/erst-test.c b/tests/qtest/erst-test.c
new file mode 100644
index 0000000..f9ad3c9
--- /dev/null
+++ b/tests/qtest/erst-test.c
@@ -0,0 +1,172 @@
+/*
+ * QTest testcase for acpi-erst
+ *
+ * Copyright (c) 2021 Oracle
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include <glib/gstdio.h>
+#include "libqos/libqos-pc.h"
+#include "libqos/libqtest.h"
+#include "qemu-common.h"
+
+#include "hw/pci/pci.h"
+
+static void save_fn(QPCIDevice *dev, int devfn, void *data)
+{
+ QPCIDevice **pdev = (QPCIDevice **) data;
+
+ *pdev = dev;
+}
+
+static QPCIDevice *get_erst_device(QPCIBus *pcibus)
+{
+ QPCIDevice *dev;
+
+ dev = NULL;
+ qpci_device_foreach(pcibus,
+ PCI_VENDOR_ID_REDHAT,
+ PCI_DEVICE_ID_REDHAT_ACPI_ERST,
+ save_fn, &dev);
+ g_assert(dev != NULL);
+
+ return dev;
+}
+
+typedef struct _ERSTState {
+ QOSState *qs;
+ QPCIBar reg_bar, mem_bar;
+ uint64_t reg_barsize, mem_barsize;
+ QPCIDevice *dev;
+} ERSTState;
+
+#define ACTION 0
+#define VALUE 8
+
+static const char *reg2str(unsigned reg)
+{
+ switch (reg) {
+ case 0:
+ return "ACTION";
+ case 8:
+ return "VALUE";
+ default:
+ return NULL;
+ }
+}
+
+static inline uint32_t in_reg32(ERSTState *s, unsigned reg)
+{
+ const char *name = reg2str(reg);
+ uint32_t res;
+
+ res = qpci_io_readl(s->dev, s->reg_bar, reg);
+ g_test_message("*%s -> %08x", name, res);
+
+ return res;
+}
+
+static inline uint64_t in_reg64(ERSTState *s, unsigned reg)
+{
+ const char *name = reg2str(reg);
+ uint64_t res;
+
+ res = qpci_io_readq(s->dev, s->reg_bar, reg);
+ g_test_message("*%s -> %016llx", name, (unsigned long long)res);
+
+ return res;
+}
+
+static inline void out_reg32(ERSTState *s, unsigned reg, uint32_t v)
+{
+ const char *name = reg2str(reg);
+
+ g_test_message("%08x -> *%s", v, name);
+ qpci_io_writel(s->dev, s->reg_bar, reg, v);
+}
+
+static inline void out_reg64(ERSTState *s, unsigned reg, uint64_t v)
+{
+ const char *name = reg2str(reg);
+
+ g_test_message("%016llx -> *%s", (unsigned long long)v, name);
+ qpci_io_writeq(s->dev, s->reg_bar, reg, v);
+}
+
+static void cleanup_vm(ERSTState *s)
+{
+ g_free(s->dev);
+ qtest_shutdown(s->qs);
+}
+
+static void setup_vm_cmd(ERSTState *s, const char *cmd)
+{
+ const char *arch = qtest_get_arch();
+
+ if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+ s->qs = qtest_pc_boot(cmd);
+ } else {
+ g_printerr("erst-test tests are only available on x86\n");
+ exit(EXIT_FAILURE);
+ }
+ s->dev = get_erst_device(s->qs->pcibus);
+
+ s->reg_bar = qpci_iomap(s->dev, 0, &s->reg_barsize);
+ g_assert_cmpuint(s->reg_barsize, ==, 16);
+
+ s->mem_bar = qpci_iomap(s->dev, 1, &s->mem_barsize);
+ g_assert_cmpuint(s->mem_barsize, ==, 0x2000);
+
+ qpci_device_enable(s->dev);
+}
+
+static void test_acpi_erst_basic(void)
+{
+ ERSTState state;
+ uint64_t log_address_range;
+ uint64_t log_address_length;
+ uint32_t log_address_attr;
+
+ setup_vm_cmd(&state,
+ "-object memory-backend-file,"
+ "mem-path=acpi-erst.XXXXXX,"
+ "size=64K,"
+ "share=on,"
+ "id=nvram "
+ "-device acpi-erst,"
+ "memdev=nvram");
+
+ out_reg32(&state, ACTION, 0xD);
+ log_address_range = in_reg64(&state, VALUE);
+ out_reg32(&state, ACTION, 0xE);
+ log_address_length = in_reg64(&state, VALUE);
+ out_reg32(&state, ACTION, 0xF);
+ log_address_attr = in_reg32(&state, VALUE);
+
+ /* Check log_address_range is not 0, ~0 or base */
+ g_assert_cmpuint(log_address_range, !=, 0ULL);
+ g_assert_cmpuint(log_address_range, !=, ~0ULL);
+ g_assert_cmpuint(log_address_range, !=, state.reg_bar.addr);
+ g_assert_cmpuint(log_address_range, ==, state.mem_bar.addr);
+
+ /* Check log_address_length is bar1_size */
+ g_assert_cmpuint(log_address_length, ==, state.mem_barsize);
+
+ /* Check log_address_attr is 0 */
+ g_assert_cmpuint(log_address_attr, ==, 0);
+
+ cleanup_vm(&state);
+}
+
+int main(int argc, char **argv)
+{
+ int ret;
+
+ g_test_init(&argc, &argv, NULL);
+ qtest_add_func("/acpi-erst/basic", test_acpi_erst_basic);
+ ret = g_test_run();
+ return ret;
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 26937de..581d217 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,6 +68,7 @@ qtests_i386 = \
(config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + \
(config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) + \
(config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) + \
+ (config_all_devices.has_key('CONFIG_ACPI_ERST') ? ['erst-test'] : []) + \
(config_all_devices.has_key('CONFIG_VIRTIO_NET') and \
config_all_devices.has_key('CONFIG_Q35') and \
config_all_devices.has_key('CONFIG_VIRTIO_PCI') and \
@@ -278,6 +279,7 @@ qtests = {
'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
'cdrom-test': files('boot-sector.c'),
'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
+ 'erst-test': files('erst-test.c'),
'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
'migration-test': files('migration-helpers.c'),
'pxe-test': files('boot-sector.c'),
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 09/10] ACPI ERST: bios-tables-test testcase
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (7 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 08/10] ACPI ERST: qtest for ERST Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 10/10] ACPI ERST: step 6 of bios-tables-test.c Eric DeVolder
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
This change implements the test suite checks for the ERST table.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Reviewed-by: Ani Sinha <ani@anisinha.ca>
---
tests/qtest/bios-tables-test.c | 54 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index e6b72d9..266b215 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1446,6 +1446,57 @@ static void test_acpi_piix4_tcg_acpi_hmat(void)
test_acpi_tcg_acpi_hmat(MACHINE_PC);
}
+static void test_acpi_erst(const char *machine)
+{
+ gchar *tmp_path = g_dir_make_tmp("qemu-test-erst.XXXXXX", NULL);
+ gchar *params;
+ test_data data;
+
+ memset(&data, 0, sizeof(data));
+ data.machine = machine;
+ data.variant = ".acpierst";
+ params = g_strdup_printf(
+ " -object memory-backend-file,id=erstnvram,"
+ "mem-path=%s,size=0x10000,share=on"
+ " -device acpi-erst,memdev=erstnvram", tmp_path);
+ test_acpi_one(params, &data);
+ free_test_data(&data);
+ g_free(params);
+ g_assert(g_rmdir(tmp_path) == 0);
+ g_free(tmp_path);
+}
+
+static void test_acpi_piix4_acpi_erst(void)
+{
+ test_acpi_erst(MACHINE_PC);
+}
+
+static void test_acpi_q35_acpi_erst(void)
+{
+ test_acpi_erst(MACHINE_Q35);
+}
+
+static void test_acpi_microvm_acpi_erst(void)
+{
+ gchar *tmp_path = g_dir_make_tmp("qemu-test-erst.XXXXXX", NULL);
+ gchar *params;
+ test_data data;
+
+ test_acpi_microvm_prepare(&data);
+ data.variant = ".pcie";
+ data.tcg_only = true; /* need constant host-phys-bits */
+ params = g_strdup_printf(" -machine microvm,"
+ "acpi=on,ioapic2=off,rtc=off,pcie=on"
+ " -object memory-backend-file,id=erstnvram,"
+ "mem-path=%s,size=0x10000,share=on"
+ " -device acpi-erst,memdev=erstnvram", tmp_path);
+ test_acpi_one(params, &data);
+ g_free(params);
+ g_assert(g_rmdir(tmp_path) == 0);
+ g_free(tmp_path);
+ free_test_data(&data);
+}
+
static void test_acpi_virt_tcg(void)
{
test_data data = {
@@ -1675,6 +1726,8 @@ int main(int argc, char *argv[])
qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
qtest_add_func("acpi/piix4/acpihmat", test_acpi_piix4_tcg_acpi_hmat);
qtest_add_func("acpi/q35/acpihmat", test_acpi_q35_tcg_acpi_hmat);
+ qtest_add_func("acpi/piix4/acpierst", test_acpi_piix4_acpi_erst);
+ qtest_add_func("acpi/q35/acpierst", test_acpi_q35_acpi_erst);
qtest_add_func("acpi/microvm", test_acpi_microvm_tcg);
qtest_add_func("acpi/microvm/usb", test_acpi_microvm_usb_tcg);
qtest_add_func("acpi/microvm/rtc", test_acpi_microvm_rtc_tcg);
@@ -1684,6 +1737,7 @@ int main(int argc, char *argv[])
qtest_add_func("acpi/q35/ivrs", test_acpi_q35_tcg_ivrs);
if (strcmp(arch, "x86_64") == 0) {
qtest_add_func("acpi/microvm/pcie", test_acpi_microvm_pcie_tcg);
+ qtest_add_func("acpi/microvm/acpierst", test_acpi_microvm_acpi_erst);
}
}
if (has_kvm) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v14 10/10] ACPI ERST: step 6 of bios-tables-test.c
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
` (8 preceding siblings ...)
2022-01-26 16:28 ` [PATCH v14 09/10] ACPI ERST: bios-tables-test testcase Eric DeVolder
@ 2022-01-26 16:28 ` Eric DeVolder
9 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-26 16:28 UTC (permalink / raw)
To: qemu-devel
Cc: berrange, ehabkost, mst, konrad.wilk, pbonzini, ani, imammedo,
boris.ostrovsky, rth
Following the guidelines in tests/qtest/bios-tables-test.c, this
is step 6.
Below is the disassembly of tests/data/acpi/pc/ERST.acpierst.
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20180508 (64-bit version)
* Copyright (c) 2000 - 2018 Intel Corporation
*
* Disassembly of tests/data/acpi/pc/ERST.acpierst, Thu Dec 2 13:32:07 2021
*
* ACPI Data Table [ERST]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue
*/
[000h 0000 4] Signature : "ERST" [Error Record Serialization Table]
[004h 0004 4] Table Length : 00000390
[008h 0008 1] Revision : 01
[009h 0009 1] Checksum : D6
[00Ah 0010 6] Oem ID : "BOCHS "
[010h 0016 8] Oem Table ID : "BXPC "
[018h 0024 4] Oem Revision : 00000001
[01Ch 0028 4] Asl Compiler ID : "BXPC"
[020h 0032 4] Asl Compiler Revision : 00000001
[024h 0036 4] Serialization Header Length : 00000030
[028h 0040 4] Reserved : 00000000
[02Ch 0044 4] Instruction Entry Count : 0000001B
[030h 0048 1] Action : 00 [Begin Write Operation]
[031h 0049 1] Instruction : 03 [Write Register Value]
[032h 0050 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[033h 0051 1] Reserved : 00
[034h 0052 12] Register Region : [Generic Address Structure]
[034h 0052 1] Space ID : 00 [SystemMemory]
[035h 0053 1] Bit Width : 20
[036h 0054 1] Bit Offset : 00
[037h 0055 1] Encoded Access Width : 03 [DWord Access:32]
[038h 0056 8] Address : 00000000FEBF3000
[040h 0064 8] Value : 0000000000000000
[048h 0072 8] Mask : 00000000000000FF
[050h 0080 1] Action : 01 [Begin Read Operation]
[051h 0081 1] Instruction : 03 [Write Register Value]
[052h 0082 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[053h 0083 1] Reserved : 00
[054h 0084 12] Register Region : [Generic Address Structure]
[054h 0084 1] Space ID : 00 [SystemMemory]
[055h 0085 1] Bit Width : 20
[056h 0086 1] Bit Offset : 00
[057h 0087 1] Encoded Access Width : 03 [DWord Access:32]
[058h 0088 8] Address : 00000000FEBF3000
[060h 0096 8] Value : 0000000000000001
[068h 0104 8] Mask : 00000000000000FF
[070h 0112 1] Action : 02 [Begin Clear Operation]
[071h 0113 1] Instruction : 03 [Write Register Value]
[072h 0114 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[073h 0115 1] Reserved : 00
[074h 0116 12] Register Region : [Generic Address Structure]
[074h 0116 1] Space ID : 00 [SystemMemory]
[075h 0117 1] Bit Width : 20
[076h 0118 1] Bit Offset : 00
[077h 0119 1] Encoded Access Width : 03 [DWord Access:32]
[078h 0120 8] Address : 00000000FEBF3000
[080h 0128 8] Value : 0000000000000002
[088h 0136 8] Mask : 00000000000000FF
[090h 0144 1] Action : 03 [End Operation]
[091h 0145 1] Instruction : 03 [Write Register Value]
[092h 0146 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[093h 0147 1] Reserved : 00
[094h 0148 12] Register Region : [Generic Address Structure]
[094h 0148 1] Space ID : 00 [SystemMemory]
[095h 0149 1] Bit Width : 20
[096h 0150 1] Bit Offset : 00
[097h 0151 1] Encoded Access Width : 03 [DWord Access:32]
[098h 0152 8] Address : 00000000FEBF3000
[0A0h 0160 8] Value : 0000000000000003
[0A8h 0168 8] Mask : 00000000000000FF
[0B0h 0176 1] Action : 04 [Set Record Offset]
[0B1h 0177 1] Instruction : 02 [Write Register]
[0B2h 0178 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[0B3h 0179 1] Reserved : 00
[0B4h 0180 12] Register Region : [Generic Address Structure]
[0B4h 0180 1] Space ID : 00 [SystemMemory]
[0B5h 0181 1] Bit Width : 20
[0B6h 0182 1] Bit Offset : 00
[0B7h 0183 1] Encoded Access Width : 03 [DWord Access:32]
[0B8h 0184 8] Address : 00000000FEBF3008
[0C0h 0192 8] Value : 0000000000000000
[0C8h 0200 8] Mask : 00000000FFFFFFFF
[0D0h 0208 1] Action : 04 [Set Record Offset]
[0D1h 0209 1] Instruction : 03 [Write Register Value]
[0D2h 0210 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[0D3h 0211 1] Reserved : 00
[0D4h 0212 12] Register Region : [Generic Address Structure]
[0D4h 0212 1] Space ID : 00 [SystemMemory]
[0D5h 0213 1] Bit Width : 20
[0D6h 0214 1] Bit Offset : 00
[0D7h 0215 1] Encoded Access Width : 03 [DWord Access:32]
[0D8h 0216 8] Address : 00000000FEBF3000
[0E0h 0224 8] Value : 0000000000000004
[0E8h 0232 8] Mask : 00000000000000FF
[0F0h 0240 1] Action : 05 [Execute Operation]
[0F1h 0241 1] Instruction : 03 [Write Register Value]
[0F2h 0242 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[0F3h 0243 1] Reserved : 00
[0F4h 0244 12] Register Region : [Generic Address Structure]
[0F4h 0244 1] Space ID : 00 [SystemMemory]
[0F5h 0245 1] Bit Width : 20
[0F6h 0246 1] Bit Offset : 00
[0F7h 0247 1] Encoded Access Width : 03 [DWord Access:32]
[0F8h 0248 8] Address : 00000000FEBF3008
[100h 0256 8] Value : 000000000000009C
[108h 0264 8] Mask : 00000000000000FF
[110h 0272 1] Action : 05 [Execute Operation]
[111h 0273 1] Instruction : 03 [Write Register Value]
[112h 0274 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[113h 0275 1] Reserved : 00
[114h 0276 12] Register Region : [Generic Address Structure]
[114h 0276 1] Space ID : 00 [SystemMemory]
[115h 0277 1] Bit Width : 20
[116h 0278 1] Bit Offset : 00
[117h 0279 1] Encoded Access Width : 03 [DWord Access:32]
[118h 0280 8] Address : 00000000FEBF3000
[120h 0288 8] Value : 0000000000000005
[128h 0296 8] Mask : 00000000000000FF
[130h 0304 1] Action : 06 [Check Busy Status]
[131h 0305 1] Instruction : 03 [Write Register Value]
[132h 0306 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[133h 0307 1] Reserved : 00
[134h 0308 12] Register Region : [Generic Address Structure]
[134h 0308 1] Space ID : 00 [SystemMemory]
[135h 0309 1] Bit Width : 20
[136h 0310 1] Bit Offset : 00
[137h 0311 1] Encoded Access Width : 03 [DWord Access:32]
[138h 0312 8] Address : 00000000FEBF3000
[140h 0320 8] Value : 0000000000000006
[148h 0328 8] Mask : 00000000000000FF
[150h 0336 1] Action : 06 [Check Busy Status]
[151h 0337 1] Instruction : 01 [Read Register Value]
[152h 0338 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[153h 0339 1] Reserved : 00
[154h 0340 12] Register Region : [Generic Address Structure]
[154h 0340 1] Space ID : 00 [SystemMemory]
[155h 0341 1] Bit Width : 20
[156h 0342 1] Bit Offset : 00
[157h 0343 1] Encoded Access Width : 03 [DWord Access:32]
[158h 0344 8] Address : 00000000FEBF3008
[160h 0352 8] Value : 0000000000000001
[168h 0360 8] Mask : 00000000000000FF
[170h 0368 1] Action : 07 [Get Command Status]
[171h 0369 1] Instruction : 03 [Write Register Value]
[172h 0370 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[173h 0371 1] Reserved : 00
[174h 0372 12] Register Region : [Generic Address Structure]
[174h 0372 1] Space ID : 00 [SystemMemory]
[175h 0373 1] Bit Width : 20
[176h 0374 1] Bit Offset : 00
[177h 0375 1] Encoded Access Width : 03 [DWord Access:32]
[178h 0376 8] Address : 00000000FEBF3000
[180h 0384 8] Value : 0000000000000007
[188h 0392 8] Mask : 00000000000000FF
[190h 0400 1] Action : 07 [Get Command Status]
[191h 0401 1] Instruction : 00 [Read Register]
[192h 0402 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[193h 0403 1] Reserved : 00
[194h 0404 12] Register Region : [Generic Address Structure]
[194h 0404 1] Space ID : 00 [SystemMemory]
[195h 0405 1] Bit Width : 20
[196h 0406 1] Bit Offset : 00
[197h 0407 1] Encoded Access Width : 03 [DWord Access:32]
[198h 0408 8] Address : 00000000FEBF3008
[1A0h 0416 8] Value : 0000000000000000
[1A8h 0424 8] Mask : 00000000000000FF
[1B0h 0432 1] Action : 08 [Get Record Identifier]
[1B1h 0433 1] Instruction : 03 [Write Register Value]
[1B2h 0434 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[1B3h 0435 1] Reserved : 00
[1B4h 0436 12] Register Region : [Generic Address Structure]
[1B4h 0436 1] Space ID : 00 [SystemMemory]
[1B5h 0437 1] Bit Width : 20
[1B6h 0438 1] Bit Offset : 00
[1B7h 0439 1] Encoded Access Width : 03 [DWord Access:32]
[1B8h 0440 8] Address : 00000000FEBF3000
[1C0h 0448 8] Value : 0000000000000008
[1C8h 0456 8] Mask : 00000000000000FF
[1D0h 0464 1] Action : 08 [Get Record Identifier]
[1D1h 0465 1] Instruction : 00 [Read Register]
[1D2h 0466 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[1D3h 0467 1] Reserved : 00
[1D4h 0468 12] Register Region : [Generic Address Structure]
[1D4h 0468 1] Space ID : 00 [SystemMemory]
[1D5h 0469 1] Bit Width : 40
[1D6h 0470 1] Bit Offset : 00
[1D7h 0471 1] Encoded Access Width : 04 [QWord Access:64]
[1D8h 0472 8] Address : 00000000FEBF3008
[1E0h 0480 8] Value : 0000000000000000
[1E8h 0488 8] Mask : FFFFFFFFFFFFFFFF
[1F0h 0496 1] Action : 09 [Set Record Identifier]
[1F1h 0497 1] Instruction : 02 [Write Register]
[1F2h 0498 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[1F3h 0499 1] Reserved : 00
[1F4h 0500 12] Register Region : [Generic Address Structure]
[1F4h 0500 1] Space ID : 00 [SystemMemory]
[1F5h 0501 1] Bit Width : 40
[1F6h 0502 1] Bit Offset : 00
[1F7h 0503 1] Encoded Access Width : 04 [QWord Access:64]
[1F8h 0504 8] Address : 00000000FEBF3008
[200h 0512 8] Value : 0000000000000000
[208h 0520 8] Mask : FFFFFFFFFFFFFFFF
[210h 0528 1] Action : 09 [Set Record Identifier]
[211h 0529 1] Instruction : 03 [Write Register Value]
[212h 0530 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[213h 0531 1] Reserved : 00
[214h 0532 12] Register Region : [Generic Address Structure]
[214h 0532 1] Space ID : 00 [SystemMemory]
[215h 0533 1] Bit Width : 20
[216h 0534 1] Bit Offset : 00
[217h 0535 1] Encoded Access Width : 03 [DWord Access:32]
[218h 0536 8] Address : 00000000FEBF3000
[220h 0544 8] Value : 0000000000000009
[228h 0552 8] Mask : 00000000000000FF
[230h 0560 1] Action : 0A [Get Record Count]
[231h 0561 1] Instruction : 03 [Write Register Value]
[232h 0562 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[233h 0563 1] Reserved : 00
[234h 0564 12] Register Region : [Generic Address Structure]
[234h 0564 1] Space ID : 00 [SystemMemory]
[235h 0565 1] Bit Width : 20
[236h 0566 1] Bit Offset : 00
[237h 0567 1] Encoded Access Width : 03 [DWord Access:32]
[238h 0568 8] Address : 00000000FEBF3000
[240h 0576 8] Value : 000000000000000A
[248h 0584 8] Mask : 00000000000000FF
[250h 0592 1] Action : 0A [Get Record Count]
[251h 0593 1] Instruction : 00 [Read Register]
[252h 0594 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[253h 0595 1] Reserved : 00
[254h 0596 12] Register Region : [Generic Address Structure]
[254h 0596 1] Space ID : 00 [SystemMemory]
[255h 0597 1] Bit Width : 20
[256h 0598 1] Bit Offset : 00
[257h 0599 1] Encoded Access Width : 03 [DWord Access:32]
[258h 0600 8] Address : 00000000FEBF3008
[260h 0608 8] Value : 0000000000000000
[268h 0616 8] Mask : 00000000FFFFFFFF
[270h 0624 1] Action : 0B [Begin Dummy Write]
[271h 0625 1] Instruction : 03 [Write Register Value]
[272h 0626 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[273h 0627 1] Reserved : 00
[274h 0628 12] Register Region : [Generic Address Structure]
[274h 0628 1] Space ID : 00 [SystemMemory]
[275h 0629 1] Bit Width : 20
[276h 0630 1] Bit Offset : 00
[277h 0631 1] Encoded Access Width : 03 [DWord Access:32]
[278h 0632 8] Address : 00000000FEBF3000
[280h 0640 8] Value : 000000000000000B
[288h 0648 8] Mask : 00000000000000FF
[290h 0656 1] Action : 0D [Get Error Address Range]
[291h 0657 1] Instruction : 03 [Write Register Value]
[292h 0658 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[293h 0659 1] Reserved : 00
[294h 0660 12] Register Region : [Generic Address Structure]
[294h 0660 1] Space ID : 00 [SystemMemory]
[295h 0661 1] Bit Width : 20
[296h 0662 1] Bit Offset : 00
[297h 0663 1] Encoded Access Width : 03 [DWord Access:32]
[298h 0664 8] Address : 00000000FEBF3000
[2A0h 0672 8] Value : 000000000000000D
[2A8h 0680 8] Mask : 00000000000000FF
[2B0h 0688 1] Action : 0D [Get Error Address Range]
[2B1h 0689 1] Instruction : 00 [Read Register]
[2B2h 0690 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[2B3h 0691 1] Reserved : 00
[2B4h 0692 12] Register Region : [Generic Address Structure]
[2B4h 0692 1] Space ID : 00 [SystemMemory]
[2B5h 0693 1] Bit Width : 40
[2B6h 0694 1] Bit Offset : 00
[2B7h 0695 1] Encoded Access Width : 04 [QWord Access:64]
[2B8h 0696 8] Address : 00000000FEBF3008
[2C0h 0704 8] Value : 0000000000000000
[2C8h 0712 8] Mask : FFFFFFFFFFFFFFFF
[2D0h 0720 1] Action : 0E [Get Error Address Length]
[2D1h 0721 1] Instruction : 03 [Write Register Value]
[2D2h 0722 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[2D3h 0723 1] Reserved : 00
[2D4h 0724 12] Register Region : [Generic Address Structure]
[2D4h 0724 1] Space ID : 00 [SystemMemory]
[2D5h 0725 1] Bit Width : 20
[2D6h 0726 1] Bit Offset : 00
[2D7h 0727 1] Encoded Access Width : 03 [DWord Access:32]
[2D8h 0728 8] Address : 00000000FEBF3000
[2E0h 0736 8] Value : 000000000000000E
[2E8h 0744 8] Mask : 00000000000000FF
[2F0h 0752 1] Action : 0E [Get Error Address Length]
[2F1h 0753 1] Instruction : 00 [Read Register]
[2F2h 0754 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[2F3h 0755 1] Reserved : 00
[2F4h 0756 12] Register Region : [Generic Address Structure]
[2F4h 0756 1] Space ID : 00 [SystemMemory]
[2F5h 0757 1] Bit Width : 40
[2F6h 0758 1] Bit Offset : 00
[2F7h 0759 1] Encoded Access Width : 04 [QWord Access:64]
[2F8h 0760 8] Address : 00000000FEBF3008
[300h 0768 8] Value : 0000000000000000
[308h 0776 8] Mask : 00000000FFFFFFFF
[310h 0784 1] Action : 0F [Get Error Attributes]
[311h 0785 1] Instruction : 03 [Write Register Value]
[312h 0786 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[313h 0787 1] Reserved : 00
[314h 0788 12] Register Region : [Generic Address Structure]
[314h 0788 1] Space ID : 00 [SystemMemory]
[315h 0789 1] Bit Width : 20
[316h 0790 1] Bit Offset : 00
[317h 0791 1] Encoded Access Width : 03 [DWord Access:32]
[318h 0792 8] Address : 00000000FEBF3000
[320h 0800 8] Value : 000000000000000F
[328h 0808 8] Mask : 00000000000000FF
[330h 0816 1] Action : 0F [Get Error Attributes]
[331h 0817 1] Instruction : 00 [Read Register]
[332h 0818 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[333h 0819 1] Reserved : 00
[334h 0820 12] Register Region : [Generic Address Structure]
[334h 0820 1] Space ID : 00 [SystemMemory]
[335h 0821 1] Bit Width : 20
[336h 0822 1] Bit Offset : 00
[337h 0823 1] Encoded Access Width : 03 [DWord Access:32]
[338h 0824 8] Address : 00000000FEBF3008
[340h 0832 8] Value : 0000000000000000
[348h 0840 8] Mask : 00000000FFFFFFFF
[350h 0848 1] Action : 10 [Execute Timings]
[351h 0849 1] Instruction : 03 [Write Register Value]
[352h 0850 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[353h 0851 1] Reserved : 00
[354h 0852 12] Register Region : [Generic Address Structure]
[354h 0852 1] Space ID : 00 [SystemMemory]
[355h 0853 1] Bit Width : 20
[356h 0854 1] Bit Offset : 00
[357h 0855 1] Encoded Access Width : 03 [DWord Access:32]
[358h 0856 8] Address : 00000000FEBF3000
[360h 0864 8] Value : 0000000000000010
[368h 0872 8] Mask : 00000000000000FF
[370h 0880 1] Action : 10 [Execute Timings]
[371h 0881 1] Instruction : 00 [Read Register]
[372h 0882 1] Flags (decoded below) : 00
Preserve Register Bits : 0
[373h 0883 1] Reserved : 00
[374h 0884 12] Register Region : [Generic Address Structure]
[374h 0884 1] Space ID : 00 [SystemMemory]
[375h 0885 1] Bit Width : 40
[376h 0886 1] Bit Offset : 00
[377h 0887 1] Encoded Access Width : 04 [QWord Access:64]
[378h 0888 8] Address : 00000000FEBF3008
[380h 0896 8] Value : 0000000000000000
[388h 0904 8] Mask : FFFFFFFFFFFFFFFF
Raw Table Data: Length 912 (0x390)
Note that the contents of tests/data/q35/ERST.acpierst and
tests/data/microvm/ERST.pcie are the same except for differences
due to assigned base address.
Files tests/data/pc/DSDT.acpierst and tests/data/acpi/q35/DSDT.acpierst
are new files (and are included as a result of 'make check' process).
Rather than provide the entire content, I am providing the differences
between pc/DSDT and pc/DSDT.acpierst, and the difference between
q35/DSDT and q35/DSDT.acpierst, with an explanation to follow.
diff pc/DSDT pc/DSDT.acpierst:
@@ -5,13 +5,13 @@
*
* Disassembling to symbolic ASL+ operators
*
- * Disassembly of tests/data/acpi/pc/DSDT, Thu Dec 2 10:10:13 2021
+ * Disassembly of tests/data/acpi/pc/DSDT.acpierst, Thu Dec 2 12:59:36 2021
*
* Original Table Header:
* Signature "DSDT"
- * Length 0x00001772 (6002)
+ * Length 0x00001751 (5969)
* Revision 0x01 **** 32-bit table (V1), no 64-bit math support
- * Checksum 0x9E
+ * Checksum 0x95
* OEM ID "BOCHS "
* OEM Table ID "BXPC "
* OEM Revision 0x00000001 (1)
@@ -964,16 +964,11 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS "
Device (S18)
{
- Name (_SUN, 0x03) // _SUN: Slot User Number
Name (_ADR, 0x00030000) // _ADR: Address
- Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device
- {
- PCEJ (BSEL, _SUN)
- }
-
+ Name (ASUN, 0x03)
Method (_DSM, 4, Serialized) // _DSM: Device-Specific Method
{
- Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, _SUN))
+ Return (PDSM (Arg0, Arg1, Arg2, Arg3, BSEL, ASUN))
}
}
@@ -1399,11 +1394,6 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS "
Method (DVNT, 2, NotSerialized)
{
- If ((Arg0 & 0x08))
- {
- Notify (S18, Arg1)
- }
-
If ((Arg0 & 0x10))
{
Notify (S20, Arg1)
diff q35/DSDT and q35/DSDT.acpierst:
@@ -5,13 +5,13 @@
*
* Disassembling to symbolic ASL+ operators
*
- * Disassembly of tests/data/acpi/q35/DSDT, Thu Dec 2 10:10:13 2021
+ * Disassembly of tests/data/acpi/q35/DSDT.acpierst, Thu Dec 2 12:59:36 2021
*
* Original Table Header:
* Signature "DSDT"
- * Length 0x00002061 (8289)
+ * Length 0x00002072 (8306)
* Revision 0x01 **** 32-bit table (V1), no 64-bit math support
- * Checksum 0xFA
+ * Checksum 0x9A
* OEM ID "BOCHS "
* OEM Table ID "BXPC "
* OEM Revision 0x00000001 (1)
@@ -3278,6 +3278,11 @@ DefinitionBlock ("", "DSDT", 1, "BOCHS "
}
}
+ Device (S10)
+ {
+ Name (_ADR, 0x00020000) // _ADR: Address
+ }
+
Method (PCNT, 0, NotSerialized)
{
}
For both pc and q35, there is but a small difference between this
DSDT.acpierst and the corresponding DSDT. In both cases, the changes
occur under the hiearchy:
Scope (\_SB)
{
Scope (PCI0)
{
which leads me to believe that the change to the DSDT was needed
due to the introduction of the ERST PCI device.
And is explained in detail by Ani Sinha:
I have convinced myself of the changes we see in the DSDT tables.
On i440fx side, we are adding a non-hotpluggable pci device on slot 3.
So the changes we see are basically replacing an empty hotpluggable
slot on the pci root port with a non-hotplugggable device.
On q35, bsel on pcie root bus is not set (its not hotpluggable bus),
so the change basically adds the address enumeration for the device.
Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
Acked-by: Ani Sinha <ani@anisinha.ca>
---
tests/data/acpi/microvm/ERST.pcie | Bin 0 -> 912 bytes
tests/data/acpi/pc/DSDT.acpierst | Bin 0 -> 5969 bytes
tests/data/acpi/pc/ERST.acpierst | Bin 0 -> 912 bytes
tests/data/acpi/q35/DSDT.acpierst | Bin 0 -> 8306 bytes
tests/data/acpi/q35/ERST.acpierst | Bin 0 -> 912 bytes
tests/qtest/bios-tables-test-allowed-diff.h | 5 -----
6 files changed, 5 deletions(-)
diff --git a/tests/data/acpi/microvm/ERST.pcie b/tests/data/acpi/microvm/ERST.pcie
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..a6d0cb783831ebc18972ec57bb6c624438ff150d 100644
GIT binary patch
literal 912
zcmaKoTMmLS5Jd;doACJehb6cK12OSWBYwCn7ocm^-rA|;CUz1YmosPDa=fm$hY?9$
z^LaU~(|o@yldVKV@Q&x+UZ@>zwpS)GZ(sO?Lc}v64j-jFC7yn9;D$INO8pFiUB7f+
zf49KN&wPvW+;jDxe>nP4Iq`z#7tC?s&HniOCHcA!tc6i7Z+t&KoWCN!@(t>{e2`4%
zZhiFB_<u1@^J|*l5O0_xNA};6-;&=E@0cS;_TQ7=lkalGAIKlbcR6ytk^GT-mm}+c
E0L%sr5C8xG
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/pc/DSDT.acpierst b/tests/data/acpi/pc/DSDT.acpierst
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..bb0593eeb8730d51a6f0fe51a00a00df9c83c419 100644
GIT binary patch
literal 5969
zcmb7I-EZ606+c&`Z0bs~DVd3rxNQU{+17PvBspKjfFTko+lnoj5#_W44T!Q_OwKZt
zNNb>p5lnFb*Ui&1&BGpsO$pT3_)pk?J?$-jLiezjJ$6`=r)uYry0Rn7KuJIp&j)|!
zckaizM=~s<dV3mx?dMnYMJtv6$kbDKF#w=v`c{kb8rUr(zffb*kj(7IQSTXVc}4q~
zkzZY4e`-2^bezw=bXeVm_2=sTdfokE&shg@+FExxrw4VmX60-4&Wx!S(@JfzXvA_a
zxun!`Mg^4GYS93-r1(AsWzj5Dg%d+3GTW_1vs!`}z{~HWAlr{slBezz%0|kp7`2pk
zt!7no;F891NmYO7aMkI=p0*umJJDXR3!RCl?K=FS9)o}VFWiMLpHymk#Y!aj#BKyL
zMsSb;9)S9z_lXVBwRg@Htz6CHis2bjF|VZc5O=t&mZvXE!jZh}ohwUu1D*X8C1j7R
zM3^)D)B<zB0mc^C25jK>YzU*-I~PkUtAn>Dr`~u=;o(_O$t$PK8~R}U#&P6`{0u*{
zz`m8fl|Wu#ucTKJu-TjNQ`rN~%rBccG0yWwF_}_<hxrOmGk(BB{1SeWe1`Y<J^me^
z<cd<8HwuYquF(oOdWBH1ce2Z7H6zlK{kdar&{cVSfqgE0E-A`v%j1yMS4x9-!sv|?
zI^vzz(rXEnS=D>zQl+X71|-_g)Pr`x1*Xgb!&xdP4yUPQ>Q<FS;RRQ4b&#Or^sT1w
zW~A(vRk($#Sb4#5?L<k?^XbAZG~~;r3X7`#Yb5CT@@gq%7LAWmHp?&TOQ2mc^}?-?
zeSxH2&ux;2`+#i%Vc0(=br-bva`?_DFT!%^=Jb6X%@{Kct2EJB4!4_$DXzY}{?C6~
zS09$1PXDKL_8)r5C9HeDaXJ^;ccF8(y~#tJsoy{HmU66z7jzR6drUWYHr+YZzRSnm
zWH<zIsJqTy9iUrR%eW`<HFb&8I@SzcGb>ealcAR5Lj8F@vR~&d_hFrfVBOumUtb3<
zL8GI#8|W0leXv|!GGL=~vE5*uM7z%Af!czNXYqlQL#IT$!9xR0zORu68XY#=M-SGy
z3b+$tZv(*Hu4BBt4F>MUo>Padde^ZZU%V_4TiQ&t&6ruaomTLcp<9-2bBZ=qyp40+
ziEQ&$6L)c>%cI)0;%&VWL5E5@F~Or>jX}g)!VlmH>3A6L#ZGj;i8(jvxl3w$XL%gc
z#4Y0Q*ces>sy7Obm6bTnr@oqih!n=P&+!w*&jZjVy;9R95=2i+)QqA9kLa1VJk2ES
zOh6C4;>puBt75SyO`ippr%I9Z{pk6j=(st4aP-WP=ov412KgP0p3z1}&)7R9%3U9d
z&O-shXOD!>df~G};j<@%&-TL`!{M=J{^0EAj)b4{!p{wbpF1J^oV~LmWA@f?c-apx
zmJl5aIOP$4%5j~Id6TV0{V^u0sG`|bSAOB2V2gb1@Ki7>g+T~D<}I$cZmy;lKl#kP
zxAm*k{f|HWWb1z8<4?h6y1Bg6FLiOw7Z@DC0gGYf$3^AUwgVosAOD1e9Hex!P-c3u
zY%#r3y2G$SrRg@$K+^S+fmJX`o|0}AmQ(0%<f<txRx6f^iVLK~37A{+3MdltNyc-!
zSxl!hX`<nu(qS#3SLc5j>hN|GcJ*{Ry{RpZVR_9(><ep`WpVsbqLcvD$6+-FbHVAH
zYY!VJ1(#1^T|8!4Ug|npDNJbv3&e(@5Md;~Hh)^T^s;h78j4;ne3%UNTxhs#<GGP+
zZ!j0D81t&<a_o0)hwV2|<+zREkY2*WN}QC=^!~6Gr={_ACxYd*hhe^UN7=Ci536b3
z-Nqhqq|%A3o?Y);J?7@E44l3Om)v6mgGPLBt3jPxLH1iW0$hg%O*!V)urs5RJBEdP
zw$?`vb19;)Sk>4G!#sMm9K*-Kb2o<v0mshep<yk#-W^t=>qvI?7dL24<1WVww+F6k
zrAw<DcR(puO+>i@3blP`ov2|RQA!4yn|HMt+&l|rp;ky|p}*s4Bhi{tS7b7IwWYtO
z($_%y0DUt+12O0t{g&d0!qm$i(>FX+UGsM;osE9mca!r!@5Ld6Jt<y2x?xxOI`%ro
z6x6erM5<S4ejOXgaXfng{`)0%{=S{gwO)_THM&CJSw!on^m>2$-nabyyC?4*zT@l2
z4LgzF3U^-%E&8T*RhMxH{B|{Nmd$hyhrlm@q(4&m{QfcO=jBztZw|gWU^0Q+lFdc4
z;t%ATAUG5ws_1bncmXfi8SEC{UBmF!TrD{!GvtiVS87Q`#ts;JTa>Png~+QvKOAg(
z=l%H)^?9Gb?Lui47fY7Bv8_MKSPiRTs@3YkU)Q{@YvnKAT;;J8U>fgQ>9qeF0+gJ!
zS_V9pumOy9sSU)%^lS1X17&Pw`o1`MA>W^ePI^H@E*S%`|HOx^c$lLH<Zfkd_+b$W
zyK9QTrIVoSoxE&iDK@#`L5M%k@I4u{WQ|T2>Vk<UnMYxe-p8Pzgb@}P;lLPkf(IU&
zK98~B7=p*Rz!(pVaYgXJpGH0pB{+uQ5fK=Xz!+Br4=ylxa16l%SFWE+G%&_B!6POx
zV!<&4kGQ~y2gbNAcuWY4iQpK52X_7bcuWSyxFL9`0z(asA$TMNMj|lA2ZBdZU?hWM
z2p;$kgg+irfiZG|2i7AWLko@}c$^X#rvhW-1&`AL<8*Kg!DCurOb5o$1&=cV<4kZ2
z!Q-sJI2#zl5IoKajB~*;1dkbkF%uZ0Ab7kbFy0D|A$YtkFy0P~F)w($BQV|xjv;uQ
z7Z~S*U}#I{j63COs1w_6;vXZsboOp~LW+bNec>hwcpCba771jDZ#SdZmhM`3K=q{%
zNdudV5*`|Tn?lmSM!~R<_iYsI+Q3HPu(<YZ6z@9OCbyf|3t->0YhlR~GImZVAKI~h
z+O@D02|KpVC?DEYlCeYxd&-j!?Kerrtt;#;Pd>DJ8RM1`cI=OKE&Yg{vc{!8uv6Z~
u5j$m$OK@OMk$l8{6J=Z)1AB{Pv}@<7F~|QN>AydkHSF$IS^vS{(*FTuhT2R3
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/pc/ERST.acpierst b/tests/data/acpi/pc/ERST.acpierst
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..7965ac2562e7b23bf1dc2caaf00ef66453c4f47a 100644
GIT binary patch
literal 912
zcmaKoOAdlC6h$9Upg{N}4xO+BmS7+z&NSk{Ww`+N;?C6GO0l^KeF>#Er>E`f@jBlg
ziAb~?&(mq{$NOdKO+_MtIsSwBP<sq!t@LIbUT;KKA5)k|#NneHkBP@Wyz+!NBFgy+
zf;nGroUh1N*8cnH!kz!z;I6-vct-nY%+auSKkh##KPTU2&tLG`zb0RkZ}UNv>(BS`
zh6Ua)#A*M6_AiN-%#j24ugI^+uZh>pkpuT{$ZyEEIpDYCx8&O#=&vKcBj4u0`Con^
B!2tjO
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/q35/DSDT.acpierst b/tests/data/acpi/q35/DSDT.acpierst
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..cad26e3f0c27a40a33101155a5282ed9bcb1d441 100644
GIT binary patch
literal 8306
zcmb7JOKcm*8J^`!tL0K!Qk3OaY{E(UN|8`@UTx8!<St*9NUgY3oPY+nvYb|SQe=_D
zKpqGnD?sA-P&8o^6g|;_0`%6NdaZ$8+M90;&{Ge+<XRN*DeU*p?8q}D1;jk8_MiQ}
z|DSJmXTQ~q7kIrlN{ktQsBXB;pj3UycT4ygj4?W=zfLQ4nf3R)YGYtz6DgyAGt19$
zsG7wed)2Lm@%!EI^DuntqtMun*p)Aw&#r7oAKVJBFaq7U5?L0VIJJdAP#yI5UCVb{
zWpmJMd3NQVWtsz5oF&ac#p^P2u+{QdaeZ#E5xA+_uQJ!Wv^wgfhr9VD%jviOb>W+{
zr+@MJrMD_~fBw&3-hS0e0dO9FP5iwO(II><w9bSh=hNXe<9(v@YpbKBPlx=nXmjXF
z6jI9_TQpIs)q_|2`_Fsbip%?$^E-aoEw7?qZa2uEwN8Ju!kA5iL%%<d$K$_>Q|7$G
zoVF=c*370~>q(Ucw2`X*UeoWjna$X}Z?i1bzv)oC^kS{!mHe(ZC>5U_1icE2s3nZ3
zY5U#Ka>9}Q$uMNYFgqR}u+hRN!)^AvZJYi3f9V5uU@eNi<*kJe=czCEzGW%K(3|f)
zNxh-*?(d&$1(g9GVw69NuK2C)X2J?B+gV$DDk*`4>hC|jR`mcD_b7?pqgS%bacj9@
zge+u+(J-#EtN42^folEz$J!fPhpfdVjb&Qtb2LKz{qyU7Z&l__iTiD6SjWmWjJK0-
zCr$I*9?yK<-Dn?j(m-Q0XK?N(?<bjg&f%QLyFK@?H`)09@zfEEm|>n{28O7Ue=tUS
zz8mz6>|NV3acd}WF?L%e9K2G0FQ{F_Ecm-^;l^btaI59oMO>(M+Fc`iPtYiEtDUWO
z$!~ctmHI|?-CbkF$9=bUG0}gNqVYJ|B}DX&ag9-q{`aZ*fEAypfHMn4xgt7*F*YoE
z8Q%-q3#0XTYTSG7AO8uSdAI$^%Gd1^e{<Ur<$CSc;poxfbv8ONyk;dZ1K#J)k&VZ8
z%ntHN<0c*0ot4q@@VYe@719ZoW7|=9Yn!oyZPRbLbyl<o_-jPF6gj_n*`SBTalhsh
z+3)uJ!rGgAg=`8>CtL5V)tERpa9(Exo7$}ef`$x|2%4;rOQ9c_Ndgx~!~$?ZT;n`I
zWk^iq=hy^i0vkufl+Xm@Oh7pkAu-V#j!kl=>_L(dQ$mx12`FbGB&Mu_c_pwsPl_`o
zG$oj_^9WU)X-#LEGbJ=Fn6mQ-Rh=14XNEH+G^6W8sOp^4bk1oy=X9M2Rh_1$)6{gD
zx=w_uPP~DXk)GFd&g(i6syZ!Ar={t%be#xQomow1R@0f)bs|)C;$5iRPg~Pz>pBst
zI&+%NoTf9U>qMyPT+nnbXgU{kod{K(i<-_wP3NMn6QQcp(R4bRPDj^?P}P~&bmldk
zd0i($RcArdS<rMAbe#xQolBa|B~9m&t`niEv#9ATYC4O$PK2t?WliU@rgK@>iBQ#f
zOw)Nx(|JtSiBQ#vF9|$-@;$Vo>0HruB2;xA*K{7&bRO4rB2;yr;LJ+=COpBJ)%az1
zLNMJ2Au+ueds1Vb)R-rACPI~YN@JeVn5T3mLX~-1W1iNSr*$SmmHCLqd_-eDqB9Yy
z%rl%RUFHmD%6I%3!IUp<gv6A0@T^wztXA`^UK63HX~IA&ih(wQ7D{t7HE@o?v4J8$
zSrH@=$f7V%f#a5BFk;F687L~ChX#sJN`rw4tTa#sN(L&RhX#rey^#hguyI5+RRa~!
zLjy&K9&s!TRAA#M*2tk`paLpGRVNHoq~%N)r~)MeMJQd73>2Z%2?G^a&V+#~P%=;f
z<u%Db5lWpfP=V!47^ngz0~JutBm+e#b;3XemNQ|X3X}{~Ksl2P6rsGIFi?TzOc<yF
zB?A>u&LjgxD0RX>1(q{mpbC@>R6seC3>2Z%2?G^a&V+#~P%=;f<xDbAgi<FARA4z1
z2C6{GKn0XD$v_cGoiI>=<xCi;0wn_#P|hR+MJRQ`Kn0dFVW0|>3{*fllMEE0)CmI>
zSk8ojDo`>|0p(0GP=rz^3{+q_69%e4$v_2^Gs!>^N}VuJf#pmXr~)Me6;RG314Srx
z!axO<Ghv_#lnhirIg<<&q0|Wj6<E%MfhtfkPyyvkGEjt4Ck#|zITHq|K*>M_lrzad
z5lWpfP=V!47^ngz0~JutBm+e#b;3XemNQ|X3X}{~Ksl2P6rt1!0~J`#gn=qhGEf2K
zOfpb}QYQ>lU^x>8szAv=1(Y+%KoLrvFi=E_fg(~36rpOM2o(cWm@rU<Nd~Gg$v_n*
z3{+vlKouq#sKO)zRhTePg$V;ym}H;|lMGa0!ax-!3{+u~fhtTgP=yHtMI`So3=|RW
z7#~bBP=vS*G9;!PTNo%JIkqrRL~?A&KoQEZB?Co>rJ|DqSSTCWL;la|0sTXICqsWK
z{hL31DoOvP(y9incNl(mvjWH6tkS`ygJ&|Vy=>O#yiSJ(9hzowskMpKfYJPW*4{1_
zS>_L{Z1L&VrrVtdpj*x_SlN#=Y@V+Ky~A!(o0Z0~6}`7>EiyVoC%RaAS)ao<LPail
z*jTQi#UK;i4)VvWrH>cbd=l=)@cw3a6JzzFxX9Om4yX(0O{JJ0c&&iFPv_PiZ&{@E
zC<cVv<?+fO=my5^-M9}_p4#K;)rsm@Jo(Pmt4j4MuU<v})aupZ?CRD2ew(z5r8F$}
z#oDdJLzH)w@-8p$PLy|#DDTGQy$SIa7PP*zdQU0u@$%k8dGCnwUR=I5Sw8g;<!eg$
z8ZTd)C|^6Gd@U|tpDdq#i1Kx%e4UrCPn548QNA9RZ%mfYJVg11Qog~<HzvwAjws)V
z%Qq*>&pkx>rc%Di%Qq*=H;*XaMEMlG$|lP@qP%>0;IjhLU98{gZN&_q^7upZI!-q^
zjjg<%4&NG6=|tPibW`)Ppr^wJ$W%JfHZ$GSJi`(&%`+(sUnNuNMBB`CQ}d>%r^Bbp
zR65Z%Gu_lYSk}|w`(-MfXq%aC>bX9qr^83hR65Z%Gu_m4x}vAUm(El=(Wa+!PCZ!9
z-d_ybRcy6jTg6Ud&EMI0LF`U6t;-Kq{QYh%kWZ<F$q8@Dx&BtE`TASYYd3yXdgGNh
zU%&Cj&MR-SzTx`oYw^>Xv#f^kmHm}%*jUwvC(+XRSGHyMZ~lOS3M;vOtz)>YPGGp*
z_KOA<z{_-~8!YL%Z4Zm=1wtm>ptfED+^h6TSZi)|T4uzk5l{Vv%UlmSBs!U^xPGf#
zUM*8M#Kuv&n0I^imlGpvxXV6t%jNR5Vl#u)`^tR((aTSz<kkiWB>}YjKG}?75er8r
zhm#jbLS!vs*?sED`r1KQY^O=q!xb_nQz665{Kfh)H*h=V5i(4-Q@d11j3c%a8CSpX
z3d5^L#I8n07_sdy*6rU8L*ug@G=<U5WJxY~vJp3BwDQ?xD{jlqm!tXNwlzkXFQZx4
zM~_X79NqV5=N9#Go88z(ukJiIdS(j7pA6i-9V;144Gb=VcVmY(7HcW7vq%ZAupPSP
z(6-ZGoh@!E7y5JK%Islo&$}Bz&pu+9XFn}d`7wCWwHad*S&7M3f?l6yk7UH^mU
z>6ym-#}Cs3&$W3#Pd8>jyVA>NEt6rjfxbhF3)nR6Wl0R%y>`8L3X`Yd*9Nunc^1!j
znn?6aIU6au#3z0{r^?T=;!l8GWi$|nJAz`zN@?)j4)3XkaN5gart(c8zE^rNYyIU`
zjwP@}fZv!7*fG3s-?t4D?FQaS_<e)R_wB<thdm_5{>wL6`E~j%9*^m>n0^VSZNHk>
z#`=@E!v3gTxr=?-Z^f0nFWi0tTefjF(MF?1dtewi?ME?fQo1P&j@YH<JHQnTj(9av
zyYfyH8<$z(;QXCf%9Jt&-|4)V@|%8F3^8pZKS+W>n(=;}&UM$mYa7S0P5HyiC?8xH
z6f!9TeaHFJgMyi|5*8Nm5IsXzNQv{a2e+ZW|787=D||1vS!-|MF*;9ZZ04#xcG6Co
z-r~+i`ZC|zYozHe_V=?vxh(dKF(f|<%Fl}ccg$cg<6zZe*tEpSd3kbyPWmvO)tCru
Lf)z9L%&`9h*%QBN
literal 0
HcmV?d00001
diff --git a/tests/data/acpi/q35/ERST.acpierst b/tests/data/acpi/q35/ERST.acpierst
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..7965ac2562e7b23bf1dc2caaf00ef66453c4f47a 100644
GIT binary patch
literal 912
zcmaKoOAdlC6h$9Upg{N}4xO+BmS7+z&NSk{Ww`+N;?C6GO0l^KeF>#Er>E`f@jBlg
ziAb~?&(mq{$NOdKO+_MtIsSwBP<sq!t@LIbUT;KKA5)k|#NneHkBP@Wyz+!NBFgy+
zf;nGroUh1N*8cnH!kz!z;I6-vct-nY%+auSKkh##KPTU2&tLG`zb0RkZ}UNv>(BS`
zh6Ua)#A*M6_AiN-%#j24ugI^+uZh>pkpuT{$ZyEEIpDYCx8&O#=&vKcBj4u0`Con^
B!2tjO
literal 0
HcmV?d00001
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 603db07..dfb8523 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,6 +1 @@
/* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/pc/DSDT.acpierst",
-"tests/data/acpi/pc/ERST.acpierst",
-"tests/data/acpi/q35/DSDT.acpierst",
-"tests/data/acpi/q35/ERST.acpierst",
-"tests/data/acpi/microvm/ERST.pcie",
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-26 16:28 ` [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table Eric DeVolder
@ 2022-01-27 8:36 ` Ani Sinha
2022-01-27 22:02 ` Eric DeVolder
0 siblings, 1 reply; 21+ messages in thread
From: Ani Sinha @ 2022-01-27 8:36 UTC (permalink / raw)
To: Eric DeVolder
Cc: berrange, ehabkost, mst, konrad.wilk, qemu-devel, pbonzini,
imammedo, boris.ostrovsky, rth
On Wed, 26 Jan 2022, Eric DeVolder wrote:
> This builds the ACPI ERST table to inform OSPM how to communicate
> with the acpi-erst device.
There might be more optimizations possible but I think we have messaged
this code enough. We can further rework the code if needed in subsequent
patches once this is pushed.
>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
with some minor comments,
Reviewed-by: Ani Sinha <ani@anisinha.ca>
> hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 225 insertions(+)
>
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index fe9ba51..5d5a639 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -59,6 +59,27 @@
> #define STATUS_RECORD_STORE_EMPTY 0x04
> #define STATUS_RECORD_NOT_FOUND 0x05
>
> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> +#define INST_READ_REGISTER 0x00
> +#define INST_READ_REGISTER_VALUE 0x01
> +#define INST_WRITE_REGISTER 0x02
> +#define INST_WRITE_REGISTER_VALUE 0x03
> +#define INST_NOOP 0x04
> +#define INST_LOAD_VAR1 0x05
> +#define INST_LOAD_VAR2 0x06
> +#define INST_STORE_VAR1 0x07
> +#define INST_ADD 0x08
> +#define INST_SUBTRACT 0x09
> +#define INST_ADD_VALUE 0x0A
> +#define INST_SUBTRACT_VALUE 0x0B
> +#define INST_STALL 0x0C
> +#define INST_STALL_WHILE_TRUE 0x0D
> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> +#define INST_GOTO 0x0F
> +#define INST_SET_SRC_ADDRESS_BASE 0x10
> +#define INST_SET_DST_ADDRESS_BASE 0x11
> +#define INST_MOVE_DATA 0x12
> +
> /* UEFI 2.1: Appendix N Common Platform Error Record */
> #define UEFI_CPER_RECORD_MIN_SIZE 128U
> #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> @@ -172,6 +193,210 @@ typedef struct {
>
> /*******************************************************************/
> /*******************************************************************/
> +typedef struct {
> + GArray *table_data;
> + pcibus_t bar;
> + uint8_t instruction;
> + uint8_t flags;
> + uint8_t register_bit_width;
> + pcibus_t register_offset;
> +} BuildSerializationInstructionEntry;
> +
> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> +static void build_serialization_instruction(
> + BuildSerializationInstructionEntry *e,
> + uint8_t serialization_action,
> + uint64_t value)
> +{
> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> + struct AcpiGenericAddress gas;
> + uint64_t mask;
> +
> + /* Serialization Action */
> + build_append_int_noprefix(e->table_data, serialization_action, 1);
> + /* Instruction */
> + build_append_int_noprefix(e->table_data, e->instruction, 1);
> + /* Flags */
> + build_append_int_noprefix(e->table_data, e->flags, 1);
> + /* Reserved */
> + build_append_int_noprefix(e->table_data, 0, 1);
> + /* Register Region */
> + gas.space_id = AML_SYSTEM_MEMORY;
> + gas.bit_width = e->register_bit_width;
> + gas.bit_offset = 0;
> + gas.access_width = ctz32(e->register_bit_width) - 2;
Should this be casted as unit8_t?
> + gas.address = (uint64_t)(e->bar + e->register_offset);
> + build_append_gas_from_struct(e->table_data, &gas);
> + /* Value */
> + build_append_int_noprefix(e->table_data, value, 8);
> + /* Mask */
> + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> + build_append_int_noprefix(e->table_data, mask, 8);
> +}
> +
> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> + const char *oem_id, const char *oem_table_id)
> +{
> + /*
> + * Serialization Action Table
> + * The serialization action table must be generated first
> + * so that its size can be known in order to populate the
> + * Instruction Entry Count field.
> + */
> + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> + .oem_table_id = oem_table_id };
> + /* Contexts for the different ways ACTION and VALUE are accessed */
> + BuildSerializationInstructionEntry rd_value_32_val = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_READ_REGISTER_VALUE,
> + .flags = 0,
> + .register_bit_width = 32,
> + .register_offset = ERST_VALUE_OFFSET,
> + };
> + BuildSerializationInstructionEntry rd_value_32 = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_READ_REGISTER,
> + .flags = 0,
> + .register_bit_width = 32,
> + .register_offset = ERST_VALUE_OFFSET,
> + };
> + BuildSerializationInstructionEntry rd_value_64 = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_READ_REGISTER,
> + .flags = 0,
> + .register_bit_width = 64,
> + .register_offset = ERST_VALUE_OFFSET,
> + };
> + BuildSerializationInstructionEntry wr_value_32_val = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_WRITE_REGISTER_VALUE,
> + .flags = 0,
> + .register_bit_width = 32,
> + .register_offset = ERST_VALUE_OFFSET,
> + };
> + BuildSerializationInstructionEntry wr_value_32 = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_WRITE_REGISTER,
> + .flags = 0,
> + .register_bit_width = 32,
> + .register_offset = ERST_VALUE_OFFSET,
> + };
> + BuildSerializationInstructionEntry wr_value_64 = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_WRITE_REGISTER,
> + .flags = 0,
> + .register_bit_width = 64,
> + .register_offset = ERST_VALUE_OFFSET,
> + };
> + BuildSerializationInstructionEntry wr_action = {
> + .table_data = table_instruction_data,
> + .bar = bar0,
> + .instruction = INST_WRITE_REGISTER_VALUE,
> + .flags = 0,
> + .register_bit_width = 32,
> + .register_offset = ERST_ACTION_OFFSET,
> + };
We can probably write a macro to simply generating these structs. I see
.bar and .flags are the same in all structs. The .bit_width can be a param
into the macro etc.
> + unsigned action;
> +
> + trace_acpi_erst_pci_bar_0(bar0);
> +
> + /* Serialization Instruction Entries */
> + action = ACTION_BEGIN_WRITE_OPERATION;
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_BEGIN_READ_OPERATION;
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_BEGIN_CLEAR_OPERATION;
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_END_OPERATION;
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_SET_RECORD_OFFSET;
> + build_serialization_instruction(&wr_value_32, action, 0);
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_EXECUTE_OPERATION;
> + build_serialization_instruction(&wr_value_32_val, action,
> + ERST_EXECUTE_OPERATION_MAGIC);
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_CHECK_BUSY_STATUS;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_32_val, action, 0x01);
> +
> + action = ACTION_GET_COMMAND_STATUS;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_32, action, 0);
> +
> + action = ACTION_GET_RECORD_IDENTIFIER;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_64, action, 0);
> +
> + action = ACTION_SET_RECORD_IDENTIFIER;
> + build_serialization_instruction(&wr_value_64, action, 0);
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_GET_RECORD_COUNT;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_32, action, 0);
> +
> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> + build_serialization_instruction(&wr_action, action, action);
> +
> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_64, action, 0);
> +
> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_64, action, 0);
> +
> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_32, action, 0);
> +
> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> + build_serialization_instruction(&wr_action, action, action);
> + build_serialization_instruction(&rd_value_64, action, 0);
> +
> + /* Serialization Header */
> + acpi_table_begin(&table, table_data);
> +
> + /* Serialization Header Size */
> + build_append_int_noprefix(table_data, 48, 4);
> +
> + /* Reserved */
> + build_append_int_noprefix(table_data, 0, 4);
> +
> + /*
> + * Instruction Entry Count
> + * Each instruction entry is 32 bytes
> + */
> + g_assert((table_instruction_data->len) % 32 == 0);
> + build_append_int_noprefix(table_data,
> + (table_instruction_data->len / 32), 4);
> +
> + /* Serialization Instruction Entries */
> + g_array_append_vals(table_data, table_instruction_data->data,
> + table_instruction_data->len);
> + g_array_free(table_instruction_data, TRUE);
> +
> + acpi_table_end(linker, &table);
> +}
> +
> +/*******************************************************************/
> +/*******************************************************************/
> static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> {
> uint8_t *rc = NULL;
> --
> 1.8.3.1
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-27 8:36 ` Ani Sinha
@ 2022-01-27 22:02 ` Eric DeVolder
2022-01-28 0:37 ` Michael S. Tsirkin
0 siblings, 1 reply; 21+ messages in thread
From: Eric DeVolder @ 2022-01-27 22:02 UTC (permalink / raw)
To: Ani Sinha
Cc: berrange, ehabkost, mst, konrad.wilk, qemu-devel, pbonzini,
imammedo, boris.ostrovsky, rth
Ani,
Thanks for the RB! Inline responses below.
eric
On 1/27/22 02:36, Ani Sinha wrote:
>
>
> On Wed, 26 Jan 2022, Eric DeVolder wrote:
>
>> This builds the ACPI ERST table to inform OSPM how to communicate
>> with the acpi-erst device.
>
> There might be more optimizations possible but I think we have messaged
> this code enough. We can further rework the code if needed in subsequent
> patches once this is pushed.
>
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>
> with some minor comments,
>
> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>
>> hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 225 insertions(+)
>>
>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
>> index fe9ba51..5d5a639 100644
>> --- a/hw/acpi/erst.c
>> +++ b/hw/acpi/erst.c
>> @@ -59,6 +59,27 @@
>> #define STATUS_RECORD_STORE_EMPTY 0x04
>> #define STATUS_RECORD_NOT_FOUND 0x05
>>
>> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
>> +#define INST_READ_REGISTER 0x00
>> +#define INST_READ_REGISTER_VALUE 0x01
>> +#define INST_WRITE_REGISTER 0x02
>> +#define INST_WRITE_REGISTER_VALUE 0x03
>> +#define INST_NOOP 0x04
>> +#define INST_LOAD_VAR1 0x05
>> +#define INST_LOAD_VAR2 0x06
>> +#define INST_STORE_VAR1 0x07
>> +#define INST_ADD 0x08
>> +#define INST_SUBTRACT 0x09
>> +#define INST_ADD_VALUE 0x0A
>> +#define INST_SUBTRACT_VALUE 0x0B
>> +#define INST_STALL 0x0C
>> +#define INST_STALL_WHILE_TRUE 0x0D
>> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
>> +#define INST_GOTO 0x0F
>> +#define INST_SET_SRC_ADDRESS_BASE 0x10
>> +#define INST_SET_DST_ADDRESS_BASE 0x11
>> +#define INST_MOVE_DATA 0x12
>> +
>> /* UEFI 2.1: Appendix N Common Platform Error Record */
>> #define UEFI_CPER_RECORD_MIN_SIZE 128U
>> #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
>> @@ -172,6 +193,210 @@ typedef struct {
>>
>> /*******************************************************************/
>> /*******************************************************************/
>> +typedef struct {
>> + GArray *table_data;
>> + pcibus_t bar;
>> + uint8_t instruction;
>> + uint8_t flags;
>> + uint8_t register_bit_width;
>> + pcibus_t register_offset;
>> +} BuildSerializationInstructionEntry;
>> +
>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
>> +static void build_serialization_instruction(
>> + BuildSerializationInstructionEntry *e,
>> + uint8_t serialization_action,
>> + uint64_t value)
>> +{
>> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
>> + struct AcpiGenericAddress gas;
>> + uint64_t mask;
>> +
>> + /* Serialization Action */
>> + build_append_int_noprefix(e->table_data, serialization_action, 1);
>> + /* Instruction */
>> + build_append_int_noprefix(e->table_data, e->instruction, 1);
>> + /* Flags */
>> + build_append_int_noprefix(e->table_data, e->flags, 1);
>> + /* Reserved */
>> + build_append_int_noprefix(e->table_data, 0, 1);
>> + /* Register Region */
>> + gas.space_id = AML_SYSTEM_MEMORY;
>> + gas.bit_width = e->register_bit_width;
>> + gas.bit_offset = 0;
>> + gas.access_width = ctz32(e->register_bit_width) - 2;
>
> Should this be casted as unit8_t?
OK, done.
>
>> + gas.address = (uint64_t)(e->bar + e->register_offset);
>> + build_append_gas_from_struct(e->table_data, &gas);
>> + /* Value */
>> + build_append_int_noprefix(e->table_data, value, 8);
>> + /* Mask */
>> + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
>> + build_append_int_noprefix(e->table_data, mask, 8);
>> +}
>> +
>> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
>> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
>> + const char *oem_id, const char *oem_table_id)
>> +{
>> + /*
>> + * Serialization Action Table
>> + * The serialization action table must be generated first
>> + * so that its size can be known in order to populate the
>> + * Instruction Entry Count field.
>> + */
>> + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
>> + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
>> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
>> + .oem_table_id = oem_table_id };
>> + /* Contexts for the different ways ACTION and VALUE are accessed */
>> + BuildSerializationInstructionEntry rd_value_32_val = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_READ_REGISTER_VALUE,
>> + .flags = 0,
>> + .register_bit_width = 32,
>> + .register_offset = ERST_VALUE_OFFSET,
>> + };
>> + BuildSerializationInstructionEntry rd_value_32 = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_READ_REGISTER,
>> + .flags = 0,
>> + .register_bit_width = 32,
>> + .register_offset = ERST_VALUE_OFFSET,
>> + };
>> + BuildSerializationInstructionEntry rd_value_64 = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_READ_REGISTER,
>> + .flags = 0,
>> + .register_bit_width = 64,
>> + .register_offset = ERST_VALUE_OFFSET,
>> + };
>> + BuildSerializationInstructionEntry wr_value_32_val = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_WRITE_REGISTER_VALUE,
>> + .flags = 0,
>> + .register_bit_width = 32,
>> + .register_offset = ERST_VALUE_OFFSET,
>> + };
>> + BuildSerializationInstructionEntry wr_value_32 = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_WRITE_REGISTER,
>> + .flags = 0,
>> + .register_bit_width = 32,
>> + .register_offset = ERST_VALUE_OFFSET,
>> + };
>> + BuildSerializationInstructionEntry wr_value_64 = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_WRITE_REGISTER,
>> + .flags = 0,
>> + .register_bit_width = 64,
>> + .register_offset = ERST_VALUE_OFFSET,
>> + };
>> + BuildSerializationInstructionEntry wr_action = {
>> + .table_data = table_instruction_data,
>> + .bar = bar0,
>> + .instruction = INST_WRITE_REGISTER_VALUE,
>> + .flags = 0,
>> + .register_bit_width = 32,
>> + .register_offset = ERST_ACTION_OFFSET,
>> + };
>
> We can probably write a macro to simply generating these structs. I see
> .bar and .flags are the same in all structs. The .bit_width can be a param
> into the macro etc.
Agree, however the request was to NOT hide the use of local variables in
macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
would be parameters, that leaves .table_data and .bar which just need the local
variables. Is the following acceptable (which hides the use of the local variables)?
#define SERIALIZATIONINSTRUCTIONCTX(name, \
inst, bit_width, offset) \
BuildSerializationInstructionEntry name = { \
.table_data = table_instruction_data, \
.bar = bar0, \
.instruction = inst, \
.flags = 0, \
.register_bit_width = bit_width, \
.register_offset = offset, \
}
SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
SERIALIZATIONINSTRUCTIONCTX(wr_action,
INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
These are the 7 accessors needed.
>
>> + unsigned action;
>> +
>> + trace_acpi_erst_pci_bar_0(bar0);
>> +
>> + /* Serialization Instruction Entries */
>> + action = ACTION_BEGIN_WRITE_OPERATION;
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_BEGIN_READ_OPERATION;
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_BEGIN_CLEAR_OPERATION;
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_END_OPERATION;
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_SET_RECORD_OFFSET;
>> + build_serialization_instruction(&wr_value_32, action, 0);
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_EXECUTE_OPERATION;
>> + build_serialization_instruction(&wr_value_32_val, action,
>> + ERST_EXECUTE_OPERATION_MAGIC);
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_CHECK_BUSY_STATUS;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_32_val, action, 0x01);
>> +
>> + action = ACTION_GET_COMMAND_STATUS;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_32, action, 0);
>> +
>> + action = ACTION_GET_RECORD_IDENTIFIER;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> + action = ACTION_SET_RECORD_IDENTIFIER;
>> + build_serialization_instruction(&wr_value_64, action, 0);
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_GET_RECORD_COUNT;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_32, action, 0);
>> +
>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>> + build_serialization_instruction(&wr_action, action, action);
>> +
>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_32, action, 0);
>> +
>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>> + build_serialization_instruction(&wr_action, action, action);
>> + build_serialization_instruction(&rd_value_64, action, 0);
>> +
>> + /* Serialization Header */
>> + acpi_table_begin(&table, table_data);
>> +
>> + /* Serialization Header Size */
>> + build_append_int_noprefix(table_data, 48, 4);
>> +
>> + /* Reserved */
>> + build_append_int_noprefix(table_data, 0, 4);
>> +
>> + /*
>> + * Instruction Entry Count
>> + * Each instruction entry is 32 bytes
>> + */
>> + g_assert((table_instruction_data->len) % 32 == 0);
>> + build_append_int_noprefix(table_data,
>> + (table_instruction_data->len / 32), 4);
>> +
>> + /* Serialization Instruction Entries */
>> + g_array_append_vals(table_data, table_instruction_data->data,
>> + table_instruction_data->len);
>> + g_array_free(table_instruction_data, TRUE);
>> +
>> + acpi_table_end(linker, &table);
>> +}
>> +
>> +/*******************************************************************/
>> +/*******************************************************************/
>> static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>> {
>> uint8_t *rc = NULL;
>> --
>> 1.8.3.1
>>
>>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-27 22:02 ` Eric DeVolder
@ 2022-01-28 0:37 ` Michael S. Tsirkin
2022-01-28 9:01 ` Ani Sinha
2022-01-28 15:11 ` Eric DeVolder
0 siblings, 2 replies; 21+ messages in thread
From: Michael S. Tsirkin @ 2022-01-28 0:37 UTC (permalink / raw)
To: Eric DeVolder
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, Ani Sinha,
imammedo, boris.ostrovsky, rth
On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> Ani,
> Thanks for the RB! Inline responses below.
> eric
>
> On 1/27/22 02:36, Ani Sinha wrote:
> >
> >
> > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> >
> > > This builds the ACPI ERST table to inform OSPM how to communicate
> > > with the acpi-erst device.
> >
> > There might be more optimizations possible but I think we have messaged
> > this code enough. We can further rework the code if needed in subsequent
> > patches once this is pushed.
> >
> > >
> > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> >
> > with some minor comments,
> >
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> >
> > > hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 225 insertions(+)
> > >
> > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > index fe9ba51..5d5a639 100644
> > > --- a/hw/acpi/erst.c
> > > +++ b/hw/acpi/erst.c
> > > @@ -59,6 +59,27 @@
> > > #define STATUS_RECORD_STORE_EMPTY 0x04
> > > #define STATUS_RECORD_NOT_FOUND 0x05
> > >
> > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > +#define INST_READ_REGISTER 0x00
> > > +#define INST_READ_REGISTER_VALUE 0x01
> > > +#define INST_WRITE_REGISTER 0x02
> > > +#define INST_WRITE_REGISTER_VALUE 0x03
> > > +#define INST_NOOP 0x04
> > > +#define INST_LOAD_VAR1 0x05
> > > +#define INST_LOAD_VAR2 0x06
> > > +#define INST_STORE_VAR1 0x07
> > > +#define INST_ADD 0x08
> > > +#define INST_SUBTRACT 0x09
> > > +#define INST_ADD_VALUE 0x0A
> > > +#define INST_SUBTRACT_VALUE 0x0B
> > > +#define INST_STALL 0x0C
> > > +#define INST_STALL_WHILE_TRUE 0x0D
> > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > +#define INST_GOTO 0x0F
> > > +#define INST_SET_SRC_ADDRESS_BASE 0x10
> > > +#define INST_SET_DST_ADDRESS_BASE 0x11
> > > +#define INST_MOVE_DATA 0x12
> > > +
> > > /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > @@ -172,6 +193,210 @@ typedef struct {
> > >
> > > /*******************************************************************/
> > > /*******************************************************************/
> > > +typedef struct {
> > > + GArray *table_data;
> > > + pcibus_t bar;
> > > + uint8_t instruction;
> > > + uint8_t flags;
> > > + uint8_t register_bit_width;
> > > + pcibus_t register_offset;
> > > +} BuildSerializationInstructionEntry;
> > > +
> > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > +static void build_serialization_instruction(
> > > + BuildSerializationInstructionEntry *e,
> > > + uint8_t serialization_action,
> > > + uint64_t value)
> > > +{
> > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > > + struct AcpiGenericAddress gas;
> > > + uint64_t mask;
> > > +
> > > + /* Serialization Action */
> > > + build_append_int_noprefix(e->table_data, serialization_action, 1);
> > > + /* Instruction */
> > > + build_append_int_noprefix(e->table_data, e->instruction, 1);
> > > + /* Flags */
> > > + build_append_int_noprefix(e->table_data, e->flags, 1);
> > > + /* Reserved */
> > > + build_append_int_noprefix(e->table_data, 0, 1);
> > > + /* Register Region */
> > > + gas.space_id = AML_SYSTEM_MEMORY;
> > > + gas.bit_width = e->register_bit_width;
> > > + gas.bit_offset = 0;
> > > + gas.access_width = ctz32(e->register_bit_width) - 2;
> >
> > Should this be casted as unit8_t?
> OK, done.
>
> >
> > > + gas.address = (uint64_t)(e->bar + e->register_offset);
> > > + build_append_gas_from_struct(e->table_data, &gas);
> > > + /* Value */
> > > + build_append_int_noprefix(e->table_data, value, 8);
> > > + /* Mask */
> > > + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > + build_append_int_noprefix(e->table_data, mask, 8);
> > > +}
> > > +
> > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > > + const char *oem_id, const char *oem_table_id)
> > > +{
> > > + /*
> > > + * Serialization Action Table
> > > + * The serialization action table must be generated first
> > > + * so that its size can be known in order to populate the
> > > + * Instruction Entry Count field.
> > > + */
> > > + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > > + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > > + .oem_table_id = oem_table_id };
> > > + /* Contexts for the different ways ACTION and VALUE are accessed */
> > > + BuildSerializationInstructionEntry rd_value_32_val = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_READ_REGISTER_VALUE,
> > > + .flags = 0,
> > > + .register_bit_width = 32,
> > > + .register_offset = ERST_VALUE_OFFSET,
> > > + };
> > > + BuildSerializationInstructionEntry rd_value_32 = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_READ_REGISTER,
> > > + .flags = 0,
> > > + .register_bit_width = 32,
> > > + .register_offset = ERST_VALUE_OFFSET,
> > > + };
> > > + BuildSerializationInstructionEntry rd_value_64 = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_READ_REGISTER,
> > > + .flags = 0,
> > > + .register_bit_width = 64,
> > > + .register_offset = ERST_VALUE_OFFSET,
> > > + };
> > > + BuildSerializationInstructionEntry wr_value_32_val = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > + .flags = 0,
> > > + .register_bit_width = 32,
> > > + .register_offset = ERST_VALUE_OFFSET,
> > > + };
> > > + BuildSerializationInstructionEntry wr_value_32 = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_WRITE_REGISTER,
> > > + .flags = 0,
> > > + .register_bit_width = 32,
> > > + .register_offset = ERST_VALUE_OFFSET,
> > > + };
> > > + BuildSerializationInstructionEntry wr_value_64 = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_WRITE_REGISTER,
> > > + .flags = 0,
> > > + .register_bit_width = 64,
> > > + .register_offset = ERST_VALUE_OFFSET,
> > > + };
> > > + BuildSerializationInstructionEntry wr_action = {
> > > + .table_data = table_instruction_data,
> > > + .bar = bar0,
> > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > + .flags = 0,
> > > + .register_bit_width = 32,
> > > + .register_offset = ERST_ACTION_OFFSET,
> > > + };
> >
> > We can probably write a macro to simply generating these structs. I see
> > .bar and .flags are the same in all structs. The .bit_width can be a param
> > into the macro etc.
> Agree, however the request was to NOT hide the use of local variables in
> macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
> would be parameters, that leaves .table_data and .bar which just need the local
> variables. Is the following acceptable (which hides the use of the local variables)?
You can just use assignment:
BuildSerializationInstructionEntry entry = {
.table_data = table_instruction_data,
.bar = bar0,
.flags = 0,
.register_bit_width = 32,
};
BuildSerializationInstructionEntry rd_value_32_val = entry;
rd_value_32_val.action = INST_READ_REGISTER_VALUE;
rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
and so on.
not sure it's worth it, it's just a couple of lines.
> #define SERIALIZATIONINSTRUCTIONCTX(name, \
> inst, bit_width, offset) \
> BuildSerializationInstructionEntry name = { \
> .table_data = table_instruction_data, \
> .bar = bar0, \
> .instruction = inst, \
> .flags = 0, \
> .register_bit_width = bit_width, \
> .register_offset = offset, \
> }
> SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> SERIALIZATIONINSTRUCTIONCTX(wr_action,
> INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>
> These are the 7 accessors needed.
not at all sure this one is worth the macro mess.
> >
> > > + unsigned action;
> > > +
> > > + trace_acpi_erst_pci_bar_0(bar0);
> > > +
> > > + /* Serialization Instruction Entries */
> > > + action = ACTION_BEGIN_WRITE_OPERATION;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_BEGIN_READ_OPERATION;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_BEGIN_CLEAR_OPERATION;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_END_OPERATION;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_SET_RECORD_OFFSET;
> > > + build_serialization_instruction(&wr_value_32, action, 0);
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_EXECUTE_OPERATION;
> > > + build_serialization_instruction(&wr_value_32_val, action,
> > > + ERST_EXECUTE_OPERATION_MAGIC);
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_CHECK_BUSY_STATUS;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > +
> > > + action = ACTION_GET_COMMAND_STATUS;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > +
> > > + action = ACTION_GET_RECORD_IDENTIFIER;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > + action = ACTION_SET_RECORD_IDENTIFIER;
> > > + build_serialization_instruction(&wr_value_64, action, 0);
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_GET_RECORD_COUNT;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > +
> > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > +
> > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > +
> > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > + build_serialization_instruction(&wr_action, action, action);
> > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > +
> > > + /* Serialization Header */
> > > + acpi_table_begin(&table, table_data);
> > > +
> > > + /* Serialization Header Size */
> > > + build_append_int_noprefix(table_data, 48, 4);
> > > +
> > > + /* Reserved */
> > > + build_append_int_noprefix(table_data, 0, 4);
> > > +
> > > + /*
> > > + * Instruction Entry Count
> > > + * Each instruction entry is 32 bytes
> > > + */
> > > + g_assert((table_instruction_data->len) % 32 == 0);
> > > + build_append_int_noprefix(table_data,
> > > + (table_instruction_data->len / 32), 4);
> > > +
> > > + /* Serialization Instruction Entries */
> > > + g_array_append_vals(table_data, table_instruction_data->data,
> > > + table_instruction_data->len);
> > > + g_array_free(table_instruction_data, TRUE);
> > > +
> > > + acpi_table_end(linker, &table);
> > > +}
> > > +
> > > +/*******************************************************************/
> > > +/*******************************************************************/
> > > static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > > {
> > > uint8_t *rc = NULL;
> > > --
> > > 1.8.3.1
> > >
> > >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 0:37 ` Michael S. Tsirkin
@ 2022-01-28 9:01 ` Ani Sinha
2022-01-28 15:11 ` Eric DeVolder
1 sibling, 0 replies; 21+ messages in thread
From: Ani Sinha @ 2022-01-28 9:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, imammedo,
boris.ostrovsky, Eric DeVolder, rth
> > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > inst, bit_width, offset) \
> > BuildSerializationInstructionEntry name = { \
> > .table_data = table_instruction_data, \
> > .bar = bar0, \
> > .instruction = inst, \
> > .flags = 0, \
> > .register_bit_width = bit_width, \
> > .register_offset = offset, \
> > }
> > SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> >
> > These are the 7 accessors needed.
>
> not at all sure this one is worth the macro mess.
>
I did not quite have this in my mind when I said macro but it's fine.
We can improve the code later.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 0:37 ` Michael S. Tsirkin
2022-01-28 9:01 ` Ani Sinha
@ 2022-01-28 15:11 ` Eric DeVolder
2022-01-28 15:54 ` Michael S. Tsirkin
1 sibling, 1 reply; 21+ messages in thread
From: Eric DeVolder @ 2022-01-28 15:11 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, Ani Sinha,
imammedo, boris.ostrovsky, rth
Michael,
Thanks for examining this. Inline response below.
eric
On 1/27/22 18:37, Michael S. Tsirkin wrote:
> On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
>> Ani,
>> Thanks for the RB! Inline responses below.
>> eric
>>
>> On 1/27/22 02:36, Ani Sinha wrote:
>>>
>>>
>>> On Wed, 26 Jan 2022, Eric DeVolder wrote:
>>>
>>>> This builds the ACPI ERST table to inform OSPM how to communicate
>>>> with the acpi-erst device.
>>>
>>> There might be more optimizations possible but I think we have messaged
>>> this code enough. We can further rework the code if needed in subsequent
>>> patches once this is pushed.
>>>
>>>>
>>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>>
>>> with some minor comments,
>>>
>>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>>
>>>> hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 225 insertions(+)
>>>>
>>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
>>>> index fe9ba51..5d5a639 100644
>>>> --- a/hw/acpi/erst.c
>>>> +++ b/hw/acpi/erst.c
>>>> @@ -59,6 +59,27 @@
>>>> #define STATUS_RECORD_STORE_EMPTY 0x04
>>>> #define STATUS_RECORD_NOT_FOUND 0x05
>>>>
>>>> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
>>>> +#define INST_READ_REGISTER 0x00
>>>> +#define INST_READ_REGISTER_VALUE 0x01
>>>> +#define INST_WRITE_REGISTER 0x02
>>>> +#define INST_WRITE_REGISTER_VALUE 0x03
>>>> +#define INST_NOOP 0x04
>>>> +#define INST_LOAD_VAR1 0x05
>>>> +#define INST_LOAD_VAR2 0x06
>>>> +#define INST_STORE_VAR1 0x07
>>>> +#define INST_ADD 0x08
>>>> +#define INST_SUBTRACT 0x09
>>>> +#define INST_ADD_VALUE 0x0A
>>>> +#define INST_SUBTRACT_VALUE 0x0B
>>>> +#define INST_STALL 0x0C
>>>> +#define INST_STALL_WHILE_TRUE 0x0D
>>>> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
>>>> +#define INST_GOTO 0x0F
>>>> +#define INST_SET_SRC_ADDRESS_BASE 0x10
>>>> +#define INST_SET_DST_ADDRESS_BASE 0x11
>>>> +#define INST_MOVE_DATA 0x12
>>>> +
>>>> /* UEFI 2.1: Appendix N Common Platform Error Record */
>>>> #define UEFI_CPER_RECORD_MIN_SIZE 128U
>>>> #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
>>>> @@ -172,6 +193,210 @@ typedef struct {
>>>>
>>>> /*******************************************************************/
>>>> /*******************************************************************/
>>>> +typedef struct {
>>>> + GArray *table_data;
>>>> + pcibus_t bar;
>>>> + uint8_t instruction;
>>>> + uint8_t flags;
>>>> + uint8_t register_bit_width;
>>>> + pcibus_t register_offset;
>>>> +} BuildSerializationInstructionEntry;
>>>> +
>>>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
>>>> +static void build_serialization_instruction(
>>>> + BuildSerializationInstructionEntry *e,
>>>> + uint8_t serialization_action,
>>>> + uint64_t value)
>>>> +{
>>>> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
>>>> + struct AcpiGenericAddress gas;
>>>> + uint64_t mask;
>>>> +
>>>> + /* Serialization Action */
>>>> + build_append_int_noprefix(e->table_data, serialization_action, 1);
>>>> + /* Instruction */
>>>> + build_append_int_noprefix(e->table_data, e->instruction, 1);
>>>> + /* Flags */
>>>> + build_append_int_noprefix(e->table_data, e->flags, 1);
>>>> + /* Reserved */
>>>> + build_append_int_noprefix(e->table_data, 0, 1);
>>>> + /* Register Region */
>>>> + gas.space_id = AML_SYSTEM_MEMORY;
>>>> + gas.bit_width = e->register_bit_width;
>>>> + gas.bit_offset = 0;
>>>> + gas.access_width = ctz32(e->register_bit_width) - 2;
>>>
>>> Should this be casted as unit8_t?
>> OK, done.
>>
>>>
>>>> + gas.address = (uint64_t)(e->bar + e->register_offset);
>>>> + build_append_gas_from_struct(e->table_data, &gas);
>>>> + /* Value */
>>>> + build_append_int_noprefix(e->table_data, value, 8);
>>>> + /* Mask */
>>>> + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
>>>> + build_append_int_noprefix(e->table_data, mask, 8);
>>>> +}
>>>> +
>>>> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
>>>> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
>>>> + const char *oem_id, const char *oem_table_id)
>>>> +{
>>>> + /*
>>>> + * Serialization Action Table
>>>> + * The serialization action table must be generated first
>>>> + * so that its size can be known in order to populate the
>>>> + * Instruction Entry Count field.
>>>> + */
>>>> + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
>>>> + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
>>>> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
>>>> + .oem_table_id = oem_table_id };
>>>> + /* Contexts for the different ways ACTION and VALUE are accessed */
>>>> + BuildSerializationInstructionEntry rd_value_32_val = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_READ_REGISTER_VALUE,
>>>> + .flags = 0,
>>>> + .register_bit_width = 32,
>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>> + };
>>>> + BuildSerializationInstructionEntry rd_value_32 = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_READ_REGISTER,
>>>> + .flags = 0,
>>>> + .register_bit_width = 32,
>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>> + };
>>>> + BuildSerializationInstructionEntry rd_value_64 = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_READ_REGISTER,
>>>> + .flags = 0,
>>>> + .register_bit_width = 64,
>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>> + };
>>>> + BuildSerializationInstructionEntry wr_value_32_val = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_WRITE_REGISTER_VALUE,
>>>> + .flags = 0,
>>>> + .register_bit_width = 32,
>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>> + };
>>>> + BuildSerializationInstructionEntry wr_value_32 = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_WRITE_REGISTER,
>>>> + .flags = 0,
>>>> + .register_bit_width = 32,
>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>> + };
>>>> + BuildSerializationInstructionEntry wr_value_64 = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_WRITE_REGISTER,
>>>> + .flags = 0,
>>>> + .register_bit_width = 64,
>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>> + };
>>>> + BuildSerializationInstructionEntry wr_action = {
>>>> + .table_data = table_instruction_data,
>>>> + .bar = bar0,
>>>> + .instruction = INST_WRITE_REGISTER_VALUE,
>>>> + .flags = 0,
>>>> + .register_bit_width = 32,
>>>> + .register_offset = ERST_ACTION_OFFSET,
>>>> + };
>>>
>>> We can probably write a macro to simply generating these structs. I see
>>> .bar and .flags are the same in all structs. The .bit_width can be a param
>>> into the macro etc.
>> Agree, however the request was to NOT hide the use of local variables in
>> macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
>> would be parameters, that leaves .table_data and .bar which just need the local
>> variables. Is the following acceptable (which hides the use of the local variables)?
>
> You can just use assignment:
>
> BuildSerializationInstructionEntry entry = {
> .table_data = table_instruction_data,
> .bar = bar0,
> .flags = 0,
> .register_bit_width = 32,
> };
> BuildSerializationInstructionEntry rd_value_32_val = entry;
> rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
>
> and so on.
> not sure it's worth it, it's just a couple of lines.
>
OK, here is the equivalent using struct assignment, is this what you were after?
BuildSerializationInstructionEntry base = {
.table_data = table_instruction_data,
.bar = bar0,
.instruction = INST_WRITE_REGISTER,
.flags = 0,
.register_bit_width = 32,
.register_offset = ERST_VALUE_OFFSET,
};
BuildSerializationInstructionEntry rd_value_32_val = base;
rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
BuildSerializationInstructionEntry rd_value_32 = base;
rd_value_32.instruction = INST_READ_REGISTER;
BuildSerializationInstructionEntry rd_value_64 = base;
rd_value_64.instruction = INST_READ_REGISTER;
rd_value_64.register_bit_width = 64;
BuildSerializationInstructionEntry wr_value_32_val = base;
wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
BuildSerializationInstructionEntry wr_value_32 = base;
BuildSerializationInstructionEntry wr_value_64 = base;
wr_value_64.register_bit_width = 64;
BuildSerializationInstructionEntry wr_action = base;
wr_action.instruction = INST_WRITE_REGISTER_VALUE;
wr_action.register_offset = ERST_ACTION_OFFSET;
>
>
>> #define SERIALIZATIONINSTRUCTIONCTX(name, \
>> inst, bit_width, offset) \
>> BuildSerializationInstructionEntry name = { \
>> .table_data = table_instruction_data, \
>> .bar = bar0, \
>> .instruction = inst, \
>> .flags = 0, \
>> .register_bit_width = bit_width, \
>> .register_offset = offset, \
>> }
>> SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>> INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>> SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>> INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>> SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>> INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>> SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>> INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>> SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>> INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>> SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>> INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>> SERIALIZATIONINSTRUCTIONCTX(wr_action,
>> INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>>
>> These are the 7 accessors needed.
>
> not at all sure this one is worth the macro mess.
I'm hoping to produce a v15 with the style you want.
eric
>
>>>
>>>> + unsigned action;
>>>> +
>>>> + trace_acpi_erst_pci_bar_0(bar0);
>>>> +
>>>> + /* Serialization Instruction Entries */
>>>> + action = ACTION_BEGIN_WRITE_OPERATION;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_BEGIN_READ_OPERATION;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_BEGIN_CLEAR_OPERATION;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_END_OPERATION;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_SET_RECORD_OFFSET;
>>>> + build_serialization_instruction(&wr_value_32, action, 0);
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_EXECUTE_OPERATION;
>>>> + build_serialization_instruction(&wr_value_32_val, action,
>>>> + ERST_EXECUTE_OPERATION_MAGIC);
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_CHECK_BUSY_STATUS;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_32_val, action, 0x01);
>>>> +
>>>> + action = ACTION_GET_COMMAND_STATUS;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_32, action, 0);
>>>> +
>>>> + action = ACTION_GET_RECORD_IDENTIFIER;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> + action = ACTION_SET_RECORD_IDENTIFIER;
>>>> + build_serialization_instruction(&wr_value_64, action, 0);
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_GET_RECORD_COUNT;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_32, action, 0);
>>>> +
>>>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> +
>>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_32, action, 0);
>>>> +
>>>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>>>> + build_serialization_instruction(&wr_action, action, action);
>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>> +
>>>> + /* Serialization Header */
>>>> + acpi_table_begin(&table, table_data);
>>>> +
>>>> + /* Serialization Header Size */
>>>> + build_append_int_noprefix(table_data, 48, 4);
>>>> +
>>>> + /* Reserved */
>>>> + build_append_int_noprefix(table_data, 0, 4);
>>>> +
>>>> + /*
>>>> + * Instruction Entry Count
>>>> + * Each instruction entry is 32 bytes
>>>> + */
>>>> + g_assert((table_instruction_data->len) % 32 == 0);
>>>> + build_append_int_noprefix(table_data,
>>>> + (table_instruction_data->len / 32), 4);
>>>> +
>>>> + /* Serialization Instruction Entries */
>>>> + g_array_append_vals(table_data, table_instruction_data->data,
>>>> + table_instruction_data->len);
>>>> + g_array_free(table_instruction_data, TRUE);
>>>> +
>>>> + acpi_table_end(linker, &table);
>>>> +}
>>>> +
>>>> +/*******************************************************************/
>>>> +/*******************************************************************/
>>>> static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>>>> {
>>>> uint8_t *rc = NULL;
>>>> --
>>>> 1.8.3.1
>>>>
>>>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 15:11 ` Eric DeVolder
@ 2022-01-28 15:54 ` Michael S. Tsirkin
2022-01-28 16:01 ` Eric DeVolder
0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2022-01-28 15:54 UTC (permalink / raw)
To: Eric DeVolder
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, Ani Sinha,
imammedo, boris.ostrovsky, rth
On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
> Michael,
> Thanks for examining this. Inline response below.
> eric
>
> On 1/27/22 18:37, Michael S. Tsirkin wrote:
> > On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> > > Ani,
> > > Thanks for the RB! Inline responses below.
> > > eric
> > >
> > > On 1/27/22 02:36, Ani Sinha wrote:
> > > >
> > > >
> > > > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > > >
> > > > > This builds the ACPI ERST table to inform OSPM how to communicate
> > > > > with the acpi-erst device.
> > > >
> > > > There might be more optimizations possible but I think we have messaged
> > > > this code enough. We can further rework the code if needed in subsequent
> > > > patches once this is pushed.
> > > >
> > > > >
> > > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > > >
> > > > with some minor comments,
> > > >
> > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > >
> > > > > hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 225 insertions(+)
> > > > >
> > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > > > index fe9ba51..5d5a639 100644
> > > > > --- a/hw/acpi/erst.c
> > > > > +++ b/hw/acpi/erst.c
> > > > > @@ -59,6 +59,27 @@
> > > > > #define STATUS_RECORD_STORE_EMPTY 0x04
> > > > > #define STATUS_RECORD_NOT_FOUND 0x05
> > > > >
> > > > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > > > +#define INST_READ_REGISTER 0x00
> > > > > +#define INST_READ_REGISTER_VALUE 0x01
> > > > > +#define INST_WRITE_REGISTER 0x02
> > > > > +#define INST_WRITE_REGISTER_VALUE 0x03
> > > > > +#define INST_NOOP 0x04
> > > > > +#define INST_LOAD_VAR1 0x05
> > > > > +#define INST_LOAD_VAR2 0x06
> > > > > +#define INST_STORE_VAR1 0x07
> > > > > +#define INST_ADD 0x08
> > > > > +#define INST_SUBTRACT 0x09
> > > > > +#define INST_ADD_VALUE 0x0A
> > > > > +#define INST_SUBTRACT_VALUE 0x0B
> > > > > +#define INST_STALL 0x0C
> > > > > +#define INST_STALL_WHILE_TRUE 0x0D
> > > > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > > > +#define INST_GOTO 0x0F
> > > > > +#define INST_SET_SRC_ADDRESS_BASE 0x10
> > > > > +#define INST_SET_DST_ADDRESS_BASE 0x11
> > > > > +#define INST_MOVE_DATA 0x12
> > > > > +
> > > > > /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > > > #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > > > #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > > > @@ -172,6 +193,210 @@ typedef struct {
> > > > >
> > > > > /*******************************************************************/
> > > > > /*******************************************************************/
> > > > > +typedef struct {
> > > > > + GArray *table_data;
> > > > > + pcibus_t bar;
> > > > > + uint8_t instruction;
> > > > > + uint8_t flags;
> > > > > + uint8_t register_bit_width;
> > > > > + pcibus_t register_offset;
> > > > > +} BuildSerializationInstructionEntry;
> > > > > +
> > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > > > +static void build_serialization_instruction(
> > > > > + BuildSerializationInstructionEntry *e,
> > > > > + uint8_t serialization_action,
> > > > > + uint64_t value)
> > > > > +{
> > > > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > > > > + struct AcpiGenericAddress gas;
> > > > > + uint64_t mask;
> > > > > +
> > > > > + /* Serialization Action */
> > > > > + build_append_int_noprefix(e->table_data, serialization_action, 1);
> > > > > + /* Instruction */
> > > > > + build_append_int_noprefix(e->table_data, e->instruction, 1);
> > > > > + /* Flags */
> > > > > + build_append_int_noprefix(e->table_data, e->flags, 1);
> > > > > + /* Reserved */
> > > > > + build_append_int_noprefix(e->table_data, 0, 1);
> > > > > + /* Register Region */
> > > > > + gas.space_id = AML_SYSTEM_MEMORY;
> > > > > + gas.bit_width = e->register_bit_width;
> > > > > + gas.bit_offset = 0;
> > > > > + gas.access_width = ctz32(e->register_bit_width) - 2;
> > > >
> > > > Should this be casted as unit8_t?
> > > OK, done.
> > >
> > > >
> > > > > + gas.address = (uint64_t)(e->bar + e->register_offset);
> > > > > + build_append_gas_from_struct(e->table_data, &gas);
> > > > > + /* Value */
> > > > > + build_append_int_noprefix(e->table_data, value, 8);
> > > > > + /* Mask */
> > > > > + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > > > + build_append_int_noprefix(e->table_data, mask, 8);
> > > > > +}
> > > > > +
> > > > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > > > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > > > > + const char *oem_id, const char *oem_table_id)
> > > > > +{
> > > > > + /*
> > > > > + * Serialization Action Table
> > > > > + * The serialization action table must be generated first
> > > > > + * so that its size can be known in order to populate the
> > > > > + * Instruction Entry Count field.
> > > > > + */
> > > > > + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > > > > + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > > > > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > > > > + .oem_table_id = oem_table_id };
> > > > > + /* Contexts for the different ways ACTION and VALUE are accessed */
> > > > > + BuildSerializationInstructionEntry rd_value_32_val = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_READ_REGISTER_VALUE,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 32,
> > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > + };
> > > > > + BuildSerializationInstructionEntry rd_value_32 = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_READ_REGISTER,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 32,
> > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > + };
> > > > > + BuildSerializationInstructionEntry rd_value_64 = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_READ_REGISTER,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 64,
> > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > + };
> > > > > + BuildSerializationInstructionEntry wr_value_32_val = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 32,
> > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > + };
> > > > > + BuildSerializationInstructionEntry wr_value_32 = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_WRITE_REGISTER,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 32,
> > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > + };
> > > > > + BuildSerializationInstructionEntry wr_value_64 = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_WRITE_REGISTER,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 64,
> > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > + };
> > > > > + BuildSerializationInstructionEntry wr_action = {
> > > > > + .table_data = table_instruction_data,
> > > > > + .bar = bar0,
> > > > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > + .flags = 0,
> > > > > + .register_bit_width = 32,
> > > > > + .register_offset = ERST_ACTION_OFFSET,
> > > > > + };
> > > >
> > > > We can probably write a macro to simply generating these structs. I see
> > > > .bar and .flags are the same in all structs. The .bit_width can be a param
> > > > into the macro etc.
> > > Agree, however the request was to NOT hide the use of local variables in
> > > macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
> > > would be parameters, that leaves .table_data and .bar which just need the local
> > > variables. Is the following acceptable (which hides the use of the local variables)?
> >
> > You can just use assignment:
> >
> > BuildSerializationInstructionEntry entry = {
> > .table_data = table_instruction_data,
> > .bar = bar0,
> > .flags = 0,
> > .register_bit_width = 32,
> > };
> > BuildSerializationInstructionEntry rd_value_32_val = entry;
> > rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> > rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> >
> > and so on.
> > not sure it's worth it, it's just a couple of lines.
> >
>
> OK, here is the equivalent using struct assignment, is this what you were after?
>
> BuildSerializationInstructionEntry base = {
> .table_data = table_instruction_data,
> .bar = bar0,
> .instruction = INST_WRITE_REGISTER,
> .flags = 0,
> .register_bit_width = 32,
> .register_offset = ERST_VALUE_OFFSET,
> };
> BuildSerializationInstructionEntry rd_value_32_val = base;
> rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
> BuildSerializationInstructionEntry rd_value_32 = base;
> rd_value_32.instruction = INST_READ_REGISTER;
> BuildSerializationInstructionEntry rd_value_64 = base;
> rd_value_64.instruction = INST_READ_REGISTER;
> rd_value_64.register_bit_width = 64;
> BuildSerializationInstructionEntry wr_value_32_val = base;
> wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
> BuildSerializationInstructionEntry wr_value_32 = base;
> BuildSerializationInstructionEntry wr_value_64 = base;
> wr_value_64.register_bit_width = 64;
> BuildSerializationInstructionEntry wr_action = base;
> wr_action.instruction = INST_WRITE_REGISTER_VALUE;
> wr_action.register_offset = ERST_ACTION_OFFSET;
>
That's what I described, yes. We should have some empty lines here I
guess. I'm ok with the original one too, there's not too much
duplication.
>
> >
> >
> > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > > inst, bit_width, offset) \
> > > BuildSerializationInstructionEntry name = { \
> > > .table_data = table_instruction_data, \
> > > .bar = bar0, \
> > > .instruction = inst, \
> > > .flags = 0, \
> > > .register_bit_width = bit_width, \
> > > .register_offset = offset, \
> > > }
> > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > > INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > > INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > > SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > > INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > > INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > > INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > > SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > > INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > > SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > > INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > >
> > > These are the 7 accessors needed.
> >
> > not at all sure this one is worth the macro mess.
>
> I'm hoping to produce a v15 with the style you want.
> eric
>
> >
> > > >
> > > > > + unsigned action;
> > > > > +
> > > > > + trace_acpi_erst_pci_bar_0(bar0);
> > > > > +
> > > > > + /* Serialization Instruction Entries */
> > > > > + action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_BEGIN_READ_OPERATION;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_END_OPERATION;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_SET_RECORD_OFFSET;
> > > > > + build_serialization_instruction(&wr_value_32, action, 0);
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_EXECUTE_OPERATION;
> > > > > + build_serialization_instruction(&wr_value_32_val, action,
> > > > > + ERST_EXECUTE_OPERATION_MAGIC);
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_CHECK_BUSY_STATUS;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > > > +
> > > > > + action = ACTION_GET_COMMAND_STATUS;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > +
> > > > > + action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > + action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > + build_serialization_instruction(&wr_value_64, action, 0);
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_GET_RECORD_COUNT;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > +
> > > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > +
> > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > +
> > > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > +
> > > > > + /* Serialization Header */
> > > > > + acpi_table_begin(&table, table_data);
> > > > > +
> > > > > + /* Serialization Header Size */
> > > > > + build_append_int_noprefix(table_data, 48, 4);
> > > > > +
> > > > > + /* Reserved */
> > > > > + build_append_int_noprefix(table_data, 0, 4);
> > > > > +
> > > > > + /*
> > > > > + * Instruction Entry Count
> > > > > + * Each instruction entry is 32 bytes
> > > > > + */
> > > > > + g_assert((table_instruction_data->len) % 32 == 0);
> > > > > + build_append_int_noprefix(table_data,
> > > > > + (table_instruction_data->len / 32), 4);
> > > > > +
> > > > > + /* Serialization Instruction Entries */
> > > > > + g_array_append_vals(table_data, table_instruction_data->data,
> > > > > + table_instruction_data->len);
> > > > > + g_array_free(table_instruction_data, TRUE);
> > > > > +
> > > > > + acpi_table_end(linker, &table);
> > > > > +}
> > > > > +
> > > > > +/*******************************************************************/
> > > > > +/*******************************************************************/
> > > > > static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > > > > {
> > > > > uint8_t *rc = NULL;
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > > > >
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 15:54 ` Michael S. Tsirkin
@ 2022-01-28 16:01 ` Eric DeVolder
2022-01-28 16:14 ` Michael S. Tsirkin
0 siblings, 1 reply; 21+ messages in thread
From: Eric DeVolder @ 2022-01-28 16:01 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, Ani Sinha,
imammedo, boris.ostrovsky, rth
Michael, thanks! See inline response below, please.
eric
On 1/28/22 09:54, Michael S. Tsirkin wrote:
> On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
>> Michael,
>> Thanks for examining this. Inline response below.
>> eric
>>
>> On 1/27/22 18:37, Michael S. Tsirkin wrote:
>>> On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
>>>> Ani,
>>>> Thanks for the RB! Inline responses below.
>>>> eric
>>>>
>>>> On 1/27/22 02:36, Ani Sinha wrote:
>>>>>
>>>>>
>>>>> On Wed, 26 Jan 2022, Eric DeVolder wrote:
>>>>>
>>>>>> This builds the ACPI ERST table to inform OSPM how to communicate
>>>>>> with the acpi-erst device.
>>>>>
>>>>> There might be more optimizations possible but I think we have messaged
>>>>> this code enough. We can further rework the code if needed in subsequent
>>>>> patches once this is pushed.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>>>>>
>>>>> with some minor comments,
>>>>>
>>>>> Reviewed-by: Ani Sinha <ani@anisinha.ca>
>>>>>
>>>>>> hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 225 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
>>>>>> index fe9ba51..5d5a639 100644
>>>>>> --- a/hw/acpi/erst.c
>>>>>> +++ b/hw/acpi/erst.c
>>>>>> @@ -59,6 +59,27 @@
>>>>>> #define STATUS_RECORD_STORE_EMPTY 0x04
>>>>>> #define STATUS_RECORD_NOT_FOUND 0x05
>>>>>>
>>>>>> +/* ACPI 4.0: Table 17-19 Serialization Instructions */
>>>>>> +#define INST_READ_REGISTER 0x00
>>>>>> +#define INST_READ_REGISTER_VALUE 0x01
>>>>>> +#define INST_WRITE_REGISTER 0x02
>>>>>> +#define INST_WRITE_REGISTER_VALUE 0x03
>>>>>> +#define INST_NOOP 0x04
>>>>>> +#define INST_LOAD_VAR1 0x05
>>>>>> +#define INST_LOAD_VAR2 0x06
>>>>>> +#define INST_STORE_VAR1 0x07
>>>>>> +#define INST_ADD 0x08
>>>>>> +#define INST_SUBTRACT 0x09
>>>>>> +#define INST_ADD_VALUE 0x0A
>>>>>> +#define INST_SUBTRACT_VALUE 0x0B
>>>>>> +#define INST_STALL 0x0C
>>>>>> +#define INST_STALL_WHILE_TRUE 0x0D
>>>>>> +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
>>>>>> +#define INST_GOTO 0x0F
>>>>>> +#define INST_SET_SRC_ADDRESS_BASE 0x10
>>>>>> +#define INST_SET_DST_ADDRESS_BASE 0x11
>>>>>> +#define INST_MOVE_DATA 0x12
>>>>>> +
>>>>>> /* UEFI 2.1: Appendix N Common Platform Error Record */
>>>>>> #define UEFI_CPER_RECORD_MIN_SIZE 128U
>>>>>> #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
>>>>>> @@ -172,6 +193,210 @@ typedef struct {
>>>>>>
>>>>>> /*******************************************************************/
>>>>>> /*******************************************************************/
>>>>>> +typedef struct {
>>>>>> + GArray *table_data;
>>>>>> + pcibus_t bar;
>>>>>> + uint8_t instruction;
>>>>>> + uint8_t flags;
>>>>>> + uint8_t register_bit_width;
>>>>>> + pcibus_t register_offset;
>>>>>> +} BuildSerializationInstructionEntry;
>>>>>> +
>>>>>> +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
>>>>>> +static void build_serialization_instruction(
>>>>>> + BuildSerializationInstructionEntry *e,
>>>>>> + uint8_t serialization_action,
>>>>>> + uint64_t value)
>>>>>> +{
>>>>>> + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
>>>>>> + struct AcpiGenericAddress gas;
>>>>>> + uint64_t mask;
>>>>>> +
>>>>>> + /* Serialization Action */
>>>>>> + build_append_int_noprefix(e->table_data, serialization_action, 1);
>>>>>> + /* Instruction */
>>>>>> + build_append_int_noprefix(e->table_data, e->instruction, 1);
>>>>>> + /* Flags */
>>>>>> + build_append_int_noprefix(e->table_data, e->flags, 1);
>>>>>> + /* Reserved */
>>>>>> + build_append_int_noprefix(e->table_data, 0, 1);
>>>>>> + /* Register Region */
>>>>>> + gas.space_id = AML_SYSTEM_MEMORY;
>>>>>> + gas.bit_width = e->register_bit_width;
>>>>>> + gas.bit_offset = 0;
>>>>>> + gas.access_width = ctz32(e->register_bit_width) - 2;
>>>>>
>>>>> Should this be casted as unit8_t?
>>>> OK, done.
>>>>
>>>>>
>>>>>> + gas.address = (uint64_t)(e->bar + e->register_offset);
>>>>>> + build_append_gas_from_struct(e->table_data, &gas);
>>>>>> + /* Value */
>>>>>> + build_append_int_noprefix(e->table_data, value, 8);
>>>>>> + /* Mask */
>>>>>> + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
>>>>>> + build_append_int_noprefix(e->table_data, mask, 8);
>>>>>> +}
>>>>>> +
>>>>>> +/* ACPI 4.0: 17.4.1 Serialization Action Table */
>>>>>> +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
>>>>>> + const char *oem_id, const char *oem_table_id)
>>>>>> +{
>>>>>> + /*
>>>>>> + * Serialization Action Table
>>>>>> + * The serialization action table must be generated first
>>>>>> + * so that its size can be known in order to populate the
>>>>>> + * Instruction Entry Count field.
>>>>>> + */
>>>>>> + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
>>>>>> + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
>>>>>> + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
>>>>>> + .oem_table_id = oem_table_id };
>>>>>> + /* Contexts for the different ways ACTION and VALUE are accessed */
>>>>>> + BuildSerializationInstructionEntry rd_value_32_val = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_READ_REGISTER_VALUE,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 32,
>>>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>>>> + };
>>>>>> + BuildSerializationInstructionEntry rd_value_32 = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_READ_REGISTER,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 32,
>>>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>>>> + };
>>>>>> + BuildSerializationInstructionEntry rd_value_64 = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_READ_REGISTER,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 64,
>>>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>>>> + };
>>>>>> + BuildSerializationInstructionEntry wr_value_32_val = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_WRITE_REGISTER_VALUE,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 32,
>>>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>>>> + };
>>>>>> + BuildSerializationInstructionEntry wr_value_32 = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_WRITE_REGISTER,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 32,
>>>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>>>> + };
>>>>>> + BuildSerializationInstructionEntry wr_value_64 = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_WRITE_REGISTER,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 64,
>>>>>> + .register_offset = ERST_VALUE_OFFSET,
>>>>>> + };
>>>>>> + BuildSerializationInstructionEntry wr_action = {
>>>>>> + .table_data = table_instruction_data,
>>>>>> + .bar = bar0,
>>>>>> + .instruction = INST_WRITE_REGISTER_VALUE,
>>>>>> + .flags = 0,
>>>>>> + .register_bit_width = 32,
>>>>>> + .register_offset = ERST_ACTION_OFFSET,
>>>>>> + };
>>>>>
>>>>> We can probably write a macro to simply generating these structs. I see
>>>>> .bar and .flags are the same in all structs. The .bit_width can be a param
>>>>> into the macro etc.
>>>> Agree, however the request was to NOT hide the use of local variables in
>>>> macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
>>>> would be parameters, that leaves .table_data and .bar which just need the local
>>>> variables. Is the following acceptable (which hides the use of the local variables)?
>>>
>>> You can just use assignment:
>>>
>>> BuildSerializationInstructionEntry entry = {
>>> .table_data = table_instruction_data,
>>> .bar = bar0,
>>> .flags = 0,
>>> .register_bit_width = 32,
>>> };
>>> BuildSerializationInstructionEntry rd_value_32_val = entry;
>>> rd_value_32_val.action = INST_READ_REGISTER_VALUE;
>>> rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
>>>
>>> and so on.
>>> not sure it's worth it, it's just a couple of lines.
>>>
>>
>> OK, here is the equivalent using struct assignment, is this what you were after?
>>
>> BuildSerializationInstructionEntry base = {
>> .table_data = table_instruction_data,
>> .bar = bar0,
>> .instruction = INST_WRITE_REGISTER,
>> .flags = 0,
>> .register_bit_width = 32,
>> .register_offset = ERST_VALUE_OFFSET,
>> };
>> BuildSerializationInstructionEntry rd_value_32_val = base;
>> rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
>> BuildSerializationInstructionEntry rd_value_32 = base;
>> rd_value_32.instruction = INST_READ_REGISTER;
>> BuildSerializationInstructionEntry rd_value_64 = base;
>> rd_value_64.instruction = INST_READ_REGISTER;
>> rd_value_64.register_bit_width = 64;
>> BuildSerializationInstructionEntry wr_value_32_val = base;
>> wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
>> BuildSerializationInstructionEntry wr_value_32 = base;
>> BuildSerializationInstructionEntry wr_value_64 = base;
>> wr_value_64.register_bit_width = 64;
>> BuildSerializationInstructionEntry wr_action = base;
>> wr_action.instruction = INST_WRITE_REGISTER_VALUE;
>> wr_action.register_offset = ERST_ACTION_OFFSET;
>>
>
> That's what I described, yes. We should have some empty lines here I
> guess. I'm ok with the original one too, there's not too much
> duplication.
Are the blank lines referring to spacing out the setup of each of the 7 accesors?
If so, I could put a one line comment between each setup? Or is a blank line also
needed?
Is it OK to post v15 with the struct assignment approach? Or would you prefer the
explicit structs (which is what I think you mean by 'the original one')?
Thanks!
eric
>
>
>>
>>>
>>>
>>>> #define SERIALIZATIONINSTRUCTIONCTX(name, \
>>>> inst, bit_width, offset) \
>>>> BuildSerializationInstructionEntry name = { \
>>>> .table_data = table_instruction_data, \
>>>> .bar = bar0, \
>>>> .instruction = inst, \
>>>> .flags = 0, \
>>>> .register_bit_width = bit_width, \
>>>> .register_offset = offset, \
>>>> }
>>>> SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
>>>> INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>>>> SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
>>>> INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
>>>> SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
>>>> INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
>>>> SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
>>>> INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
>>>> SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
>>>> INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
>>>> SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
>>>> INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
>>>> SERIALIZATIONINSTRUCTIONCTX(wr_action,
>>>> INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
>>>>
>>>> These are the 7 accessors needed.
>>>
>>> not at all sure this one is worth the macro mess.
>>
>> I'm hoping to produce a v15 with the style you want.
>> eric
>>
>>>
>>>>>
>>>>>> + unsigned action;
>>>>>> +
>>>>>> + trace_acpi_erst_pci_bar_0(bar0);
>>>>>> +
>>>>>> + /* Serialization Instruction Entries */
>>>>>> + action = ACTION_BEGIN_WRITE_OPERATION;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_BEGIN_READ_OPERATION;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_BEGIN_CLEAR_OPERATION;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_END_OPERATION;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_SET_RECORD_OFFSET;
>>>>>> + build_serialization_instruction(&wr_value_32, action, 0);
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_EXECUTE_OPERATION;
>>>>>> + build_serialization_instruction(&wr_value_32_val, action,
>>>>>> + ERST_EXECUTE_OPERATION_MAGIC);
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_CHECK_BUSY_STATUS;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_32_val, action, 0x01);
>>>>>> +
>>>>>> + action = ACTION_GET_COMMAND_STATUS;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_32, action, 0);
>>>>>> +
>>>>>> + action = ACTION_GET_RECORD_IDENTIFIER;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> + action = ACTION_SET_RECORD_IDENTIFIER;
>>>>>> + build_serialization_instruction(&wr_value_64, action, 0);
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_GET_RECORD_COUNT;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_32, action, 0);
>>>>>> +
>>>>>> + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> +
>>>>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_32, action, 0);
>>>>>> +
>>>>>> + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
>>>>>> + build_serialization_instruction(&wr_action, action, action);
>>>>>> + build_serialization_instruction(&rd_value_64, action, 0);
>>>>>> +
>>>>>> + /* Serialization Header */
>>>>>> + acpi_table_begin(&table, table_data);
>>>>>> +
>>>>>> + /* Serialization Header Size */
>>>>>> + build_append_int_noprefix(table_data, 48, 4);
>>>>>> +
>>>>>> + /* Reserved */
>>>>>> + build_append_int_noprefix(table_data, 0, 4);
>>>>>> +
>>>>>> + /*
>>>>>> + * Instruction Entry Count
>>>>>> + * Each instruction entry is 32 bytes
>>>>>> + */
>>>>>> + g_assert((table_instruction_data->len) % 32 == 0);
>>>>>> + build_append_int_noprefix(table_data,
>>>>>> + (table_instruction_data->len / 32), 4);
>>>>>> +
>>>>>> + /* Serialization Instruction Entries */
>>>>>> + g_array_append_vals(table_data, table_instruction_data->data,
>>>>>> + table_instruction_data->len);
>>>>>> + g_array_free(table_instruction_data, TRUE);
>>>>>> +
>>>>>> + acpi_table_end(linker, &table);
>>>>>> +}
>>>>>> +
>>>>>> +/*******************************************************************/
>>>>>> +/*******************************************************************/
>>>>>> static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
>>>>>> {
>>>>>> uint8_t *rc = NULL;
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>>
>>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 16:01 ` Eric DeVolder
@ 2022-01-28 16:14 ` Michael S. Tsirkin
2022-01-28 17:25 ` Ani Sinha
0 siblings, 1 reply; 21+ messages in thread
From: Michael S. Tsirkin @ 2022-01-28 16:14 UTC (permalink / raw)
To: Eric DeVolder
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, Ani Sinha,
imammedo, boris.ostrovsky, rth
On Fri, Jan 28, 2022 at 10:01:18AM -0600, Eric DeVolder wrote:
> Michael, thanks! See inline response below, please.
> eric
>
> On 1/28/22 09:54, Michael S. Tsirkin wrote:
> > On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
> > > Michael,
> > > Thanks for examining this. Inline response below.
> > > eric
> > >
> > > On 1/27/22 18:37, Michael S. Tsirkin wrote:
> > > > On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> > > > > Ani,
> > > > > Thanks for the RB! Inline responses below.
> > > > > eric
> > > > >
> > > > > On 1/27/22 02:36, Ani Sinha wrote:
> > > > > >
> > > > > >
> > > > > > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > > > > >
> > > > > > > This builds the ACPI ERST table to inform OSPM how to communicate
> > > > > > > with the acpi-erst device.
> > > > > >
> > > > > > There might be more optimizations possible but I think we have messaged
> > > > > > this code enough. We can further rework the code if needed in subsequent
> > > > > > patches once this is pushed.
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > > > > >
> > > > > > with some minor comments,
> > > > > >
> > > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > > >
> > > > > > > hw/acpi/erst.c | 225 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > 1 file changed, 225 insertions(+)
> > > > > > >
> > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > > > > > index fe9ba51..5d5a639 100644
> > > > > > > --- a/hw/acpi/erst.c
> > > > > > > +++ b/hw/acpi/erst.c
> > > > > > > @@ -59,6 +59,27 @@
> > > > > > > #define STATUS_RECORD_STORE_EMPTY 0x04
> > > > > > > #define STATUS_RECORD_NOT_FOUND 0x05
> > > > > > >
> > > > > > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > > > > > +#define INST_READ_REGISTER 0x00
> > > > > > > +#define INST_READ_REGISTER_VALUE 0x01
> > > > > > > +#define INST_WRITE_REGISTER 0x02
> > > > > > > +#define INST_WRITE_REGISTER_VALUE 0x03
> > > > > > > +#define INST_NOOP 0x04
> > > > > > > +#define INST_LOAD_VAR1 0x05
> > > > > > > +#define INST_LOAD_VAR2 0x06
> > > > > > > +#define INST_STORE_VAR1 0x07
> > > > > > > +#define INST_ADD 0x08
> > > > > > > +#define INST_SUBTRACT 0x09
> > > > > > > +#define INST_ADD_VALUE 0x0A
> > > > > > > +#define INST_SUBTRACT_VALUE 0x0B
> > > > > > > +#define INST_STALL 0x0C
> > > > > > > +#define INST_STALL_WHILE_TRUE 0x0D
> > > > > > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > > > > > +#define INST_GOTO 0x0F
> > > > > > > +#define INST_SET_SRC_ADDRESS_BASE 0x10
> > > > > > > +#define INST_SET_DST_ADDRESS_BASE 0x11
> > > > > > > +#define INST_MOVE_DATA 0x12
> > > > > > > +
> > > > > > > /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > > > > > #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > > > > > #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > > > > > @@ -172,6 +193,210 @@ typedef struct {
> > > > > > >
> > > > > > > /*******************************************************************/
> > > > > > > /*******************************************************************/
> > > > > > > +typedef struct {
> > > > > > > + GArray *table_data;
> > > > > > > + pcibus_t bar;
> > > > > > > + uint8_t instruction;
> > > > > > > + uint8_t flags;
> > > > > > > + uint8_t register_bit_width;
> > > > > > > + pcibus_t register_offset;
> > > > > > > +} BuildSerializationInstructionEntry;
> > > > > > > +
> > > > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > > > > > +static void build_serialization_instruction(
> > > > > > > + BuildSerializationInstructionEntry *e,
> > > > > > > + uint8_t serialization_action,
> > > > > > > + uint64_t value)
> > > > > > > +{
> > > > > > > + /* ACPI 4.0: Table 17-18 Serialization Instruction Entry */
> > > > > > > + struct AcpiGenericAddress gas;
> > > > > > > + uint64_t mask;
> > > > > > > +
> > > > > > > + /* Serialization Action */
> > > > > > > + build_append_int_noprefix(e->table_data, serialization_action, 1);
> > > > > > > + /* Instruction */
> > > > > > > + build_append_int_noprefix(e->table_data, e->instruction, 1);
> > > > > > > + /* Flags */
> > > > > > > + build_append_int_noprefix(e->table_data, e->flags, 1);
> > > > > > > + /* Reserved */
> > > > > > > + build_append_int_noprefix(e->table_data, 0, 1);
> > > > > > > + /* Register Region */
> > > > > > > + gas.space_id = AML_SYSTEM_MEMORY;
> > > > > > > + gas.bit_width = e->register_bit_width;
> > > > > > > + gas.bit_offset = 0;
> > > > > > > + gas.access_width = ctz32(e->register_bit_width) - 2;
> > > > > >
> > > > > > Should this be casted as unit8_t?
> > > > > OK, done.
> > > > >
> > > > > >
> > > > > > > + gas.address = (uint64_t)(e->bar + e->register_offset);
> > > > > > > + build_append_gas_from_struct(e->table_data, &gas);
> > > > > > > + /* Value */
> > > > > > > + build_append_int_noprefix(e->table_data, value, 8);
> > > > > > > + /* Mask */
> > > > > > > + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > > > > > + build_append_int_noprefix(e->table_data, mask, 8);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > > > > > +void build_erst(GArray *table_data, BIOSLinker *linker, Object *erst_dev,
> > > > > > > + const char *oem_id, const char *oem_table_id)
> > > > > > > +{
> > > > > > > + /*
> > > > > > > + * Serialization Action Table
> > > > > > > + * The serialization action table must be generated first
> > > > > > > + * so that its size can be known in order to populate the
> > > > > > > + * Instruction Entry Count field.
> > > > > > > + */
> > > > > > > + GArray *table_instruction_data = g_array_new(FALSE, FALSE, sizeof(char));
> > > > > > > + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev), 0);
> > > > > > > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id = oem_id,
> > > > > > > + .oem_table_id = oem_table_id };
> > > > > > > + /* Contexts for the different ways ACTION and VALUE are accessed */
> > > > > > > + BuildSerializationInstructionEntry rd_value_32_val = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_READ_REGISTER_VALUE,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 32,
> > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > + };
> > > > > > > + BuildSerializationInstructionEntry rd_value_32 = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_READ_REGISTER,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 32,
> > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > + };
> > > > > > > + BuildSerializationInstructionEntry rd_value_64 = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_READ_REGISTER,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 64,
> > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > + };
> > > > > > > + BuildSerializationInstructionEntry wr_value_32_val = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 32,
> > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > + };
> > > > > > > + BuildSerializationInstructionEntry wr_value_32 = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_WRITE_REGISTER,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 32,
> > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > + };
> > > > > > > + BuildSerializationInstructionEntry wr_value_64 = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_WRITE_REGISTER,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 64,
> > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > + };
> > > > > > > + BuildSerializationInstructionEntry wr_action = {
> > > > > > > + .table_data = table_instruction_data,
> > > > > > > + .bar = bar0,
> > > > > > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > + .flags = 0,
> > > > > > > + .register_bit_width = 32,
> > > > > > > + .register_offset = ERST_ACTION_OFFSET,
> > > > > > > + };
> > > > > >
> > > > > > We can probably write a macro to simply generating these structs. I see
> > > > > > .bar and .flags are the same in all structs. The .bit_width can be a param
> > > > > > into the macro etc.
> > > > > Agree, however the request was to NOT hide the use of local variables in
> > > > > macros, so while .flags is always 0, .instruction, .register_bit_width and .register_offset
> > > > > would be parameters, that leaves .table_data and .bar which just need the local
> > > > > variables. Is the following acceptable (which hides the use of the local variables)?
> > > >
> > > > You can just use assignment:
> > > >
> > > > BuildSerializationInstructionEntry entry = {
> > > > .table_data = table_instruction_data,
> > > > .bar = bar0,
> > > > .flags = 0,
> > > > .register_bit_width = 32,
> > > > };
> > > > BuildSerializationInstructionEntry rd_value_32_val = entry;
> > > > rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> > > > rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> > > >
> > > > and so on.
> > > > not sure it's worth it, it's just a couple of lines.
> > > >
> > >
> > > OK, here is the equivalent using struct assignment, is this what you were after?
> > >
> > > BuildSerializationInstructionEntry base = {
> > > .table_data = table_instruction_data,
> > > .bar = bar0,
> > > .instruction = INST_WRITE_REGISTER,
> > > .flags = 0,
> > > .register_bit_width = 32,
> > > .register_offset = ERST_VALUE_OFFSET,
> > > };
> > > BuildSerializationInstructionEntry rd_value_32_val = base;
> > > rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
> > > BuildSerializationInstructionEntry rd_value_32 = base;
> > > rd_value_32.instruction = INST_READ_REGISTER;
> > > BuildSerializationInstructionEntry rd_value_64 = base;
> > > rd_value_64.instruction = INST_READ_REGISTER;
> > > rd_value_64.register_bit_width = 64;
> > > BuildSerializationInstructionEntry wr_value_32_val = base;
> > > wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
> > > BuildSerializationInstructionEntry wr_value_32 = base;
> > > BuildSerializationInstructionEntry wr_value_64 = base;
> > > wr_value_64.register_bit_width = 64;
> > > BuildSerializationInstructionEntry wr_action = base;
> > > wr_action.instruction = INST_WRITE_REGISTER_VALUE;
> > > wr_action.register_offset = ERST_ACTION_OFFSET;
> > >
> >
> > That's what I described, yes. We should have some empty lines here I
> > guess. I'm ok with the original one too, there's not too much
> > duplication.
>
> Are the blank lines referring to spacing out the setup of each of the 7 accesors?
> If so, I could put a one line comment between each setup? Or is a blank line also
> needed?
A blank line between declarations and code is usually a good idea.
> Is it OK to post v15 with the struct assignment approach? Or would you prefer the
> explicit structs (which is what I think you mean by 'the original one')?
>
> Thanks!
> eric
I don't care either way.
> >
> >
> > >
> > > >
> > > >
> > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > > > > inst, bit_width, offset) \
> > > > > BuildSerializationInstructionEntry name = { \
> > > > > .table_data = table_instruction_data, \
> > > > > .bar = bar0, \
> > > > > .instruction = inst, \
> > > > > .flags = 0, \
> > > > > .register_bit_width = bit_width, \
> > > > > .register_offset = offset, \
> > > > > }
> > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > > > > INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > > > > INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > > > > INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > > > > INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > > > > INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > > > > INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > > > > INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > > > >
> > > > > These are the 7 accessors needed.
> > > >
> > > > not at all sure this one is worth the macro mess.
> > >
> > > I'm hoping to produce a v15 with the style you want.
> > > eric
> > >
> > > >
> > > > > >
> > > > > > > + unsigned action;
> > > > > > > +
> > > > > > > + trace_acpi_erst_pci_bar_0(bar0);
> > > > > > > +
> > > > > > > + /* Serialization Instruction Entries */
> > > > > > > + action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_BEGIN_READ_OPERATION;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_END_OPERATION;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_SET_RECORD_OFFSET;
> > > > > > > + build_serialization_instruction(&wr_value_32, action, 0);
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_EXECUTE_OPERATION;
> > > > > > > + build_serialization_instruction(&wr_value_32_val, action,
> > > > > > > + ERST_EXECUTE_OPERATION_MAGIC);
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_CHECK_BUSY_STATUS;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > > > > > +
> > > > > > > + action = ACTION_GET_COMMAND_STATUS;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > +
> > > > > > > + action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > + action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > > > + build_serialization_instruction(&wr_value_64, action, 0);
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_GET_RECORD_COUNT;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > +
> > > > > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > +
> > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > +
> > > > > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > +
> > > > > > > + /* Serialization Header */
> > > > > > > + acpi_table_begin(&table, table_data);
> > > > > > > +
> > > > > > > + /* Serialization Header Size */
> > > > > > > + build_append_int_noprefix(table_data, 48, 4);
> > > > > > > +
> > > > > > > + /* Reserved */
> > > > > > > + build_append_int_noprefix(table_data, 0, 4);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * Instruction Entry Count
> > > > > > > + * Each instruction entry is 32 bytes
> > > > > > > + */
> > > > > > > + g_assert((table_instruction_data->len) % 32 == 0);
> > > > > > > + build_append_int_noprefix(table_data,
> > > > > > > + (table_instruction_data->len / 32), 4);
> > > > > > > +
> > > > > > > + /* Serialization Instruction Entries */
> > > > > > > + g_array_append_vals(table_data, table_instruction_data->data,
> > > > > > > + table_instruction_data->len);
> > > > > > > + g_array_free(table_instruction_data, TRUE);
> > > > > > > +
> > > > > > > + acpi_table_end(linker, &table);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*******************************************************************/
> > > > > > > +/*******************************************************************/
> > > > > > > static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > > > > > > {
> > > > > > > uint8_t *rc = NULL;
> > > > > > > --
> > > > > > > 1.8.3.1
> > > > > > >
> > > > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 16:14 ` Michael S. Tsirkin
@ 2022-01-28 17:25 ` Ani Sinha
2022-01-28 18:18 ` Eric DeVolder
0 siblings, 1 reply; 21+ messages in thread
From: Ani Sinha @ 2022-01-28 17:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, imammedo,
boris.ostrovsky, Eric DeVolder, rth
[-- Attachment #1: Type: text/plain, Size: 20087 bytes --]
On Fri, Jan 28, 2022 at 9:44 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jan 28, 2022 at 10:01:18AM -0600, Eric DeVolder wrote:
> > Michael, thanks! See inline response below, please.
> > eric
> >
> > On 1/28/22 09:54, Michael S. Tsirkin wrote:
> > > On Fri, Jan 28, 2022 at 09:11:41AM -0600, Eric DeVolder wrote:
> > > > Michael,
> > > > Thanks for examining this. Inline response below.
> > > > eric
> > > >
> > > > On 1/27/22 18:37, Michael S. Tsirkin wrote:
> > > > > On Thu, Jan 27, 2022 at 04:02:07PM -0600, Eric DeVolder wrote:
> > > > > > Ani,
> > > > > > Thanks for the RB! Inline responses below.
> > > > > > eric
> > > > > >
> > > > > > On 1/27/22 02:36, Ani Sinha wrote:
> > > > > > >
> > > > > > >
> > > > > > > On Wed, 26 Jan 2022, Eric DeVolder wrote:
> > > > > > >
> > > > > > > > This builds the ACPI ERST table to inform OSPM how to
> communicate
> > > > > > > > with the acpi-erst device.
> > > > > > >
> > > > > > > There might be more optimizations possible but I think we have
> messaged
> > > > > > > this code enough. We can further rework the code if needed in
> subsequent
> > > > > > > patches once this is pushed.
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> > > > > > >
> > > > > > > with some minor comments,
> > > > > > >
> > > > > > > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > > > > > >
> > > > > > > > hw/acpi/erst.c | 225
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > 1 file changed, 225 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > > > > > > > index fe9ba51..5d5a639 100644
> > > > > > > > --- a/hw/acpi/erst.c
> > > > > > > > +++ b/hw/acpi/erst.c
> > > > > > > > @@ -59,6 +59,27 @@
> > > > > > > > #define STATUS_RECORD_STORE_EMPTY 0x04
> > > > > > > > #define STATUS_RECORD_NOT_FOUND 0x05
> > > > > > > >
> > > > > > > > +/* ACPI 4.0: Table 17-19 Serialization Instructions */
> > > > > > > > +#define INST_READ_REGISTER 0x00
> > > > > > > > +#define INST_READ_REGISTER_VALUE 0x01
> > > > > > > > +#define INST_WRITE_REGISTER 0x02
> > > > > > > > +#define INST_WRITE_REGISTER_VALUE 0x03
> > > > > > > > +#define INST_NOOP 0x04
> > > > > > > > +#define INST_LOAD_VAR1 0x05
> > > > > > > > +#define INST_LOAD_VAR2 0x06
> > > > > > > > +#define INST_STORE_VAR1 0x07
> > > > > > > > +#define INST_ADD 0x08
> > > > > > > > +#define INST_SUBTRACT 0x09
> > > > > > > > +#define INST_ADD_VALUE 0x0A
> > > > > > > > +#define INST_SUBTRACT_VALUE 0x0B
> > > > > > > > +#define INST_STALL 0x0C
> > > > > > > > +#define INST_STALL_WHILE_TRUE 0x0D
> > > > > > > > +#define INST_SKIP_NEXT_INSTRUCTION_IF_TRUE 0x0E
> > > > > > > > +#define INST_GOTO 0x0F
> > > > > > > > +#define INST_SET_SRC_ADDRESS_BASE 0x10
> > > > > > > > +#define INST_SET_DST_ADDRESS_BASE 0x11
> > > > > > > > +#define INST_MOVE_DATA 0x12
> > > > > > > > +
> > > > > > > > /* UEFI 2.1: Appendix N Common Platform Error Record */
> > > > > > > > #define UEFI_CPER_RECORD_MIN_SIZE 128U
> > > > > > > > #define UEFI_CPER_RECORD_LENGTH_OFFSET 20U
> > > > > > > > @@ -172,6 +193,210 @@ typedef struct {
> > > > > > > >
> > > > > > > >
> /*******************************************************************/
> > > > > > > >
> /*******************************************************************/
> > > > > > > > +typedef struct {
> > > > > > > > + GArray *table_data;
> > > > > > > > + pcibus_t bar;
> > > > > > > > + uint8_t instruction;
> > > > > > > > + uint8_t flags;
> > > > > > > > + uint8_t register_bit_width;
> > > > > > > > + pcibus_t register_offset;
> > > > > > > > +} BuildSerializationInstructionEntry;
> > > > > > > > +
> > > > > > > > +/* ACPI 4.0: 17.4.1.2 Serialization Instruction Entries */
> > > > > > > > +static void build_serialization_instruction(
> > > > > > > > + BuildSerializationInstructionEntry *e,
> > > > > > > > + uint8_t serialization_action,
> > > > > > > > + uint64_t value)
> > > > > > > > +{
> > > > > > > > + /* ACPI 4.0: Table 17-18 Serialization Instruction
> Entry */
> > > > > > > > + struct AcpiGenericAddress gas;
> > > > > > > > + uint64_t mask;
> > > > > > > > +
> > > > > > > > + /* Serialization Action */
> > > > > > > > + build_append_int_noprefix(e->table_data,
> serialization_action, 1);
> > > > > > > > + /* Instruction */
> > > > > > > > + build_append_int_noprefix(e->table_data,
> e->instruction, 1);
> > > > > > > > + /* Flags */
> > > > > > > > + build_append_int_noprefix(e->table_data, e->flags, 1);
> > > > > > > > + /* Reserved */
> > > > > > > > + build_append_int_noprefix(e->table_data, 0, 1);
> > > > > > > > + /* Register Region */
> > > > > > > > + gas.space_id = AML_SYSTEM_MEMORY;
> > > > > > > > + gas.bit_width = e->register_bit_width;
> > > > > > > > + gas.bit_offset = 0;
> > > > > > > > + gas.access_width = ctz32(e->register_bit_width) - 2;
> > > > > > >
> > > > > > > Should this be casted as unit8_t?
> > > > > > OK, done.
> > > > > >
> > > > > > >
> > > > > > > > + gas.address = (uint64_t)(e->bar + e->register_offset);
> > > > > > > > + build_append_gas_from_struct(e->table_data, &gas);
> > > > > > > > + /* Value */
> > > > > > > > + build_append_int_noprefix(e->table_data, value, 8);
> > > > > > > > + /* Mask */
> > > > > > > > + mask = (1ULL << (e->register_bit_width - 1) << 1) - 1;
> > > > > > > > + build_append_int_noprefix(e->table_data, mask, 8);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/* ACPI 4.0: 17.4.1 Serialization Action Table */
> > > > > > > > +void build_erst(GArray *table_data, BIOSLinker *linker,
> Object *erst_dev,
> > > > > > > > + const char *oem_id, const char *oem_table_id)
> > > > > > > > +{
> > > > > > > > + /*
> > > > > > > > + * Serialization Action Table
> > > > > > > > + * The serialization action table must be generated
> first
> > > > > > > > + * so that its size can be known in order to populate
> the
> > > > > > > > + * Instruction Entry Count field.
> > > > > > > > + */
> > > > > > > > + GArray *table_instruction_data = g_array_new(FALSE,
> FALSE, sizeof(char));
> > > > > > > > + pcibus_t bar0 = pci_get_bar_addr(PCI_DEVICE(erst_dev),
> 0);
> > > > > > > > + AcpiTable table = { .sig = "ERST", .rev = 1, .oem_id =
> oem_id,
> > > > > > > > + .oem_table_id = oem_table_id };
> > > > > > > > + /* Contexts for the different ways ACTION and VALUE are
> accessed */
> > > > > > > > + BuildSerializationInstructionEntry rd_value_32_val = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_READ_REGISTER_VALUE,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 32,
> > > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > + };
> > > > > > > > + BuildSerializationInstructionEntry rd_value_32 = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_READ_REGISTER,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 32,
> > > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > + };
> > > > > > > > + BuildSerializationInstructionEntry rd_value_64 = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_READ_REGISTER,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 64,
> > > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > + };
> > > > > > > > + BuildSerializationInstructionEntry wr_value_32_val = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 32,
> > > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > + };
> > > > > > > > + BuildSerializationInstructionEntry wr_value_32 = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_WRITE_REGISTER,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 32,
> > > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > + };
> > > > > > > > + BuildSerializationInstructionEntry wr_value_64 = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_WRITE_REGISTER,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 64,
> > > > > > > > + .register_offset = ERST_VALUE_OFFSET,
> > > > > > > > + };
> > > > > > > > + BuildSerializationInstructionEntry wr_action = {
> > > > > > > > + .table_data = table_instruction_data,
> > > > > > > > + .bar = bar0,
> > > > > > > > + .instruction = INST_WRITE_REGISTER_VALUE,
> > > > > > > > + .flags = 0,
> > > > > > > > + .register_bit_width = 32,
> > > > > > > > + .register_offset = ERST_ACTION_OFFSET,
> > > > > > > > + };
> > > > > > >
> > > > > > > We can probably write a macro to simply generating these
> structs. I see
> > > > > > > .bar and .flags are the same in all structs. The .bit_width
> can be a param
> > > > > > > into the macro etc.
> > > > > > Agree, however the request was to NOT hide the use of local
> variables in
> > > > > > macros, so while .flags is always 0, .instruction,
> .register_bit_width and .register_offset
> > > > > > would be parameters, that leaves .table_data and .bar which just
> need the local
> > > > > > variables. Is the following acceptable (which hides the use of
> the local variables)?
> > > > >
> > > > > You can just use assignment:
> > > > >
> > > > > BuildSerializationInstructionEntry entry = {
> > > > > .table_data = table_instruction_data,
> > > > > .bar = bar0,
> > > > > .flags = 0,
> > > > > .register_bit_width = 32,
> > > > > };
> > > > > BuildSerializationInstructionEntry rd_value_32_val = entry;
> > > > > rd_value_32_val.action = INST_READ_REGISTER_VALUE;
> > > > > rd_value_32_val.register_offset = ERST_ACTION_OFFSET;
> > > > >
> > > > > and so on.
> > > > > not sure it's worth it, it's just a couple of lines.
> > > > >
> > > >
> > > > OK, here is the equivalent using struct assignment, is this what you
> were after?
> > > >
> > > > BuildSerializationInstructionEntry base = {
> > > > .table_data = table_instruction_data,
> > > > .bar = bar0,
> > > > .instruction = INST_WRITE_REGISTER,
> > > > .flags = 0,
> > > > .register_bit_width = 32,
> > > > .register_offset = ERST_VALUE_OFFSET,
> > > > };
> > > > BuildSerializationInstructionEntry rd_value_32_val = base;
> > > > rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
> > > > BuildSerializationInstructionEntry rd_value_32 = base;
> > > > rd_value_32.instruction = INST_READ_REGISTER;
> > > > BuildSerializationInstructionEntry rd_value_64 = base;
> > > > rd_value_64.instruction = INST_READ_REGISTER;
> > > > rd_value_64.register_bit_width = 64;
> > > > BuildSerializationInstructionEntry wr_value_32_val = base;
> > > > wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
> > > > BuildSerializationInstructionEntry wr_value_32 = base;
> > > > BuildSerializationInstructionEntry wr_value_64 = base;
> > > > wr_value_64.register_bit_width = 64;
> > > > BuildSerializationInstructionEntry wr_action = base;
> > > > wr_action.instruction = INST_WRITE_REGISTER_VALUE;
> > > > wr_action.register_offset = ERST_ACTION_OFFSET;
> > > >
> > >
> > > That's what I described, yes. We should have some empty lines here I
> > > guess. I'm ok with the original one too, there's not too much
> > > duplication.
> >
> > Are the blank lines referring to spacing out the setup of each of the 7
> accesors?
> > If so, I could put a one line comment between each setup? Or is a blank
> line also
> > needed?
>
> A blank line between declarations and code is usually a good idea.
>
>
> > Is it OK to post v15 with the struct assignment approach? Or would you
> prefer the
> > explicit structs (which is what I think you mean by 'the original one')?
I prefer the explicit structs as you had posted before.
> >
> > Thanks!
> > eric
>
> I don't care either way.
>
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > > > > > inst, bit_width, offset) \
> > > > > > BuildSerializationInstructionEntry name = { \
> > > > > > .table_data = table_instruction_data, \
> > > > > > .bar = bar0, \
> > > > > > .instruction = inst, \
> > > > > > .flags = 0, \
> > > > > > .register_bit_width = bit_width, \
> > > > > > .register_offset = offset, \
> > > > > > }
> > > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > > > > > INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > > > > > INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > > > > > INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > > > > > INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > > > > > INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > > > > > INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > > > > > INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > > > > >
> > > > > > These are the 7 accessors needed.
> > > > >
> > > > > not at all sure this one is worth the macro mess.
> > > >
> > > > I'm hoping to produce a v15 with the style you want.
> > > > eric
> > > >
> > > > >
> > > > > > >
> > > > > > > > + unsigned action;
> > > > > > > > +
> > > > > > > > + trace_acpi_erst_pci_bar_0(bar0);
> > > > > > > > +
> > > > > > > > + /* Serialization Instruction Entries */
> > > > > > > > + action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_BEGIN_READ_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_END_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_SET_RECORD_OFFSET;
> > > > > > > > + build_serialization_instruction(&wr_value_32, action,
> 0);
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_EXECUTE_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_value_32_val,
> action,
> > > > > > > > + ERST_EXECUTE_OPERATION_MAGIC);
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_CHECK_BUSY_STATUS;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_32_val,
> action, 0x01);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_COMMAND_STATUS;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_32, action,
> 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > > > > + build_serialization_instruction(&wr_value_64, action,
> 0);
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_RECORD_COUNT;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_32, action,
> 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_32, action,
> 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > > > > + build_serialization_instruction(&wr_action, action,
> action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action,
> 0);
> > > > > > > > +
> > > > > > > > + /* Serialization Header */
> > > > > > > > + acpi_table_begin(&table, table_data);
> > > > > > > > +
> > > > > > > > + /* Serialization Header Size */
> > > > > > > > + build_append_int_noprefix(table_data, 48, 4);
> > > > > > > > +
> > > > > > > > + /* Reserved */
> > > > > > > > + build_append_int_noprefix(table_data, 0, 4);
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * Instruction Entry Count
> > > > > > > > + * Each instruction entry is 32 bytes
> > > > > > > > + */
> > > > > > > > + g_assert((table_instruction_data->len) % 32 == 0);
> > > > > > > > + build_append_int_noprefix(table_data,
> > > > > > > > + (table_instruction_data->len / 32), 4);
> > > > > > > > +
> > > > > > > > + /* Serialization Instruction Entries */
> > > > > > > > + g_array_append_vals(table_data,
> table_instruction_data->data,
> > > > > > > > + table_instruction_data->len);
> > > > > > > > + g_array_free(table_instruction_data, TRUE);
> > > > > > > > +
> > > > > > > > + acpi_table_end(linker, &table);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >
> +/*******************************************************************/
> > > > > > > >
> +/*******************************************************************/
> > > > > > > > static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState
> *s, unsigned index)
> > > > > > > > {
> > > > > > > > uint8_t *rc = NULL;
> > > > > > > > --
> > > > > > > > 1.8.3.1
> > > > > > > >
> > > > > > > >
> > > > >
> > >
>
>
[-- Attachment #2: Type: text/html, Size: 30044 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table
2022-01-28 17:25 ` Ani Sinha
@ 2022-01-28 18:18 ` Eric DeVolder
0 siblings, 0 replies; 21+ messages in thread
From: Eric DeVolder @ 2022-01-28 18:18 UTC (permalink / raw)
To: Ani Sinha, Michael S. Tsirkin
Cc: berrange, ehabkost, konrad.wilk, qemu-devel, pbonzini, imammedo,
boris.ostrovsky, rth
On 1/28/22 11:25, Ani Sinha wrote:
>
> [snip]
> On Fri, Jan 28, 2022 at 9:44 PM Michael S. Tsirkin <mst@redhat.com <mailto:mst@redhat.com>> wrote:
>
> > > > OK, here is the equivalent using struct assignment, is this what you were after?
> > > >
> > > > BuildSerializationInstructionEntry base = {
> > > > .table_data = table_instruction_data,
> > > > .bar = bar0,
> > > > .instruction = INST_WRITE_REGISTER,
> > > > .flags = 0,
> > > > .register_bit_width = 32,
> > > > .register_offset = ERST_VALUE_OFFSET,
> > > > };
> > > > BuildSerializationInstructionEntry rd_value_32_val = base;
> > > > rd_value_32_val.instruction = INST_READ_REGISTER_VALUE;
> > > > BuildSerializationInstructionEntry rd_value_32 = base;
> > > > rd_value_32.instruction = INST_READ_REGISTER;
> > > > BuildSerializationInstructionEntry rd_value_64 = base;
> > > > rd_value_64.instruction = INST_READ_REGISTER;
> > > > rd_value_64.register_bit_width = 64;
> > > > BuildSerializationInstructionEntry wr_value_32_val = base;
> > > > wr_value_32_val.instruction = INST_WRITE_REGISTER_VALUE;
> > > > BuildSerializationInstructionEntry wr_value_32 = base;
> > > > BuildSerializationInstructionEntry wr_value_64 = base;
> > > > wr_value_64.register_bit_width = 64;
> > > > BuildSerializationInstructionEntry wr_action = base;
> > > > wr_action.instruction = INST_WRITE_REGISTER_VALUE;
> > > > wr_action.register_offset = ERST_ACTION_OFFSET;
> > > >
> > >
> > > That's what I described, yes. We should have some empty lines here I
> > > guess. I'm ok with the original one too, there's not too much
> > > duplication.
> >
> > Are the blank lines referring to spacing out the setup of each of the 7 accesors?
> > If so, I could put a one line comment between each setup? Or is a blank line also
> > needed?
>
> A blank line between declarations and code is usually a good idea.
>
>
> > Is it OK to post v15 with the struct assignment approach? Or would you prefer the
> > explicit structs (which is what I think you mean by 'the original one')?
>
>
> I prefer the explicit structs as you had posted before.
Ok, as Michael does not have a preference, so let's go with your preference of the
explicit structs!
Thank you!
eric
>
>
> >
> > Thanks!
> > eric
>
> I don't care either way.
>
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > #define SERIALIZATIONINSTRUCTIONCTX(name, \
> > > > > > inst, bit_width, offset) \
> > > > > > BuildSerializationInstructionEntry name = { \
> > > > > > .table_data = table_instruction_data, \
> > > > > > .bar = bar0, \
> > > > > > .instruction = inst, \
> > > > > > .flags = 0, \
> > > > > > .register_bit_width = bit_width, \
> > > > > > .register_offset = offset, \
> > > > > > }
> > > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32_val,
> > > > > > INST_READ_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_32,
> > > > > > INST_READ_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(rd_value_64,
> > > > > > INST_READ_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32_val,
> > > > > > INST_WRITE_REGISTER_VALUE, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_32,
> > > > > > INST_WRITE_REGISTER, 32, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_value_64,
> > > > > > INST_WRITE_REGISTER, 64, ERST_VALUE_OFFSET);
> > > > > > SERIALIZATIONINSTRUCTIONCTX(wr_action,
> > > > > > INST_WRITE_REGISTER_VALUE, 32, ERST_ACTION_OFFSET);
> > > > > >
> > > > > > These are the 7 accessors needed.
> > > > >
> > > > > not at all sure this one is worth the macro mess.
> > > >
> > > > I'm hoping to produce a v15 with the style you want.
> > > > eric
> > > >
> > > > >
> > > > > > >
> > > > > > > > + unsigned action;
> > > > > > > > +
> > > > > > > > + trace_acpi_erst_pci_bar_0(bar0);
> > > > > > > > +
> > > > > > > > + /* Serialization Instruction Entries */
> > > > > > > > + action = ACTION_BEGIN_WRITE_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_BEGIN_READ_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_BEGIN_CLEAR_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_END_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_SET_RECORD_OFFSET;
> > > > > > > > + build_serialization_instruction(&wr_value_32, action, 0);
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_EXECUTE_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_value_32_val, action,
> > > > > > > > + ERST_EXECUTE_OPERATION_MAGIC);
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_CHECK_BUSY_STATUS;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_32_val, action, 0x01);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_COMMAND_STATUS;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_RECORD_IDENTIFIER;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_SET_RECORD_IDENTIFIER;
> > > > > > > > + build_serialization_instruction(&wr_value_64, action, 0);
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_RECORD_COUNT;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_BEGIN_DUMMY_WRITE_OPERATION;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_LENGTH;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_ERROR_LOG_ADDRESS_RANGE_ATTRIBUTES;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_32, action, 0);
> > > > > > > > +
> > > > > > > > + action = ACTION_GET_EXECUTE_OPERATION_TIMINGS;
> > > > > > > > + build_serialization_instruction(&wr_action, action, action);
> > > > > > > > + build_serialization_instruction(&rd_value_64, action, 0);
> > > > > > > > +
> > > > > > > > + /* Serialization Header */
> > > > > > > > + acpi_table_begin(&table, table_data);
> > > > > > > > +
> > > > > > > > + /* Serialization Header Size */
> > > > > > > > + build_append_int_noprefix(table_data, 48, 4);
> > > > > > > > +
> > > > > > > > + /* Reserved */
> > > > > > > > + build_append_int_noprefix(table_data, 0, 4);
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * Instruction Entry Count
> > > > > > > > + * Each instruction entry is 32 bytes
> > > > > > > > + */
> > > > > > > > + g_assert((table_instruction_data->len) % 32 == 0);
> > > > > > > > + build_append_int_noprefix(table_data,
> > > > > > > > + (table_instruction_data->len / 32), 4);
> > > > > > > > +
> > > > > > > > + /* Serialization Instruction Entries */
> > > > > > > > + g_array_append_vals(table_data, table_instruction_data->data,
> > > > > > > > + table_instruction_data->len);
> > > > > > > > + g_array_free(table_instruction_data, TRUE);
> > > > > > > > +
> > > > > > > > + acpi_table_end(linker, &table);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*******************************************************************/
> > > > > > > > +/*******************************************************************/
> > > > > > > > static uint8_t *get_nvram_ptr_by_index(ERSTDeviceState *s, unsigned index)
> > > > > > > > {
> > > > > > > > uint8_t *rc = NULL;
> > > > > > > > --
> > > > > > > > 1.8.3.1
> > > > > > > >
> > > > > > > >
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-01-28 18:27 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-26 16:28 [PATCH v14 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 01/10] ACPI ERST: bios-tables-test.c steps 1 and 2 Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 02/10] ACPI ERST: specification for ERST support Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 03/10] ACPI ERST: PCI device_id for ERST Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 04/10] ACPI ERST: header file " Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 05/10] ACPI ERST: support for ACPI ERST feature Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 06/10] ACPI ERST: build the ACPI ERST table Eric DeVolder
2022-01-27 8:36 ` Ani Sinha
2022-01-27 22:02 ` Eric DeVolder
2022-01-28 0:37 ` Michael S. Tsirkin
2022-01-28 9:01 ` Ani Sinha
2022-01-28 15:11 ` Eric DeVolder
2022-01-28 15:54 ` Michael S. Tsirkin
2022-01-28 16:01 ` Eric DeVolder
2022-01-28 16:14 ` Michael S. Tsirkin
2022-01-28 17:25 ` Ani Sinha
2022-01-28 18:18 ` Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 07/10] ACPI ERST: create ACPI ERST table for pc/x86 machines Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 08/10] ACPI ERST: qtest for ERST Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 09/10] ACPI ERST: bios-tables-test testcase Eric DeVolder
2022-01-26 16:28 ` [PATCH v14 10/10] ACPI ERST: step 6 of bios-tables-test.c Eric DeVolder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.