Re: [kvm-unit-tests RFC PATCH v2 0/5] arm64: Statistical Profiling Extension Tests

[Auger Eric <eric.auger@redhat.com>]

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
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm