Re: [PATCH v3 1/8] arm64: KVM: Use static keys for selecting the GIC backend

[Vladimir Murzin <vladimir.murzin@arm.com>]

On 09/09/16 15:17, Marc Zyngier wrote:
> On 09/09/16 14:45, Vladimir Murzin wrote:
>> On 09/09/16 10:33, Vladimir Murzin wrote:
>>> Hi Marc,
>>>
>>> On 09/09/16 10:19, Marc Zyngier wrote:
>>>>> Hi Vladimir,
>>>>>
>>> ...
>>>>>>>  
>>>>>>> +extern struct static_key_false kvm_gicv3_cpuif;
>>>>>
>>>>> I think we should follow the model set by kvm_vgic_global_state, which
>>>>> is declared in arm_vgic.h. Even better, we should *embed* the static key
>>>>> in this structure. This will reduce the clutter and we wouldn't have to
>>>>> deal with all the section stuff (the hyp_data thing is a good cleanup,
>>>>> but I'd like to see it as a separate patch if possible).
>>> Yes, it is what I was thinking about too, but was not sure about which
>>> way to go, so hyp_data seemed me something we might reuse latter.
>>> However, I agree that we can defer hyp_data thing...
>>>
>>
>> I've just tried it out and it seems that static keys are not happy to
>> accept a key after kern_hyp_va is applied at &kvm_vgic_global_state:
> 
> Ah, there is a trick. You do not need kern_hyp_va at all, because this
> is not evaluated as an expression at runtime (so the pointer doesn't matter).
> 

Ah, right, thank for a tip! ;)

>>> In file included from ./include/linux/jump_label.h:105:0,
>>>                  from arch/arm64/kvm/hyp/switch.c:19:
>>> ./arch/arm64/include/asm/jump_label.h: In function ‘__guest_run’:
>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn’t match constraints
>>>   asm goto("1: nop\n\t"
>>>   ^
>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn’t match constraints
>>>   asm goto("1: nop\n\t"
>>>   ^
>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in ‘asm’
>>>   asm goto("1: nop\n\t"
>>>   ^
>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in ‘asm’
>>>   asm goto("1: nop\n\t"
>>>   ^
>>> make[1]: *** [arch/arm64/kvm/hyp/switch.o] Error 1
>>> make: *** [arch/arm64/kvm/hyp/switch.o] Error 2
>>
>> it looks like we cannot avoid hyp_data thing... if you don't mind I can
>> do hyp_data clean-up in separate patch. Alternatively, we can do
>> conversion to static keys for both architectures later as an
>> optimisation step.
> 
> Can you try the above first? I've just tried the same approach with my
> vgic-trap series, and it compiles fine (untested though):

I was about to try it out, but didn't manage to find a branch with
vgic-trap series, so I did a quick fixup for my series and now it is
running non-VHE boot tests and I don't expect issues with VHE one. I
think diff bellow should work runtime too, but if you do want me to give
it a try it'd be handy to have a branch I can pull from ;)

> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 194184c..397e240 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -301,7 +301,7 @@ again:
>  	if (exit_code == ARM_EXCEPTION_TRAP && !__populate_fault_info(vcpu))
>  		goto again;
>  
> -	if (static_branch_unlikely(&vgic_v2_cpuif_trap) &&
> +	if (static_branch_unlikely(&kvm_vgic_global_state.vgic_v2_cpuif_trap) &&
>  	    exit_code == ARM_EXCEPTION_TRAP) {
>  		bool valid;
>  
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index bb46c03..cc59a28 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -67,6 +67,9 @@ struct vgic_global {
>  
>  	/* Only needed for the legacy KVM_CREATE_IRQCHIP */
>  	bool			can_emulate_gicv2;
> +
> +	/* Trapping GICv2 GICV region? */
> +	struct static_key_false	vgic_v2_cpuif_trap;
>  };
>  
>  extern struct vgic_global kvm_vgic_global_state;
> @@ -269,8 +272,6 @@ struct vgic_cpu {
>  	bool lpis_enabled;
>  };
>  
> -extern struct static_key_false vgic_v2_cpuif_trap;
> -
>  int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
>  void kvm_vgic_early_init(struct kvm *kvm);
>  int kvm_vgic_create(struct kvm *kvm, u32 type);
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index 0a063af..4af600e 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -278,7 +278,7 @@ int vgic_v2_map_resources(struct kvm *kvm)
>  		goto out;
>  	}
>  
> -	if (!static_branch_unlikely(&vgic_v2_cpuif_trap)) {
> +	if (!static_branch_unlikely(&kvm_vgic_global_state.vgic_v2_cpuif_trap)) {
>  		ret = kvm_phys_addr_ioremap(kvm, dist->vgic_cpu_base,
>  					    kvm_vgic_global_state.vcpu_base,
>  					    KVM_VGIC_V2_CPU_SIZE, true);
> @@ -296,8 +296,6 @@ out:
>  	return ret;
>  }
>  
> -DEFINE_STATIC_KEY_FALSE(vgic_v2_cpuif_trap);
> -
>  /**
>   * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
>   * @node:	pointer to the DT node
> @@ -314,6 +312,8 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
>  		return -ENXIO;
>  	}
>  
> +	kvm_vgic_global_state.vgic_v2_cpuif_trap = STATIC_KEY_FALSE_INIT;
> +

I did this a bit different:

+struct vgic_global __section(.hyp.text) kvm_vgic_global_state =
{.gicv3_cpuif = STATIC_KEY_FALSE_INIT,};

Thanks
Vladimir

>  	if (!PAGE_ALIGNED(info->vcpu.start) ||
>  	    !PAGE_ALIGNED(resource_size(&info->vcpu))) {
>  		kvm_info("GICV region size/alignment is unsafe, using trapping (reduced performance)\n");
> @@ -332,7 +332,7 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
>  			goto out;
>  		}
>  
> -		static_branch_enable(&vgic_v2_cpuif_trap);
> +		static_branch_enable(&kvm_vgic_global_state.vgic_v2_cpuif_trap);
>  	}
>  
>  	kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start,
> 
> 
> Thanks,
> 
> 	M.
>