| Message ID | 20241202172134.384923-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: Add NV timer support | expand |
typo: accelerate On Mon, Dec 02, 2024 at 05:21:29PM +0000, Marc Zyngier wrote: > Similarly to handling the physical timer accesses early when FEAT_ECV > causes a trap, we try to handle the physical counter without returning > to the general sysreg handling. > > More surprisingly, we introduce something similar for the virtual > counter. Although this isn't necessary yet, it will prove useful on > systems that have a broken CNTVOFF_EL2 implementation. Yes, they exist. > > Special care is taken to offset reads of the counter with the host's > CNTPOFF_EL2, as we perform this with TGE clear. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++++ > arch/arm64/kvm/hyp/vhe/switch.c | 13 +++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 34f53707892df..30e572de28749 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -501,6 +501,11 @@ static inline bool handle_tx2_tvm(struct kvm_vcpu *vcpu) > return true; > } > > +static inline u64 compute_counter_value(struct arch_timer_context *ctxt) > +{ > + return arch_timer_read_cntpct_el0() - timer_get_offset(ctxt); > +} > + > static bool kvm_hyp_handle_cntpct(struct kvm_vcpu *vcpu) > { > struct arch_timer_context *ctxt; > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c > index b014b0b10bf5d..49815a8a4c9bc 100644 > --- a/arch/arm64/kvm/hyp/vhe/switch.c > +++ b/arch/arm64/kvm/hyp/vhe/switch.c > @@ -296,6 +296,13 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) > val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0); > } > break; > + case SYS_CNTPCT_EL0: > + case SYS_CNTPCTSS_EL0: > + /* If !ELIsInHost(EL0), the guest's CNTPOFF_EL2 applies */ > + val = compute_counter_value(!(vcpu_el2_e2h_is_set(vcpu) && > + vcpu_el2_tge_is_set(vcpu)) ? > + vcpu_ptimer(vcpu) : vcpu_hptimer(vcpu)); > + break; > case SYS_CNTV_CTL_EL02: > val = __vcpu_sys_reg(vcpu, CNTV_CTL_EL0); > break; > @@ -314,6 +321,12 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) > else > val = __vcpu_sys_reg(vcpu, CNTV_CVAL_EL0); > break; > + case SYS_CNTVCT_EL0: > + case SYS_CNTVCTSS_EL0: > + /* If !ELIsInHost(EL2), the guest's CNTVOFF_EL2 applies */ !ELIsInHost(EL0) > + val = compute_counter_value(!vcpu_el2_e2h_is_set(vcpu) ? > + vcpu_vtimer(vcpu) : vcpu_hvtimer(vcpu)); > + break; > default: > return false; > } > -- > 2.39.2 >
On Thu, 05 Dec 2024 00:37:34 +0000, Oliver Upton <oliver.upton@linux.dev> wrote: > > typo: accelerate Huh, thanks! > > On Mon, Dec 02, 2024 at 05:21:29PM +0000, Marc Zyngier wrote: [...] > > + case SYS_CNTVCT_EL0: > > + case SYS_CNTVCTSS_EL0: > > + /* If !ELIsInHost(EL2), the guest's CNTVOFF_EL2 applies */ > > !ELIsInHost(EL0) No, and that's the whole point. CNTVOFF_EL2 applies at all times when HCR_EL2==0 and that we're at EL2. From the pseudocode for CNTVCT_EL0: <quote> [...] elsif PSTATE.EL == EL2 then if !ELIsInHost(EL2) then X[t, 64] = PhysicalCountInt() - CNTVOFF_EL2; else X[t, 64] = PhysicalCountInt(); [...] </quote> Which is why we only check E2H, and not E2H+TGE. It is CNTPOFF_EL2 that applies when !ELIsInHost(EL0), and this is why it cannot be reliably emulated as we don't (and cannot) track changes to HCR_EL2.TGE. Yes, this is nonsense. M.
On Mon, Dec 02, 2024 at 05:21:29PM +0000, Marc Zyngier wrote: > Similarly to handling the physical timer accesses early when FEAT_ECV > causes a trap, we try to handle the physical counter without returning > to the general sysreg handling. > > More surprisingly, we introduce something similar for the virtual > counter. Although this isn't necessary yet, it will prove useful on > systems that have a broken CNTVOFF_EL2 implementation. Yes, they exist. > > Special care is taken to offset reads of the counter with the host's > CNTPOFF_EL2, as we perform this with TGE clear. Can you explain this part a bit more? I'm assuming it's somehow related to the arch_timer_read_cntpct_el0() call. However I think we're at EL2 inside kvm_hyp_handle_timer(), so reading CNTPCT_EL0 won't involve CNTPOFF_EL2. What am I missing/misunderstanding? > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++++ > arch/arm64/kvm/hyp/vhe/switch.c | 13 +++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > index 34f53707892df..30e572de28749 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -501,6 +501,11 @@ static inline bool handle_tx2_tvm(struct kvm_vcpu *vcpu) > return true; > } > > +static inline u64 compute_counter_value(struct arch_timer_context *ctxt) > +{ > + return arch_timer_read_cntpct_el0() - timer_get_offset(ctxt); > +} > + > static bool kvm_hyp_handle_cntpct(struct kvm_vcpu *vcpu) > { > struct arch_timer_context *ctxt; > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c > index b014b0b10bf5d..49815a8a4c9bc 100644 > --- a/arch/arm64/kvm/hyp/vhe/switch.c > +++ b/arch/arm64/kvm/hyp/vhe/switch.c > @@ -296,6 +296,13 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) > val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0); > } > break; > + case SYS_CNTPCT_EL0: > + case SYS_CNTPCTSS_EL0: > + /* If !ELIsInHost(EL0), the guest's CNTPOFF_EL2 applies */ > + val = compute_counter_value(!(vcpu_el2_e2h_is_set(vcpu) && > + vcpu_el2_tge_is_set(vcpu)) ? > + vcpu_ptimer(vcpu) : vcpu_hptimer(vcpu)); > + break; > case SYS_CNTV_CTL_EL02: > val = __vcpu_sys_reg(vcpu, CNTV_CTL_EL0); > break; > @@ -314,6 +321,12 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) > else > val = __vcpu_sys_reg(vcpu, CNTV_CVAL_EL0); > break; > + case SYS_CNTVCT_EL0: > + case SYS_CNTVCTSS_EL0: > + /* If !ELIsInHost(EL2), the guest's CNTVOFF_EL2 applies */ > + val = compute_counter_value(!vcpu_el2_e2h_is_set(vcpu) ? > + vcpu_vtimer(vcpu) : vcpu_hvtimer(vcpu)); > + break; > default: > return false; > } Thanks, Joey
On Thu, 05 Dec 2024 12:07:07 +0000, Joey Gouly <joey.gouly@arm.com> wrote: > > On Mon, Dec 02, 2024 at 05:21:29PM +0000, Marc Zyngier wrote: > > Similarly to handling the physical timer accesses early when FEAT_ECV > > causes a trap, we try to handle the physical counter without returning > > to the general sysreg handling. > > > > More surprisingly, we introduce something similar for the virtual > > counter. Although this isn't necessary yet, it will prove useful on > > systems that have a broken CNTVOFF_EL2 implementation. Yes, they exist. > > > > > Special care is taken to offset reads of the counter with the host's > > CNTPOFF_EL2, as we perform this with TGE clear. > > Can you explain this part a bit more? I'm assuming it's somehow related to the > arch_timer_read_cntpct_el0() call. > > However I think we're at EL2 inside kvm_hyp_handle_timer(), so reading > CNTPCT_EL0 won't involve CNTPOFF_EL2. > > What am I missing/misunderstanding? I think the wording above is particularly misleading, and leads to some bad confusion. Let's go back to basics: we're at host EL2, and handling a trap from guest EL2. Although you are right that CNTPOFF_EL2 doesn't affect a direct read of CNTPCT_EL0 from EL2, we are emulating it as if it was executed from EL1 (guest EL2 being EL1). So any offsetting that would be applied if we were not trapping CNTPCT_EL0 must still be applied. In the case of a read from hypervisor context, this is the VM-wide offset. But this makes me realise that... > > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++++ > > arch/arm64/kvm/hyp/vhe/switch.c | 13 +++++++++++++ > > 2 files changed, 18 insertions(+) > > > > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h > > index 34f53707892df..30e572de28749 100644 > > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > > @@ -501,6 +501,11 @@ static inline bool handle_tx2_tvm(struct kvm_vcpu *vcpu) > > return true; > > } > > > > +static inline u64 compute_counter_value(struct arch_timer_context *ctxt) > > +{ > > + return arch_timer_read_cntpct_el0() - timer_get_offset(ctxt); > > +} > > + > > static bool kvm_hyp_handle_cntpct(struct kvm_vcpu *vcpu) > > { > > struct arch_timer_context *ctxt; > > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c > > index b014b0b10bf5d..49815a8a4c9bc 100644 > > --- a/arch/arm64/kvm/hyp/vhe/switch.c > > +++ b/arch/arm64/kvm/hyp/vhe/switch.c > > @@ -296,6 +296,13 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) > > val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0); > > } > > break; > > + case SYS_CNTPCT_EL0: > > + case SYS_CNTPCTSS_EL0: > > + /* If !ELIsInHost(EL0), the guest's CNTPOFF_EL2 applies */ > > + val = compute_counter_value(!(vcpu_el2_e2h_is_set(vcpu) && > > + vcpu_el2_tge_is_set(vcpu)) ? > > + vcpu_ptimer(vcpu) : vcpu_hptimer(vcpu)); ... this should be simplified, because there is no case where we can be in HYP context *and* not be using the hptimer() context (everything else would be handled by kvm_handle_cntxct()). I'm minded to change this to: diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c index a8b1a23712329..5a79ed0aac613 100644 --- a/arch/arm64/kvm/hyp/vhe/switch.c +++ b/arch/arm64/kvm/hyp/vhe/switch.c @@ -305,10 +305,7 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) break; case SYS_CNTPCT_EL0: case SYS_CNTPCTSS_EL0: - /* If !ELIsInHost(EL0), the guest's CNTPOFF_EL2 applies */ - val = compute_counter_value(!(vcpu_el2_e2h_is_set(vcpu) && - vcpu_el2_tge_is_set(vcpu)) ? - vcpu_ptimer(vcpu) : vcpu_hptimer(vcpu)); + val = compute_counter_value(vcpu_hptimer(vcpu)); break; case SYS_CNTV_CTL_EL02: val = __vcpu_sys_reg(vcpu, CNTV_CTL_EL0); Thanks, M.
On Thu, Dec 05, 2024 at 11:03:41AM +0000, Marc Zyngier wrote: > On Thu, 05 Dec 2024 00:37:34 +0000, > Oliver Upton <oliver.upton@linux.dev> wrote: > > > > typo: accelerate > > Huh, thanks! > > > > > On Mon, Dec 02, 2024 at 05:21:29PM +0000, Marc Zyngier wrote: > > [...] > > > > + case SYS_CNTVCT_EL0: > > > + case SYS_CNTVCTSS_EL0: > > > + /* If !ELIsInHost(EL2), the guest's CNTVOFF_EL2 applies */ > > > > !ELIsInHost(EL0) > > No, and that's the whole point. CNTVOFF_EL2 applies at all times when > HCR_EL2==0 and that we're at EL2. From the pseudocode for CNTVCT_EL0: Dammit, I don't know how many times I misread "CNTPCT" here rather than CNTVCT. Sorry for the noise.
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h index 34f53707892df..30e572de28749 100644 --- a/arch/arm64/kvm/hyp/include/hyp/switch.h +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h @@ -501,6 +501,11 @@ static inline bool handle_tx2_tvm(struct kvm_vcpu *vcpu) return true; } +static inline u64 compute_counter_value(struct arch_timer_context *ctxt) +{ + return arch_timer_read_cntpct_el0() - timer_get_offset(ctxt); +} + static bool kvm_hyp_handle_cntpct(struct kvm_vcpu *vcpu) { struct arch_timer_context *ctxt; diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c index b014b0b10bf5d..49815a8a4c9bc 100644 --- a/arch/arm64/kvm/hyp/vhe/switch.c +++ b/arch/arm64/kvm/hyp/vhe/switch.c @@ -296,6 +296,13 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) val = __vcpu_sys_reg(vcpu, CNTP_CVAL_EL0); } break; + case SYS_CNTPCT_EL0: + case SYS_CNTPCTSS_EL0: + /* If !ELIsInHost(EL0), the guest's CNTPOFF_EL2 applies */ + val = compute_counter_value(!(vcpu_el2_e2h_is_set(vcpu) && + vcpu_el2_tge_is_set(vcpu)) ? + vcpu_ptimer(vcpu) : vcpu_hptimer(vcpu)); + break; case SYS_CNTV_CTL_EL02: val = __vcpu_sys_reg(vcpu, CNTV_CTL_EL0); break; @@ -314,6 +321,12 @@ static bool kvm_hyp_handle_timer(struct kvm_vcpu *vcpu, u64 *exit_code) else val = __vcpu_sys_reg(vcpu, CNTV_CVAL_EL0); break; + case SYS_CNTVCT_EL0: + case SYS_CNTVCTSS_EL0: + /* If !ELIsInHost(EL2), the guest's CNTVOFF_EL2 applies */ + val = compute_counter_value(!vcpu_el2_e2h_is_set(vcpu) ? + vcpu_vtimer(vcpu) : vcpu_hvtimer(vcpu)); + break; default: return false; }
Similarly to handling the physical timer accesses early when FEAT_ECV causes a trap, we try to handle the physical counter without returning to the general sysreg handling. More surprisingly, we introduce something similar for the virtual counter. Although this isn't necessary yet, it will prove useful on systems that have a broken CNTVOFF_EL2 implementation. Yes, they exist. Special care is taken to offset reads of the counter with the host's CNTPOFF_EL2, as we perform this with TGE clear. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++++ arch/arm64/kvm/hyp/vhe/switch.c | 13 +++++++++++++ 2 files changed, 18 insertions(+)