Re: [PATCH v2] KVM: arm64: Remove host_cpu_context member from vcpu structure
On Mon, Jun 08, 2020 at 04:42:42PM +0100, Marc Zyngier wrote: > Hi Andrew, > > On 2020-06-08 15:51, Andrew Scull wrote: > > On Mon, Jun 08, 2020 at 09:56:57AM +0100, Marc Zyngier wrote: > > > For very long, we have kept this pointer back to the per-cpu > > > host state, despite having working per-cpu accessors at EL2 > > > for some time now. > > > > > > Recent investigations have shown that this pointer is easy > > > to abuse in preemptible context, which is a sure sign that > > > it would better be gone. Not to mention that a per-cpu > > > pointer is faster to access at all times. > > > > Helps to make the references to `kvm_host_data` clearer with there now > > being just one way to get to it and shows that it is scoped to the > > current CPU. A good change IMO! > > Thanks! Can I take this as a Reviewed-by or Acked-by tag? Just let me know. Build and booted your kvm-arm64/ptrauth-fixes branch contianing this patch with VHE and nVHE on qemu. Booted a VM within each with kvmtool. Reviewed-by: Andrew Scull ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm64: Remove host_cpu_context member from vcpu structure
On Mon, Jun 08, 2020 at 09:56:57AM +0100, Marc Zyngier wrote: > For very long, we have kept this pointer back to the per-cpu > host state, despite having working per-cpu accessors at EL2 > for some time now. > > Recent investigations have shown that this pointer is easy > to abuse in preemptible context, which is a sure sign that > it would better be gone. Not to mention that a per-cpu > pointer is faster to access at all times. Helps to make the references to `kvm_host_data` clearer with there now being just one way to get to it and shows that it is scoped to the current CPU. A good change IMO! ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] KVM: arm64: Remove host_cpu_context member from vcpu structure
On Mon, Jun 08, 2020 at 09:56:57AM +0100, Marc Zyngier wrote:
> For very long, we have kept this pointer back to the per-cpu
> host state, despite having working per-cpu accessors at EL2
> for some time now.
>
> Recent investigations have shown that this pointer is easy
> to abuse in preemptible context, which is a sure sign that
> it would better be gone. Not to mention that a per-cpu
> pointer is faster to access at all times.
>
> Reported-by: Andrew Scull
> Signed-off-by: Marc Zyngier
>From a quick scan, this looks sane to me, so FWIW:
Acked-by: Mark Rutland
Mark.
> ---
>
> Notes:
> v2: Stick to this_cpu_ptr() in pmu.c, as this only used on the
> kernel side and not the hypervisor.
>
> arch/arm64/include/asm/kvm_host.h | 3 ---
> arch/arm64/kvm/arm.c | 3 ---
> arch/arm64/kvm/hyp/debug-sr.c | 4 ++--
> arch/arm64/kvm/hyp/switch.c | 6 +++---
> arch/arm64/kvm/hyp/sysreg-sr.c| 6 --
> arch/arm64/kvm/pmu.c | 8 ++--
> 6 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 59029e90b557..ada1faa92211 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -284,9 +284,6 @@ struct kvm_vcpu_arch {
> struct kvm_guest_debug_arch vcpu_debug_state;
> struct kvm_guest_debug_arch external_debug_state;
>
> - /* Pointer to host CPU context */
> - struct kvm_cpu_context *host_cpu_context;
> -
> struct thread_info *host_thread_info; /* hyp VA */
> struct user_fpsimd_state *host_fpsimd_state;/* hyp VA */
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 14b747266607..6ddaa23ef346 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -340,10 +340,8 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
> void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> {
> int *last_ran;
> - kvm_host_data_t *cpu_data;
>
> last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
> - cpu_data = this_cpu_ptr(&kvm_host_data);
>
> /*
>* We might get preempted before the vCPU actually runs, but
> @@ -355,7 +353,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> }
>
> vcpu->cpu = cpu;
> - vcpu->arch.host_cpu_context = &cpu_data->host_ctxt;
>
> kvm_vgic_load(vcpu);
> kvm_timer_vcpu_load(vcpu);
> diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c
> index 0fc9872a1467..e95af204fec7 100644
> --- a/arch/arm64/kvm/hyp/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/debug-sr.c
> @@ -185,7 +185,7 @@ void __hyp_text __debug_switch_to_guest(struct kvm_vcpu
> *vcpu)
> if (!(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> return;
>
> - host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> + host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
> guest_ctxt = &vcpu->arch.ctxt;
> host_dbg = &vcpu->arch.host_debug_state.regs;
> guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> @@ -207,7 +207,7 @@ void __hyp_text __debug_switch_to_host(struct kvm_vcpu
> *vcpu)
> if (!(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
> return;
>
> - host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> + host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
> guest_ctxt = &vcpu->arch.ctxt;
> host_dbg = &vcpu->arch.host_debug_state.regs;
> guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index fc09c3dfa466..fc671426c14b 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -544,7 +544,7 @@ static bool __hyp_text __hyp_handle_ptrauth(struct
> kvm_vcpu *vcpu)
> return false;
> }
>
> - ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> + ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
> __ptrauth_save_key(ctxt->sys_regs, APIA);
> __ptrauth_save_key(ctxt->sys_regs, APIB);
> __ptrauth_save_key(ctxt->sys_regs, APDA);
> @@ -715,7 +715,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> struct kvm_cpu_context *guest_ctxt;
> u64 exit_code;
>
> - host_ctxt = vcpu->arch.host_cpu_context;
> + host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
> host_ctxt->__hyp_running_vcpu = vcpu;
> guest_ctxt = &vcpu->arch.ctxt;
>
> @@ -820,7 +820,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
>
> vcpu = kern_hyp_va(vcpu);
>
> - host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
> + host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
> host_ctxt->__hyp_running_vcpu = vcpu;
> guest_ctxt = &vcpu->arch.ctxt;
>
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 6d2df9fe0b5d..143d7b7358f2 100644
> --- a/arch/arm64/kvm/hyp/sysreg-s
Re: [PATCH v2] KVM: arm64: Remove host_cpu_context member from vcpu structure
Hi Andrew, On 2020-06-08 15:51, Andrew Scull wrote: On Mon, Jun 08, 2020 at 09:56:57AM +0100, Marc Zyngier wrote: For very long, we have kept this pointer back to the per-cpu host state, despite having working per-cpu accessors at EL2 for some time now. Recent investigations have shown that this pointer is easy to abuse in preemptible context, which is a sure sign that it would better be gone. Not to mention that a per-cpu pointer is faster to access at all times. Helps to make the references to `kvm_host_data` clearer with there now being just one way to get to it and shows that it is scoped to the current CPU. A good change IMO! Thanks! Can I take this as a Reviewed-by or Acked-by tag? Just let me know. Cheers, M. -- Jazz is not dead. It just smells funny... ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v2] KVM: arm64: Remove host_cpu_context member from vcpu structure
For very long, we have kept this pointer back to the per-cpu
host state, despite having working per-cpu accessors at EL2
for some time now.
Recent investigations have shown that this pointer is easy
to abuse in preemptible context, which is a sure sign that
it would better be gone. Not to mention that a per-cpu
pointer is faster to access at all times.
Reported-by: Andrew Scull
Signed-off-by: Marc Zyngier
---
Notes:
v2: Stick to this_cpu_ptr() in pmu.c, as this only used on the
kernel side and not the hypervisor.
arch/arm64/include/asm/kvm_host.h | 3 ---
arch/arm64/kvm/arm.c | 3 ---
arch/arm64/kvm/hyp/debug-sr.c | 4 ++--
arch/arm64/kvm/hyp/switch.c | 6 +++---
arch/arm64/kvm/hyp/sysreg-sr.c| 6 --
arch/arm64/kvm/pmu.c | 8 ++--
6 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h
b/arch/arm64/include/asm/kvm_host.h
index 59029e90b557..ada1faa92211 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -284,9 +284,6 @@ struct kvm_vcpu_arch {
struct kvm_guest_debug_arch vcpu_debug_state;
struct kvm_guest_debug_arch external_debug_state;
- /* Pointer to host CPU context */
- struct kvm_cpu_context *host_cpu_context;
-
struct thread_info *host_thread_info; /* hyp VA */
struct user_fpsimd_state *host_fpsimd_state;/* hyp VA */
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 14b747266607..6ddaa23ef346 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -340,10 +340,8 @@ void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
{
int *last_ran;
- kvm_host_data_t *cpu_data;
last_ran = this_cpu_ptr(vcpu->kvm->arch.last_vcpu_ran);
- cpu_data = this_cpu_ptr(&kvm_host_data);
/*
* We might get preempted before the vCPU actually runs, but
@@ -355,7 +353,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
}
vcpu->cpu = cpu;
- vcpu->arch.host_cpu_context = &cpu_data->host_ctxt;
kvm_vgic_load(vcpu);
kvm_timer_vcpu_load(vcpu);
diff --git a/arch/arm64/kvm/hyp/debug-sr.c b/arch/arm64/kvm/hyp/debug-sr.c
index 0fc9872a1467..e95af204fec7 100644
--- a/arch/arm64/kvm/hyp/debug-sr.c
+++ b/arch/arm64/kvm/hyp/debug-sr.c
@@ -185,7 +185,7 @@ void __hyp_text __debug_switch_to_guest(struct kvm_vcpu
*vcpu)
if (!(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
return;
- host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+ host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
guest_ctxt = &vcpu->arch.ctxt;
host_dbg = &vcpu->arch.host_debug_state.regs;
guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
@@ -207,7 +207,7 @@ void __hyp_text __debug_switch_to_host(struct kvm_vcpu
*vcpu)
if (!(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY))
return;
- host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+ host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
guest_ctxt = &vcpu->arch.ctxt;
host_dbg = &vcpu->arch.host_debug_state.regs;
guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index fc09c3dfa466..fc671426c14b 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -544,7 +544,7 @@ static bool __hyp_text __hyp_handle_ptrauth(struct kvm_vcpu
*vcpu)
return false;
}
- ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+ ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
__ptrauth_save_key(ctxt->sys_regs, APIA);
__ptrauth_save_key(ctxt->sys_regs, APIB);
__ptrauth_save_key(ctxt->sys_regs, APDA);
@@ -715,7 +715,7 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
struct kvm_cpu_context *guest_ctxt;
u64 exit_code;
- host_ctxt = vcpu->arch.host_cpu_context;
+ host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
host_ctxt->__hyp_running_vcpu = vcpu;
guest_ctxt = &vcpu->arch.ctxt;
@@ -820,7 +820,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
vcpu = kern_hyp_va(vcpu);
- host_ctxt = kern_hyp_va(vcpu->arch.host_cpu_context);
+ host_ctxt = &__hyp_this_cpu_ptr(kvm_host_data)->host_ctxt;
host_ctxt->__hyp_running_vcpu = vcpu;
guest_ctxt = &vcpu->arch.ctxt;
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 6d2df9fe0b5d..143d7b7358f2 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -265,12 +265,13 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu
*vcpu)
*/
void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
{
- struct kvm_cpu_context *host_ctxt = vcpu->arch.host_cpu_context;
struct kvm_cpu_context *gu
