| Message ID | 20201027171944.13933-1-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: Statistical Profiling Extension Tests | expand |
Hi, Adding Andrew to CC list, somehow I forgot, sorry for that. Thanks, Alex On 10/27/20 5:19 PM, Alexandru Elisei wrote: > This series implements two basic tests for KVM SPE: one that checks that > the reported features match what is specified in the architecture, > implemented in patch #3 ("arm64: spe: Add introspection test"), and another > test that checks that the buffer management interrupt is asserted > correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest > of the patches are either preparatory patches, or a fix, in the case of > patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers"). > > This series builds on Eric's initial version [1], to which I've added the > buffer tests that I used while developing SPE support for KVM. > > KVM SPE support is current in RFC phase, hence why this series is also sent > as RFC. The KVM patches needed for the tests to work can be found at [2]. > Userspace support is also necessary, which I've implemented for kvmtool; > this can be found at [3]. This series is also available in a repo [4] to make > testing easier. > > [1] https://www.spinics.net/lists/kvm/msg223792.html > [2] https://gitlab.arm.com/linux-arm/linux-ae/-/tree/kvm-spe-v3 > [3] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/kvm-spe-v3 > [4] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/kvm-spe-v2 > > Alexandru Elisei (3): > lib/{bitops,alloc_page}.h: Add missing headers > lib: arm/arm64: Add function to unmap a page > am64: spe: Add buffer test > > Eric Auger (2): > arm64: Move get_id_aa64dfr0() in processor.h > arm64: spe: Add introspection test > > arm/Makefile.arm64 | 1 + > lib/arm/asm/mmu-api.h | 1 + > lib/arm64/asm/processor.h | 5 + > lib/alloc_page.h | 2 + > lib/bitops.h | 2 + > lib/libcflat.h | 1 + > lib/arm/mmu.c | 32 +++ > arm/pmu.c | 1 - > arm/spe.c | 506 ++++++++++++++++++++++++++++++++++++++ > arm/unittests.cfg | 15 ++ > 10 files changed, 565 insertions(+), 1 deletion(-) > create mode 100644 arm/spe.c >
Hi Alexandru, [+ Drew] On 10/27/20 6:19 PM, Alexandru Elisei wrote: > This series implements two basic tests for KVM SPE: one that checks that > the reported features match what is specified in the architecture, > implemented in patch #3 ("arm64: spe: Add introspection test"), and another > test that checks that the buffer management interrupt is asserted > correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest > of the patches are either preparatory patches, or a fix, in the case of > patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers"). > > This series builds on Eric's initial version [1], to which I've added the > buffer tests that I used while developing SPE support for KVM. As you respin my series, with my prior agreement, I expected to find most of the code I wrote, obviously with some potential fixes and adaptations to fit your needs for additional tests. However, in this v2, two significant v1 patches have disappeared: 1) [3/4] spe: Add profiling buffer test (170 LOC diffstat) 2) [4/4] spe: Test Profiling Buffer Events (150 LOC diffstat) They were actually the crux of the original series (the introspection test being required as a prerequisite but not testing much really ;-). 1) consists in a "spe-buffer" test starting the profiling on a mastered sequence of instructions (as done for PMU event counters). It introduces the infra to start the profiling, prepare SPE reset config, the macro definitions, start/stop/drain, the code under profiling and basically checks that the buffer was effectively written. We also check we do not get any spurious event as it is not expected. => This test has disappeared and the infra now is diluted in [kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test. However no credit is given to my work as my S-o-b has disappeared. 2) consists in a "spe-events" test checking we effectively get the buffer full event when duly expected. This introduces the infra to handle interrupts, check the occurence of events by analyzing the syndrome registers, compare occurences against expected ones. This largely mimics what we have with PMU tests. => This test is part of [kvm-unit-tests RFC PATCH v2 5/5], relying on a different stop condition, and again the infra is diluted in the same patch, with large arbitrary changes, without any credit given to my work. Those changes may explain why you removed my S-o-b but given the anteriority of my series, this does not look normal to me, in a community environment. As discussed privately, this gives me the impression that those two patches were totally ignored while respinning. Please could you restructure the series at least to keep the buffer-full test + infra separate from the new tests and reset a collaborative S-o-b. Then if you think there are issues wrt the 1st test, "spe-buffer", not included in this series, please let's discuss and fix/improve but not simply trash it as is (in an everyone growing spirit). An alternative is I can take back the ownership of my series and push it upstream in a standard way. Then either you rebase your new tests on top of it or I will be happy to do it for you after discussions on the technical comments. Thanks Eric > > KVM SPE support is current in RFC phase, hence why this series is also sent > as RFC. The KVM patches needed for the tests to work can be found at [2]. > Userspace support is also necessary, which I've implemented for kvmtool; > this can be found at [3]. This series is also available in a repo [4] to make > testing easier. > > [1] https://www.spinics.net/lists/kvm/msg223792.html > [2] https://gitlab.arm.com/linux-arm/linux-ae/-/tree/kvm-spe-v3 > [3] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/kvm-spe-v3 > [4] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/kvm-spe-v2 > > Alexandru Elisei (3): > lib/{bitops,alloc_page}.h: Add missing headers > lib: arm/arm64: Add function to unmap a page > am64: spe: Add buffer test > > Eric Auger (2): > arm64: Move get_id_aa64dfr0() in processor.h > arm64: spe: Add introspection test > > arm/Makefile.arm64 | 1 + > lib/arm/asm/mmu-api.h | 1 + > lib/arm64/asm/processor.h | 5 + > lib/alloc_page.h | 2 + > lib/bitops.h | 2 + > lib/libcflat.h | 1 + > lib/arm/mmu.c | 32 +++ > arm/pmu.c | 1 - > arm/spe.c | 506 ++++++++++++++++++++++++++++++++++++++ > arm/unittests.cfg | 15 ++ > 10 files changed, 565 insertions(+), 1 deletion(-) > create mode 100644 arm/spe.c >
Hi Eric, On 10/30/20 6:17 PM, Auger Eric wrote: > Hi Alexandru, > > [+ Drew] > > On 10/27/20 6:19 PM, Alexandru Elisei wrote: >> This series implements two basic tests for KVM SPE: one that checks that >> the reported features match what is specified in the architecture, >> implemented in patch #3 ("arm64: spe: Add introspection test"), and another >> test that checks that the buffer management interrupt is asserted >> correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest >> of the patches are either preparatory patches, or a fix, in the case of >> patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers"). >> >> This series builds on Eric's initial version [1], to which I've added the >> buffer tests that I used while developing SPE support for KVM. > As you respin my series, with my prior agreement, I expected to find > most of the code I wrote, obviously with some potential fixes and > adaptations to fit your needs for additional tests. I believe there has been a misunderstanding. I asked you if I can pickup *some* of your patches, not all of them. > > However, in this v2, two significant v1 patches have disappeared: > > 1) [3/4] spe: Add profiling buffer test (170 LOC diffstat) > 2) [4/4] spe: Test Profiling Buffer Events (150 LOC diffstat) > > They were actually the crux of the original series (the introspection > test being required as a prerequisite but not testing much really ;-). > > 1) consists in a "spe-buffer" test starting the profiling on a mastered > sequence of instructions (as done for PMU event counters). It introduces > the infra to start the profiling, prepare SPE reset config, the macro > definitions, start/stop/drain, the code under profiling and basically > checks that the buffer was effectively written. We also check we do not > get any spurious event as it is not expected. > > => This test has disappeared and the infra now is diluted in > [kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test. However no > credit is given to my work as my S-o-b has disappeared. > > 2) consists in a "spe-events" test checking we effectively get the > buffer full event when duly expected. This introduces the infra to > handle interrupts, check the occurence of events by analyzing the > syndrome registers, compare occurences against expected ones. This > largely mimics what we have with PMU tests. > > => This test is part of [kvm-unit-tests RFC PATCH v2 5/5], relying on a > different stop condition, and again the infra is diluted in the same > patch, with large arbitrary changes, without any credit given to my > work. Those changes may explain why you removed my S-o-b but given the > anteriority of my series, this does not look normal to me, in a > community environment. > > As discussed privately, this gives me the impression that those two > patches were totally ignored while respinning. Your impression is correct. The buffer test is my original work. No code has been borrowed from your patches, hence why the differences might look like arbitrary changes. I believe there are some correctness issues with your patches, and I decided to send my own test which I used when developing KVM SPE support instead of rebasing your tests. > > Please could you restructure the series at least to keep the buffer-full > test + infra separate from the new tests and reset a collaborative S-o-b. > > Then if you think there are issues wrt the 1st test, "spe-buffer", not > included in this series, please let's discuss and fix/improve but not > simply trash it as is (in an everyone growing spirit). > > An alternative is I can take back the ownership of my series and push it > upstream in a standard way. Then either you rebase your new tests on top > of it or I will be happy to do it for you after discussions on the > technical comments. It was not my intention to make you feel that your contribution is not appreciated or ignored. Let's work on merging your series first and then I'll rebase and resend any tests from my series which were not included. Thanks, Alex
Hi Alexandru, On 10/30/20 11:31 PM, Alexandru Elisei wrote: > Hi Eric, > > On 10/30/20 6:17 PM, Auger Eric wrote: >> Hi Alexandru, >> >> [+ Drew] >> >> On 10/27/20 6:19 PM, Alexandru Elisei wrote: >>> This series implements two basic tests for KVM SPE: one that checks that >>> the reported features match what is specified in the architecture, >>> implemented in patch #3 ("arm64: spe: Add introspection test"), and another >>> test that checks that the buffer management interrupt is asserted >>> correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest >>> of the patches are either preparatory patches, or a fix, in the case of >>> patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers"). >>> >>> This series builds on Eric's initial version [1], to which I've added the >>> buffer tests that I used while developing SPE support for KVM. >> As you respin my series, with my prior agreement, I expected to find >> most of the code I wrote, obviously with some potential fixes and >> adaptations to fit your needs for additional tests. > > I believe there has been a misunderstanding. I asked you if I can pickup *some* of > your patches, not all of them. > >> >> However, in this v2, two significant v1 patches have disappeared: >> >> 1) [3/4] spe: Add profiling buffer test (170 LOC diffstat) >> 2) [4/4] spe: Test Profiling Buffer Events (150 LOC diffstat) >> >> They were actually the crux of the original series (the introspection >> test being required as a prerequisite but not testing much really ;-). >> >> 1) consists in a "spe-buffer" test starting the profiling on a mastered >> sequence of instructions (as done for PMU event counters). It introduces >> the infra to start the profiling, prepare SPE reset config, the macro >> definitions, start/stop/drain, the code under profiling and basically >> checks that the buffer was effectively written. We also check we do not >> get any spurious event as it is not expected. >> >> => This test has disappeared and the infra now is diluted in >> [kvm-unit-tests RFC PATCH v2 5/5] am64: spe: Add buffer test. However no >> credit is given to my work as my S-o-b has disappeared. >> >> 2) consists in a "spe-events" test checking we effectively get the >> buffer full event when duly expected. This introduces the infra to >> handle interrupts, check the occurence of events by analyzing the >> syndrome registers, compare occurences against expected ones. This >> largely mimics what we have with PMU tests. >> >> => This test is part of [kvm-unit-tests RFC PATCH v2 5/5], relying on a >> different stop condition, and again the infra is diluted in the same >> patch, with large arbitrary changes, without any credit given to my >> work. Those changes may explain why you removed my S-o-b but given the >> anteriority of my series, this does not look normal to me, in a >> community environment. >> >> As discussed privately, this gives me the impression that those two >> patches were totally ignored while respinning. > > Your impression is correct. The buffer test is my original work. No code has been > borrowed from your patches, hence why the differences might look like arbitrary > changes. > > I believe there are some correctness issues with your patches, and I decided to > send my own test which I used when developing KVM SPE support instead of rebasing > your tests. > >> >> Please could you restructure the series at least to keep the buffer-full >> test + infra separate from the new tests and reset a collaborative S-o-b. >> >> Then if you think there are issues wrt the 1st test, "spe-buffer", not >> included in this series, please let's discuss and fix/improve but not >> simply trash it as is (in an everyone growing spirit). >> >> An alternative is I can take back the ownership of my series and push it >> upstream in a standard way. Then either you rebase your new tests on top >> of it or I will be happy to do it for you after discussions on the >> technical comments. > > It was not my intention to make you feel that your contribution is not appreciated > or ignored. Let's work on merging your series first and then I'll rebase and > resend any tests from my series which were not included. Sure. So let's continue our technical discussion on both your respin and my original series to identify issues and potential improvements to get the best of them. Thanks Eric > > Thanks, > Alex >
This series implements two basic tests for KVM SPE: one that checks that the reported features match what is specified in the architecture, implemented in patch #3 ("arm64: spe: Add introspection test"), and another test that checks that the buffer management interrupt is asserted correctly, implemented in patch #5 ("am64: spe: Add buffer test"). The rest of the patches are either preparatory patches, or a fix, in the case of patch #2 ("lib/{bitops,alloc_page}.h: Add missing headers"). This series builds on Eric's initial version [1], to which I've added the buffer tests that I used while developing SPE support for KVM. KVM SPE support is current in RFC phase, hence why this series is also sent as RFC. The KVM patches needed for the tests to work can be found at [2]. Userspace support is also necessary, which I've implemented for kvmtool; this can be found at [3]. This series is also available in a repo [4] to make testing easier. [1] https://www.spinics.net/lists/kvm/msg223792.html [2] https://gitlab.arm.com/linux-arm/linux-ae/-/tree/kvm-spe-v3 [3] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/kvm-spe-v3 [4] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/kvm-spe-v2 Alexandru Elisei (3): lib/{bitops,alloc_page}.h: Add missing headers lib: arm/arm64: Add function to unmap a page am64: spe: Add buffer test Eric Auger (2): arm64: Move get_id_aa64dfr0() in processor.h arm64: spe: Add introspection test arm/Makefile.arm64 | 1 + lib/arm/asm/mmu-api.h | 1 + lib/arm64/asm/processor.h | 5 + lib/alloc_page.h | 2 + lib/bitops.h | 2 + lib/libcflat.h | 1 + lib/arm/mmu.c | 32 +++ arm/pmu.c | 1 - arm/spe.c | 506 ++++++++++++++++++++++++++++++++++++++ arm/unittests.cfg | 15 ++ 10 files changed, 565 insertions(+), 1 deletion(-) create mode 100644 arm/spe.c