[PATCH] kvm/arm64: change gicv3_cpuif to static likely branch

[PATCH] kvm/arm64: change gicv3_cpuif to static likely branch

[Heyi Guo]

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(-)

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);
-- 
2.19.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] kvm/arm64: change gicv3_cpuif to static likely branch

[Marc Zyngier]

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.

-- 
Jazz is not dead, it just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] kvm/arm64: change gicv3_cpuif to static likely branch

[Guoheyi]

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.
>


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm