| Message ID | 20191130031443.41696-1-guoheyi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm/arm64: change gicv3_cpuif to static likely branch | expand |
On Sat, 30 Nov 2019 03:14:43 +0000, Heyi Guo <guoheyi@huawei.com> wrote: > > Platforms running hypervisor nowadays are normally powerful servers > which at least support GICv3, so it should be better to switch > kvm_vgic_global_state.gicv3_cpuif to static likely branch, which can > reduce two "b" instructions to a single "nop" for GICv3 branches. > > We don't update arm32 specific code for they may still only have > GICv2. There is a number of disputable statements here. Out of the fairly large zoo of arm64 systems I have access to, 75% of them are based on GICv2, so they are still the overwhelming majority. Yes, they all run KVM (otherwise I would ignore them). Furthermore, I would expect that "powerful servers" are perfectly capable to execute a couple of branches without breaking a sweat. Finally, you don't provide any number supporting that: - GICv3 systems see a performance improvement across the large variety of CPU implementations - GICv2 systems don't see a performance regression Once you provide such numbers, I'll reevaluate my position. Until then, I'm not considering this kind of change. Thanks, M.
On 2019/11/30 14:39, Marc Zyngier wrote: > On Sat, 30 Nov 2019 03:14:43 +0000, > Heyi Guo <guoheyi@huawei.com> wrote: >> Platforms running hypervisor nowadays are normally powerful servers >> which at least support GICv3, so it should be better to switch >> kvm_vgic_global_state.gicv3_cpuif to static likely branch, which can >> reduce two "b" instructions to a single "nop" for GICv3 branches. >> >> We don't update arm32 specific code for they may still only have >> GICv2. > There is a number of disputable statements here. > > Out of the fairly large zoo of arm64 systems I have access to, 75% of > them are based on GICv2, so they are still the overwhelming majority. > Yes, they all run KVM (otherwise I would ignore them). Really? I'm surprised to know that... Sorry I didn't see such GICv2 platforms in my work, so I made the wrong assumption. I don't expect much performance improvement for GICv3 platforms. The precondition for this patch is that few platforms running KVM are using GICv2. If it is not right, please just ignore it. Thanks, HG > > Furthermore, I would expect that "powerful servers" are perfectly > capable to execute a couple of branches without breaking a sweat. > > Finally, you don't provide any number supporting that: > > - GICv3 systems see a performance improvement across the large variety > of CPU implementations > - GICv2 systems don't see a performance regression > > Once you provide such numbers, I'll reevaluate my position. Until > then, I'm not considering this kind of change. > > Thanks, > > M. >
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 799e84a40335..57e7d314211a 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -219,7 +219,7 @@ static void __hyp_text __deactivate_vm(struct kvm_vcpu *vcpu) /* Save VGICv3 state on non-VHE systems */ static void __hyp_text __hyp_vgic_save_state(struct kvm_vcpu *vcpu) { - if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) { + if (static_branch_likely(&kvm_vgic_global_state.gicv3_cpuif)) { __vgic_v3_save_state(vcpu); __vgic_v3_deactivate_traps(vcpu); } @@ -228,7 +228,7 @@ static void __hyp_text __hyp_vgic_save_state(struct kvm_vcpu *vcpu) /* Restore VGICv3 state on non_VEH systems */ static void __hyp_text __hyp_vgic_restore_state(struct kvm_vcpu *vcpu) { - if (static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) { + if (static_branch_likely(&kvm_vgic_global_state.gicv3_cpuif)) { __vgic_v3_activate_traps(vcpu); __vgic_v3_restore_state(vcpu); } diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h index af4f09c02bf1..474e73dd3112 100644 --- a/include/kvm/arm_vgic.h +++ b/include/kvm/arm_vgic.h @@ -72,7 +72,7 @@ struct vgic_global { bool has_gicv4; /* GIC system register CPU interface */ - struct static_key_false gicv3_cpuif; + struct static_key_true gicv3_cpuif; u32 ich_vtr_el2; }; diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c index 6f50c429196d..b03e5c8e1731 100644 --- a/virt/kvm/arm/vgic/vgic-init.c +++ b/virt/kvm/arm/vgic/vgic-init.c @@ -509,13 +509,14 @@ int kvm_vgic_hyp_init(void) switch (gic_kvm_info->type) { case GIC_V2: ret = vgic_v2_probe(gic_kvm_info); + if (!ret) { + static_branch_disable( + &kvm_vgic_global_state.gicv3_cpuif); + kvm_info("GIC system register CPU interface disabled\n"); + } break; case GIC_V3: ret = vgic_v3_probe(gic_kvm_info); - if (!ret) { - static_branch_enable(&kvm_vgic_global_state.gicv3_cpuif); - kvm_info("GIC system register CPU interface enabled\n"); - } break; default: ret = -ENODEV; diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index 45a870cb63f5..9dafeeb1457b 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c @@ -18,7 +18,7 @@ #include "trace.h" struct vgic_global kvm_vgic_global_state __ro_after_init = { - .gicv3_cpuif = STATIC_KEY_FALSE_INIT, + .gicv3_cpuif = STATIC_KEY_TRUE_INIT, }; /* @@ -841,12 +841,13 @@ static inline bool can_access_vgic_from_kernel(void) * memory-mapped, and VHE systems can access GICv3 EL2 system * registers. */ - return !static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif) || has_vhe(); + return !static_branch_likely(&kvm_vgic_global_state.gicv3_cpuif) || + has_vhe(); } static inline void vgic_save_state(struct kvm_vcpu *vcpu) { - if (!static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) + if (!static_branch_likely(&kvm_vgic_global_state.gicv3_cpuif)) vgic_v2_save_state(vcpu); else __vgic_v3_save_state(vcpu); @@ -873,7 +874,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu) static inline void vgic_restore_state(struct kvm_vcpu *vcpu) { - if (!static_branch_unlikely(&kvm_vgic_global_state.gicv3_cpuif)) + if (!static_branch_likely(&kvm_vgic_global_state.gicv3_cpuif)) vgic_v2_restore_state(vcpu); else __vgic_v3_restore_state(vcpu);
Platforms running hypervisor nowadays are normally powerful servers which at least support GICv3, so it should be better to switch kvm_vgic_global_state.gicv3_cpuif to static likely branch, which can reduce two "b" instructions to a single "nop" for GICv3 branches. We don't update arm32 specific code for they may still only have GICv2. Signed-off-by: Heyi Guo <guoheyi@huawei.com> Cc: Marc Zyngier <maz@kernel.org> Cc: James Morse <james.morse@arm.com> Cc: Julien Thierry <julien.thierry.kdev@gmail.com> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Will Deacon <will@kernel.org> --- arch/arm64/kvm/hyp/switch.c | 4 ++-- include/kvm/arm_vgic.h | 2 +- virt/kvm/arm/vgic/vgic-init.c | 9 +++++---- virt/kvm/arm/vgic/vgic.c | 9 +++++---- 4 files changed, 13 insertions(+), 11 deletions(-)