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).
>> 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):
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;
+
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.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
