Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene

From: Christoffer Dall <cdall@linaro.org>
To: Andrew Jones <drjones@redhat.com>
Cc: "Christoffer Dall" <christoffer.dall@linaro.org>,
	"Marc Zyngier" <marc.zyngier@arm.com>,
	kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	"Alexander Graf" <agraf@suse.de>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Jintack Lim" <jintack@cs.columbia.edu>
Subject: Re: [PATCH kvm-unit-tests 2/3] arm64: timer: Fix test on APM X-Gene
Date: Tue, 18 Jul 2017 12:35:12 +0200	[thread overview]
Message-ID: <20170718103512.GB14069@cbox> (raw)
In-Reply-To: <20170718100503.crn2dgqlalrirzqw@kamzik.brq.redhat.com>

On Tue, Jul 18, 2017 at 12:05:03PM +0200, Andrew Jones wrote:
> On Fri, Jul 14, 2017 at 08:45:12AM -0700, Christoffer Dall wrote:
> > On Fri, Jul 14, 2017 at 09:04:10AM +0100, Marc Zyngier wrote:
> > > On 13/07/17 20:20, Christoffer Dall wrote:
> > > > When running the vtimer test on an APM X-Gene, setting the timer value
> > > > to (2^64 - 1) apparently results in the timer always firing, even
> > > > thought the counter is mich lower than the cval.
> > > 
> > > Note that the system counter is only guaranteed to be at least 56 bit
> > > wide (see DDI0487B.a G5.1.2), and I seem to remember that X-Gene only
> > > has the minimum. This could explain why setting the comparator to a
> > > value greater than (2^56 - 1) leads to a firing timer (the comparator
> > > appears to be in the past).
> 
> Ah, that explains why when I tried setting it to ~0 on my mustang, and
> then reading it back, it was always 2^56 - 1 instead. However my mustang
> still also requires me to clear bit 31, otherwise the vcpu hangs.
> 
> > 
> > Thanks for pointing that out, that makes good sense.  So then we should
> > definitely fix the test.
> > 
> > We could either set it to 2^56 - 1 instead, or just keep the 10s as used
> > in this patch, because the whole test times out after 2s anyway.
> 
> With the 10s version the test runs and passes on my mustang, so on one
> hand I prefer it. OTOH, testing to the spec, by using 2^56 - 1, seems
> more correct and allows one to find issues like the one I have on my
> mustang, i.e. a vcpu hang when bit 31 isn't clear.
> 
> I guess I lean more towards testings to the spec, but not enough to
> ask for a v2 of the patch. It's up to you.

I think we should have something that tests KVM on the platforms we have
and that are available for people's use.  I don't think we should verify
the architecture as much.  People use the m400 (basically
Mustang) in CloudLab, for example, which is we I keep caring about that.

I think this test was designed to test "if I program a timer to some
time in the future it shouldn't fire right away", which is still what
we test with this patch.

If we want to add a "the platform provides a timer with 56 valid bits in
the counter and compare register", then I think it should be a separate
test, and the the user can see that "basic stuff works", "architecture
compliance not so much" and shrug accordingly.

Thanks,
-Christoffer