| Message ID | 1603743573-9870-1-git-send-email-eric.devolder@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | acpi: Implement ACPI ERST support for guests | expand |
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
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 >
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
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