Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
Hi Marc, On Fri, Jul 30, 2021 at 9:48 AM Oliver Upton wrote: > > On Fri, Jul 30, 2021 at 9:18 AM Marc Zyngier 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 have another spin of this series ready to kick out the door that implements ECV support but ran into some issues testing it... Seems that the ARM Foundation model only implements ECV=0x01, when we need ECV=0x02 for CNTPOFF_EL2 to be valid. Any thoughts, or shall I just send out the series and stare at it long enough to make sure the ECV parts look right ;-) -- Thanks, Oliver ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
On Fri, Jul 30, 2021 at 9:18 AM Marc Zyngier 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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier 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..
> 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.
> > +static inline bool __hyp_handle_counter(struct kvm_vcpu *vcpu)
> > +{
> > + u32 sysreg = esr_sys64_to_sysreg(kvm_vcpu_get_esr(vcpu));
> > + int rt = kvm_vcpu_sys_get_rt(vcpu);
> > + u64 rv;
> > +
> > + if (sysreg != SYS_CNTPCT_EL0)
>
> That's feels really optimistic. You don't check for the exception
> class, and pray that the bits will align? ;-)
Doh! Missed the EC check.
> Please don't add more small includes like this. It makes things hard
> to read and hides bugs
Ack.
> 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 :)
> >
> > -/*
> > - * Should only be called on non-VHE systems.
> > - * VHE systems use EL2 timers and configure EL1 timers in
> > kvm_timer_init_vhe().
> > - */
>
> This comment still applies. this function *is* nVHE specific.
>
Ack.
>
> You now pay the price of reprogramming CNTHCTL_EL2 on every entry for
> VHE, and that's not right. On VHE, it should be enough to perform the
> programming on vcpu_load() only.
>
True. I'll rework the programming in the next series.
> Overall, this patch needs a bit of splitting up (userspace interface,
> HV rework), re-optimise VHE, and it *definitely* wants the ECV support
> for the physical timer.
>
Sure thing, thanks for the review Marc!
--
Best,
Oliver
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
On Fri, 30 Jul 2021 16:22:01 +0100,
Oliver Upton wrote:
>
> On Fri, Jul 30, 2021 at 4:08 AM Marc Zyngier 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
Re: [PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
On Thu, 29 Jul 2021 18:32:58 +0100,
Oliver Upton wrote:
>
> Presently, KVM provides no facilities for correctly migrating a guest
> that depends on the physical counter-timer. While most guests (barring
> NV, of course) should not depend on the physical counter-timer, an
> operator may still wish to provide a consistent view of the physical
> counter-timer across migrations.
>
> Provide userspace with a new vCPU attribute to modify the guest physical
> counter-timer offset. Since the base architecture doesn't provide a
> physical counter-timer offset register, emulate the correct behavior by
> trapping accesses to the physical counter-timer whenever the offset
> value is non-zero.
>
> Uphold the same behavior as CNTVOFF_EL2 and broadcast the physical
> offset to all vCPUs whenever written. This guarantees that the
> counter-timer we provide the guest remains architectural, wherein all
> views of the counter-timer are consistent across vCPUs. Reconfigure
> timer traps for VHE on every guest entry, as different VMs will now have
> different traps enabled. Enable physical counter traps for nVHE whenever
> the offset is nonzero (we already trap physical timer registers in
> nVHE).
>
> 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.
>
> Reviewed-by: Andrew Jones
> Signed-off-by: Oliver Upton
> ---
> Documentation/virt/kvm/devices/vcpu.rst | 22 +++
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/include/asm/kvm_hyp.h | 2 -
> arch/arm64/include/asm/sysreg.h | 1 +
> arch/arm64/include/uapi/asm/kvm.h | 1 +
> arch/arm64/kvm/arch_timer.c | 72 ++-
> arch/arm64/kvm/arm.c | 4 +-
> arch/arm64/kvm/hyp/include/hyp/switch.h | 23
> arch/arm64/kvm/hyp/include/hyp/timer-sr.h | 26
> arch/arm64/kvm/hyp/nvhe/switch.c | 2 -
> arch/arm64/kvm/hyp/nvhe/timer-sr.c| 21 +++
> arch/arm64/kvm/hyp/vhe/timer-sr.c | 27 +
> include/kvm/arm_arch_timer.h | 2 -
> 13 files changed, 158 insertions(+), 46 deletions(-)
> create mode 100644 arch/arm64/kvm/hyp/include/hyp/timer-sr.h
>
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
> b/Documentation/virt/kvm/devices/vcpu.rst
> index ecbab7adc602..2756e3c09ab9 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -161,6 +161,28 @@ the following equation:
> KVM does not allow the use of varying offset values for different vCPUs;
> the last written offset value will be broadcasted to all vCPUs in a VM.
>
> +2.3. ATTRIBUTE: KVM_ARM_VCPU_TIMER_OFFSET_PTIMER
> +
> +
> +:Parameters: Pointer to a 64-bit unsigned counter-timer offset.
> +
> +Returns:
> +
> + === ==
> + -EFAULT Error reading/writing the provided
> + parameter address
> + -ENXIO Attribute not supported
> + === ==
> +
> +Specifies the guest's physical counter-timer offset from the host's
> +virtual counter. The guest's physical counter is then derived by
> +the following equation:
> +
> + guest_cntpct = host_cntvct - KVM_ARM_VCPU_TIMER_OFFSET_PTIMER
> +
> +KVM does not allow the use of varying offset values for different vCPUs;
> +the last written offset value will be broadcasted to all vCPUs in a VM.
> +
> 3. GROUP: KVM_ARM_VCPU_PVTIME_CTRL
> ==
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 41911585ae0c..de92fa678924 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -204,6 +204,7 @@ enum vcpu_sysreg {
> SP_EL1,
> SPSR_EL1,
>
> + CNTPOFF_EL2,
> CNTVOFF_EL2,
> CNTV_CVAL_EL0,
> CNTV_CTL_EL0,
> diff --git a/arch/arm64/include/asm/kvm_hyp.h
> b/arch/arm64/include/asm/kvm_hyp.h
> index 9d60b3006efc..01eb3864e50f 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -65,10 +65,8 @@ void __vgic_v3_save_aprs(struct vgic_v3_cpu_if *cpu_if);
> void __vgic_v3_restore_aprs(struct vgic_v3_cpu_if *cpu_if);
> int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu);
>
> -#ifdef __KVM_NVHE_HYPERVISOR__
> void __timer_enable_traps(struct kvm_vcpu *vcpu);
> void __timer_disable_traps(struct kvm_vc
[PATCH v5 11/13] KVM: arm64: Provide userspace access to the physical counter offset
Presently, KVM provides no facilities for correctly migrating a guest
that depends on the physical counter-timer. While most guests (barring
NV, of course) should not depend on the physical counter-timer, an
operator may still wish to provide a consistent view of the physical
counter-timer across migrations.
Provide userspace with a new vCPU attribute to modify the guest physical
counter-timer offset. Since the base architecture doesn't provide a
physical counter-timer offset register, emulate the correct behavior by
trapping accesses to the physical counter-timer whenever the offset
value is non-zero.
Uphold the same behavior as CNTVOFF_EL2 and broadcast the physical
offset to all vCPUs whenever written. This guarantees that the
counter-timer we provide the guest remains architectural, wherein all
views of the counter-timer are consistent across vCPUs. Reconfigure
timer traps for VHE on every guest entry, as different VMs will now have
different traps enabled. Enable physical counter traps for nVHE whenever
the offset is nonzero (we already trap physical timer registers in
nVHE).
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 :)
Reviewed-by: Andrew Jones
Signed-off-by: Oliver Upton
---
Documentation/virt/kvm/devices/vcpu.rst | 22 +++
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/include/asm/kvm_hyp.h | 2 -
arch/arm64/include/asm/sysreg.h | 1 +
arch/arm64/include/uapi/asm/kvm.h | 1 +
arch/arm64/kvm/arch_timer.c | 72 ++-
arch/arm64/kvm/arm.c | 4 +-
arch/arm64/kvm/hyp/include/hyp/switch.h | 23
arch/arm64/kvm/hyp/include/hyp/timer-sr.h | 26
arch/arm64/kvm/hyp/nvhe/switch.c | 2 -
arch/arm64/kvm/hyp/nvhe/timer-sr.c| 21 +++
arch/arm64/kvm/hyp/vhe/timer-sr.c | 27 +
include/kvm/arm_arch_timer.h | 2 -
13 files changed, 158 insertions(+), 46 deletions(-)
create mode 100644 arch/arm64/kvm/hyp/include/hyp/timer-sr.h
diff --git a/Documentation/virt/kvm/devices/vcpu.rst
b/Documentation/virt/kvm/devices/vcpu.rst
index ecbab7adc602..2756e3c09ab9 100644
--- a/Documentation/virt/kvm/devices/vcpu.rst
+++ b/Documentation/virt/kvm/devices/vcpu.rst
@@ -161,6 +161,28 @@ the following equation:
KVM does not allow the use of varying offset values for different vCPUs;
the last written offset value will be broadcasted to all vCPUs in a VM.
+2.3. ATTRIBUTE: KVM_ARM_VCPU_TIMER_OFFSET_PTIMER
+
+
+:Parameters: Pointer to a 64-bit unsigned counter-timer offset.
+
+Returns:
+
+ === ==
+ -EFAULT Error reading/writing the provided
+ parameter address
+ -ENXIO Attribute not supported
+ === ==
+
+Specifies the guest's physical counter-timer offset from the host's
+virtual counter. The guest's physical counter is then derived by
+the following equation:
+
+ guest_cntpct = host_cntvct - KVM_ARM_VCPU_TIMER_OFFSET_PTIMER
+
+KVM does not allow the use of varying offset values for different vCPUs;
+the last written offset value will be broadcasted to all vCPUs in a VM.
+
3. GROUP: KVM_ARM_VCPU_PVTIME_CTRL
==
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 41911585ae0c..de92fa678924 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -204,6 +204,7 @@ enum vcpu_sysreg {
SP_EL1,
SPSR_EL1,
+ CNTPOFF_EL2,
CNTVOFF_EL2,
CNTV_CVAL_EL0,
CNTV_CTL_EL0,
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 9d60b3006efc..01eb3864e50f 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -65,10 +65,8 @@ void __vgic_v3_save_aprs(struct vgic_v3_cpu_if *cpu_if);
void __vgic_v3_restore_aprs(struct vgic_v3_cpu_if *cpu_if);
int __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu);
-#ifdef __KVM_NVHE_HYPERVISOR__
void __timer_enable_traps(struct kvm_vcpu *vcpu);
void __timer_disable_traps(struct kvm_vcpu *vcpu);
-#endif
#ifdef __KVM_NVHE_HYPERVISOR__
void __sysreg_save_state_nvhe(struct kvm_cpu_context *ctxt);
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 7b9c3acba684..d3890a44b7f7 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -505,6 +505,7 @@
#define SYS_AMEVCNTR0_MEM_STALLSYS_AMEVCNTR0_EL0(3)
#define SYS_CNTFRQ_EL0 sys_reg(3, 3, 14, 0, 0)
+#define SYS_CNTPCT_EL0 sys_reg(3, 3, 14, 0, 1)
#define SYS_CNTP_TVAL_EL0 sys_reg(3, 3, 14, 2, 0)
#define SYS_CNTP_CTL_EL0
