KVM: arm/arm64: Allow to use KVM without in-kernel irqchip

Makes qemu working again with kernel-irqchip=off option

Signed-off-by Pavel Fedin <[email protected]>

---
 arch/arm/kvm/arm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/kvm/arm.c +0 -0

index d9631ec..5668c4e 100644

I'd appreciate a better commit log. Which patch broke it, why is that necessary.

Allows to use KVM on hardware without vGIC. Interrupt controller has to be emulated in userspace in this case.

-ENODEV return code from probe function means there's no GIC at all. -ENXIO happens when, for example, there is GIC node in the device tree, but it does not specify vGIC resources. Normally this means that vGIC hardware is defunct. Any other code is still treated as full stop because it might mean some really serious problems.

Signed-off-by Pavel Fedin <[email protected]>

---
 arch/arm/kvm/arm.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

arch/arm/kvm/arm.c +0 -0

index 5668c4e..7e389fd 100644

arch/arm/kvm/arm.c +18 -2

 static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
 {
 	BUG_ON(preemptible());

@@ -131,7 +133,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	kvm->arch.vmid_gen = 0;
 
 	/* The maximum number of VCPUs is limited by the host's GIC model */
-	kvm->arch.max_vcpus = kvm_vgic_get_max_vcpus();
+	kvm->arch.max_vcpus = vgic_present ?
+				kvm_vgic_get_max_vcpus() : KVM_MAX_VCPUS;
 
 	return ret;
 out_free_stage2_pgd:

@@ -172,6 +175,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	switch (ext) {
 	case KVM_CAP_IRQCHIP:
 	case KVM_CAP_IRQFD:
+		r = vgic_present;
+		break;
 	case KVM_CAP_IOEVENTFD:
 	case KVM_CAP_DEVICE_CTRL:
 	case KVM_CAP_USER_MEMORY:

@@ -850,6 +855,8 @@ static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
 
 	switch (dev_id) {
 	case KVM_ARM_DEVICE_VGIC_V2:
+		if (!vgic_present)
+			return -ENXIO;
 		return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
 	default:
 		return -ENODEV;

@@ -864,6 +871,8 @@ long kvm_arch_vm_ioctl(struct file *filp,
 
 	switch (ioctl) {
 	case KVM_CREATE_IRQCHIP: {
+		if (!vgic_present)
+			return -ENXIO;
 		return kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2);
 	}
 	case KVM_ARM_SET_DEVICE_ADDR: {

@@ -1046,8 +1055,17 @@ static int init_hyp_mode(void)
 	 * Init HYP view of VGIC
 	 */
 	err = kvm_vgic_hyp_init();
-	if (err)
+	switch (err) {
+	case 0:
+		vgic_present = true;
+		break;
+	case -ENODEV:
+	case -ENXIO:
+		vgic_present = false;
+		break;
+	default:
 		goto out_free_context;
+	}
 
 	/*
 	 * Init HYP architected timer support

Necessary for KVM without irqchip to function on ARM64. Allows to use software GICv2 emulation where hardware acceleration is not available.

Signed-off-by Pavel Fedin <[email protected]>

---
 arch/arm/include/asm/kvm_host.h   | 5 +++++
 arch/arm64/include/asm/kvm_asm.h  | 1 +
 arch/arm64/include/asm/kvm_host.h | 8 ++++++++
 arch/arm64/kvm/vgic-v2-switch.S   | 5 +++++
 virt/kvm/arm/vgic.c               | 2 ++
 5 files changed, 21 insertions(+)

arch/arm/include/asm/kvm_host.h +5 -0

index d71607c..c469c95 100644
@@ -218,6 +218,11 @@ static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 	return 0;
 }
 
+static inline void vgic_arch_pre_setup(void)
+{
+
+}
+
 static inline void vgic_arch_setup(const struct vgic_params *vgic)
 {
 	BUG_ON(vgic->type != VGIC_V2);

arch/arm64/include/asm/kvm_asm.h +1 -0

index 4f7310f..933a442 100644
@@ -132,6 +132,7 @@ extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
 extern u64 __vgic_v3_get_ich_vtr_el2(void);
 
+extern char __dummy_vgic_save_restore[];
 extern char __save_vgic_v2_state[];
 extern char __restore_vgic_v2_state[];
 extern char __save_vgic_v3_state[];

arch/arm64/include/asm/kvm_host.h +0 -0

index f0f58c9..b322895 100644
@@ -221,6 +221,14 @@ struct vgic_sr_vectors {
 	void	*restore_vgic;

You may want to check commit 8a14849, which makes this whole patch irrelevant.

Also, we do check whether or not the GIC is enabled for a given VM, and not touching the registers if they are not. What is wrong with that?

Hello!

Quoted 2 lines

You may want to check commit 8a14849, which makes this whole patch irrelevant.

Thank you very much for pointing out, i will recheck.
My patches are based on linux-stable.git, should i base them on something else?

You may want to check commit 8a14849, which makes this whole patch irrelevant.

This patch set brings back functionality which was broken in v4.1. The overall goal is to eventually enable using virtual timer too, but for now changes affect only a possibility to run KVM itself. The guest currently has to use another timer because there's no API to propagate interrupts from in-kernel virtual timer to the interrupt controller emulated in userspace.

v1=>v2:
- Do not use defensive approach in patch 0001. Use correct conditions in callers instead
- Added ARM64-specific code, without which attempt to run a VM ends in a HYP crash because of unset vGIC save/restore function pointers

Christoffer Dall: I decided to leave original approach of 0002 as it is, because:
- It is much simpler to implement than doing changes inside both probe functions.
- These are all use cases for vgic_present flag. Nothing else is going to rely on it. IMHO this switch is very short and stylish :) If you don't like two error codes, we could change -ENXIO to -ENODEV where appropriate, this change would be small enough.
Actually i tried to discuss some things with you, but you stopped replying, therefore i respin in order to revive the thing. On top of this i have

complete implementation of API which allows to emulate GIC in userspace by qemu, and now i can run any virtual machine, including generic timer, on vGIC-less machine. RasPI-2 is expected to benefit too.

Believe it or not, we're not only patch reviewing machines, and if you count the number of pending patches, you'll quickly notice that yours are basically noise in the grand scheme of things.

So please cut us some slack.

Quoted 3 lines

complete implementation of API which allows to emulate GIC in userspace by qemu, and now i can run any virtual machine, including generic timer, on vGIC-less machine. RasPI-2 is expected to benefit too.

Do you mean feeding interrupts back to userspace?

How is that going to
work with the active-timer series that really mandates a full blown GIC? Your pet platform might cope with it, but I can't see that happening on the RPi.

Hello!

Quoted 4 lines

Believe it or not, we're not only patch reviewing machines, and if you count the number of pending patches, you'll quickly notice that yours are basically noise in the grand scheme of things.

Quoted 1 line

So please cut us some slack.

What do you mean exactly? Stop discussions and just do everything you suggest before respin? Plz don't attempt to read this between lines, i can be bad at communication; this is not a complain and/or hurt indication, i am just asking. :)

Quoted 5 lines

Quoted 3 lines

complete implementation of API which allows to emulate GIC in userspace by qemu, and now i can run any virtual machine, including generic timer, on vGIC-less machine. RasPI-2 is expected to benefit too.

Do you mean feeding interrupts back to userspace?

Yes, exactly. I have a working proof-of-concept here.

Quoted 4 lines

How is that going to
work with the active-timer series that really mandates a full blown GIC? Your pet platform might cope with it, but I can't see that happening on the RPi.

I don't know, sorry, things are happening too fast and i can't follow everything.

Well, actually it is possible to work without virtual timer at all. Yes, this means qemu has to implement some memory-mapped timer. With HW models like vexpress this is already done and works fine. I even tried to address this:
https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06627.html,
but Peter suggested to complete an API instead.
I have done this as a proof-of-concept and case study for my project, so i want to upstream this in order not to be lost.

After all, recent specs say that generic timer can also be memory-mapped. So, making KVM working without both vGIC and vTimer is not a real problem. This patchset addresses this scenario too, just needs some extra bits on vtimer side (ability to init KVM without it).

It means that a pause in the discussion for a week (or even more) is not uncommon at all. It just means that the other party is busy with things of higher priority. In all cases, a gentle "ping" will be better received than this "you've stopped replying so I'm going to repost until you stop ignoring me" kind of behaviour.

I don't care much for such an API, mostly because it will obviously bitrot very quickly (your "broken VGIC" platform is hopefully a one off that won't be repeated again, and the RPi is out of scope anyway).

It would have to be extremely non-intrusive and completely safe for this to be taken in...

Quoted 6 lines

I don't care much for such an API, mostly because it will obviously bitrot very quickly (your "broken VGIC" platform is hopefully a one off that won't be repeated again, and the RPi is out of scope anyway).

It would have to be extremely non-intrusive and completely safe for this to be taken in...

Ok, let's leave this API alone then for now...
Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both vGIC
and vTimer ? It will be an emulator's problem how to handle them then.

Quoted 2 lines

Ok, let's leave this API alone then for now...
Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both

Quoted 1 line

and vTimer ? It will be an emulator's problem how to handle them then.

Well, let's see the patches first, and how invasive they are.

Quoted 2 lines

Quoted 2 lines

Ok, let's leave this API alone then for now...
Will then be a concensus if i tweak the thing a little bit and we just enable KVM without both

vGIC

Quoted 2 lines

Quoted 1 line

and vTimer ? It will be an emulator's problem how to handle them then.

Quoted 1 line

Well, let's see the patches first, and how invasive they are.

Please, i've just posted an RFC and cc'ed you.
I would say the implementation is very small. However, perhaps, it's not going to work with active
state switching patch, because as far as i understand the whole thing depends on manipulations with ARCH_TIMER_CTRL_IT_MASK. Without it i'm going to get multiple interrupts instead of only one. It would be possible to signal the interrupt as real level-sensitive (since we will be tracking active state), but... it's not possible to know when to deactivate the line, hardware doesn't report that.

As I said before, not having a VGIC pretty much puts you in an uncharted territory, and the amount of hacks you have to introduce are not really worth the effort in mainline.

Hello!

Quoted 3 lines

As I said before, not having a VGIC pretty much puts you in an uncharted territory, and the amount of hacks you have to introduce are not really worth the effort in mainline.

And i agreed. So, i repeat the question. Will it be OK if we just enable KVM without both vGIC and vTimer, and that's all we do? It will be userland's problem how to handle them. I need to tweak my first patchset a very small bit in order to do this.

Quoted 4 lines

And i agreed. So, i repeat the question. Will it be OK if we just enable KVM without both vGIC and vTimer, and that's all we do? It will be userland's problem how to handle them. I need to tweak my first patchset a very small bit in order to do this.

I I'll repeat what I already said: "Show me the patches".

I'll only give you an answer once I exactly know what you're up to.

Hello!

Quoted 5 lines

Quoted 4 lines

And i agreed. So, i repeat the question. Will it be OK if we just enable KVM without both vGIC and vTimer, and that's all we do? It will be userland's problem how to handle them. I need to tweak my first patchset a very small bit in order to do this.

Quoted 3 lines

I I'll repeat what I already said: "Show me the patches".

I'll only give you an answer once I exactly know what you're up to.

You have already seen them, it is the very same patchset we are discussing in this thread.