[kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Matthias Rosenfelder]

[-- Attachment #1: Type: text/plain, Size: 1602 bytes --]

Hello,

I think one of the test conditions for the KVM PMU unit test "basic_event_count" is not strong enough. It only checks whether an overflow occurred for counter #0, but it should also check that none happened for the other counter(s):

report(read_sysreg(pmovsclr_el0) & 0x1,
      "check overflow happened on #0 only");

This should be "==" instead of "&".

Note that this test uses one more counter (#1), which must not overflow. This should also be checked, even though this would be visible through the "report_info()" a few lines above. But the latter does not mark the test failing - it is purely informational, so any test automation will not notice.


I apologize in advance if my email program at work messes up any formatting. Please let me know and I will try to reconfigure and resend if necessary. Thank you.

Best Regards,

Matthias
[Banner]<http://www.nio.io>
This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. You may NOT use, disclose, copy or disseminate this information. If you have received this email in error, please notify the sender and destroy all copies of the original message and all attachments. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company. Finally, the recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-kvm-unit-test-pmu-Fix-test-condition.patch --]
[-- Type: text/x-patch; name="0001-kvm-unit-test-pmu-Fix-test-condition.patch", Size: 1171 bytes --]

From 0a3ca4c88818e752ca63dee3d50945a26a16d29b Mon Sep 17 00:00:00 2001
From: Matthias Rosenfelder <matthias.rosenfelder@nio.io>
Date: Tue, 26 Sep 2023 19:07:18 +0200
Subject: [kvm-unit-tests PATCH] arm: PMU: Fix test condition

The test description says: "check overflow happened on #0 only",
but the test condition actually tests: "check overflow happened
AT LEAST of #0, maybe of some other counters as well."
This basically disregards any overflow of other counters, but it
should ensure that there are none.

With the updated test condition the test will fail if there was an
overflow of counter #1, too.

Signed-off-by: Matthias Rosenfelder <matthias.rosenfelder@nio.io>
---
 arm/pmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index b164f21..1956b42 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -532,7 +532,7 @@ static void test_basic_event_count(bool overflow_at_64bits)
 		    read_regn_el0(pmevcntr, 1));
 
 	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
-	report(read_sysreg(pmovsclr_el0) & 0x1,
+	report(read_sysreg(pmovsclr_el0) == 0x1,
 		"check overflow happened on #0 only");
 }
 
-- 
2.34.1

Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Andrew Jones]

On Fri, Sep 29, 2023 at 09:19:37PM +0000, Matthias Rosenfelder wrote:
> Hello,
> 
> I think one of the test conditions for the KVM PMU unit test "basic_event_count" is not strong enough. It only checks whether an overflow occurred for counter #0, but it should also check that none happened for the other counter(s):
> 
> report(read_sysreg(pmovsclr_el0) & 0x1,
>       "check overflow happened on #0 only");
> 
> This should be "==" instead of "&".
> 
> Note that this test uses one more counter (#1), which must not overflow. This should also be checked, even though this would be visible through the "report_info()" a few lines above. But the latter does not mark the test failing - it is purely informational, so any test automation will not notice.
> 
> 
> I apologize in advance if my email program at work messes up any formatting. Please let me know and I will try to reconfigure and resend if necessary. Thank you.

Hi Matthias,

Your mail client sent the patch as an attachment. Please use
get-send-email to submit patches and put the justifications for the
patches, like you've written above, in the commit messages.

Thanks,
drew

> 
> Best Regards,
> 
> Matthias
> [Banner]<http://www.nio.io>
> This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. You may NOT use, disclose, copy or disseminate this information. If you have received this email in error, please notify the sender and destroy all copies of the original message and all attachments. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company. Finally, the recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.

Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Eric Auger]

Hi Matthias,

On 9/29/23 23:19, Matthias Rosenfelder wrote:
> Hello,
>
> I think one of the test conditions for the KVM PMU unit test "basic_event_count" is not strong enough. It only checks whether an overflow occurred for counter #0, but it should also check that none happened for the other counter(s):
>
> report(read_sysreg(pmovsclr_el0) & 0x1,
>       "check overflow happened on #0 only");
>
> This should be "==" instead of "&".

Your change indeed makes sense to match the comment. Please resubmit
following Drew's hint and feel free to add my

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

Thanks

Eric
>
> Note that this test uses one more counter (#1), which must not overflow. This should also be checked, even though this would be visible through the "report_info()" a few lines above. But the latter does not mark the test failing - it is purely informational, so any test automation will not notice.
>
>
> I apologize in advance if my email program at work messes up any formatting. Please let me know and I will try to reconfigure and resend if necessary. Thank you.
>
> Best Regards,
>
> Matthias
> [Banner]<http://www.nio.io>
> This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. You may NOT use, disclose, copy or disseminate this information. If you have received this email in error, please notify the sender and destroy all copies of the original message and all attachments. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company. Finally, the recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.

Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Andrew Jones]

On Fri, Sep 29, 2023 at 09:19:37PM +0000, Matthias Rosenfelder wrote:
> Hello,
> 
> I think one of the test conditions for the KVM PMU unit test "basic_event_count" is not strong enough. It only checks whether an overflow occurred for counter #0, but it should also check that none happened for the other counter(s):
> 
> report(read_sysreg(pmovsclr_el0) & 0x1,
>       "check overflow happened on #0 only");
> 
> This should be "==" instead of "&".
> 
> Note that this test uses one more counter (#1), which must not overflow. This should also be checked, even though this would be visible through the "report_info()" a few lines above. But the latter does not mark the test failing - it is purely informational, so any test automation will not notice.
> 
> 
> I apologize in advance if my email program at work messes up any formatting. Please let me know and I will try to reconfigure and resend if necessary. Thank you.

Hey Matthias,

We let you know the formatting was wrong, but we haven't yet received a
resend. But, since Eric already reviewed it, I've gone ahead and applied
it to arm/queue with this fixes tag

Fixes: 4ce2a8045624 ("arm: pmu: Basic event counter Tests")

drew

Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Matthias Rosenfelder]

Hi drew,

thanks for coming back to me. I tried using "git-send-email" but was struggling with the SMTP configuration of my company (Microsoft Outlook online account...). So far I've not found a way to authenticate with SMTP, so I was unfortunately unable to send the patch (with improved rationale, as requested).

Since giving back to the open source community is more of a personal wish and is not required by management (but also not forbidden), it has low priority and I already spent some time on this. I will send patches in the future from my personal email account.

I am totally fine with someone else submitting the patch.
If it's not too inconvenient, could you please add a "reported-by" to the patch? (No problem if not)
Thank you.

Best Regards,

Matthias

________________________________________
From: Andrew Jones <ajones@ventanamicro.com>
Sent: Tuesday, October 24, 2023 13:31
To: Matthias Rosenfelder
Cc: kvm@vger.kernel.org; Andrew Jones; Alexandru Elisei; Eric Auger; kvmarm@lists.linux.dev
Subject: Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

CAUTION! External Email. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On Fri, Sep 29, 2023 at 09:19:37PM +0000, Matthias Rosenfelder wrote:
> Hello,
>
> I think one of the test conditions for the KVM PMU unit test "basic_event_count" is not strong enough. It only checks whether an overflow occurred for counter #0, but it should also check that none happened for the other counter(s):
>
> report(read_sysreg(pmovsclr_el0) & 0x1,
>       "check overflow happened on #0 only");
>
> This should be "==" instead of "&".
>
> Note that this test uses one more counter (#1), which must not overflow. This should also be checked, even though this would be visible through the "report_info()" a few lines above. But the latter does not mark the test failing - it is purely informational, so any test automation will not notice.
>
>
> I apologize in advance if my email program at work messes up any formatting. Please let me know and I will try to reconfigure and resend if necessary. Thank you.

Hey Matthias,

We let you know the formatting was wrong, but we haven't yet received a
resend. But, since Eric already reviewed it, I've gone ahead and applied
it to arm/queue with this fixes tag

Fixes: 4ce2a8045624 ("arm: pmu: Basic event counter Tests")

drew
[Banner]<http://www.nio.io>
This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. You may NOT use, disclose, copy or disseminate this information. If you have received this email in error, please notify the sender and destroy all copies of the original message and all attachments. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company. Finally, the recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.

Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Andrew Jones]

On Thu, Oct 26, 2023 at 02:24:07PM +0000, Matthias Rosenfelder wrote:
> Hi drew,
> 
> thanks for coming back to me. I tried using "git-send-email" but was struggling with the SMTP configuration of my company (Microsoft Outlook online account...). So far I've not found a way to authenticate with SMTP, so I was unfortunately unable to send the patch (with improved rationale, as requested).
> 
> Since giving back to the open source community is more of a personal wish and is not required by management (but also not forbidden), it has low priority and I already spent some time on this. I will send patches in the future from my personal email account.
> 
> I am totally fine with someone else submitting the patch.
> If it's not too inconvenient, could you please add a "reported-by" to the patch? (No problem if not)
> Thank you.

You have the authorship

https://gitlab.com/jones-drew/kvm-unit-tests/-/commit/52d963e95aa2fa3ce4faa9557cb99c002b177ec7

Thanks,
drew

> 
> Best Regards,
> 
> Matthias
> 
> ________________________________________
> From: Andrew Jones <ajones@ventanamicro.com>
> Sent: Tuesday, October 24, 2023 13:31
> To: Matthias Rosenfelder
> Cc: kvm@vger.kernel.org; Andrew Jones; Alexandru Elisei; Eric Auger; kvmarm@lists.linux.dev
> Subject: Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition
> 
> CAUTION! External Email. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> On Fri, Sep 29, 2023 at 09:19:37PM +0000, Matthias Rosenfelder wrote:
> > Hello,
> >
> > I think one of the test conditions for the KVM PMU unit test "basic_event_count" is not strong enough. It only checks whether an overflow occurred for counter #0, but it should also check that none happened for the other counter(s):
> >
> > report(read_sysreg(pmovsclr_el0) & 0x1,
> >       "check overflow happened on #0 only");
> >
> > This should be "==" instead of "&".
> >
> > Note that this test uses one more counter (#1), which must not overflow. This should also be checked, even though this would be visible through the "report_info()" a few lines above. But the latter does not mark the test failing - it is purely informational, so any test automation will not notice.
> >
> >
> > I apologize in advance if my email program at work messes up any formatting. Please let me know and I will try to reconfigure and resend if necessary. Thank you.
> 
> Hey Matthias,
> 
> We let you know the formatting was wrong, but we haven't yet received a
> resend. But, since Eric already reviewed it, I've gone ahead and applied
> it to arm/queue with this fixes tag
> 
> Fixes: 4ce2a8045624 ("arm: pmu: Basic event counter Tests")
> 
> drew
> [Banner]<http://www.nio.io>
> This email and any files transmitted with it are confidential and intended solely for the use of the individual or entity to whom they are addressed. You may NOT use, disclose, copy or disseminate this information. If you have received this email in error, please notify the sender and destroy all copies of the original message and all attachments. Please note that any views or opinions presented in this email are solely those of the author and do not necessarily represent those of the company. Finally, the recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.

Re: [kvm-unit-tests PATCH] arm: pmu: Fix overflow test condition

[Andrew Jones]

On Thu, Oct 26, 2023 at 04:31:51PM +0200, Andrew Jones wrote:
> On Thu, Oct 26, 2023 at 02:24:07PM +0000, Matthias Rosenfelder wrote:
> > Hi drew,
> > 
> > thanks for coming back to me. I tried using "git-send-email" but was struggling with the SMTP configuration of my company (Microsoft Outlook online account...). So far I've not found a way to authenticate with SMTP, so I was unfortunately unable to send the patch (with improved rationale, as requested).
> > 
> > Since giving back to the open source community is more of a personal wish and is not required by management (but also not forbidden), it has low priority and I already spent some time on this. I will send patches in the future from my personal email account.
> > 
> > I am totally fine with someone else submitting the patch.
> > If it's not too inconvenient, could you please add a "reported-by" to the patch? (No problem if not)
> > Thank you.
> 
> You have the authorship
> 
> https://gitlab.com/jones-drew/kvm-unit-tests/-/commit/52d963e95aa2fa3ce4faa9557cb99c002b177ec7
>

Merged into https://gitlab.com/kvm-unit-tests/kvm-unit-tests master

Thanks,
drew