Re: [PATCH v5 00/10] acpi: Error Record Serialization Table, ERST, support for QEMU

[Michael S. Tsirkin]

On Wed, Jun 30, 2021 at 03:07:11PM -0400, Eric DeVolder wrote:
> =============================
> I believe I have corrected for all feedback on v4, but with
> responses to certain feedback below.
> 
> In patch 1/6, Igor asks:
> "you are adding empty template files here
> but the later matching bios-tables-test is nowhere to be found
> Was testcase lost somewhere along the way?
> 
> also it seems you add ERST only to pc/q35,
> so why tests/data/acpi/microvm/ERST is here?"
> 
> I did miss setting up microvm. That has been corrected.
> 
> As for the question about lost test cases, if you are referring
> to the new binary blobs for pc,q35, those were in patch
> 6/6. There is a qtest in patch 5/6. If I don't understand the
> question, please indicate as such.
> 
> 
> In patch 3/6, Igor asks:
> "Also spec (ERST) is rather (maybe intentionally) vague on specifics,
> so it would be better that before a patch that implements hw part
> were a doc patch describing concrete implementation. As model
> you can use docs/specs/acpi_hest_ghes.rst or other docs/specs/acpi_* files.
> I'd start posting/discussing that spec within these thread
> to avoid spamming list until doc is settled up."
> 
> I'm thinking that this cover letter is the bulk of the spec? But as
> you say, to avoid spamming the group, we can use this thread to make
> suggested changes to this cover letter which I will then convert
> into a spec, for v6.
> 
> 
> In patch 3/6, in many places Igor mentions utilizing the hostmem
> mapped directly in the guest in order to avoid need-less copying.
> 
> It is true that the ERST has an "NVRAM" mode that would allow for
> all the simplifications Igor points out, however, Linux does not
> support this mode. This mode puts the burden of managing the NVRAM
> space on the OS. So this implementation, like BIOS, is the non-NVRAM
> mode.
> 
> I did go ahead and separate the registers from the exchange buffer,
> which would facilitate the support of NVRAM mode.
> 
>  linux/drivers/acpi/apei/erst.c:
>  /* NVRAM ERST Error Log Address Range is not supported yet */
>  static void pr_unimpl_nvram(void)
>  {
>     if (printk_ratelimit())
>         pr_warn("NVRAM ERST Log Address Range not implemented yet.\n");
>  }
> 
>  static int __erst_write_to_nvram(const struct cper_record_header *record)
>  {
>     /* do not print message, because printk is not safe for NMI */
>     return -ENOSYS;
>  }
> 
>  static int __erst_read_to_erange_from_nvram(u64 record_id, u64 *offset)
>  {
>     pr_unimpl_nvram();
>     return -ENOSYS;
>  }
> 
>  static int __erst_clear_from_nvram(u64 record_id)
>  {
>     pr_unimpl_nvram();
>     return -ENOSYS;
>  }
> 
> =============================
> 
> This patchset introduces support for the ACPI Error Record
> Serialization Table, ERST.
> 
> For background and implementation information, please see
> docs/specs/acpi_erst.txt, which is patch 2/10.
> 
> Suggested-by: Konrad Wilk <konrad.wilk@oracle.com>
> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>


../hw/acpi/erst.c: In function ‘build_erst’:
../hw/acpi/erst.c:754:13: error: this statement may fall through 
[-Werror=implicit-fallthrough=]
  754 |             build_serialization_instruction_entry(table_data, action,
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  755 |                 ACPI_ERST_INST_READ_REGISTER       , 0, 64,
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  756 |                 s->bar0 + ERST_CSR_VALUE, 0, MASK64);
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../hw/acpi/erst.c:757:9: note: here
  757 |         default:
      |         ^~~~~~~
cc1: all warnings being treated as errors


Pls correct.
mingw32 build also failed. Pls take a look.


Thanks!


> ---
> 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: trace support
>   ACPI ERST: create ACPI ERST table for pc/x86 machines.
>   ACPI ERST: qtest for ERST
>   ACPI ERST: step 6 of bios-tables-test.c
> 
>  docs/specs/acpi_erst.txt     | 152 +++++++
>  hw/acpi/erst.c               | 918 
> +++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/meson.build          |   1 +
>  hw/acpi/trace-events         |  14 +
>  hw/i386/acpi-build.c         |   9 +
>  hw/i386/acpi-microvm.c       |   9 +
>  include/hw/acpi/erst.h       |  84 ++++
>  include/hw/pci/pci.h         |   1 +
>  tests/data/acpi/microvm/ERST | Bin 0 -> 976 bytes
>  tests/data/acpi/pc/ERST      | Bin 0 -> 976 bytes
>  tests/data/acpi/q35/ERST     | Bin 0 -> 976 bytes
>  tests/qtest/erst-test.c      | 129 ++++++
>  tests/qtest/meson.build      |   2 +
>  13 files changed, 1319 insertions(+)
>  create mode 100644 docs/specs/acpi_erst.txt
>  create mode 100644 hw/acpi/erst.c
>  create mode 100644 include/hw/acpi/erst.h
>  create mode 100644 tests/data/acpi/microvm/ERST
>  create mode 100644 tests/data/acpi/pc/ERST
>  create mode 100644 tests/data/acpi/q35/ERST
>  create mode 100644 tests/qtest/erst-test.c
> 
> -- 
> 1.8.3.1