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

[Igor Mammedov]

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
>