[PATCH 0/5] Fix vgic initialization problems

[PATCH 0/5] Fix vgic initialization problems

[Christoffer Dall]

This series fixes problems with initializing the VGIC.

The problem is that we were initializing the state of the VGIC on-demand
partially, and doing some final initializtion at the time when we were
going to run a VCPU for the first time.

This broke migration, because the first-vcpu-run init would overwrite
restored state.

We also cleanup the naming of the init functions and add checks when
creating VCPUs and when injecting IRQs from userspace.

This series invalidates patch 3 "KVM: arm/arm64: check vgic_initialized
before VCPU creation" in Eric Auger's vgic init ioctl series.

Eric's series should be applied after this one, making future ABIs
(IRQFD, VFIO, GICv3, ...) require explicit userspace vgic initialization
instead of this on-demand approach that we now have to maintain for
legacy userspace compatiblity.

I've tested this with 32-bit and 64-bit QEMU and kvmtool.

Christoffer Dall (4):
  arm/arm64: KVM: Rename vgic_initialized to vgic_ready
  arm/arm64: KVM: Add (new) vgic_initialized macro
  arm/arm64: KVM: Don't allow creating VCPUs after vgic_initialized
  arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

Peter Maydell (1):
  arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps()

 arch/arm/kvm/arm.c     | 13 +++++---
 include/kvm/arm_vgic.h | 12 +++++--
 virt/kvm/arm/vgic.c    | 91 +++++++++++++++++++++++---------------------------
 3 files changed, 60 insertions(+), 56 deletions(-)

-- 
2.1.2.330.g565301e.dirty

[PATCH 1/5] arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps()

[Christoffer Dall]

From: Peter Maydell <peter.maydell@linaro.org>

VGIC initialization currently happens in three phases:
 (1) kvm_vgic_create() (triggered by userspace GIC creation)
 (2) vgic_init_maps() (triggered by userspace GIC register read/write
     requests, or from kvm_vgic_init() if not already run)
 (3) kvm_vgic_init() (triggered by first VM run)

We were doing initialization of some state to correspond with the
state of a freshly-reset GIC in kvm_vgic_init(); this is too late,
since it will overwrite changes made by userspace using the
register access APIs before the VM is run. Move this initialization
earlier, into the vgic_init_maps() phase.

This fixes a bug where QEMU could successfully restore a saved
VM state snapshot into a VM that had already been run, but could
not restore it "from cold" using the -loadvm command line option
(the symptoms being that the restored VM would run but interrupts
were ignored).

Finally rename vgic_init_maps to vgic_init and renamed kvm_vgic_init to
kvm_vgic_map_resources.

  [ This patch is originally written by Peter Maydell, but I have
    modified it somewhat heavily, renaming various bits and moving code
    around.  If something is broken, I am to be blamed. - Christoffer ]

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
This patch was originally named "vgic: move reset initialization into
vgic_init_maps()" but I renamed it slightly to match the other vgic
patches in the kernel.  I also did the additional changes since the
original patch:
 - Renamed kvm_vgic_init to kvm_vgic_map_resources
 - Renamed vgic_init_maps to vgic_init
 - Moved vgic_enable call into existing vcpu loop in vgic_init
 - Moved ITARGETSRn initializtion above vcpu loop in vgic_init (the idea
   is to init global state first, then vcpu state).
 - Added comment in kvm_vgic_map_resources

 arch/arm/kvm/arm.c     |  6 ++--
 include/kvm/arm_vgic.h |  4 +--
 virt/kvm/arm/vgic.c    | 77 +++++++++++++++++++++-----------------------------
 3 files changed, 37 insertions(+), 50 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 9e193c8..a56cbb5 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -427,11 +427,11 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 	vcpu->arch.has_run_once = true;
 
 	/*
-	 * Initialize the VGIC before running a vcpu the first time on
-	 * this VM.
+	 * Map the VGIC hardware resources before running a vcpu the first
+	 * time on this VM.
 	 */
 	if (unlikely(!vgic_initialized(vcpu->kvm))) {
-		ret = kvm_vgic_init(vcpu->kvm);
+		ret = kvm_vgic_map_resources(vcpu->kvm);
 		if (ret)
 			return ret;
 	}
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 206dcc3..fe9783b 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -274,7 +274,7 @@ struct kvm_exit_mmio;
 #ifdef CONFIG_KVM_ARM_VGIC
 int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
 int kvm_vgic_hyp_init(void);
-int kvm_vgic_init(struct kvm *kvm);
+int kvm_vgic_map_resources(struct kvm *kvm);
 int kvm_vgic_create(struct kvm *kvm);
 void kvm_vgic_destroy(struct kvm *kvm);
 void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
@@ -321,7 +321,7 @@ static inline int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr,
 	return -ENXIO;
 }
 
-static inline int kvm_vgic_init(struct kvm *kvm)
+static inline int kvm_vgic_map_resources(struct kvm *kvm)
 {
 	return 0;
 }
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index aacdb59..91e6bfc 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -91,6 +91,7 @@
 #define ACCESS_WRITE_VALUE	(3 << 1)
 #define ACCESS_WRITE_MASK(x)	((x) & (3 << 1))
 
+static int vgic_init(struct kvm *kvm);
 static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
 static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
 static void vgic_update_state(struct kvm *kvm);
@@ -1726,39 +1727,14 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
 
 	int sz = (nr_irqs - VGIC_NR_PRIVATE_IRQS) / 8;
 	vgic_cpu->pending_shared = kzalloc(sz, GFP_KERNEL);
-	vgic_cpu->vgic_irq_lr_map = kzalloc(nr_irqs, GFP_KERNEL);
+	vgic_cpu->vgic_irq_lr_map = kmalloc(nr_irqs, GFP_KERNEL);
 
 	if (!vgic_cpu->pending_shared || !vgic_cpu->vgic_irq_lr_map) {
 		kvm_vgic_vcpu_destroy(vcpu);
 		return -ENOMEM;
 	}
 
-	return 0;
-}
-
-/**
- * kvm_vgic_vcpu_init - Initialize per-vcpu VGIC state
- * @vcpu: pointer to the vcpu struct
- *
- * Initialize the vgic_cpu struct and vgic_dist struct fields pertaining to
- * this vcpu and enable the VGIC for this VCPU
- */
-static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
-{
-	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
-	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
-	int i;
-
-	for (i = 0; i < dist->nr_irqs; i++) {
-		if (i < VGIC_NR_PPIS)
-			vgic_bitmap_set_irq_val(&dist->irq_enabled,
-						vcpu->vcpu_id, i, 1);
-		if (i < VGIC_NR_PRIVATE_IRQS)
-			vgic_bitmap_set_irq_val(&dist->irq_cfg,
-						vcpu->vcpu_id, i, VGIC_CFG_EDGE);
-
-		vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY;
-	}
+	memset(vgic_cpu->vgic_irq_lr_map, LR_EMPTY, nr_irqs);
 
 	/*
 	 * Store the number of LRs per vcpu, so we don't have to go
@@ -1767,7 +1743,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
 	 */
 	vgic_cpu->nr_lr = vgic->nr_lr;
 
-	vgic_enable(vcpu);
+	return 0;
 }
 
 void kvm_vgic_destroy(struct kvm *kvm)
@@ -1804,12 +1780,12 @@ void kvm_vgic_destroy(struct kvm *kvm)
  * Allocate and initialize the various data structures. Must be called
  * with kvm->lock held!
  */
-static int vgic_init_maps(struct kvm *kvm)
+static int vgic_init(struct kvm *kvm)
 {
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	struct kvm_vcpu *vcpu;
 	int nr_cpus, nr_irqs;
-	int ret, i;
+	int ret, i, vcpu_id;
 
 	if (dist->nr_cpus)	/* Already allocated */
 		return 0;
@@ -1859,16 +1835,28 @@ static int vgic_init_maps(struct kvm *kvm)
 	if (ret)
 		goto out;
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
+	for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
+		vgic_set_target_reg(kvm, 0, i);
+
+	kvm_for_each_vcpu(vcpu_id, vcpu, kvm) {
 		ret = vgic_vcpu_init_maps(vcpu, nr_irqs);
 		if (ret) {
 			kvm_err("VGIC: Failed to allocate vcpu memory\n");
 			break;
 		}
-	}
 
-	for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
-		vgic_set_target_reg(kvm, 0, i);
+		for (i = 0; i < dist->nr_irqs; i++) {
+			if (i < VGIC_NR_PPIS)
+				vgic_bitmap_set_irq_val(&dist->irq_enabled,
+							vcpu->vcpu_id, i, 1);
+			if (i < VGIC_NR_PRIVATE_IRQS)
+				vgic_bitmap_set_irq_val(&dist->irq_cfg,
+							vcpu->vcpu_id, i,
+							VGIC_CFG_EDGE);
+		}
+
+		vgic_enable(vcpu);
+	}
 
 out:
 	if (ret)
@@ -1878,18 +1866,16 @@ out:
 }
 
 /**
- * kvm_vgic_init - Initialize global VGIC state before running any VCPUs
+ * kvm_vgic_map_resources - Configure global VGIC state before running any VCPUs
  * @kvm: pointer to the kvm struct
  *
  * Map the virtual CPU interface into the VM before running any VCPUs.  We
  * can't do this at creation time, because user space must first set the
- * virtual CPU interface address in the guest physical address space.  Also
- * initialize the ITARGETSRn regs to 0 on the emulated distributor.
+ * virtual CPU interface address in the guest physical address space.
  */
-int kvm_vgic_init(struct kvm *kvm)
+int kvm_vgic_map_resources(struct kvm *kvm)
 {
-	struct kvm_vcpu *vcpu;
-	int ret = 0, i;
+	int ret = 0;
 
 	if (!irqchip_in_kernel(kvm))
 		return 0;
@@ -1906,7 +1892,11 @@ int kvm_vgic_init(struct kvm *kvm)
 		goto out;
 	}
 
-	ret = vgic_init_maps(kvm);
+	/*
+	 * Initialize the vgic if this hasn't already been done on demand by
+	 * accessing the vgic state from userspace.
+	 */
+	ret = vgic_init(kvm);
 	if (ret) {
 		kvm_err("Unable to allocate maps\n");
 		goto out;
@@ -1920,9 +1910,6 @@ int kvm_vgic_init(struct kvm *kvm)
 		goto out;
 	}
 
-	kvm_for_each_vcpu(i, vcpu, kvm)
-		kvm_vgic_vcpu_init(vcpu);
-
 	kvm->arch.vgic.ready = true;
 out:
 	if (ret)
@@ -2167,7 +2154,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
 
 	mutex_lock(&dev->kvm->lock);
 
-	ret = vgic_init_maps(dev->kvm);
+	ret = vgic_init(dev->kvm);
 	if (ret)
 		goto out;
 
-- 
2.1.2.330.g565301e.dirty

[PATCH 2/5] arm/arm64: KVM: Rename vgic_initialized to vgic_ready

[Christoffer Dall]

The vgic_initialized() macro currently returns the state of the
vgic->ready flag, which indicates if the vgic is ready to be used when
running a VM, not specifically if its internal state has been
initialized.

Rename the macro accordingly in preparation for a more nuanced
initialization flow.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/arm.c     | 2 +-
 include/kvm/arm_vgic.h | 4 ++--
 virt/kvm/arm/vgic.c    | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index a56cbb5..a9d005f 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -430,7 +430,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 	 * Map the VGIC hardware resources before running a vcpu the first
 	 * time on this VM.
 	 */
-	if (unlikely(!vgic_initialized(vcpu->kvm))) {
+	if (unlikely(!vgic_ready(vcpu->kvm))) {
 		ret = kvm_vgic_map_resources(vcpu->kvm);
 		if (ret)
 			return ret;
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index fe9783b..3e262b9 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -287,7 +287,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		      struct kvm_exit_mmio *mmio);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
-#define vgic_initialized(k)	((k)->arch.vgic.ready)
+#define vgic_ready(k)		((k)->arch.vgic.ready)
 
 int vgic_v2_probe(struct device_node *vgic_node,
 		  const struct vgic_ops **ops,
@@ -369,7 +369,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return 0;
 }
 
-static inline bool vgic_initialized(struct kvm *kvm)
+static inline bool vgic_ready(struct kvm *kvm)
 {
 	return true;
 }
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 91e6bfc..6293349 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1693,7 +1693,7 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (likely(vgic_initialized(kvm)) &&
+	if (likely(vgic_ready(kvm)) &&
 	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
 		vgic_kick_vcpus(kvm);
 
@@ -1882,7 +1882,7 @@ int kvm_vgic_map_resources(struct kvm *kvm)
 
 	mutex_lock(&kvm->lock);
 
-	if (vgic_initialized(kvm))
+	if (vgic_ready(kvm))
 		goto out;
 
 	if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) ||
@@ -2276,7 +2276,7 @@ static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
 
 		mutex_lock(&dev->kvm->lock);
 
-		if (vgic_initialized(dev->kvm) || dev->kvm->arch.vgic.nr_irqs)
+		if (vgic_ready(dev->kvm) || dev->kvm->arch.vgic.nr_irqs)
 			ret = -EBUSY;
 		else
 			dev->kvm->arch.vgic.nr_irqs = val;
-- 
2.1.2.330.g565301e.dirty

[PATCH 3/5] arm/arm64: KVM: Add (new) vgic_initialized macro

[Christoffer Dall]

Some code paths will need to check to see if the internal state of the
vgic has been initialized (such as when creating new VCPUs), so
introduce such a macro that checks the nr_cpus field which is set when
the vgic has been initialized.

Also set nr_cpus = 0 in kvm_vgic_destroy, because the error path in
vgic_init() will call this function, and code should never errornously
assume the vgic to be properly initialized after an error.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/kvm/arm_vgic.h | 6 ++++++
 virt/kvm/arm/vgic.c    | 1 +
 2 files changed, 7 insertions(+)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 3e262b9..ac4888d 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -287,6 +287,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		      struct kvm_exit_mmio *mmio);
 
 #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
+#define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
 #define vgic_ready(k)		((k)->arch.vgic.ready)
 
 int vgic_v2_probe(struct device_node *vgic_node,
@@ -369,6 +370,11 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
 	return 0;
 }
 
+static inline bool vgic_initialized(struct kvm *kvm)
+{
+	return true;
+}
+
 static inline bool vgic_ready(struct kvm *kvm)
 {
 	return true;
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 6293349..c98cc6b 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1774,6 +1774,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
 	dist->irq_spi_cpu = NULL;
 	dist->irq_spi_target = NULL;
 	dist->irq_pending_on_cpu = NULL;
+	dist->nr_cpus = 0;
 }
 
 /*
-- 
2.1.2.330.g565301e.dirty

[PATCH 4/5] arm/arm64: KVM: Don't allow creating VCPUs after vgic_initialized

[Christoffer Dall]

When the vgic initializes its internal state it does so based on the
number of VCPUs available at the time.  If we allow KVM to create more
VCPUs after the VGIC has been initialized, we are likely to error out in
unfortunate ways later, perform buffer overflows etc.

Cc: Eric Auger <eric.auger@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
This replaces Eric Auger's previous patch
(https://lists.cs.columbia.edu/pipermail/kvmarm/2014-December/012646.html),
because it fits better with testing to include it in this series and I
realized that we need to add a check against irqchip_in_kernel() as
well.

 arch/arm/kvm/arm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index a9d005f..d4da244 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -213,6 +213,11 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
 	int err;
 	struct kvm_vcpu *vcpu;
 
+	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm)) {
+		err = -EBUSY;
+		goto out;
+	}
+
 	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
 	if (!vcpu) {
 		err = -ENOMEM;
-- 
2.1.2.330.g565301e.dirty

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Christoffer Dall]

Userspace assumes that it can wire up IRQ injections after having
created all VCPUs and after having created the VGIC, but potentially
before starting the first VCPU.  This can currently lead to lost IRQs
because the state of that IRQ injection is not stored anywhere and we
don't return an error to userspace.

We haven't seen this problem manifest itself yet, presumably because
guests reset the devices on boot, but this could cause issues with
migration and other non-standard startup configurations.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index c98cc6b..feef015 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1693,8 +1693,13 @@ out:
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
 			bool level)
 {
-	if (likely(vgic_ready(kvm)) &&
-	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
+	if (unlikely(!vgic_initialized(kvm))) {
+		mutex_lock(&kvm->lock);
+		vgic_init(kvm);
+		mutex_unlock(&kvm->lock);
+	}
+
+	if (vgic_update_irq_pending(kvm, cpuid, irq_num, level))
 		vgic_kick_vcpus(kvm);
 
 	return 0;
-- 
2.1.2.330.g565301e.dirty

[PATCH 1/5] arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps()

[Eric Auger]

Hi Christoffer,
Reviewed-by: Eric Auger <eric.auger@linaro.org>
see few comments below.
On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
> 
> VGIC initialization currently happens in three phases:
>  (1) kvm_vgic_create() (triggered by userspace GIC creation)
>  (2) vgic_init_maps() (triggered by userspace GIC register read/write
>      requests, or from kvm_vgic_init() if not already run)
>  (3) kvm_vgic_init() (triggered by first VM run)
> 
> We were doing initialization of some state to correspond with the
> state of a freshly-reset GIC in kvm_vgic_init(); this is too late,
> since it will overwrite changes made by userspace using the
> register access APIs before the VM is run. Move this initialization
> earlier, into the vgic_init_maps() phase.
> 
> This fixes a bug where QEMU could successfully restore a saved
> VM state snapshot into a VM that had already been run, but could
> not restore it "from cold" using the -loadvm command line option
> (the symptoms being that the restored VM would run but interrupts
> were ignored).
> 
> Finally rename vgic_init_maps to vgic_init and renamed kvm_vgic_init to
> kvm_vgic_map_resources.
> 
>   [ This patch is originally written by Peter Maydell, but I have
>     modified it somewhat heavily, renaming various bits and moving code
>     around.  If something is broken, I am to be blamed. - Christoffer ]
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> This patch was originally named "vgic: move reset initialization into
> vgic_init_maps()" but I renamed it slightly to match the other vgic
> patches in the kernel.  I also did the additional changes since the
> original patch:
>  - Renamed kvm_vgic_init to kvm_vgic_map_resources
>  - Renamed vgic_init_maps to vgic_init
>  - Moved vgic_enable call into existing vcpu loop in vgic_init
>  - Moved ITARGETSRn initializtion above vcpu loop in vgic_init (the idea
typo
>    is to init global state first, then vcpu state).

kvm_vgic_vcpu_init also has disappeared and PPI settings of
dist->irq_enabled and dist->irq_cfg now are in former vgic_init_maps.

Maybe it would be simpler to review if there were 2 patches: one for
init redistribution from kvm_vgic_init to vgic_init_maps and one for the
renaming.

kvm_vgic_map_resources: difficult to understand it also inits the
internal states. Wouldn't kvm_vgic_set_ready be aligned with ready
terminology?

Best Regards

Eric

>  - Added comment in kvm_vgic_map_resources




> 
>  arch/arm/kvm/arm.c     |  6 ++--
>  include/kvm/arm_vgic.h |  4 +--
>  virt/kvm/arm/vgic.c    | 77 +++++++++++++++++++++-----------------------------
>  3 files changed, 37 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 9e193c8..a56cbb5 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -427,11 +427,11 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  	vcpu->arch.has_run_once = true;
>  
>  	/*
> -	 * Initialize the VGIC before running a vcpu the first time on
> -	 * this VM.
> +	 * Map the VGIC hardware resources before running a vcpu the first
> +	 * time on this VM.
>  	 */
>  	if (unlikely(!vgic_initialized(vcpu->kvm))) {
> -		ret = kvm_vgic_init(vcpu->kvm);
> +		ret = kvm_vgic_map_resources(vcpu->kvm);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 206dcc3..fe9783b 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -274,7 +274,7 @@ struct kvm_exit_mmio;
>  #ifdef CONFIG_KVM_ARM_VGIC
>  int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr, bool write);
>  int kvm_vgic_hyp_init(void);
> -int kvm_vgic_init(struct kvm *kvm);
> +int kvm_vgic_map_resources(struct kvm *kvm);
>  int kvm_vgic_create(struct kvm *kvm);
>  void kvm_vgic_destroy(struct kvm *kvm);
>  void kvm_vgic_vcpu_destroy(struct kvm_vcpu *vcpu);
> @@ -321,7 +321,7 @@ static inline int kvm_vgic_addr(struct kvm *kvm, unsigned long type, u64 *addr,
>  	return -ENXIO;
>  }
>  
> -static inline int kvm_vgic_init(struct kvm *kvm)
> +static inline int kvm_vgic_map_resources(struct kvm *kvm)
>  {
>  	return 0;
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index aacdb59..91e6bfc 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -91,6 +91,7 @@
>  #define ACCESS_WRITE_VALUE	(3 << 1)
>  #define ACCESS_WRITE_MASK(x)	((x) & (3 << 1))
>  
> +static int vgic_init(struct kvm *kvm);
>  static void vgic_retire_disabled_irqs(struct kvm_vcpu *vcpu);
>  static void vgic_retire_lr(int lr_nr, int irq, struct kvm_vcpu *vcpu);
>  static void vgic_update_state(struct kvm *kvm);
> @@ -1726,39 +1727,14 @@ static int vgic_vcpu_init_maps(struct kvm_vcpu *vcpu, int nr_irqs)
>  
>  	int sz = (nr_irqs - VGIC_NR_PRIVATE_IRQS) / 8;
>  	vgic_cpu->pending_shared = kzalloc(sz, GFP_KERNEL);
> -	vgic_cpu->vgic_irq_lr_map = kzalloc(nr_irqs, GFP_KERNEL);
> +	vgic_cpu->vgic_irq_lr_map = kmalloc(nr_irqs, GFP_KERNEL);
>  
>  	if (!vgic_cpu->pending_shared || !vgic_cpu->vgic_irq_lr_map) {
>  		kvm_vgic_vcpu_destroy(vcpu);
>  		return -ENOMEM;
>  	}
>  
> -	return 0;
> -}
> -
> -/**
> - * kvm_vgic_vcpu_init - Initialize per-vcpu VGIC state
> - * @vcpu: pointer to the vcpu struct
> - *
> - * Initialize the vgic_cpu struct and vgic_dist struct fields pertaining to
> - * this vcpu and enable the VGIC for this VCPU
> - */
> -static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
> -{
> -	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
> -	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
> -	int i;
> -
> -	for (i = 0; i < dist->nr_irqs; i++) {
> -		if (i < VGIC_NR_PPIS)
> -			vgic_bitmap_set_irq_val(&dist->irq_enabled,
> -						vcpu->vcpu_id, i, 1);
> -		if (i < VGIC_NR_PRIVATE_IRQS)
> -			vgic_bitmap_set_irq_val(&dist->irq_cfg,
> -						vcpu->vcpu_id, i, VGIC_CFG_EDGE);
> -
> -		vgic_cpu->vgic_irq_lr_map[i] = LR_EMPTY;
> -	}
> +	memset(vgic_cpu->vgic_irq_lr_map, LR_EMPTY, nr_irqs);
>  
>  	/*
>  	 * Store the number of LRs per vcpu, so we don't have to go
> @@ -1767,7 +1743,7 @@ static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>  	 */
>  	vgic_cpu->nr_lr = vgic->nr_lr;
>  
> -	vgic_enable(vcpu);
> +	return 0;
>  }
>  
>  void kvm_vgic_destroy(struct kvm *kvm)
> @@ -1804,12 +1780,12 @@ void kvm_vgic_destroy(struct kvm *kvm)
>   * Allocate and initialize the various data structures. Must be called
>   * with kvm->lock held!
>   */
> -static int vgic_init_maps(struct kvm *kvm)
> +static int vgic_init(struct kvm *kvm)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	struct kvm_vcpu *vcpu;
>  	int nr_cpus, nr_irqs;
> -	int ret, i;
> +	int ret, i, vcpu_id;
>  
>  	if (dist->nr_cpus)	/* Already allocated */
>  		return 0;
> @@ -1859,16 +1835,28 @@ static int vgic_init_maps(struct kvm *kvm)
>  	if (ret)
>  		goto out;
>  
> -	kvm_for_each_vcpu(i, vcpu, kvm) {
> +	for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
> +		vgic_set_target_reg(kvm, 0, i);
> +
> +	kvm_for_each_vcpu(vcpu_id, vcpu, kvm) {
>  		ret = vgic_vcpu_init_maps(vcpu, nr_irqs);
>  		if (ret) {
>  			kvm_err("VGIC: Failed to allocate vcpu memory\n");
>  			break;
>  		}
> -	}
>  
> -	for (i = VGIC_NR_PRIVATE_IRQS; i < dist->nr_irqs; i += 4)
> -		vgic_set_target_reg(kvm, 0, i);
> +		for (i = 0; i < dist->nr_irqs; i++) {
> +			if (i < VGIC_NR_PPIS)
> +				vgic_bitmap_set_irq_val(&dist->irq_enabled,
> +							vcpu->vcpu_id, i, 1);
> +			if (i < VGIC_NR_PRIVATE_IRQS)
> +				vgic_bitmap_set_irq_val(&dist->irq_cfg,
> +							vcpu->vcpu_id, i,
> +							VGIC_CFG_EDGE);
> +		}
> +
> +		vgic_enable(vcpu);
> +	}
>  
>  out:
>  	if (ret)
> @@ -1878,18 +1866,16 @@ out:
>  }
>  
>  /**
> - * kvm_vgic_init - Initialize global VGIC state before running any VCPUs
> + * kvm_vgic_map_resources - Configure global VGIC state before running any VCPUs
>   * @kvm: pointer to the kvm struct
>   *
>   * Map the virtual CPU interface into the VM before running any VCPUs.  We
>   * can't do this at creation time, because user space must first set the
> - * virtual CPU interface address in the guest physical address space.  Also
> - * initialize the ITARGETSRn regs to 0 on the emulated distributor.
> + * virtual CPU interface address in the guest physical address space.
>   */
> -int kvm_vgic_init(struct kvm *kvm)
> +int kvm_vgic_map_resources(struct kvm *kvm)
>  {
> -	struct kvm_vcpu *vcpu;
> -	int ret = 0, i;
> +	int ret = 0;
>  
>  	if (!irqchip_in_kernel(kvm))
>  		return 0;
> @@ -1906,7 +1892,11 @@ int kvm_vgic_init(struct kvm *kvm)
>  		goto out;
>  	}
>  
> -	ret = vgic_init_maps(kvm);
> +	/*
> +	 * Initialize the vgic if this hasn't already been done on demand by
> +	 * accessing the vgic state from userspace.
> +	 */
> +	ret = vgic_init(kvm);
>  	if (ret) {
>  		kvm_err("Unable to allocate maps\n");
>  		goto out;
> @@ -1920,9 +1910,6 @@ int kvm_vgic_init(struct kvm *kvm)
>  		goto out;
>  	}
>  
> -	kvm_for_each_vcpu(i, vcpu, kvm)
> -		kvm_vgic_vcpu_init(vcpu);
> -
>  	kvm->arch.vgic.ready = true;
>  out:
>  	if (ret)
> @@ -2167,7 +2154,7 @@ static int vgic_attr_regs_access(struct kvm_device *dev,
>  
>  	mutex_lock(&dev->kvm->lock);
>  
> -	ret = vgic_init_maps(dev->kvm);
> +	ret = vgic_init(dev->kvm);
>  	if (ret)
>  		goto out;
>  
> 

[PATCH 3/5] arm/arm64: KVM: Add (new) vgic_initialized macro

[Eric Auger]

On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> Some code paths will need to check to see if the internal state of the
> vgic has been initialized (such as when creating new VCPUs), so
> introduce such a macro that checks the nr_cpus field which is set when
> the vgic has been initialized.
> 
> Also set nr_cpus = 0 in kvm_vgic_destroy, because the error path in
> vgic_init() will call this function, and code should never errornously
> assume the vgic to be properly initialized after an error.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/kvm/arm_vgic.h | 6 ++++++
>  virt/kvm/arm/vgic.c    | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 3e262b9..ac4888d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -287,6 +287,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		      struct kvm_exit_mmio *mmio);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
> +#define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
>  #define vgic_ready(k)		((k)->arch.vgic.ready)
>  
>  int vgic_v2_probe(struct device_node *vgic_node,
> @@ -369,6 +370,11 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>  	return 0;
>  }
>  
> +static inline bool vgic_initialized(struct kvm *kvm)
> +{
> +	return true;
> +}
> +
>  static inline bool vgic_ready(struct kvm *kvm)
>  {
>  	return true;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6293349..c98cc6b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1774,6 +1774,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
>  	dist->irq_spi_cpu = NULL;
>  	dist->irq_spi_target = NULL;
>  	dist->irq_pending_on_cpu = NULL;
> +	dist->nr_cpus = 0;
Reviewed-by: Eric Auger <eric.auger@linaro.org>
we could use that new vgic_initialized at the entry of vgic_init instead
of testing dist->nr_cpus, hence introducing one user.

Eric
>  }
>  
>  /*
> 

[PATCH 4/5] arm/arm64: KVM: Don't allow creating VCPUs after vgic_initialized

[Eric Auger]

On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> When the vgic initializes its internal state it does so based on the
> number of VCPUs available at the time.  If we allow KVM to create more
> VCPUs after the VGIC has been initialized, we are likely to error out in
> unfortunate ways later, perform buffer overflows etc.
> 
> Cc: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> This replaces Eric Auger's previous patch
> (https://lists.cs.columbia.edu/pipermail/kvmarm/2014-December/012646.html),
> because it fits better with testing to include it in this series and I
> realized that we need to add a check against irqchip_in_kernel() as
> well.
> 
>  arch/arm/kvm/arm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a9d005f..d4da244 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -213,6 +213,11 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
>  	int err;
>  	struct kvm_vcpu *vcpu;
>  
> +	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm)) {
Reviewed-by: Eric Auger <eric.auger@linaro.org>
a question about that irqchip_in_kernel(kvm):
kvm->arch.vgic.in_kernel is set in kvm_vgic_create but nobody resets it,
especially in destroy, am i wrong?
if the vgic is initialized shouldn't it be also created? Shouldn't we
test irqchip_in_kernel in vgic_init instead?
Also in case we need irqchip_in_kernel(kvm) here we might need it also
in kvm_vgic_inject_irq because dist->lock is grabbed in
vgic_update_irq_pending.

Eric
> +		err = -EBUSY;
> +		goto out;
> +	}
> +
>  	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
>  	if (!vcpu) {
>  		err = -ENOMEM;
> 

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Eric Auger]

On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> Userspace assumes that it can wire up IRQ injections after having
> created all VCPUs and after having created the VGIC, but potentially
> before starting the first VCPU.  This can currently lead to lost IRQs
> because the state of that IRQ injection is not stored anywhere and we
> don't return an error to userspace.
> 
> We haven't seen this problem manifest itself yet, 
Actually we did with VFIO signaling setup before VGIC init!
presumably because
> guests reset the devices on boot, but this could cause issues with
> migration and other non-standard startup configurations.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index c98cc6b..feef015 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1693,8 +1693,13 @@ out:
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level)
>  {
> -	if (likely(vgic_ready(kvm)) &&
> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> +	if (unlikely(!vgic_initialized(kvm))) {
> +		mutex_lock(&kvm->lock);
> +		vgic_init(kvm);
> +		mutex_unlock(&kvm->lock);
> +	}
I was previously encouraged to test the virtual interrupt controller
readiness when setting irqfd up(proposal made in
https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
correct? Reviewed-by on the whole series.

Eric
> +
> +	if (vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>  		vgic_kick_vcpus(kvm);
>  
>  	return 0;
> 

[PATCH 1/5] arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps()

[Christoffer Dall]

On Wed, Dec 10, 2014 at 11:11:41AM +0100, Eric Auger wrote:
> Hi Christoffer,
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> see few comments below.
> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> > From: Peter Maydell <peter.maydell@linaro.org>
> > 
> > VGIC initialization currently happens in three phases:
> >  (1) kvm_vgic_create() (triggered by userspace GIC creation)
> >  (2) vgic_init_maps() (triggered by userspace GIC register read/write
> >      requests, or from kvm_vgic_init() if not already run)
> >  (3) kvm_vgic_init() (triggered by first VM run)
> > 
> > We were doing initialization of some state to correspond with the
> > state of a freshly-reset GIC in kvm_vgic_init(); this is too late,
> > since it will overwrite changes made by userspace using the
> > register access APIs before the VM is run. Move this initialization
> > earlier, into the vgic_init_maps() phase.
> > 
> > This fixes a bug where QEMU could successfully restore a saved
> > VM state snapshot into a VM that had already been run, but could
> > not restore it "from cold" using the -loadvm command line option
> > (the symptoms being that the restored VM would run but interrupts
> > were ignored).
> > 
> > Finally rename vgic_init_maps to vgic_init and renamed kvm_vgic_init to
> > kvm_vgic_map_resources.
> > 
> >   [ This patch is originally written by Peter Maydell, but I have
> >     modified it somewhat heavily, renaming various bits and moving code
> >     around.  If something is broken, I am to be blamed. - Christoffer ]
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > This patch was originally named "vgic: move reset initialization into
> > vgic_init_maps()" but I renamed it slightly to match the other vgic
> > patches in the kernel.  I also did the additional changes since the
> > original patch:
> >  - Renamed kvm_vgic_init to kvm_vgic_map_resources
> >  - Renamed vgic_init_maps to vgic_init
> >  - Moved vgic_enable call into existing vcpu loop in vgic_init
> >  - Moved ITARGETSRn initializtion above vcpu loop in vgic_init (the idea
> typo
> >    is to init global state first, then vcpu state).
> 
> kvm_vgic_vcpu_init also has disappeared and PPI settings of
> dist->irq_enabled and dist->irq_cfg now are in former vgic_init_maps.
> 
> Maybe it would be simpler to review if there were 2 patches: one for
> init redistribution from kvm_vgic_init to vgic_init_maps and one for the
> renaming.

Maybe, but if you apply this patch and review it that way, it becomes
much easier.  I'd really like to get this in soon, so given you already
gave me your reviewed-by, I'm going to wait and see what Marc says and
only respin if he finds it necessary.

> 
> kvm_vgic_map_resources: difficult to understand it also inits the
> internal states. Wouldn't kvm_vgic_set_ready be aligned with ready
> terminology?
> 
So I thought about that, but really, the fact that we now call
vgic_init() from all sorts of places shouldn't require us to rename all
those functions, just because they call init.  So I ended up naming this
function for what it really does, and then just added a comment about
the on-demand approach.

I think that's cleanest.

Thanks,
-Christoffer

[PATCH 3/5] arm/arm64: KVM: Add (new) vgic_initialized macro

[Christoffer Dall]

On Wed, Dec 10, 2014 at 11:27:34AM +0100, Eric Auger wrote:
> 
> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> > Some code paths will need to check to see if the internal state of the
> > vgic has been initialized (such as when creating new VCPUs), so
> > introduce such a macro that checks the nr_cpus field which is set when
> > the vgic has been initialized.
> > 
> > Also set nr_cpus = 0 in kvm_vgic_destroy, because the error path in
> > vgic_init() will call this function, and code should never errornously
> > assume the vgic to be properly initialized after an error.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  include/kvm/arm_vgic.h | 6 ++++++
> >  virt/kvm/arm/vgic.c    | 1 +
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 3e262b9..ac4888d 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -287,6 +287,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >  		      struct kvm_exit_mmio *mmio);
> >  
> >  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
> > +#define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
> >  #define vgic_ready(k)		((k)->arch.vgic.ready)
> >  
> >  int vgic_v2_probe(struct device_node *vgic_node,
> > @@ -369,6 +370,11 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
> >  	return 0;
> >  }
> >  
> > +static inline bool vgic_initialized(struct kvm *kvm)
> > +{
> > +	return true;
> > +}
> > +
> >  static inline bool vgic_ready(struct kvm *kvm)
> >  {
> >  	return true;
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 6293349..c98cc6b 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1774,6 +1774,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
> >  	dist->irq_spi_cpu = NULL;
> >  	dist->irq_spi_target = NULL;
> >  	dist->irq_pending_on_cpu = NULL;
> > +	dist->nr_cpus = 0;
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> we could use that new vgic_initialized at the entry of vgic_init instead
> of testing dist->nr_cpus, hence introducing one user.
> 
Yeah, we could, I can fix that up when applying/respinning.

Thanks,
-Christoffer

[PATCH 4/5] arm/arm64: KVM: Don't allow creating VCPUs after vgic_initialized

[Christoffer Dall]

On Wed, Dec 10, 2014 at 01:35:08PM +0100, Eric Auger wrote:
> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> > When the vgic initializes its internal state it does so based on the
> > number of VCPUs available at the time.  If we allow KVM to create more
> > VCPUs after the VGIC has been initialized, we are likely to error out in
> > unfortunate ways later, perform buffer overflows etc.
> > 
> > Cc: Eric Auger <eric.auger@linaro.org>
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > This replaces Eric Auger's previous patch
> > (https://lists.cs.columbia.edu/pipermail/kvmarm/2014-December/012646.html),
> > because it fits better with testing to include it in this series and I
> > realized that we need to add a check against irqchip_in_kernel() as
> > well.
> > 
> >  arch/arm/kvm/arm.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> > index a9d005f..d4da244 100644
> > --- a/arch/arm/kvm/arm.c
> > +++ b/arch/arm/kvm/arm.c
> > @@ -213,6 +213,11 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
> >  	int err;
> >  	struct kvm_vcpu *vcpu;
> >  
> > +	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm)) {
> Reviewed-by: Eric Auger <eric.auger@linaro.org>
> a question about that irqchip_in_kernel(kvm):
> kvm->arch.vgic.in_kernel is set in kvm_vgic_create but nobody resets it,
> especially in destroy, am i wrong?

no, because we don't allow creating a vgic in the kernel for a VM and
then letting the VM go back to having a userspace driven gic.

> if the vgic is initialized shouldn't it be also created? Shouldn't we
> test irqchip_in_kernel in vgic_init instead?

no, vgic_init will never be called if you didn't create a vgic, and
irqchip_in_kernel() should always return false in that case.

If you can find a flow where this breaks, please let me know, because
then it's a bug, but it looks right to me.

> Also in case we need irqchip_in_kernel(kvm) here we might need it also
> in kvm_vgic_inject_irq because dist->lock is grabbed in
> vgic_update_irq_pending.
> 
Huh, you're right about that.  In fact, I don't think we should allow
initializing the arch timers if userspace didn't create an in-kernel
irqchip, avoiding the call path alltogether.

We probaby need to add that to this series.

Unless I missed something obvious here: Nice catch!

Thanks,
-Christoffer

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Christoffer Dall]

On Wed, Dec 10, 2014 at 01:45:50PM +0100, Eric Auger wrote:
> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> > Userspace assumes that it can wire up IRQ injections after having
> > created all VCPUs and after having created the VGIC, but potentially
> > before starting the first VCPU.  This can currently lead to lost IRQs
> > because the state of that IRQ injection is not stored anywhere and we
> > don't return an error to userspace.
> > 
> > We haven't seen this problem manifest itself yet, 
> Actually we did with VFIO signaling setup before VGIC init!
> presumably because

well, not with code in mainline

> > guests reset the devices on boot, but this could cause issues with
> > migration and other non-standard startup configurations.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index c98cc6b..feef015 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1693,8 +1693,13 @@ out:
> >  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> >  			bool level)
> >  {
> > -	if (likely(vgic_ready(kvm)) &&
> > -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> > +	if (unlikely(!vgic_initialized(kvm))) {
> > +		mutex_lock(&kvm->lock);
> > +		vgic_init(kvm);
> > +		mutex_unlock(&kvm->lock);
> > +	}
> I was previously encouraged to test the virtual interrupt controller
> readiness when setting irqfd up(proposal made in
> https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
> correct? Reviewed-by on the whole series.
> 
I think we should move to your userspace explicit init for all
non-legacy userspace and only support gicv3 and vfio/irqfd stuff with
userspace explicitly initializing the vgic.

Thanks for the review!

-Christoffer

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Eric Auger]

On 12/11/2014 01:01 PM, Christoffer Dall wrote:
> On Wed, Dec 10, 2014 at 01:45:50PM +0100, Eric Auger wrote:
>> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
>>> Userspace assumes that it can wire up IRQ injections after having
>>> created all VCPUs and after having created the VGIC, but potentially
>>> before starting the first VCPU.  This can currently lead to lost IRQs
>>> because the state of that IRQ injection is not stored anywhere and we
>>> don't return an error to userspace.
>>>
>>> We haven't seen this problem manifest itself yet, 
>> Actually we did with VFIO signaling setup before VGIC init!
>> presumably because
> 
> well, not with code in mainline
> 
>>> guests reset the devices on boot, but this could cause issues with
>>> migration and other non-standard startup configurations.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  virt/kvm/arm/vgic.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index c98cc6b..feef015 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1693,8 +1693,13 @@ out:
>>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>  			bool level)
>>>  {
>>> -	if (likely(vgic_ready(kvm)) &&
>>> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>>> +	if (unlikely(!vgic_initialized(kvm))) {
>>> +		mutex_lock(&kvm->lock);
>>> +		vgic_init(kvm);
>>> +		mutex_unlock(&kvm->lock);
>>> +	}
>> I was previously encouraged to test the virtual interrupt controller
>> readiness when setting irqfd up(proposal made in
>> https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
>> correct? Reviewed-by on the whole series.
>>
> I think we should move to your userspace explicit init for all
> non-legacy userspace and only support gicv3 and vfio/irqfd stuff with
> userspace explicitly initializing the vgic.

Hi Christoffer,

The use case I have in mind still is VFIO+irqfd:
since we cannot preclude the user from ignoring the userspace explicit
init and setting up VFIO signaling+irqfd before vgic init, to me there
is a risk injection starts even before creation. Either we test
irqchip_in_kernel in kvm_vgic_inject_irq or we must have a test when
setting up irqfd as proposed in above patch.

Actually before being able to inject any virtual IRQ we weed even more:
if virtual IRQ settings were not yet defined by the guest we do not know
what to do with the IRQ. We must a least know whether it is level or
edge. Current irq_cfg bitmap might be insufficient to store the info
since it only has 2 states and by chance I use a level-sensitive IRQ and
my QEMU pieces pay attention to that sequencing. I guess the problem is
the same for user-space injection, isn't it?

Eric

> 
> Thanks for the review!
> 
> -Christoffer
> 

[PATCH 1/5] arm/arm64: KVM: vgic: move reset initialization into vgic_init_maps()

[Marc Zyngier]

On 11/12/14 11:48, Christoffer Dall wrote:
> On Wed, Dec 10, 2014 at 11:11:41AM +0100, Eric Auger wrote:
>> Hi Christoffer,
>> Reviewed-by: Eric Auger <eric.auger@linaro.org>
>> see few comments below.
>> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
>>> From: Peter Maydell <peter.maydell@linaro.org>
>>>
>>> VGIC initialization currently happens in three phases:
>>>  (1) kvm_vgic_create() (triggered by userspace GIC creation)
>>>  (2) vgic_init_maps() (triggered by userspace GIC register read/write
>>>      requests, or from kvm_vgic_init() if not already run)
>>>  (3) kvm_vgic_init() (triggered by first VM run)
>>>
>>> We were doing initialization of some state to correspond with the
>>> state of a freshly-reset GIC in kvm_vgic_init(); this is too late,
>>> since it will overwrite changes made by userspace using the
>>> register access APIs before the VM is run. Move this initialization
>>> earlier, into the vgic_init_maps() phase.
>>>
>>> This fixes a bug where QEMU could successfully restore a saved
>>> VM state snapshot into a VM that had already been run, but could
>>> not restore it "from cold" using the -loadvm command line option
>>> (the symptoms being that the restored VM would run but interrupts
>>> were ignored).
>>>
>>> Finally rename vgic_init_maps to vgic_init and renamed kvm_vgic_init to
>>> kvm_vgic_map_resources.
>>>
>>>   [ This patch is originally written by Peter Maydell, but I have
>>>     modified it somewhat heavily, renaming various bits and moving code
>>>     around.  If something is broken, I am to be blamed. - Christoffer ]
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>> This patch was originally named "vgic: move reset initialization into
>>> vgic_init_maps()" but I renamed it slightly to match the other vgic
>>> patches in the kernel.  I also did the additional changes since the
>>> original patch:
>>>  - Renamed kvm_vgic_init to kvm_vgic_map_resources
>>>  - Renamed vgic_init_maps to vgic_init
>>>  - Moved vgic_enable call into existing vcpu loop in vgic_init
>>>  - Moved ITARGETSRn initializtion above vcpu loop in vgic_init (the idea
>> typo
>>>    is to init global state first, then vcpu state).
>>
>> kvm_vgic_vcpu_init also has disappeared and PPI settings of
>> dist->irq_enabled and dist->irq_cfg now are in former vgic_init_maps.
>>
>> Maybe it would be simpler to review if there were 2 patches: one for
>> init redistribution from kvm_vgic_init to vgic_init_maps and one for the
>> renaming.
> 
> Maybe, but if you apply this patch and review it that way, it becomes
> much easier.  I'd really like to get this in soon, so given you already
> gave me your reviewed-by, I'm going to wait and see what Marc says and
> only respin if he finds it necessary.

No, I think this is fine as it is. This is a good cleanup, and it seems
to simplify the whole thing (and yes, we could do with simplicity in
this file...).

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/5] arm/arm64: KVM: Rename vgic_initialized to vgic_ready

[Marc Zyngier]

On 09/12/14 15:44, Christoffer Dall wrote:
> The vgic_initialized() macro currently returns the state of the
> vgic->ready flag, which indicates if the vgic is ready to be used when
> running a VM, not specifically if its internal state has been
> initialized.
> 
> Rename the macro accordingly in preparation for a more nuanced
> initialization flow.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  arch/arm/kvm/arm.c     | 2 +-
>  include/kvm/arm_vgic.h | 4 ++--
>  virt/kvm/arm/vgic.c    | 6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a56cbb5..a9d005f 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -430,7 +430,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  	 * Map the VGIC hardware resources before running a vcpu the first
>  	 * time on this VM.
>  	 */
> -	if (unlikely(!vgic_initialized(vcpu->kvm))) {
> +	if (unlikely(!vgic_ready(vcpu->kvm))) {
>  		ret = kvm_vgic_map_resources(vcpu->kvm);
>  		if (ret)
>  			return ret;
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index fe9783b..3e262b9 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -287,7 +287,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		      struct kvm_exit_mmio *mmio);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
> -#define vgic_initialized(k)	((k)->arch.vgic.ready)
> +#define vgic_ready(k)		((k)->arch.vgic.ready)
>  
>  int vgic_v2_probe(struct device_node *vgic_node,
>  		  const struct vgic_ops **ops,
> @@ -369,7 +369,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>  	return 0;
>  }
>  
> -static inline bool vgic_initialized(struct kvm *kvm)
> +static inline bool vgic_ready(struct kvm *kvm)
>  {
>  	return true;
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 91e6bfc..6293349 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1693,7 +1693,7 @@ out:
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level)
>  {
> -	if (likely(vgic_initialized(kvm)) &&
> +	if (likely(vgic_ready(kvm)) &&
>  	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>  		vgic_kick_vcpus(kvm);
>  
> @@ -1882,7 +1882,7 @@ int kvm_vgic_map_resources(struct kvm *kvm)
>  
>  	mutex_lock(&kvm->lock);
>  
> -	if (vgic_initialized(kvm))
> +	if (vgic_ready(kvm))
>  		goto out;
>  
>  	if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) ||
> @@ -2276,7 +2276,7 @@ static int vgic_set_attr(struct kvm_device *dev, struct kvm_device_attr *attr)
>  
>  		mutex_lock(&dev->kvm->lock);
>  
> -		if (vgic_initialized(dev->kvm) || dev->kvm->arch.vgic.nr_irqs)
> +		if (vgic_ready(dev->kvm) || dev->kvm->arch.vgic.nr_irqs)
>  			ret = -EBUSY;
>  		else
>  			dev->kvm->arch.vgic.nr_irqs = val;
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/5] arm/arm64: KVM: Add (new) vgic_initialized macro

[Marc Zyngier]

On 09/12/14 15:44, Christoffer Dall wrote:
> Some code paths will need to check to see if the internal state of the
> vgic has been initialized (such as when creating new VCPUs), so
> introduce such a macro that checks the nr_cpus field which is set when
> the vgic has been initialized.
> 
> Also set nr_cpus = 0 in kvm_vgic_destroy, because the error path in
> vgic_init() will call this function, and code should never errornously

                                                            erroneously

> assume the vgic to be properly initialized after an error.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  include/kvm/arm_vgic.h | 6 ++++++
>  virt/kvm/arm/vgic.c    | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 3e262b9..ac4888d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -287,6 +287,7 @@ bool vgic_handle_mmio(struct kvm_vcpu *vcpu, struct kvm_run *run,
>  		      struct kvm_exit_mmio *mmio);
>  
>  #define irqchip_in_kernel(k)	(!!((k)->arch.vgic.in_kernel))
> +#define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
>  #define vgic_ready(k)		((k)->arch.vgic.ready)
>  
>  int vgic_v2_probe(struct device_node *vgic_node,
> @@ -369,6 +370,11 @@ static inline int irqchip_in_kernel(struct kvm *kvm)
>  	return 0;
>  }
>  
> +static inline bool vgic_initialized(struct kvm *kvm)
> +{
> +	return true;
> +}
> +
>  static inline bool vgic_ready(struct kvm *kvm)
>  {
>  	return true;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6293349..c98cc6b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1774,6 +1774,7 @@ void kvm_vgic_destroy(struct kvm *kvm)
>  	dist->irq_spi_cpu = NULL;
>  	dist->irq_spi_target = NULL;
>  	dist->irq_pending_on_cpu = NULL;
> +	dist->nr_cpus = 0;
>  }
>  
>  /*
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 4/5] arm/arm64: KVM: Don't allow creating VCPUs after vgic_initialized

[Marc Zyngier]

On 09/12/14 15:44, Christoffer Dall wrote:
> When the vgic initializes its internal state it does so based on the
> number of VCPUs available at the time.  If we allow KVM to create more
> VCPUs after the VGIC has been initialized, we are likely to error out in
> unfortunate ways later, perform buffer overflows etc.
> 
> Cc: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> This replaces Eric Auger's previous patch
> (https://lists.cs.columbia.edu/pipermail/kvmarm/2014-December/012646.html),
> because it fits better with testing to include it in this series and I
> realized that we need to add a check against irqchip_in_kernel() as
> well.
> 
>  arch/arm/kvm/arm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a9d005f..d4da244 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -213,6 +213,11 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id)
>  	int err;
>  	struct kvm_vcpu *vcpu;
>  
> +	if (irqchip_in_kernel(kvm) && vgic_initialized(kvm)) {
> +		err = -EBUSY;
> +		goto out;
> +	}
> +
>  	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
>  	if (!vcpu) {
>  		err = -ENOMEM;
> 

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Marc Zyngier]

On 09/12/14 15:44, Christoffer Dall wrote:
> Userspace assumes that it can wire up IRQ injections after having
> created all VCPUs and after having created the VGIC, but potentially
> before starting the first VCPU.  This can currently lead to lost IRQs
> because the state of that IRQ injection is not stored anywhere and we
> don't return an error to userspace.
> 
> We haven't seen this problem manifest itself yet, presumably because
> guests reset the devices on boot, but this could cause issues with
> migration and other non-standard startup configurations.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  virt/kvm/arm/vgic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index c98cc6b..feef015 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1693,8 +1693,13 @@ out:
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>  			bool level)
>  {
> -	if (likely(vgic_ready(kvm)) &&
> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> +	if (unlikely(!vgic_initialized(kvm))) {
> +		mutex_lock(&kvm->lock);
> +		vgic_init(kvm);

What if this fails?

> +		mutex_unlock(&kvm->lock);
> +	}
> +
> +	if (vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>  		vgic_kick_vcpus(kvm);
>  
>  	return 0;
> 

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Christoffer Dall]

On Thu, Dec 11, 2014 at 01:38:16PM +0100, Eric Auger wrote:
> On 12/11/2014 01:01 PM, Christoffer Dall wrote:
> > On Wed, Dec 10, 2014 at 01:45:50PM +0100, Eric Auger wrote:
> >> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
> >>> Userspace assumes that it can wire up IRQ injections after having
> >>> created all VCPUs and after having created the VGIC, but potentially
> >>> before starting the first VCPU.  This can currently lead to lost IRQs
> >>> because the state of that IRQ injection is not stored anywhere and we
> >>> don't return an error to userspace.
> >>>
> >>> We haven't seen this problem manifest itself yet, 
> >> Actually we did with VFIO signaling setup before VGIC init!
> >> presumably because
> > 
> > well, not with code in mainline
> > 
> >>> guests reset the devices on boot, but this could cause issues with
> >>> migration and other non-standard startup configurations.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >>> ---
> >>>  virt/kvm/arm/vgic.c | 9 +++++++--
> >>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >>> index c98cc6b..feef015 100644
> >>> --- a/virt/kvm/arm/vgic.c
> >>> +++ b/virt/kvm/arm/vgic.c
> >>> @@ -1693,8 +1693,13 @@ out:
> >>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> >>>  			bool level)
> >>>  {
> >>> -	if (likely(vgic_ready(kvm)) &&
> >>> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> >>> +	if (unlikely(!vgic_initialized(kvm))) {
> >>> +		mutex_lock(&kvm->lock);
> >>> +		vgic_init(kvm);
> >>> +		mutex_unlock(&kvm->lock);
> >>> +	}
> >> I was previously encouraged to test the virtual interrupt controller
> >> readiness when setting irqfd up(proposal made in
> >> https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
> >> correct? Reviewed-by on the whole series.
> >>
> > I think we should move to your userspace explicit init for all
> > non-legacy userspace and only support gicv3 and vfio/irqfd stuff with
> > userspace explicitly initializing the vgic.
> 
> Hi Christoffer,
> 
> The use case I have in mind still is VFIO+irqfd:
> since we cannot preclude the user from ignoring the userspace explicit
> init and setting up VFIO signaling+irqfd before vgic init, to me there
> is a risk injection starts even before creation. Either we test
> irqchip_in_kernel in kvm_vgic_inject_irq or we must have a test when
> setting up irqfd as proposed in above patch.

yes, test if the vgic has been initialized when setting up irqfd and
return an error if not.

> 
> Actually before being able to inject any virtual IRQ we weed even more:
> if virtual IRQ settings were not yet defined by the guest we do not know
> what to do with the IRQ. We must a least know whether it is level or
> edge. Current irq_cfg bitmap might be insufficient to store the info
> since it only has 2 states and by chance I use a level-sensitive IRQ and
> my QEMU pieces pay attention to that sequencing. I guess the problem is
> the same for user-space injection, isn't it?
> 
that's no different from on the hardware is it?  We assume the interrupt
is what it is (as per its default reset value in GICD_ICFGRn) and when
the guest boots up, it must reconfigure the interrupt.

The higher level picture here is that we don't know when the guest is
done configuring things, from some time before we run any vcpu to some
time after we've run vcpus, and there is no error return path on
injecting the IRQ to the VGIC for this sort of matter, so we just have
to cope with things in the same way that hardware does.

My guess is that no sane guests will actually depend on these interrupts
being raised/lowered before configuring the GIC will have any real
effect, as all guests should configure the gic, clear that interrupt,
unmask it, and only then care about things.

-Christoffer

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Christoffer Dall]

On Thu, Dec 11, 2014 at 06:35:40PM +0000, Marc Zyngier wrote:
> On 09/12/14 15:44, Christoffer Dall wrote:
> > Userspace assumes that it can wire up IRQ injections after having
> > created all VCPUs and after having created the VGIC, but potentially
> > before starting the first VCPU.  This can currently lead to lost IRQs
> > because the state of that IRQ injection is not stored anywhere and we
> > don't return an error to userspace.
> > 
> > We haven't seen this problem manifest itself yet, presumably because
> > guests reset the devices on boot, but this could cause issues with
> > migration and other non-standard startup configurations.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  virt/kvm/arm/vgic.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index c98cc6b..feef015 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1693,8 +1693,13 @@ out:
> >  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> >  			bool level)
> >  {
> > -	if (likely(vgic_ready(kvm)) &&
> > -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> > +	if (unlikely(!vgic_initialized(kvm))) {
> > +		mutex_lock(&kvm->lock);
> > +		vgic_init(kvm);
> 
> What if this fails?
> 
yeah, not good.  The thing is that we also don't check the return value
from kvm_vgic_inject_irq(), so we can do two things:

(1) change this function to a void, carry out the check for
vgic_initialized in kvm_vm_ioctl_irq_line() in arm.c and export
vgic_init() outside of vgic.c.

(2) just error out if vgic_init() fails and print a kernel error (or
even a BUG_ON?) in kvm_timer_inject_irq() in arch_timer.c.

In both cases we need to make sure that we never configure the timer to
begin injecting IRQs before the vgic is initialized, as Eric pointed out
before.

What do you think?

-Christoffer

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Marc Zyngier]

On 12/12/14 11:14, Christoffer Dall wrote:
> On Thu, Dec 11, 2014 at 06:35:40PM +0000, Marc Zyngier wrote:
>> On 09/12/14 15:44, Christoffer Dall wrote:
>>> Userspace assumes that it can wire up IRQ injections after having
>>> created all VCPUs and after having created the VGIC, but potentially
>>> before starting the first VCPU.  This can currently lead to lost IRQs
>>> because the state of that IRQ injection is not stored anywhere and we
>>> don't return an error to userspace.
>>>
>>> We haven't seen this problem manifest itself yet, presumably because
>>> guests reset the devices on boot, but this could cause issues with
>>> migration and other non-standard startup configurations.
>>>
>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>> ---
>>>  virt/kvm/arm/vgic.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>> index c98cc6b..feef015 100644
>>> --- a/virt/kvm/arm/vgic.c
>>> +++ b/virt/kvm/arm/vgic.c
>>> @@ -1693,8 +1693,13 @@ out:
>>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>  			bool level)
>>>  {
>>> -	if (likely(vgic_ready(kvm)) &&
>>> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>>> +	if (unlikely(!vgic_initialized(kvm))) {
>>> +		mutex_lock(&kvm->lock);
>>> +		vgic_init(kvm);
>>
>> What if this fails?
>>
> yeah, not good.  The thing is that we also don't check the return value
> from kvm_vgic_inject_irq(), so we can do two things:
> 
> (1) change this function to a void, carry out the check for
> vgic_initialized in kvm_vm_ioctl_irq_line() in arm.c and export
> vgic_init() outside of vgic.c.
> 
> (2) just error out if vgic_init() fails and print a kernel error (or
> even a BUG_ON?) in kvm_timer_inject_irq() in arch_timer.c.
> 
> In both cases we need to make sure that we never configure the timer to
> begin injecting IRQs before the vgic is initialized, as Eric pointed out
> before.
> 
> What do you think?

I'd favour option two.

My reasoning is that the timer interrupt is triggered by the HW. If it
has fired, that's because we've programmed it to trigger, with means a
vcpu has run. At that point, the vgic would better be initialized, or we
have something much nastier on our hands.

Thoughts?

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Christoffer Dall]

On Fri, Dec 12, 2014 at 11:23:35AM +0000, Marc Zyngier wrote:
> On 12/12/14 11:14, Christoffer Dall wrote:
> > On Thu, Dec 11, 2014 at 06:35:40PM +0000, Marc Zyngier wrote:
> >> On 09/12/14 15:44, Christoffer Dall wrote:
> >>> Userspace assumes that it can wire up IRQ injections after having
> >>> created all VCPUs and after having created the VGIC, but potentially
> >>> before starting the first VCPU.  This can currently lead to lost IRQs
> >>> because the state of that IRQ injection is not stored anywhere and we
> >>> don't return an error to userspace.
> >>>
> >>> We haven't seen this problem manifest itself yet, presumably because
> >>> guests reset the devices on boot, but this could cause issues with
> >>> migration and other non-standard startup configurations.
> >>>
> >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >>> ---
> >>>  virt/kvm/arm/vgic.c | 9 +++++++--
> >>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> >>> index c98cc6b..feef015 100644
> >>> --- a/virt/kvm/arm/vgic.c
> >>> +++ b/virt/kvm/arm/vgic.c
> >>> @@ -1693,8 +1693,13 @@ out:
> >>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> >>>  			bool level)
> >>>  {
> >>> -	if (likely(vgic_ready(kvm)) &&
> >>> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> >>> +	if (unlikely(!vgic_initialized(kvm))) {
> >>> +		mutex_lock(&kvm->lock);
> >>> +		vgic_init(kvm);
> >>
> >> What if this fails?
> >>
> > yeah, not good.  The thing is that we also don't check the return value
> > from kvm_vgic_inject_irq(), so we can do two things:
> > 
> > (1) change this function to a void, carry out the check for
> > vgic_initialized in kvm_vm_ioctl_irq_line() in arm.c and export
> > vgic_init() outside of vgic.c.
> > 
> > (2) just error out if vgic_init() fails and print a kernel error (or
> > even a BUG_ON?) in kvm_timer_inject_irq() in arch_timer.c.
> > 
> > In both cases we need to make sure that we never configure the timer to
> > begin injecting IRQs before the vgic is initialized, as Eric pointed out
> > before.
> > 
> > What do you think?
> 
> I'd favour option two.
> 
> My reasoning is that the timer interrupt is triggered by the HW. If it
> has fired, that's because we've programmed it to trigger, with means a
> vcpu has run. At that point, the vgic would better be initialized, or we
> have something much nastier on our hands.
> 
Sounds reasonable to me, I'll do a quick respin with the check for the
timer (to ensure the user even created a vgic).

-Christoffer

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Christoffer Dall]

On Fri, Dec 12, 2014 at 12:37:52PM +0100, Christoffer Dall wrote:
> On Fri, Dec 12, 2014 at 11:23:35AM +0000, Marc Zyngier wrote:
> > On 12/12/14 11:14, Christoffer Dall wrote:
> > > On Thu, Dec 11, 2014 at 06:35:40PM +0000, Marc Zyngier wrote:
> > >> On 09/12/14 15:44, Christoffer Dall wrote:
> > >>> Userspace assumes that it can wire up IRQ injections after having
> > >>> created all VCPUs and after having created the VGIC, but potentially
> > >>> before starting the first VCPU.  This can currently lead to lost IRQs
> > >>> because the state of that IRQ injection is not stored anywhere and we
> > >>> don't return an error to userspace.
> > >>>
> > >>> We haven't seen this problem manifest itself yet, presumably because
> > >>> guests reset the devices on boot, but this could cause issues with
> > >>> migration and other non-standard startup configurations.
> > >>>
> > >>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > >>> ---
> > >>>  virt/kvm/arm/vgic.c | 9 +++++++--
> > >>>  1 file changed, 7 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > >>> index c98cc6b..feef015 100644
> > >>> --- a/virt/kvm/arm/vgic.c
> > >>> +++ b/virt/kvm/arm/vgic.c
> > >>> @@ -1693,8 +1693,13 @@ out:
> > >>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
> > >>>  			bool level)
> > >>>  {
> > >>> -	if (likely(vgic_ready(kvm)) &&
> > >>> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
> > >>> +	if (unlikely(!vgic_initialized(kvm))) {
> > >>> +		mutex_lock(&kvm->lock);
> > >>> +		vgic_init(kvm);
> > >>
> > >> What if this fails?
> > >>
> > > yeah, not good.  The thing is that we also don't check the return value
> > > from kvm_vgic_inject_irq(), so we can do two things:
> > > 
> > > (1) change this function to a void, carry out the check for
> > > vgic_initialized in kvm_vm_ioctl_irq_line() in arm.c and export
> > > vgic_init() outside of vgic.c.
> > > 
> > > (2) just error out if vgic_init() fails and print a kernel error (or
> > > even a BUG_ON?) in kvm_timer_inject_irq() in arch_timer.c.
> > > 
> > > In both cases we need to make sure that we never configure the timer to
> > > begin injecting IRQs before the vgic is initialized, as Eric pointed out
> > > before.
> > > 
> > > What do you think?
> > 
> > I'd favour option two.
> > 
> > My reasoning is that the timer interrupt is triggered by the HW. If it
> > has fired, that's because we've programmed it to trigger, with means a
> > vcpu has run. At that point, the vgic would better be initialized, or we
> > have something much nastier on our hands.
> > 
> Sounds reasonable to me, I'll do a quick respin with the check for the
> timer (to ensure the user even created a vgic).
> 
Just to double-check, it is going to look something like this for the
arch-timer path:

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index d4da244..c61d51d 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -441,6 +441,16 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 			return ret;
 	}
 
+#ifdef CONFIG_KVM_ARM_TIMER
+	/*
+	 * If the Architected Timers are supported, userspace must have
+	 * created an in-kernel irqchip, since otherwise we will receive
+	 * virtual timer interrupt and have nowhere to route them to.
+	 */
+	if (!irqchip_in_kernel(kvm))
+		return -ENODEV;
+#endif
+
 	return 0;
 }
 
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 22fa819..b10e495 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -61,12 +61,14 @@ static void timer_disarm(struct arch_timer_cpu *timer)
 
 static void kvm_timer_inject_irq(struct kvm_vcpu *vcpu)
 {
+	int ret;
 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
 
 	timer->cntv_ctl |= ARCH_TIMER_CTRL_IT_MASK;
-	kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
-			    timer->irq->irq,
-			    timer->irq->level);
+	ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
+				  timer->irq->irq,
+				  timer->irq->level);
+	BUG_ON(ret);
 }
 
 static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id)
-- 

Does this look reasonable to you?

-Christoffer

[PATCH 5/5] arm/arm64: KVM: Initialize the vgic on-demand when injecting IRQs

[Eric Auger]

On 12/12/2014 12:06 PM, Christoffer Dall wrote:
> On Thu, Dec 11, 2014 at 01:38:16PM +0100, Eric Auger wrote:
>> On 12/11/2014 01:01 PM, Christoffer Dall wrote:
>>> On Wed, Dec 10, 2014 at 01:45:50PM +0100, Eric Auger wrote:
>>>> On 12/09/2014 04:44 PM, Christoffer Dall wrote:
>>>>> Userspace assumes that it can wire up IRQ injections after having
>>>>> created all VCPUs and after having created the VGIC, but potentially
>>>>> before starting the first VCPU.  This can currently lead to lost IRQs
>>>>> because the state of that IRQ injection is not stored anywhere and we
>>>>> don't return an error to userspace.
>>>>>
>>>>> We haven't seen this problem manifest itself yet, 
>>>> Actually we did with VFIO signaling setup before VGIC init!
>>>> presumably because
>>>
>>> well, not with code in mainline
>>>
>>>>> guests reset the devices on boot, but this could cause issues with
>>>>> migration and other non-standard startup configurations.
>>>>>
>>>>> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>>>>> ---
>>>>>  virt/kvm/arm/vgic.c | 9 +++++++--
>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>>>>> index c98cc6b..feef015 100644
>>>>> --- a/virt/kvm/arm/vgic.c
>>>>> +++ b/virt/kvm/arm/vgic.c
>>>>> @@ -1693,8 +1693,13 @@ out:
>>>>>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>>>>>  			bool level)
>>>>>  {
>>>>> -	if (likely(vgic_ready(kvm)) &&
>>>>> -	    vgic_update_irq_pending(kvm, cpuid, irq_num, level))
>>>>> +	if (unlikely(!vgic_initialized(kvm))) {
>>>>> +		mutex_lock(&kvm->lock);
>>>>> +		vgic_init(kvm);
>>>>> +		mutex_unlock(&kvm->lock);
>>>>> +	}
>>>> I was previously encouraged to test the virtual interrupt controller
>>>> readiness when setting irqfd up(proposal made in
>>>> https://lkml.org/lkml/2014/12/3/601). I guess this becomes useless now,
>>>> correct? Reviewed-by on the whole series.
>>>>
>>> I think we should move to your userspace explicit init for all
>>> non-legacy userspace and only support gicv3 and vfio/irqfd stuff with
>>> userspace explicitly initializing the vgic.
>>
>> Hi Christoffer,
>>
>> The use case I have in mind still is VFIO+irqfd:
>> since we cannot preclude the user from ignoring the userspace explicit
>> init and setting up VFIO signaling+irqfd before vgic init, to me there
>> is a risk injection starts even before creation. Either we test
>> irqchip_in_kernel in kvm_vgic_inject_irq or we must have a test when
>> setting up irqfd as proposed in above patch.
> 
> yes, test if the vgic has been initialized when setting up irqfd and
> return an error if not.
> 
>>
>> Actually before being able to inject any virtual IRQ we weed even more:
>> if virtual IRQ settings were not yet defined by the guest we do not know
>> what to do with the IRQ. We must a least know whether it is level or
>> edge. Current irq_cfg bitmap might be insufficient to store the info
>> since it only has 2 states and by chance I use a level-sensitive IRQ and
>> my QEMU pieces pay attention to that sequencing. I guess the problem is
>> the same for user-space injection, isn't it?
>>
> that's no different from on the hardware is it?  We assume the interrupt
> is what it is (as per its default reset value in GICD_ICFGRn) and when
> the guest boots up, it must reconfigure the interrupt.
> 
> The higher level picture here is that we don't know when the guest is
> done configuring things, from some time before we run any vcpu to some
> time after we've run vcpus, and there is no error return path on
> injecting the IRQ to the VGIC for this sort of matter, so we just have
> to cope with things in the same way that hardware does.
> 
> My guess is that no sane guests will actually depend on these interrupts
> being raised/lowered before configuring the GIC will have any real
> effect, as all guests should configure the gic, clear that interrupt,
> unmask it, and only then care about things.

Hi Christoffer,

makes sense to me too, after reading the spec once more ;-)

Best Regards

Eric
> 
> -Christoffer
>