| Message ID | 20180112120747.27999-18-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Christoffer, On 01/12/2018 12:07 PM, Christoffer Dall wrote: > The VHE switch function calls __timer_enable_traps and > __timer_disable_traps which don't do anything on VHE systems. > Therefore, simply remove these calls from the VHE switch function and > make the functions non-conditional as they are now only called from the > non-VHE switch path. > > Acked-by: Marc Zyngier <marc.zyngier@arm.com> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > arch/arm64/kvm/hyp/switch.c | 2 -- > virt/kvm/arm/hyp/timer-sr.c | 44 ++++++++++++++++++++++---------------------- > 2 files changed, 22 insertions(+), 24 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 9aadef6966bf..6175fcb33ed2 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -354,7 +354,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > __activate_vm(vcpu->kvm); > > __vgic_restore_state(vcpu); > - __timer_enable_traps(vcpu); > > /* > * We must restore the 32-bit state before the sysregs, thanks > @@ -373,7 +372,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > > __sysreg_save_guest_state(guest_ctxt); > __sysreg32_save_state(vcpu); > - __timer_disable_traps(vcpu); > __vgic_save_state(vcpu); > > __deactivate_traps(vcpu); > diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c > index f24404b3c8df..77754a62eb0c 100644 > --- a/virt/kvm/arm/hyp/timer-sr.c > +++ b/virt/kvm/arm/hyp/timer-sr.c > @@ -27,34 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, u32 cntvoff_high) > write_sysreg(cntvoff, cntvoff_el2); > } > > +/* > + * Should only be called on non-VHE systems. > + * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > + */ > void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu) Would it be worth to suffix the function with nvhe? So it would be clear that it should not be called for VHE system? > { > - /* > - * We don't need to do this for VHE since the host kernel runs in EL2 > - * with HCR_EL2.TGE ==1, which makes those bits have no impact. > - */ > - if (!has_vhe()) { > - u64 val; > + u64 val; > > - /* Allow physical timer/counter access for the host */ > - val = read_sysreg(cnthctl_el2); > - val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; > - write_sysreg(val, cnthctl_el2); > - } > + /* Allow physical timer/counter access for the host */ > + val = read_sysreg(cnthctl_el2); > + val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; > + write_sysreg(val, cnthctl_el2); > } > > +/* > + * Should only be called on non-VHE systems. > + * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > + */ > void __hyp_text __timer_enable_traps(struct kvm_vcpu *vcpu) Same here. > { > - if (!has_vhe()) { > - u64 val; > + u64 val; > > - /* > - * Disallow physical timer access for the guest > - * Physical counter access is allowed > - */ > - val = read_sysreg(cnthctl_el2); > - val &= ~CNTHCTL_EL1PCEN; > - val |= CNTHCTL_EL1PCTEN; > - write_sysreg(val, cnthctl_el2); > - } > + /* > + * Disallow physical timer access for the guest > + * Physical counter access is allowed > + */ > + val = read_sysreg(cnthctl_el2); > + val &= ~CNTHCTL_EL1PCEN; > + val |= CNTHCTL_EL1PCTEN; > + write_sysreg(val, cnthctl_el2); > } > Cheers,
On Fri, Feb 09, 2018 at 05:53:43PM +0000, Julien Grall wrote: > Hi Christoffer, > > On 01/12/2018 12:07 PM, Christoffer Dall wrote: > >The VHE switch function calls __timer_enable_traps and > >__timer_disable_traps which don't do anything on VHE systems. > >Therefore, simply remove these calls from the VHE switch function and > >make the functions non-conditional as they are now only called from the > >non-VHE switch path. > > > >Acked-by: Marc Zyngier <marc.zyngier@arm.com> > >Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > >--- > > arch/arm64/kvm/hyp/switch.c | 2 -- > > virt/kvm/arm/hyp/timer-sr.c | 44 ++++++++++++++++++++++---------------------- > > 2 files changed, 22 insertions(+), 24 deletions(-) > > > >diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > >index 9aadef6966bf..6175fcb33ed2 100644 > >--- a/arch/arm64/kvm/hyp/switch.c > >+++ b/arch/arm64/kvm/hyp/switch.c > >@@ -354,7 +354,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > > __activate_vm(vcpu->kvm); > > __vgic_restore_state(vcpu); > >- __timer_enable_traps(vcpu); > > /* > > * We must restore the 32-bit state before the sysregs, thanks > >@@ -373,7 +372,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > > __sysreg_save_guest_state(guest_ctxt); > > __sysreg32_save_state(vcpu); > >- __timer_disable_traps(vcpu); > > __vgic_save_state(vcpu); > > __deactivate_traps(vcpu); > >diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c > >index f24404b3c8df..77754a62eb0c 100644 > >--- a/virt/kvm/arm/hyp/timer-sr.c > >+++ b/virt/kvm/arm/hyp/timer-sr.c > >@@ -27,34 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, u32 cntvoff_high) > > write_sysreg(cntvoff, cntvoff_el2); > > } > >+/* > >+ * Should only be called on non-VHE systems. > >+ * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > >+ */ > > void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu) > > Would it be worth to suffix the function with nvhe? So it would be clear > that it should not be called for VHE system? > > > { > >- /* > >- * We don't need to do this for VHE since the host kernel runs in EL2 > >- * with HCR_EL2.TGE ==1, which makes those bits have no impact. > >- */ > >- if (!has_vhe()) { > >- u64 val; > >+ u64 val; > >- /* Allow physical timer/counter access for the host */ > >- val = read_sysreg(cnthctl_el2); > >- val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; > >- write_sysreg(val, cnthctl_el2); > >- } > >+ /* Allow physical timer/counter access for the host */ > >+ val = read_sysreg(cnthctl_el2); > >+ val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; > >+ write_sysreg(val, cnthctl_el2); > > } > >+/* > >+ * Should only be called on non-VHE systems. > >+ * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > >+ */ > > void __hyp_text __timer_enable_traps(struct kvm_vcpu *vcpu) > > Same here. > I'll have a look. Thanks, -Christoffer
On Fri, Feb 09, 2018 at 05:53:43PM +0000, Julien Grall wrote: > Hi Christoffer, > > On 01/12/2018 12:07 PM, Christoffer Dall wrote: > >The VHE switch function calls __timer_enable_traps and > >__timer_disable_traps which don't do anything on VHE systems. > >Therefore, simply remove these calls from the VHE switch function and > >make the functions non-conditional as they are now only called from the > >non-VHE switch path. > > > >Acked-by: Marc Zyngier <marc.zyngier@arm.com> > >Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > >--- > > arch/arm64/kvm/hyp/switch.c | 2 -- > > virt/kvm/arm/hyp/timer-sr.c | 44 ++++++++++++++++++++++---------------------- > > 2 files changed, 22 insertions(+), 24 deletions(-) > > > >diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > >index 9aadef6966bf..6175fcb33ed2 100644 > >--- a/arch/arm64/kvm/hyp/switch.c > >+++ b/arch/arm64/kvm/hyp/switch.c > >@@ -354,7 +354,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > > __activate_vm(vcpu->kvm); > > __vgic_restore_state(vcpu); > >- __timer_enable_traps(vcpu); > > /* > > * We must restore the 32-bit state before the sysregs, thanks > >@@ -373,7 +372,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) > > __sysreg_save_guest_state(guest_ctxt); > > __sysreg32_save_state(vcpu); > >- __timer_disable_traps(vcpu); > > __vgic_save_state(vcpu); > > __deactivate_traps(vcpu); > >diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c > >index f24404b3c8df..77754a62eb0c 100644 > >--- a/virt/kvm/arm/hyp/timer-sr.c > >+++ b/virt/kvm/arm/hyp/timer-sr.c > >@@ -27,34 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, u32 cntvoff_high) > > write_sysreg(cntvoff, cntvoff_el2); > > } > >+/* > >+ * Should only be called on non-VHE systems. > >+ * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). > >+ */ > > void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu) > > Would it be worth to suffix the function with nvhe? So it would be clear > that it should not be called for VHE system? > Actually, I decided against this, because it's also called from the 32-bit code and it looks a little strange there, and it's not like we have an equivalent _vhe version. Thanks, -Christoffer
Hi Christoffer, Sorry for the late reply. On 13/02/18 22:31, Christoffer Dall wrote: > On Fri, Feb 09, 2018 at 05:53:43PM +0000, Julien Grall wrote: >> Hi Christoffer, >> >> On 01/12/2018 12:07 PM, Christoffer Dall wrote: >>> The VHE switch function calls __timer_enable_traps and >>> __timer_disable_traps which don't do anything on VHE systems. >>> Therefore, simply remove these calls from the VHE switch function and >>> make the functions non-conditional as they are now only called from the >>> non-VHE switch path. >>> >>> Acked-by: Marc Zyngier <marc.zyngier@arm.com> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> >>> --- >>> arch/arm64/kvm/hyp/switch.c | 2 -- >>> virt/kvm/arm/hyp/timer-sr.c | 44 ++++++++++++++++++++++---------------------- >>> 2 files changed, 22 insertions(+), 24 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c >>> index 9aadef6966bf..6175fcb33ed2 100644 >>> --- a/arch/arm64/kvm/hyp/switch.c >>> +++ b/arch/arm64/kvm/hyp/switch.c >>> @@ -354,7 +354,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) >>> __activate_vm(vcpu->kvm); >>> __vgic_restore_state(vcpu); >>> - __timer_enable_traps(vcpu); >>> /* >>> * We must restore the 32-bit state before the sysregs, thanks >>> @@ -373,7 +372,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) >>> __sysreg_save_guest_state(guest_ctxt); >>> __sysreg32_save_state(vcpu); >>> - __timer_disable_traps(vcpu); >>> __vgic_save_state(vcpu); >>> __deactivate_traps(vcpu); >>> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c >>> index f24404b3c8df..77754a62eb0c 100644 >>> --- a/virt/kvm/arm/hyp/timer-sr.c >>> +++ b/virt/kvm/arm/hyp/timer-sr.c >>> @@ -27,34 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, u32 cntvoff_high) >>> write_sysreg(cntvoff, cntvoff_el2); >>> } >>> +/* >>> + * Should only be called on non-VHE systems. >>> + * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). >>> + */ >>> void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu) >> >> Would it be worth to suffix the function with nvhe? So it would be clear >> that it should not be called for VHE system? >> > Actually, I decided against this, because it's also called from the > 32-bit code and it looks a little strange there, and it's not like we > have an equivalent _vhe version. The main goal was to provide a naming that would prevent someone to use it in VHE case. This would have also been inline with other patches where you rename some helpers to nvhe/vhe even in arm32 code. Anyway, I guess the reviewers will be careful enough to spot that :). Cheers,
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 9aadef6966bf..6175fcb33ed2 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -354,7 +354,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) __activate_vm(vcpu->kvm); __vgic_restore_state(vcpu); - __timer_enable_traps(vcpu); /* * We must restore the 32-bit state before the sysregs, thanks @@ -373,7 +372,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu) __sysreg_save_guest_state(guest_ctxt); __sysreg32_save_state(vcpu); - __timer_disable_traps(vcpu); __vgic_save_state(vcpu); __deactivate_traps(vcpu); diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c index f24404b3c8df..77754a62eb0c 100644 --- a/virt/kvm/arm/hyp/timer-sr.c +++ b/virt/kvm/arm/hyp/timer-sr.c @@ -27,34 +27,34 @@ void __hyp_text __kvm_timer_set_cntvoff(u32 cntvoff_low, u32 cntvoff_high) write_sysreg(cntvoff, cntvoff_el2); } +/* + * Should only be called on non-VHE systems. + * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). + */ void __hyp_text __timer_disable_traps(struct kvm_vcpu *vcpu) { - /* - * We don't need to do this for VHE since the host kernel runs in EL2 - * with HCR_EL2.TGE ==1, which makes those bits have no impact. - */ - if (!has_vhe()) { - u64 val; + u64 val; - /* Allow physical timer/counter access for the host */ - val = read_sysreg(cnthctl_el2); - val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; - write_sysreg(val, cnthctl_el2); - } + /* Allow physical timer/counter access for the host */ + val = read_sysreg(cnthctl_el2); + val |= CNTHCTL_EL1PCTEN | CNTHCTL_EL1PCEN; + write_sysreg(val, cnthctl_el2); } +/* + * Should only be called on non-VHE systems. + * VHE systems use EL2 timers and configure EL1 timers in kvm_timer_init_vhe(). + */ void __hyp_text __timer_enable_traps(struct kvm_vcpu *vcpu) { - if (!has_vhe()) { - u64 val; + u64 val; - /* - * Disallow physical timer access for the guest - * Physical counter access is allowed - */ - val = read_sysreg(cnthctl_el2); - val &= ~CNTHCTL_EL1PCEN; - val |= CNTHCTL_EL1PCTEN; - write_sysreg(val, cnthctl_el2); - } + /* + * Disallow physical timer access for the guest + * Physical counter access is allowed + */ + val = read_sysreg(cnthctl_el2); + val &= ~CNTHCTL_EL1PCEN; + val |= CNTHCTL_EL1PCTEN; + write_sysreg(val, cnthctl_el2); }