mbox series

[0/1] acpi: Implement ACPI ERST support for guests

Message ID 1603743573-9870-1-git-send-email-eric.devolder@oracle.com
Headers show
Series acpi: Implement ACPI ERST support for guests | expand

Message

Eric DeVolder Oct. 26, 2020, 8:19 p.m. UTC 
This changeset introduces support for the ACPI Error Record
Serialization Table, ERST.

The change to hw/acpi/meson.build simply adds in the new .c file
for compilation.

The change to hw/i386/acpi-build.c calls out the building of the
ERST table (and also creates the associated device).

The new file hw/acpi/erst.c contains the building of the ERST
table, as well as the simple device for exchanging error records.

The new file include/hw/acpi/erst.h contains associated definitions
and declarations for ERST.

The primary description of this changeset is in the patch commit
message.

NOTES: When reviewing, I would especially appreciate feedback
on the following topics:

- The hope is to have ERST always present if ACPI is enabled, however,
  I have found it difficult to devise a method for passing the base
  address that does not require the workaround at the bottom of
  build_erst(). The issues I encountered are:
  - desire to keep this is common ACPI code
  - the device requires a qdev_new(), this needs to happen early,
    thus the workaround in build_erst()
  - the base address is machine/arch specific (eg ARM vs x86)
  I've not found a nice way to thread this needle, so what I've settled
  on is to simply lump ERST on to the CONFIG_ACPI (rather than a
  separate CONFIG_ACPI_ERST), and the workaround at the bottom of
  build_erst(). I suspect there is a better way for a built-in/
  always present device. This does not support "-device acpi-erst,...".

- I found a base address that "worked", but would like an address
  that would be known to be availabe, and then to document/reserve
  it for ERST. This takes into account that the base address can be
  different for x86 vs ARM.

- I've run this through checkpatch, and all issues addressed except
  for the long lines in build_erst(). For readable I left the long
  lines, but will change if asked.

- What else do I need to provide?

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>

---
 hw/acpi/erst.c         | 909 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/meson.build    |   1 +
 hw/i386/acpi-build.c   |   4 +
 include/hw/acpi/erst.h |  97 ++++++
 4 files changed, 1011 insertions(+)
 create mode 100644 hw/acpi/erst.c
 create mode 100644 include/hw/acpi/erst.h

Comments

no-reply@patchew.org Oct. 26, 2020, 8:54 p.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/1603743573-9870-1-git-send-email-eric.devolder@oracle.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1603743573-9870-1-git-send-email-eric.devolder@oracle.com
Subject: [PATCH 0/1] acpi: Implement ACPI ERST support for guests

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1603743573-9870-1-git-send-email-eric.devolder@oracle.com -> patchew/1603743573-9870-1-git-send-email-eric.devolder@oracle.com
 - [tag update]      patchew/20201026105504.4023620-1-philmd@redhat.com -> patchew/20201026105504.4023620-1-philmd@redhat.com
Switched to a new branch 'test'
94c25ac acpi: Implement ACPI ERST support for guests

=== OUTPUT BEGIN ===
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#48: 
new file mode 100644

WARNING: line over 80 characters
#736: FILE: hw/acpi/erst.c:684:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#739: FILE: hw/acpi/erst.c:687:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#742: FILE: hw/acpi/erst.c:690:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#745: FILE: hw/acpi/erst.c:693:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#748: FILE: hw/acpi/erst.c:696:
+            insns += BEA(WRITE_REGISTER      , 0, 32, ERST_CSR_VALUE , 0, MASK32);

WARNING: line over 80 characters
#749: FILE: hw/acpi/erst.c:697:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

ERROR: line over 90 characters
#752: FILE: hw/acpi/erst.c:700:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_VALUE , ERST_EXECUTE_OPERATION_MAGIC, MASK8);

WARNING: line over 80 characters
#753: FILE: hw/acpi/erst.c:701:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#756: FILE: hw/acpi/erst.c:704:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#757: FILE: hw/acpi/erst.c:705:
+            insns += BEA(READ_REGISTER_VALUE , 0, 32, ERST_CSR_VALUE, 0x01, MASK8);

WARNING: line over 80 characters
#760: FILE: hw/acpi/erst.c:708:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#764: FILE: hw/acpi/erst.c:712:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#765: FILE: hw/acpi/erst.c:713:
+            insns += BEA(READ_REGISTER       , 0, 64, ERST_CSR_VALUE, 0, MASK64);

WARNING: line over 80 characters
#768: FILE: hw/acpi/erst.c:716:
+            insns += BEA(WRITE_REGISTER      , 0, 64, ERST_CSR_VALUE , 0, MASK64);

WARNING: line over 80 characters
#769: FILE: hw/acpi/erst.c:717:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#772: FILE: hw/acpi/erst.c:720:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#773: FILE: hw/acpi/erst.c:721:
+            insns += BEA(READ_REGISTER       , 0, 32, ERST_CSR_VALUE, 0, MASK32);

WARNING: line over 80 characters
#776: FILE: hw/acpi/erst.c:724:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#779: FILE: hw/acpi/erst.c:727:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#782: FILE: hw/acpi/erst.c:730:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#783: FILE: hw/acpi/erst.c:731:
+            insns += BEA(READ_REGISTER       , 0, 64, ERST_CSR_VALUE, 0, MASK64);

WARNING: line over 80 characters
#786: FILE: hw/acpi/erst.c:734:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#787: FILE: hw/acpi/erst.c:735:
+            insns += BEA(READ_REGISTER       , 0, 64, ERST_CSR_VALUE, 0, MASK32);

WARNING: line over 80 characters
#790: FILE: hw/acpi/erst.c:738:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#791: FILE: hw/acpi/erst.c:739:
+            insns += BEA(READ_REGISTER       , 0, 32, ERST_CSR_VALUE, 0, MASK32);

WARNING: line over 80 characters
#794: FILE: hw/acpi/erst.c:742:
+            insns += BEA(WRITE_REGISTER_VALUE, 0, 32, ERST_CSR_ACTION, action, MASK8);

WARNING: line over 80 characters
#795: FILE: hw/acpi/erst.c:743:
+            insns += BEA(READ_REGISTER       , 0, 64, ERST_CSR_VALUE, 0, MASK64);

WARNING: line over 80 characters
#1007: FILE: include/hw/acpi/erst.h:6:
+ * See ACPI specification, "ACPI Platform Error Interfaces" : "Error Serialization"

WARNING: line over 80 characters
#1068: FILE: include/hw/acpi/erst.h:67:
+#define ACPI_ERST_MAX_ACTIONS (ACPI_ERST_ACTION_GET_EXECUTE_OPERATION_TIMINGS + 1)

total: 1 errors, 29 warnings, 1029 lines checked

Commit 94c25acdc0f1 (acpi: Implement ACPI ERST support for guests) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/1603743573-9870-1-git-send-email-eric.devolder@oracle.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Igor Mammedov Nov. 3, 2020, 2:57 p.m. UTC | #2 
On Mon, 26 Oct 2020 16:19:32 -0400
Eric DeVolder <eric.devolder@oracle.com> wrote:

> This changeset introduces support for the ACPI Error Record
> Serialization Table, ERST.
> 
> The change to hw/acpi/meson.build simply adds in the new .c file
> for compilation.
> 
> The change to hw/i386/acpi-build.c calls out the building of the
> ERST table (and also creates the associated device).
> 
> The new file hw/acpi/erst.c contains the building of the ERST
> table, as well as the simple device for exchanging error records.
> 
> The new file include/hw/acpi/erst.h contains associated definitions
> and declarations for ERST.
> 
> The primary description of this changeset is in the patch commit
> message.
> 
> NOTES: When reviewing, I would especially appreciate feedback
> on the following topics:
> 
> - The hope is to have ERST always present if ACPI is enabled, however,
>   I have found it difficult to devise a method for passing the base
>   address that does not require the workaround at the bottom of
>   build_erst(). The issues I encountered are:
>   - desire to keep this is common ACPI code
>   - the device requires a qdev_new(), this needs to happen early,
>     thus the workaround in build_erst()
>   - the base address is machine/arch specific (eg ARM vs x86)
>   I've not found a nice way to thread this needle, so what I've settled
>   on is to simply lump ERST on to the CONFIG_ACPI (rather than a
>   separate CONFIG_ACPI_ERST), and the workaround at the bottom of
>   build_erst(). I suspect there is a better way for a built-in/
>   always present device. This does not support "-device acpi-erst,...".
> 
> - I found a base address that "worked", but would like an address
>   that would be known to be availabe, and then to document/reserve
>   it for ERST. This takes into account that the base address can be
>   different for x86 vs ARM.
> 
> - I've run this through checkpatch, and all issues addressed except
>   for the long lines in build_erst(). For readable I left the long
>   lines, but will change if asked.
> 
> - What else do I need to provide?

For now, I have just several generic comments:

1. that's quite a lot code to maintain, why not use existing UEFI vars
   as pstore storage instead? 
   Not sure ancient ACPI table is a way to go, with NVDIMMs around
   it probably possible to use pstores ram backend or make it work
   with nvdimms directly. The only benefit of ERST is that it should
   just work without extra configuration, but then UEFI backend
   would probably also just work.

2. patch is too big to review, please split it up in smaller chunks.

3. Use of packed structures is discouraged in new ACPI code,
   see build_ghes_v2() as an example for building ACPI tables.

4. Maybe instead of SYSBUS device, implement it as a PCI device and
   use its BAR/control registers for pstore storage and control interface.
   It could save you headache of picking address where to map it +
   it would take care of migration part automatically, as firmware
   would do it for you and then QEMU could pickup firmware programmed
   address and put it into ERST table.

5. instead of dealing with file for storage directly, reuse hostmem backend
   to provide it to for your device. ex: pc-dimm. i.e. split device
   on frontend and backend


> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> 
> ---
>  hw/acpi/erst.c         | 909 +++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/meson.build    |   1 +
>  hw/i386/acpi-build.c   |   4 +
>  include/hw/acpi/erst.h |  97 ++++++
>  4 files changed, 1011 insertions(+)
>  create mode 100644 hw/acpi/erst.c
>  create mode 100644 include/hw/acpi/erst.h
>
Eric DeVolder Feb. 8, 2021, 9:08 p.m. UTC | #3 
Paolo,
Thanks for the feedback. I've posted v2 with changes based on Igor's feedback.
I've also included a qtest per your feedback.
Eric