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

[Eric DeVolder <eric.devolder@oracle.com>]

On 7/13/21 3:38 PM, Michael S. Tsirkin wrote:
> 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.

I've corrected the above build error.
I've also corrected the mingw32 build error.
eric

> 
> 
> 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
>