On Fri, 30 Jul 2021 16:22:01 +0100,
Oliver Upton [off-list ref] wrote:

On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier [off-list ref] wrote:

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel