mbox series

[v4,0/7] ARM virt: Add NVDIMM support

Message ID 20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com (mailing list archive)
Headers show
Series ARM virt: Add NVDIMM support | expand

Message

Shameerali Kolothum Thodi April 21, 2020, 12:59 p.m. UTC
This series adds NVDIMM support to arm/virt platform.
The series reuses some of the patches posted by Eric
in his earlier attempt here[1].

This series previously had few fixes to qemu in general
which were discovered while adding nvdimm support to arm/virt.
Those were sent out seperately[2] and are now part of Qemu.

Patch #1 is another fix to the nvdimm aml issue discussed
here[3].

I have done a basic sanity testing of NVDIMM devices
with Guest booting with ACPI. Further testing is always
welcome.

Please let me know your feedback.

Thanks,
Shameer

[1] https://patchwork.kernel.org/cover/10830777/
[2] https://patchwork.kernel.org/cover/11472501/
[3] https://patchwork.kernel.org/cover/11174959/#23020961

v3 --> v4
 -Removed patches #1 to #3 from v3 as they are now part of Qemu.
 -Addressed comments from Igor(#6) and Shannon(#4).
 -Added R-by from Igor(#1,#2,#3).

v2 --> v3
 - Added patch #1 and # 2 to fix the inconsistency in acpi
   table memory region sizes during migration. Thanks to
   David H.
 - The fix for qemu_ram_resize() callback was modified to
   the one in patch #3. Again thanks to David H.
 - Addressed comments from MST and Eric on tests added.
 - Addressed comments from Igor/MST on Integer size in patch #4
 - Added Eric's R-by to patch #7.

v1 --> v2
 -Reworked patch #1 and now fix is inside qemu_ram_resize().
 -Added patch #2 to fix the nvdim aml issue.
 -Dropped support to DT cold plug.
 -Updated test_acpi_virt_tcg_memhp() with pc-dimm and nvdimms(patch #7)

Kwangwoo Lee (2):
  nvdimm: Use configurable ACPI IO base and size
  hw/arm/virt: Add nvdimm hot-plug infrastructure

Shameer Kolothum (5):
  hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
  hw/arm/virt: Add nvdimm hotplug support
  tests: Update ACPI tables list for upcoming arm/virt test changes
  bios-tables-test: test pc-dimm and nvdimm coldplug for arm/virt
  tests/acpi: add expected tables for bios-tables-test

 docs/specs/acpi_hw_reduced_hotplug.rst |   3 +-
 hw/acpi/generic_event_device.c         |  15 +++++-
 hw/acpi/nvdimm.c                       |  72 ++++++++++++++++++++-----
 hw/arm/Kconfig                         |   1 +
 hw/arm/virt-acpi-build.c               |   6 +++
 hw/arm/virt.c                          |  35 ++++++++++--
 hw/i386/acpi-build.c                   |   6 +++
 hw/i386/acpi-build.h                   |   3 ++
 hw/i386/pc_piix.c                      |   2 +
 hw/i386/pc_q35.c                       |   2 +
 hw/mem/Kconfig                         |   2 +-
 include/hw/acpi/generic_event_device.h |   1 +
 include/hw/arm/virt.h                  |   1 +
 include/hw/mem/nvdimm.h                |   3 ++
 tests/data/acpi/pc/SSDT.dimmpxm        | Bin 685 -> 734 bytes
 tests/data/acpi/q35/SSDT.dimmpxm       | Bin 685 -> 734 bytes
 tests/data/acpi/virt/DSDT.memhp        | Bin 6644 -> 6668 bytes
 tests/data/acpi/virt/NFIT.memhp        | Bin 0 -> 224 bytes
 tests/data/acpi/virt/SSDT.memhp        | Bin 0 -> 736 bytes
 tests/qtest/bios-tables-test.c         |   9 +++-
 20 files changed, 138 insertions(+), 23 deletions(-)
 create mode 100644 tests/data/acpi/virt/NFIT.memhp
 create mode 100644 tests/data/acpi/virt/SSDT.memhp

Comments

no-reply@patchew.org April 21, 2020, 3:12 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v4 0/7] ARM virt: Add NVDIMM support
Message-id: 20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com
Type: series

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

Switched to a new branch 'test'
c4f3ad1 tests/acpi: add expected tables for bios-tables-test
5b55be7 bios-tables-test: test pc-dimm and nvdimm coldplug for arm/virt
f0c9bb6 tests: Update ACPI tables list for upcoming arm/virt test changes
c2dd728 hw/arm/virt: Add nvdimm hotplug support
f7dad84 hw/arm/virt: Add nvdimm hot-plug infrastructure
5554e78 nvdimm: Use configurable ACPI IO base and size
8058b6f hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length

=== OUTPUT BEGIN ===
1/7 Checking commit 8058b6f6d753 (hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found

total: 2 errors, 0 warnings, 59 lines checked

Patch 1/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/7 Checking commit 5554e78b18ea (nvdimm: Use configurable ACPI IO base and size)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_piix.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_piix.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_q35.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_q35.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/mem/nvdimm.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/mem/nvdimm.h found

total: 12 errors, 0 warnings, 158 lines checked

Patch 2/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/7 Checking commit f7dad84068ce (hw/arm/virt: Add nvdimm hot-plug infrastructure)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/Kconfig found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/Kconfig found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt-acpi-build.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt-acpi-build.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/mem/Kconfig found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/mem/Kconfig found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/arm/virt.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/arm/virt.h found

total: 10 errors, 0 warnings, 80 lines checked

Patch 3/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/7 Checking commit c2dd7289fec4 (hw/arm/virt: Add nvdimm hotplug support)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and docs/specs/acpi_hw_reduced_hotplug.rst found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and docs/specs/acpi_hw_reduced_hotplug.rst found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/generic_event_device.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/generic_event_device.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found

total: 8 errors, 0 warnings, 103 lines checked

Patch 4/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/7 Checking commit f0c9bb65828f (tests: Update ACPI tables list for upcoming arm/virt test changes)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/NFIT.memhp and include/hw/acpi/generic_event_device.h found

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#17: 
new file mode 100644

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/SSDT.memhp and include/hw/acpi/generic_event_device.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found

total: 4 errors, 1 warnings, 6 lines checked

Patch 5/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/7 Checking commit 5b55be7a85b5 (bios-tables-test: test pc-dimm and nvdimm coldplug for arm/virt)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found

total: 2 errors, 0 warnings, 19 lines checked

Patch 6/7 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

7/7 Checking commit c4f3ad1b593c (tests/acpi: add expected tables for bios-tables-test)
ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/pc/SSDT.dimmpxm and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/q35/SSDT.dimmpxm and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/DSDT.memhp and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/NFIT.memhp and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/SSDT.memhp and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found

ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found

total: 7 errors, 0 warnings, 1 lines checked

Patch 7/7 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/20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Michael S. Tsirkin May 4, 2020, 5:13 a.m. UTC | #2
On Tue, Apr 21, 2020 at 01:59:27PM +0100, Shameer Kolothum wrote:
> This series adds NVDIMM support to arm/virt platform.
> The series reuses some of the patches posted by Eric
> in his earlier attempt here[1].
> 
> This series previously had few fixes to qemu in general
> which were discovered while adding nvdimm support to arm/virt.
> Those were sent out seperately[2] and are now part of Qemu.


Mostly ACPI stuff so I can merge it if I get an ack for ARM side.

Alternatively, for ACPI things:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>



> Patch #1 is another fix to the nvdimm aml issue discussed
> here[3].
> 
> I have done a basic sanity testing of NVDIMM devices
> with Guest booting with ACPI. Further testing is always
> welcome.
> 
> Please let me know your feedback.
> 
> Thanks,
> Shameer
> 
> [1] https://patchwork.kernel.org/cover/10830777/
> [2] https://patchwork.kernel.org/cover/11472501/
> [3] https://patchwork.kernel.org/cover/11174959/#23020961
> 
> v3 --> v4
>  -Removed patches #1 to #3 from v3 as they are now part of Qemu.
>  -Addressed comments from Igor(#6) and Shannon(#4).
>  -Added R-by from Igor(#1,#2,#3).
> 
> v2 --> v3
>  - Added patch #1 and # 2 to fix the inconsistency in acpi
>    table memory region sizes during migration. Thanks to
>    David H.
>  - The fix for qemu_ram_resize() callback was modified to
>    the one in patch #3. Again thanks to David H.
>  - Addressed comments from MST and Eric on tests added.
>  - Addressed comments from Igor/MST on Integer size in patch #4
>  - Added Eric's R-by to patch #7.
> 
> v1 --> v2
>  -Reworked patch #1 and now fix is inside qemu_ram_resize().
>  -Added patch #2 to fix the nvdim aml issue.
>  -Dropped support to DT cold plug.
>  -Updated test_acpi_virt_tcg_memhp() with pc-dimm and nvdimms(patch #7)
> 
> Kwangwoo Lee (2):
>   nvdimm: Use configurable ACPI IO base and size
>   hw/arm/virt: Add nvdimm hot-plug infrastructure
> 
> Shameer Kolothum (5):
>   hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
>   hw/arm/virt: Add nvdimm hotplug support
>   tests: Update ACPI tables list for upcoming arm/virt test changes
>   bios-tables-test: test pc-dimm and nvdimm coldplug for arm/virt
>   tests/acpi: add expected tables for bios-tables-test
> 
>  docs/specs/acpi_hw_reduced_hotplug.rst |   3 +-
>  hw/acpi/generic_event_device.c         |  15 +++++-
>  hw/acpi/nvdimm.c                       |  72 ++++++++++++++++++++-----
>  hw/arm/Kconfig                         |   1 +
>  hw/arm/virt-acpi-build.c               |   6 +++
>  hw/arm/virt.c                          |  35 ++++++++++--
>  hw/i386/acpi-build.c                   |   6 +++
>  hw/i386/acpi-build.h                   |   3 ++
>  hw/i386/pc_piix.c                      |   2 +
>  hw/i386/pc_q35.c                       |   2 +
>  hw/mem/Kconfig                         |   2 +-
>  include/hw/acpi/generic_event_device.h |   1 +
>  include/hw/arm/virt.h                  |   1 +
>  include/hw/mem/nvdimm.h                |   3 ++
>  tests/data/acpi/pc/SSDT.dimmpxm        | Bin 685 -> 734 bytes
>  tests/data/acpi/q35/SSDT.dimmpxm       | Bin 685 -> 734 bytes
>  tests/data/acpi/virt/DSDT.memhp        | Bin 6644 -> 6668 bytes
>  tests/data/acpi/virt/NFIT.memhp        | Bin 0 -> 224 bytes
>  tests/data/acpi/virt/SSDT.memhp        | Bin 0 -> 736 bytes
>  tests/qtest/bios-tables-test.c         |   9 +++-
>  20 files changed, 138 insertions(+), 23 deletions(-)
>  create mode 100644 tests/data/acpi/virt/NFIT.memhp
>  create mode 100644 tests/data/acpi/virt/SSDT.memhp
> 
> -- 
> 2.17.1
>
Peter Maydell May 4, 2020, 9:29 a.m. UTC | #3
On Mon, 4 May 2020 at 06:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 21, 2020 at 01:59:27PM +0100, Shameer Kolothum wrote:
> > This series adds NVDIMM support to arm/virt platform.
> > The series reuses some of the patches posted by Eric
> > in his earlier attempt here[1].
> >
> > This series previously had few fixes to qemu in general
> > which were discovered while adding nvdimm support to arm/virt.
> > Those were sent out seperately[2] and are now part of Qemu.
>
>
> Mostly ACPI stuff so I can merge it if I get an ack for ARM side.

Happy for you to take it; all the arm-specific bits have
been reviewed by various people (thanks!) so here's my

Acked-by: Peter Maydell <peter.maydell@linaro.org>

I notice that checkpatch complains a lot about

ERROR: Do not add expected files together with tests, follow
instructions in tests/qtest/bios-tables-test.c: both
tests/qtest/bios-tables-test-allowed-diff.h and
tests/qtest/bios-tables-test.c found

Does that need fixing, or is the checkpatch test wrong?

thanks
-- PMM
Eric Auger May 4, 2020, 9:44 a.m. UTC | #4
Hi,

On 5/4/20 11:29 AM, Peter Maydell wrote:
> On Mon, 4 May 2020 at 06:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Apr 21, 2020 at 01:59:27PM +0100, Shameer Kolothum wrote:
>>> This series adds NVDIMM support to arm/virt platform.
>>> The series reuses some of the patches posted by Eric
>>> in his earlier attempt here[1].
>>>
>>> This series previously had few fixes to qemu in general
>>> which were discovered while adding nvdimm support to arm/virt.
>>> Those were sent out seperately[2] and are now part of Qemu.
>>
>>
>> Mostly ACPI stuff so I can merge it if I get an ack for ARM side.
> 
> Happy for you to take it; all the arm-specific bits have
> been reviewed by various people (thanks!) so here's my
> 
> Acked-by: Peter Maydell <peter.maydell@linaro.org>

Tested-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> 
> I notice that checkpatch complains a lot about
> 
> ERROR: Do not add expected files together with tests, follow
> instructions in tests/qtest/bios-tables-test.c: both
> tests/qtest/bios-tables-test-allowed-diff.h and
> tests/qtest/bios-tables-test.c found
> 
> Does that need fixing, or is the checkpatch test wrong?
> 
> thanks
> -- PMM
>
Michael S. Tsirkin May 4, 2020, 9:46 a.m. UTC | #5
On Mon, May 04, 2020 at 10:29:18AM +0100, Peter Maydell wrote:
> On Mon, 4 May 2020 at 06:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Apr 21, 2020 at 01:59:27PM +0100, Shameer Kolothum wrote:
> > > This series adds NVDIMM support to arm/virt platform.
> > > The series reuses some of the patches posted by Eric
> > > in his earlier attempt here[1].
> > >
> > > This series previously had few fixes to qemu in general
> > > which were discovered while adding nvdimm support to arm/virt.
> > > Those were sent out seperately[2] and are now part of Qemu.
> >
> >
> > Mostly ACPI stuff so I can merge it if I get an ack for ARM side.
> 
> Happy for you to take it; all the arm-specific bits have
> been reviewed by various people (thanks!) so here's my
> 
> Acked-by: Peter Maydell <peter.maydell@linaro.org>
> 
> I notice that checkpatch complains a lot about
> 
> ERROR: Do not add expected files together with tests, follow
> instructions in tests/qtest/bios-tables-test.c: both
> tests/qtest/bios-tables-test-allowed-diff.h and
> tests/qtest/bios-tables-test.c found
> 
> Does that need fixing, or is the checkpatch test wrong?
> 
> thanks
> -- PMM


Hmm I don't see a patch in this series that changes both
tests/qtest/bios-tables-test-allowed-diff.h and
tests/qtest/bios-tables-test.c. Do you?
Peter Maydell May 4, 2020, 9:57 a.m. UTC | #6
On Mon, 4 May 2020 at 10:46, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 04, 2020 at 10:29:18AM +0100, Peter Maydell wrote:
> > I notice that checkpatch complains a lot about
> >
> > ERROR: Do not add expected files together with tests, follow
> > instructions in tests/qtest/bios-tables-test.c: both
> > tests/qtest/bios-tables-test-allowed-diff.h and
> > tests/qtest/bios-tables-test.c found
> >
> > Does that need fixing, or is the checkpatch test wrong?

> Hmm I don't see a patch in this series that changes both
> tests/qtest/bios-tables-test-allowed-diff.h and
> tests/qtest/bios-tables-test.c. Do you?

No, but that's not the pair of files that checkpatch is
complaining about. It warns about:

patch 1:
 tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c

patch 2:
 tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c
 tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.c
 tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.h
 tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_piix.c
 tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_q35.c
 tests/qtest/bios-tables-test-allowed-diff.h and include/hw/mem/nvdimm.h

patch 3:
 tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/Kconfig
 tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt-acpi-build.c
 tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c
 tests/qtest/bios-tables-test-allowed-diff.h and hw/mem/Kconfig
 tests/qtest/bios-tables-test-allowed-diff.h and include/hw/arm/virt.h

etc, and the patches really do touch the pairs of files listed.
(It also seems to warn about each file combination multiple times.)

Are these false positives (if so, we should change the checkpatch test,
since it's clearly misfiring a lot) or problems with the patches?

thanks
-- PMM
Michael S. Tsirkin May 4, 2020, 9:57 a.m. UTC | #7
On Tue, Apr 21, 2020 at 08:12:57AM -0700, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Subject: [PATCH v4 0/7] ARM virt: Add NVDIMM support
> Message-id: 20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com
> Type: series
> 
> === 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 ===
> 
> Switched to a new branch 'test'
> c4f3ad1 tests/acpi: add expected tables for bios-tables-test
> 5b55be7 bios-tables-test: test pc-dimm and nvdimm coldplug for arm/virt
> f0c9bb6 tests: Update ACPI tables list for upcoming arm/virt test changes
> c2dd728 hw/arm/virt: Add nvdimm hotplug support
> f7dad84 hw/arm/virt: Add nvdimm hot-plug infrastructure
> 5554e78 nvdimm: Use configurable ACPI IO base and size
> 8058b6f hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length
> 
> === OUTPUT BEGIN ===
> 1/7 Checking commit 8058b6f6d753 (hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found
> 
> total: 2 errors, 0 warnings, 59 lines checked

OK so this is a false positive in the script. I will fix it.

> Patch 1/7 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 2/7 Checking commit 5554e78b18ea (nvdimm: Use configurable ACPI IO base and size)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found

This beats me. Where did we get
tests/qtest/bios-tables-test-allowed-diff.h from?
It's a different patch, isn't it?

> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/acpi-build.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_piix.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_piix.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_q35.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/i386/pc_q35.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/mem/nvdimm.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/mem/nvdimm.h found
> 
> total: 12 errors, 0 warnings, 158 lines checked
> 
> Patch 2/7 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 3/7 Checking commit f7dad84068ce (hw/arm/virt: Add nvdimm hot-plug infrastructure)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/Kconfig found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/Kconfig found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt-acpi-build.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt-acpi-build.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/mem/Kconfig found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/mem/Kconfig found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/arm/virt.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/arm/virt.h found
> 
> total: 10 errors, 0 warnings, 80 lines checked
> 
> Patch 3/7 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 4/7 Checking commit c2dd7289fec4 (hw/arm/virt: Add nvdimm hotplug support)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and docs/specs/acpi_hw_reduced_hotplug.rst found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and docs/specs/acpi_hw_reduced_hotplug.rst found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/generic_event_device.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/generic_event_device.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/arm/virt.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found
> 
> total: 8 errors, 0 warnings, 103 lines checked
> 
> Patch 4/7 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 5/7 Checking commit f0c9bb65828f (tests: Update ACPI tables list for upcoming arm/virt test changes)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/NFIT.memhp and include/hw/acpi/generic_event_device.h found
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #17: 
> new file mode 100644
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/SSDT.memhp and include/hw/acpi/generic_event_device.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and include/hw/acpi/generic_event_device.h found
> 
> total: 4 errors, 1 warnings, 6 lines checked
> 
> Patch 5/7 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 6/7 Checking commit 5b55be7a85b5 (bios-tables-test: test pc-dimm and nvdimm coldplug for arm/virt)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found
> 
> total: 2 errors, 0 warnings, 19 lines checked
> 
> Patch 6/7 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 7/7 Checking commit c4f3ad1b593c (tests/acpi: add expected tables for bios-tables-test)
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/pc/SSDT.dimmpxm and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/q35/SSDT.dimmpxm and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/DSDT.memhp and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/NFIT.memhp and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/data/acpi/virt/SSDT.memhp and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found
> 
> ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and tests/qtest/bios-tables-test.c found
> 
> total: 7 errors, 0 warnings, 1 lines checked
> 
> Patch 7/7 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/20200421125934.14952-1-shameerali.kolothum.thodi@huawei.com/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
Peter Maydell May 4, 2020, 10:06 a.m. UTC | #8
On Mon, 4 May 2020 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:

> > ./scripts/checkpatch.pl --mailback base..

> > 2/7 Checking commit 5554e78b18ea (nvdimm: Use configurable ACPI IO base and size)
> > ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found
>
> This beats me. Where did we get
> tests/qtest/bios-tables-test-allowed-diff.h from?
> It's a different patch, isn't it?

Ah, this is a bug in the checkfilename() function -- it uses
some globals $acpi_testexpected and $acpi_nontestexpected, but
there is no code to reset these when checkpatch starts checking
a new patch. So if you only check one patch in a checkpatch run
(eg by just passing it a single patch file) then it will work, but if
a single checkpatch execution is checking several commits
(eg in the way patchew runs it to check the whole series of
git commits at once, or if you pass it several patch files) then
it will give wrong results for the second and later patches.
I think the variables need to be reset at the top of 'sub process()'.

thanks
-- PMM
Michael S. Tsirkin May 4, 2020, 11:10 a.m. UTC | #9
On Mon, May 04, 2020 at 11:06:48AM +0100, Peter Maydell wrote:
> On Mon, 4 May 2020 at 10:57, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> > > ./scripts/checkpatch.pl --mailback base..
> 
> > > 2/7 Checking commit 5554e78b18ea (nvdimm: Use configurable ACPI IO base and size)
> > > ERROR: Do not add expected files together with tests, follow instructions in tests/qtest/bios-tables-test.c: both tests/qtest/bios-tables-test-allowed-diff.h and hw/acpi/nvdimm.c found
> >
> > This beats me. Where did we get
> > tests/qtest/bios-tables-test-allowed-diff.h from?
> > It's a different patch, isn't it?
> 
> Ah, this is a bug in the checkfilename() function -- it uses
> some globals $acpi_testexpected and $acpi_nontestexpected, but
> there is no code to reset these when checkpatch starts checking
> a new patch. So if you only check one patch in a checkpatch run
> (eg by just passing it a single patch file) then it will work, but if
> a single checkpatch execution is checking several commits
> (eg in the way patchew runs it to check the whole series of
> git commits at once, or if you pass it several patch files) then
> it will give wrong results for the second and later patches.
> I think the variables need to be reset at the top of 'sub process()'.
> 
> thanks
> -- PMM

Good point. Will fix.