| Message ID | 1625080041-29010-1-git-send-email-eric.devolder@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | acpi: Error Record Serialization Table, ERST, support for QEMU | expand |
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
On Wed, 30 Jun 2021 15:07:11 -0400 Eric DeVolder <eric.devolder@oracle.com> 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. All I see in this series is [PATCH v5 09/10] ACPI ERST: qtest for ERST which is not related to bios-tables-test and blobs whatsoever. Blobs are for use with bios-tables-test and I'm referring to missing test case in bios-tables-test.c > > > 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. see per patch comments where copying is not necessary regardless of the implemented 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; > } > > ============================= PS: it's inconvenient when you copy questions/parts of unfinished discussion from previous revision with a little context. Usually discussion should continue in the original thread and once some sort of consensus is reached new series based on it is posted. Above blob shouldn't be here. (You can look at how others handle multiple revisions) The way you do it now, makes reviewer to repeat job done earlier to point to the the same issues, so it wastes your and reviewer's time. So please finish discussions in threads they started at and then post new revision. > 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> > > --- > 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 >
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 >
On 7/20/21 9:57 AM, Igor Mammedov wrote: > On Wed, 30 Jun 2021 15:07:11 -0400 > Eric DeVolder <eric.devolder@oracle.com> 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. > > All I see in this series is > [PATCH v5 09/10] ACPI ERST: qtest for ERST > which is not related to bios-tables-test and blobs whatsoever. > > Blobs are for use with bios-tables-test and I'm referring to > missing test case in bios-tables-test.c I now understand that "missing test case" are in bios-tables-test.c. I've got those implemented, though I'm debugging a problem with the microvm version. > >> >> >> 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. > see per patch comments where copying is not necessary regardless of > the implemented 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; >> } >> >> ============================= > PS: > it's inconvenient when you copy questions/parts of unfinished discussion > from previous revision with a little context. > Usually discussion should continue in the original thread and > once some sort of consensus is reached new series based on it > is posted. Above blob shouldn't be here. (You can look at how others > handle multiple revisions) > > The way you do it now, makes reviewer to repeat job done earlier > to point to the the same issues, so it wastes your and reviewer's time. > So please finish discussions in threads they started at and then post > new revision. Yes, I apologize. The email tool I was using did not handle threads very well. I've switched email tools and I'm able to respond to each thread much better now. > >> 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> >> >> --- >> 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 >> >
Igor, Pending your responses to a few questions, I have v6 ready to go. Thanks, eric On 7/20/21 9:57 AM, Igor Mammedov wrote: > On Wed, 30 Jun 2021 15:07:11 -0400 > Eric DeVolder <eric.devolder@oracle.com> 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. > > All I see in this series is > [PATCH v5 09/10] ACPI ERST: qtest for ERST > which is not related to bios-tables-test and blobs whatsoever. > > Blobs are for use with bios-tables-test and I'm referring to > missing test case in bios-tables-test.c > >> >> >> 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. > see per patch comments where copying is not necessary regardless of > the implemented 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; >> } >> >> ============================= > PS: > it's inconvenient when you copy questions/parts of unfinished discussion > from previous revision with a little context. > Usually discussion should continue in the original thread and > once some sort of consensus is reached new series based on it > is posted. Above blob shouldn't be here. (You can look at how others > handle multiple revisions) > > The way you do it now, makes reviewer to repeat job done earlier > to point to the the same issues, so it wastes your and reviewer's time. > So please finish discussions in threads they started at and then post > new revision. > >> 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> >> >> --- >> 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 >> >
PS: If I haven't said it already, use checkpatch script before posting patches.
On 7/27/21 7:55 AM, Igor Mammedov wrote: > PS: > If I haven't said it already, use checkpatch script before posting patches. > I do run checkpatch. On occasion I allow a warning about a line too long. And there is the MAINTAINERs message due to the new files. Is there something else that I'm missing?
On Wed, 28 Jul 2021 10:19:51 -0500 Eric DeVolder <eric.devolder@oracle.com> wrote: > On 7/27/21 7:55 AM, Igor Mammedov wrote: > > PS: > > If I haven't said it already, use checkpatch script before posting patches. > > > > I do run checkpatch. On occasion I allow a warning about a line too long. And > there is the MAINTAINERs message due to the new files. Is there something else > that I'm missing? > there were warnings about new line or something like this.
============================= 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> --- 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