Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset

From: Oliver Upton <oupton@google.com>
To: Marc Zyngier <maz@kernel.org>
Cc: kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu,
	Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Peter Shier <pshier@google.com>,
	Jim Mattson <jmattson@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <Alexandru.Elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Andrew Jones <drjones@redhat.com>
Subject: Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
Date: Fri, 30 Jul 2021 09:48:31 -0700	[thread overview]
Message-ID: <CAOQ_QsiwuancUsFEVr3TBeP6yLZMfAqNRv3ww2H+hcUGfxs9LA@mail.gmail.com> (raw)
In-Reply-To: <87wnp722tm.wl-maz@kernel.org>

On Fri, Jul 30, 2021 at 9:18 AM Marc Zyngier <maz@kernel.org> wrote:
> You want the ARM FVP model, or maybe even the Foundation model. It has
> support all the way to ARMv8.7 apparently. I personally use the FVP,
> get in touch offline and I'll help you with the setup.
>
> In general, I tend to trust the ARM models a lot more than QEMU for
> the quality of the emulation. You can tune it in some bizarre way
> (the cache modelling is terrifying), and it will definitely do all
> kind of crazy reordering and speculation.

Awesome, thanks. I'll give this a try.

>
> >
> > > I really dislike the fallback to !vhe here. I'd rather you
> > > special-case the emulated ptimer in the VHE case:
> > >
> > >         if (has_vhe()) {
> > >                 map->direct_vtimer = vcpu_vtimer(vcpu);
> > >                 if (!timer_get_offset(vcpu_ptimer(vcpu)))
> > >                         map->direct_ptimer = vcpu_ptimer(vcpu);
> > >                         map->emul_ptimer = NULL;
> > >                 } else {
> > >                         map->direct_ptimer = NULL;
> > >                         map->emul_ptimer = vcpu_ptimer(vcpu);
> > >                 }
> > >         } else {
> >
> > Ack.
> >
> > > and you can drop the timer_emulation_required() helper which doesn't
> > > serve any purpose.
> >
> > Especially if I add ECV to this series, I'd prefer to carry it than
> > open-code the check for CNTPOFF && !ECV in get_timer_map(). Do you
> > concur? I can tighten it to ptimer_emulation_required() and avoid
> > taking an arch_timer context if you prefer.
>
> Sure, whatever you prefer. I'm not set on one way or another, but in
> the above case, it was clearly easier to get rid of the helper.

Agreed, it made sense to prune before.

> There has been a trap, so that already was a context synchronisation
> event. but it would be pretty common for the counter to be speculated
> way early, when entering EL2. That's not the end of the world, but
> that's not an accurate emulation. You'd want it to be as close as
> possible to the reentry point into the guest.

Cool, I'll add the barrier to kick the can closer to guest entry.

--
Thanks,
Oliver