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

[Marc Zyngier <maz@kernel.org>]

On Fri, 30 Jul 2021 16:22:01 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier <maz@kernel.org> wrote:
> > > FEAT_ECV provides a guest physical counter-timer offset register
> > > (CNTPOFF_EL2), but ECV-enabled hardware is nonexistent at the time of
> > > writing so support for it was elided for the sake of the author :)
> >
> > You seem to have done most the work for that, and there are SW models
> > out there that implement ECV. If anything, the ECV support should come
> > first, and its emulation only as a poor man's workaround.
> >
> > It is also good to show silicon vendors that they suck at providing
> > useful features, and pointing them to the code that would use these
> > features *is* an incentive.
> 
> Lol, then so be it. I'll add ECV support to this series. What can I
> test with, though? I don't recall QEMU supporting ECV last I checked..

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.

>
> > 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.

> > the counter read can (and will) be speculated,
> > so you definitely need an ISB before the access. Please also look at
> > __arch_counter_get_cntpct() and arch_counter_enforce_ordering().
> 
> Wouldn't it be up to the guest to decide if it wants the counter to be
> speculated or not? I debated this a bit myself in the implementation,
> as we would trap both ordered and un-ordered reads. Well, no way to
> detect it :)

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.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm