kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1
@ 2021-05-10 13:48 Marc Zyngier
  2021-05-10 13:48 ` [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code Marc Zyngier
                   ` (9 more replies)
  0 siblings, 10 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

This is a new version of the series previously posted at [2], reworking
the vGIC and timer code to cope with the M1 braindead^Wamusing nature.

Hardly any change this time around, mostly rebased on top of upstream
now that the dependencies have made it in.

Tested with multiple concurrent VMs running from an initramfs.

* From v2:
  - Rebased on 5.13-rc1
  - Fixed a couple of nits in the GIC registration code

* From v1 [1]:
  - Rebased on Hector's v4 posting[0]
  - Dropped a couple of patches that have been merged in the above series
  - Fixed irq_ack callback on the timer path

[0] https://lore.kernel.org/r/20210402090542.131194-1-marcan@marcan.st
[1] https://lore.kernel.org/r/20210316174617.173033-1-maz@kernel.org
[2] https://lore.kernel.org/r/20210403112931.1043452-1-maz@kernel.org

Marc Zyngier (9):
  irqchip/gic: Split vGIC probing information from the GIC code
  KVM: arm64: Handle physical FIQ as an IRQ while running a guest
  KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt
  KVM: arm64: vgic: Let an interrupt controller advertise lack of HW
    deactivation
  KVM: arm64: vgic: move irq->get_input_level into an ops structure
  KVM: arm64: vgic: Implement SW-driven deactivation
  KVM: arm64: timer: Refactor IRQ configuration
  KVM: arm64: timer: Add support for SW-based deactivation
  irqchip/apple-aic: Advertise some level of vGICv3 compatibility

 arch/arm64/kvm/arch_timer.c            | 161 +++++++++++++++++++++----
 arch/arm64/kvm/hyp/hyp-entry.S         |   6 +-
 arch/arm64/kvm/vgic/vgic-init.c        |  34 +++++-
 arch/arm64/kvm/vgic/vgic-v2.c          |  19 ++-
 arch/arm64/kvm/vgic/vgic-v3.c          |  19 ++-
 arch/arm64/kvm/vgic/vgic.c             |  14 +--
 drivers/irqchip/irq-apple-aic.c        |   8 ++
 drivers/irqchip/irq-gic-common.c       |  13 --
 drivers/irqchip/irq-gic-common.h       |   2 -
 drivers/irqchip/irq-gic-v3.c           |   6 +-
 drivers/irqchip/irq-gic.c              |   6 +-
 include/kvm/arm_vgic.h                 |  41 +++++--
 include/linux/irqchip/arm-gic-common.h |  25 +---
 include/linux/irqchip/arm-vgic-info.h  |  43 +++++++
 14 files changed, 291 insertions(+), 106 deletions(-)
 create mode 100644 include/linux/irqchip/arm-vgic-info.h

-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-18 16:51   ` Alexandru Elisei
  2021-05-10 13:48 ` [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest Marc Zyngier
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

The vGIC advertising code is unsurprisingly very much tied to
the GIC implementations. However, we are about to extend the
support to lesser implementations.

Let's dissociate the vgic registration from the GIC code and
move it into KVM, where it makes a bit more sense. This also
allows us to mark the gic_kvm_info structures as __initdata.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-init.c        | 18 +++++++++--
 drivers/irqchip/irq-gic-common.c       | 13 --------
 drivers/irqchip/irq-gic-common.h       |  2 --
 drivers/irqchip/irq-gic-v3.c           |  6 ++--
 drivers/irqchip/irq-gic.c              |  6 ++--
 include/linux/irqchip/arm-gic-common.h | 25 +---------------
 include/linux/irqchip/arm-vgic-info.h  | 41 ++++++++++++++++++++++++++
 7 files changed, 63 insertions(+), 48 deletions(-)
 create mode 100644 include/linux/irqchip/arm-vgic-info.h

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 58cbda00e56d..2fdb65529594 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -482,6 +482,16 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static struct gic_kvm_info *gic_kvm_info;
+
+void __init vgic_set_kvm_info(const struct gic_kvm_info *info)
+{
+	BUG_ON(gic_kvm_info != NULL);
+	gic_kvm_info = kmalloc(sizeof(*info), GFP_KERNEL);
+	if (gic_kvm_info)
+		*gic_kvm_info = *info;
+}
+
 /**
  * kvm_vgic_init_cpu_hardware - initialize the GIC VE hardware
  *
@@ -509,10 +519,8 @@ void kvm_vgic_init_cpu_hardware(void)
  */
 int kvm_vgic_hyp_init(void)
 {
-	const struct gic_kvm_info *gic_kvm_info;
 	int ret;
 
-	gic_kvm_info = gic_get_kvm_info();
 	if (!gic_kvm_info)
 		return -ENODEV;
 
@@ -536,10 +544,14 @@ int kvm_vgic_hyp_init(void)
 		ret = -ENODEV;
 	}
 
+	kvm_vgic_global_state.maint_irq = gic_kvm_info->maint_irq;
+
+	kfree(gic_kvm_info);
+	gic_kvm_info = NULL;
+
 	if (ret)
 		return ret;
 
-	kvm_vgic_global_state.maint_irq = gic_kvm_info->maint_irq;
 	ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
 				 vgic_maintenance_handler,
 				 "vgic", kvm_get_running_vcpus());
diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f47b41dfd023..a610821c8ff2 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -12,19 +12,6 @@
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
-static const struct gic_kvm_info *gic_kvm_info;
-
-const struct gic_kvm_info *gic_get_kvm_info(void)
-{
-	return gic_kvm_info;
-}
-
-void gic_set_kvm_info(const struct gic_kvm_info *info)
-{
-	BUG_ON(gic_kvm_info != NULL);
-	gic_kvm_info = info;
-}
-
 void gic_enable_of_quirks(const struct device_node *np,
 			  const struct gic_quirk *quirks, void *data)
 {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index ccba8b0fe0f5..27e3d4ed4f32 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -28,6 +28,4 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
 void gic_enable_of_quirks(const struct device_node *np,
 			  const struct gic_quirk *quirks, void *data);
 
-void gic_set_kvm_info(const struct gic_kvm_info *info);
-
 #endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 37a23aa6de37..453fc425eede 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -103,7 +103,7 @@ EXPORT_SYMBOL(gic_nonsecure_priorities);
 /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
 static refcount_t *ppi_nmi_refs;
 
-static struct gic_kvm_info gic_v3_kvm_info;
+static struct gic_kvm_info gic_v3_kvm_info __initdata;
 static DEFINE_PER_CPU(bool, has_rss);
 
 #define MPIDR_RS(mpidr)			(((mpidr) & 0xF0UL) >> 4)
@@ -1852,7 +1852,7 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
 
 	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
 	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
-	gic_set_kvm_info(&gic_v3_kvm_info);
+	vgic_set_kvm_info(&gic_v3_kvm_info);
 }
 
 static int __init gic_of_init(struct device_node *node, struct device_node *parent)
@@ -2168,7 +2168,7 @@ static void __init gic_acpi_setup_kvm_info(void)
 
 	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
 	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
-	gic_set_kvm_info(&gic_v3_kvm_info);
+	vgic_set_kvm_info(&gic_v3_kvm_info);
 }
 
 static int __init
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index b1d9c22caf2e..2de9ec8ece0c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -119,7 +119,7 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
 
 static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
 
-static struct gic_kvm_info gic_v2_kvm_info;
+static struct gic_kvm_info gic_v2_kvm_info __initdata;
 
 static DEFINE_PER_CPU(u32, sgi_intid);
 
@@ -1451,7 +1451,7 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
 		return;
 
 	if (static_branch_likely(&supports_deactivate_key))
-		gic_set_kvm_info(&gic_v2_kvm_info);
+		vgic_set_kvm_info(&gic_v2_kvm_info);
 }
 
 int __init
@@ -1618,7 +1618,7 @@ static void __init gic_acpi_setup_kvm_info(void)
 
 	gic_v2_kvm_info.maint_irq = irq;
 
-	gic_set_kvm_info(&gic_v2_kvm_info);
+	vgic_set_kvm_info(&gic_v2_kvm_info);
 }
 
 static int __init gic_v2_acpi_init(union acpi_subtable_headers *header,
diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
index fa8c0455c352..1177f3a1aed5 100644
--- a/include/linux/irqchip/arm-gic-common.h
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -7,8 +7,7 @@
 #ifndef __LINUX_IRQCHIP_ARM_GIC_COMMON_H
 #define __LINUX_IRQCHIP_ARM_GIC_COMMON_H
 
-#include <linux/types.h>
-#include <linux/ioport.h>
+#include <linux/irqchip/arm-vgic-info.h>
 
 #define GICD_INT_DEF_PRI		0xa0
 #define GICD_INT_DEF_PRI_X4		((GICD_INT_DEF_PRI << 24) |\
@@ -16,28 +15,6 @@
 					(GICD_INT_DEF_PRI << 8) |\
 					GICD_INT_DEF_PRI)
 
-enum gic_type {
-	GIC_V2,
-	GIC_V3,
-};
-
-struct gic_kvm_info {
-	/* GIC type */
-	enum gic_type	type;
-	/* Virtual CPU interface */
-	struct resource vcpu;
-	/* Interrupt number */
-	unsigned int	maint_irq;
-	/* Virtual control interface */
-	struct resource vctrl;
-	/* vlpi support */
-	bool		has_v4;
-	/* rvpeid support */
-	bool		has_v4_1;
-};
-
-const struct gic_kvm_info *gic_get_kvm_info(void);
-
 struct irq_domain;
 struct fwnode_handle;
 int gicv2m_init(struct fwnode_handle *parent_handle,
diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h
new file mode 100644
index 000000000000..0319636be928
--- /dev/null
+++ b/include/linux/irqchip/arm-vgic-info.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * include/linux/irqchip/arm-vgic-info.h
+ *
+ * Copyright (C) 2016 ARM Limited, All Rights Reserved.
+ */
+#ifndef __ARM_VGIC_INFO_H
+#define __ARM_VGIC_INFO_H
+
+#include <linux/types.h>
+#include <linux/ioport.h>
+
+enum gic_type {
+	/* Full GICv2 */
+	GIC_V2,
+	/* Full GICv3, optionally with v2 compat */
+	GIC_V3,
+};
+
+struct gic_kvm_info {
+	/* GIC type */
+	enum gic_type	type;
+	/* Virtual CPU interface */
+	struct resource vcpu;
+	/* Interrupt number */
+	unsigned int	maint_irq;
+	/* Virtual control interface */
+	struct resource vctrl;
+	/* vlpi support */
+	bool		has_v4;
+	/* rvpeid support */
+	bool		has_v4_1;
+};
+
+#ifdef CONFIG_KVM
+void vgic_set_kvm_info(const struct gic_kvm_info *info);
+#else
+static inline void vgic_set_kvm_info(const struct gic_kvm_info *info) {}
+#endif
+
+#endif
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
  2021-05-10 13:48 ` [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-20 17:46   ` Alexandru Elisei
  2021-05-10 13:48 ` [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt Marc Zyngier
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

As we we now entertain the possibility of FIQ being used on the host,
treat the signalling of a FIQ while running a guest as an IRQ,
causing an exit instead of a HYP panic.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/hyp/hyp-entry.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 5f49df4ffdd8..9aa9b73475c9 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -76,6 +76,7 @@ el1_trap:
 	b	__guest_exit
 
 el1_irq:
+el1_fiq:
 	get_vcpu_ptr	x1, x0
 	mov	x0, #ARM_EXCEPTION_IRQ
 	b	__guest_exit
@@ -131,7 +132,6 @@ SYM_CODE_END(\label)
 	invalid_vector	el2t_error_invalid
 	invalid_vector	el2h_irq_invalid
 	invalid_vector	el2h_fiq_invalid
-	invalid_vector	el1_fiq_invalid
 
 	.ltorg
 
@@ -179,12 +179,12 @@ SYM_CODE_START(__kvm_hyp_vector)
 
 	valid_vect	el1_sync		// Synchronous 64-bit EL1
 	valid_vect	el1_irq			// IRQ 64-bit EL1
-	invalid_vect	el1_fiq_invalid		// FIQ 64-bit EL1
+	valid_vect	el1_fiq			// FIQ 64-bit EL1
 	valid_vect	el1_error		// Error 64-bit EL1
 
 	valid_vect	el1_sync		// Synchronous 32-bit EL1
 	valid_vect	el1_irq			// IRQ 32-bit EL1
-	invalid_vect	el1_fiq_invalid		// FIQ 32-bit EL1
+	valid_vect	el1_fiq			// FIQ 32-bit EL1
 	valid_vect	el1_error		// Error 32-bit EL1
 SYM_CODE_END(__kvm_hyp_vector)
 
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
  2021-05-10 13:48 ` [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code Marc Zyngier
  2021-05-10 13:48 ` [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-10 16:19   ` Mark Rutland
  2021-05-10 13:48 ` [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation Marc Zyngier
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

As it turns out, not all the interrupt controllers are able to
expose a vGIC maintenance interrupt as a distrete signal.
And to be fair, it doesn't really matter as all we require is
for *something* to kick us out of guest mode out way or another.

On systems that do not expose a maintenance interrupt as such,
there are two outcomes:

- either the virtual CPUIF does generate an interrupt, and
  by the time we are back to the host the interrupt will have long
  been disabled (as we set ICH_HCR_EL2.EN to 0 on exit). In this case,
  interrupt latency is as good as it gets.

- or some other event (physical timer) will take us out of the guest
  anyway, and the only drawback is a bad interrupt latency.

So let's be tolerant to the lack of maintenance interrupt, and just let
the user know that their mileage may vary...

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-init.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 2fdb65529594..9fd23f32aa54 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -524,11 +524,6 @@ int kvm_vgic_hyp_init(void)
 	if (!gic_kvm_info)
 		return -ENODEV;
 
-	if (!gic_kvm_info->maint_irq) {
-		kvm_err("No vgic maintenance irq\n");
-		return -ENXIO;
-	}
-
 	switch (gic_kvm_info->type) {
 	case GIC_V2:
 		ret = vgic_v2_probe(gic_kvm_info);
@@ -552,6 +547,11 @@ int kvm_vgic_hyp_init(void)
 	if (ret)
 		return ret;
 
+	if (!kvm_vgic_global_state.maint_irq) {
+		kvm_err("No maintenance interrupt available, fingers crossed...\n");
+		return 0;
+	}
+
 	ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
 				 vgic_maintenance_handler,
 				 "vgic", kvm_get_running_vcpus());
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (2 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-21 17:01   ` Alexandru Elisei
  2021-05-10 13:48 ` [PATCH v3 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure Marc Zyngier
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

The vGIC, as architected by ARM, allows a virtual interrupt to
trigger the deactivation of a physical interrupt. This allows
the following interrupt to be delivered without requiring an exit.

However, some implementations have choosen not to implement this,
meaning that we will need some unsavoury workarounds to deal with this.

On detecting such a case, taint the kernel and spit a nastygram.
We'll deal with this in later patches.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-init.c       | 10 ++++++++++
 include/kvm/arm_vgic.h                |  3 +++
 include/linux/irqchip/arm-vgic-info.h |  2 ++
 3 files changed, 15 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 9fd23f32aa54..5b06a9970a57 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -524,6 +524,16 @@ int kvm_vgic_hyp_init(void)
 	if (!gic_kvm_info)
 		return -ENODEV;
 
+	/*
+	 * If we get one of these oddball non-GICs, taint the kernel,
+	 * as we have no idea of how they *really* behave.
+	 */
+	if (gic_kvm_info->no_hw_deactivation) {
+		kvm_info("Non-architectural vgic, tainting kernel\n");
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		kvm_vgic_global_state.no_hw_deactivation = true;
+	}
+
 	switch (gic_kvm_info->type) {
 	case GIC_V2:
 		ret = vgic_v2_probe(gic_kvm_info);
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index ec621180ef09..e45b26e8d479 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -72,6 +72,9 @@ struct vgic_global {
 	bool			has_gicv4;
 	bool			has_gicv4_1;
 
+	/* Pseudo GICv3 from outer space */
+	bool			no_hw_deactivation;
+
 	/* GIC system register CPU interface */
 	struct static_key_false gicv3_cpuif;
 
diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h
index 0319636be928..d39d0b591c5a 100644
--- a/include/linux/irqchip/arm-vgic-info.h
+++ b/include/linux/irqchip/arm-vgic-info.h
@@ -30,6 +30,8 @@ struct gic_kvm_info {
 	bool		has_v4;
 	/* rvpeid support */
 	bool		has_v4_1;
+	/* Deactivation impared, subpar stuff */
+	bool		no_hw_deactivation;
 };
 
 #ifdef CONFIG_KVM
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (3 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-10 13:48 ` [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation Marc Zyngier
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

We already have the option to attach a callback to an interrupt
to retrieve its pending state. As we are planning to expand this
facility, move this callback into its own data structure.

This will limit the size of individual interrupts as the ops
structures can be shared across multiple interrupts.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arch_timer.c |  8 ++++++--
 arch/arm64/kvm/vgic/vgic.c  | 14 +++++++-------
 include/kvm/arm_vgic.h      | 28 +++++++++++++++++-----------
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 74e0699661e9..e2288b6bf435 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1116,6 +1116,10 @@ bool kvm_arch_timer_get_input_level(int vintid)
 	return kvm_timer_should_fire(timer);
 }
 
+static struct irq_ops arch_timer_irq_ops = {
+	.get_input_level = kvm_arch_timer_get_input_level,
+};
+
 int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = vcpu_timer(vcpu);
@@ -1143,7 +1147,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	ret = kvm_vgic_map_phys_irq(vcpu,
 				    map.direct_vtimer->host_timer_irq,
 				    map.direct_vtimer->irq.irq,
-				    kvm_arch_timer_get_input_level);
+				    &arch_timer_irq_ops);
 	if (ret)
 		return ret;
 
@@ -1151,7 +1155,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 		ret = kvm_vgic_map_phys_irq(vcpu,
 					    map.direct_ptimer->host_timer_irq,
 					    map.direct_ptimer->irq.irq,
-					    kvm_arch_timer_get_input_level);
+					    &arch_timer_irq_ops);
 	}
 
 	if (ret)
diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c
index 15b666200f0b..111bff47e471 100644
--- a/arch/arm64/kvm/vgic/vgic.c
+++ b/arch/arm64/kvm/vgic/vgic.c
@@ -182,8 +182,8 @@ bool vgic_get_phys_line_level(struct vgic_irq *irq)
 
 	BUG_ON(!irq->hw);
 
-	if (irq->get_input_level)
-		return irq->get_input_level(irq->intid);
+	if (irq->ops && irq->ops->get_input_level)
+		return irq->ops->get_input_level(irq->intid);
 
 	WARN_ON(irq_get_irqchip_state(irq->host_irq,
 				      IRQCHIP_STATE_PENDING,
@@ -480,7 +480,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 /* @irq->irq_lock must be held */
 static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 			    unsigned int host_irq,
-			    bool (*get_input_level)(int vindid))
+			    struct irq_ops *ops)
 {
 	struct irq_desc *desc;
 	struct irq_data *data;
@@ -500,7 +500,7 @@ static int kvm_vgic_map_irq(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	irq->hw = true;
 	irq->host_irq = host_irq;
 	irq->hwintid = data->hwirq;
-	irq->get_input_level = get_input_level;
+	irq->ops = ops;
 	return 0;
 }
 
@@ -509,11 +509,11 @@ static inline void kvm_vgic_unmap_irq(struct vgic_irq *irq)
 {
 	irq->hw = false;
 	irq->hwintid = 0;
-	irq->get_input_level = NULL;
+	irq->ops = NULL;
 }
 
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
-			  u32 vintid, bool (*get_input_level)(int vindid))
+			  u32 vintid, struct irq_ops *ops)
 {
 	struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, vintid);
 	unsigned long flags;
@@ -522,7 +522,7 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
 	BUG_ON(!irq);
 
 	raw_spin_lock_irqsave(&irq->irq_lock, flags);
-	ret = kvm_vgic_map_irq(vcpu, irq, host_irq, get_input_level);
+	ret = kvm_vgic_map_irq(vcpu, irq, host_irq, ops);
 	raw_spin_unlock_irqrestore(&irq->irq_lock, flags);
 	vgic_put_irq(vcpu->kvm, irq);
 
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index e45b26e8d479..e5f06df000f2 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -92,6 +92,21 @@ enum vgic_irq_config {
 	VGIC_CONFIG_LEVEL
 };
 
+/*
+ * Per-irq ops overriding some common behavious.
+ *
+ * Always called in non-preemptible section and the functions can use
+ * kvm_arm_get_running_vcpu() to get the vcpu pointer for private IRQs.
+ */
+struct irq_ops {
+	/*
+	 * Callback function pointer to in-kernel devices that can tell us the
+	 * state of the input level of mapped level-triggered IRQ faster than
+	 * peaking into the physical GIC.
+	 */
+	bool (*get_input_level)(int vintid);
+};
+
 struct vgic_irq {
 	raw_spinlock_t irq_lock;	/* Protects the content of the struct */
 	struct list_head lpi_list;	/* Used to link all LPIs together */
@@ -129,16 +144,7 @@ struct vgic_irq {
 	u8 group;			/* 0 == group 0, 1 == group 1 */
 	enum vgic_irq_config config;	/* Level or edge */
 
-	/*
-	 * Callback function pointer to in-kernel devices that can tell us the
-	 * state of the input level of mapped level-triggered IRQ faster than
-	 * peaking into the physical GIC.
-	 *
-	 * Always called in non-preemptible section and the functions can use
-	 * kvm_arm_get_running_vcpu() to get the vcpu pointer for private
-	 * IRQs.
-	 */
-	bool (*get_input_level)(int vintid);
+	struct irq_ops *ops;
 
 	void *owner;			/* Opaque pointer to reserve an interrupt
 					   for in-kernel devices. */
@@ -355,7 +361,7 @@ void kvm_vgic_init_cpu_hardware(void);
 int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
 			bool level, void *owner);
 int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, unsigned int host_irq,
-			  u32 vintid, bool (*get_input_level)(int vindid));
+			  u32 vintid, struct irq_ops *ops);
 int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int vintid);
 bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int vintid);
 
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (4 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-24 16:53   ` Alexandru Elisei
  2021-05-10 13:48 ` [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration Marc Zyngier
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

In order to deal with these systems that do not offer HW-based
deactivation of interrupts, let implement a SW-based approach:

- When the irq is queued into a LR, treat it as a pure virtual
  interrupt and set the EOI flag in the LR.

- When the interrupt state is read back from the LR, force a
  deactivation when the state is invalid (neither active nor
  pending)

Interrupts requiring such treatment get the VGIC_SW_RESAMPLE flag.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/vgic/vgic-v2.c | 19 +++++++++++++++----
 arch/arm64/kvm/vgic/vgic-v3.c | 19 +++++++++++++++----
 include/kvm/arm_vgic.h        | 10 ++++++++++
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 11934c2af2f4..2c580204f1dc 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -108,11 +108,22 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 		 * If this causes us to lower the level, we have to also clear
 		 * the physical active state, since we will otherwise never be
 		 * told when the interrupt becomes asserted again.
+		 *
+		 * Another case is when the interrupt requires a helping hand
+		 * on deactivation (no HW deactivation, for example).
 		 */
-		if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
-			irq->line_level = vgic_get_phys_line_level(irq);
+		if (vgic_irq_is_mapped_level(irq)) {
+			bool resample = false;
+
+			if (val & GICH_LR_PENDING_BIT) {
+				irq->line_level = vgic_get_phys_line_level(irq);
+				resample = !irq->line_level;
+			} else if (vgic_irq_needs_resampling(irq) &&
+				   !(irq->active || irq->pending_latch)) {
+				resample = true;
+			}
 
-			if (!irq->line_level)
+			if (resample)
 				vgic_irq_set_phys_active(irq, false);
 		}
 
@@ -152,7 +163,7 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (irq->group)
 		val |= GICH_LR_GROUP1;
 
-	if (irq->hw) {
+	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
 		val |= GICH_LR_HW;
 		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
 		/*
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 41ecf219c333..66004f61cd83 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -101,11 +101,22 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 		 * If this causes us to lower the level, we have to also clear
 		 * the physical active state, since we will otherwise never be
 		 * told when the interrupt becomes asserted again.
+		 *
+		 * Another case is when the interrupt requires a helping hand
+		 * on deactivation (no HW deactivation, for example).
 		 */
-		if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) {
-			irq->line_level = vgic_get_phys_line_level(irq);
+		if (vgic_irq_is_mapped_level(irq)) {
+			bool resample = false;
+
+			if (val & ICH_LR_PENDING_BIT) {
+				irq->line_level = vgic_get_phys_line_level(irq);
+				resample = !irq->line_level;
+			} else if (vgic_irq_needs_resampling(irq) &&
+				   !(irq->active || irq->pending_latch)) {
+				resample = true;
+			}
 
-			if (!irq->line_level)
+			if (resample)
 				vgic_irq_set_phys_active(irq, false);
 		}
 
@@ -136,7 +147,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 		}
 	}
 
-	if (irq->hw) {
+	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
 		val |= ICH_LR_HW;
 		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
 		/*
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index e5f06df000f2..e602d848fc1a 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -99,6 +99,11 @@ enum vgic_irq_config {
  * kvm_arm_get_running_vcpu() to get the vcpu pointer for private IRQs.
  */
 struct irq_ops {
+	/* Per interrupt flags for special-cased interrupts */
+	unsigned long flags;
+
+#define VGIC_IRQ_SW_RESAMPLE	BIT(0)	/* Clear the active state for resampling */
+
 	/*
 	 * Callback function pointer to in-kernel devices that can tell us the
 	 * state of the input level of mapped level-triggered IRQ faster than
@@ -150,6 +155,11 @@ struct vgic_irq {
 					   for in-kernel devices. */
 };
 
+static inline bool vgic_irq_needs_resampling(struct vgic_irq *irq)
+{
+	return irq->ops && (irq->ops->flags & VGIC_IRQ_SW_RESAMPLE);
+}
+
 struct vgic_register_region;
 struct vgic_its;
 
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (5 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-14 12:46   ` Zenghui Yu
  2021-05-10 13:48 ` [PATCH v3 8/9] KVM: arm64: timer: Add support for SW-based deactivation Marc Zyngier
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

As we are about to add some more things to the timer IRQ
configuration, move this code out of the main timer init code
into its own set of functions.

No functional changes.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arch_timer.c | 61 ++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index e2288b6bf435..7fa4f446456a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -973,6 +973,39 @@ static int kvm_timer_dying_cpu(unsigned int cpu)
 	return 0;
 }
 
+static void kvm_irq_fixup_flags(unsigned int virq, u32 *flags)
+{
+	*flags = irq_get_trigger_type(virq);
+	if (*flags != IRQF_TRIGGER_HIGH && *flags != IRQF_TRIGGER_LOW) {
+		kvm_err("Invalid trigger for timer IRQ%d, assuming level low\n",
+			virq);
+		*flags = IRQF_TRIGGER_LOW;
+	}
+}
+
+static int kvm_irq_init(struct arch_timer_kvm_info *info)
+{
+	struct irq_domain *domain = NULL;
+	struct fwnode_handle *fwnode;
+	struct irq_data *data;
+
+	if (info->virtual_irq <= 0) {
+		kvm_err("kvm_arch_timer: invalid virtual timer IRQ: %d\n",
+			info->virtual_irq);
+		return -ENODEV;
+	}
+
+	host_vtimer_irq = info->virtual_irq;
+	kvm_irq_fixup_flags(host_vtimer_irq, &host_vtimer_irq_flags);
+
+	if (info->physical_irq > 0) {
+		host_ptimer_irq = info->physical_irq;
+		kvm_irq_fixup_flags(host_ptimer_irq, &host_ptimer_irq_flags);
+	}
+
+	return 0;
+}
+
 int kvm_timer_hyp_init(bool has_gic)
 {
 	struct arch_timer_kvm_info *info;
@@ -986,22 +1019,11 @@ int kvm_timer_hyp_init(bool has_gic)
 		return -ENODEV;
 	}
 
-	/* First, do the virtual EL1 timer irq */
-
-	if (info->virtual_irq <= 0) {
-		kvm_err("kvm_arch_timer: invalid virtual timer IRQ: %d\n",
-			info->virtual_irq);
-		return -ENODEV;
-	}
-	host_vtimer_irq = info->virtual_irq;
+	err = kvm_irq_init(info);
+	if (err)
+		return err;
 
-	host_vtimer_irq_flags = irq_get_trigger_type(host_vtimer_irq);
-	if (host_vtimer_irq_flags != IRQF_TRIGGER_HIGH &&
-	    host_vtimer_irq_flags != IRQF_TRIGGER_LOW) {
-		kvm_err("Invalid trigger for vtimer IRQ%d, assuming level low\n",
-			host_vtimer_irq);
-		host_vtimer_irq_flags = IRQF_TRIGGER_LOW;
-	}
+	/* First, do the virtual EL1 timer irq */
 
 	err = request_percpu_irq(host_vtimer_irq, kvm_arch_timer_handler,
 				 "kvm guest vtimer", kvm_get_running_vcpus());
@@ -1027,15 +1049,6 @@ int kvm_timer_hyp_init(bool has_gic)
 	/* Now let's do the physical EL1 timer irq */
 
 	if (info->physical_irq > 0) {
-		host_ptimer_irq = info->physical_irq;
-		host_ptimer_irq_flags = irq_get_trigger_type(host_ptimer_irq);
-		if (host_ptimer_irq_flags != IRQF_TRIGGER_HIGH &&
-		    host_ptimer_irq_flags != IRQF_TRIGGER_LOW) {
-			kvm_err("Invalid trigger for ptimer IRQ%d, assuming level low\n",
-				host_ptimer_irq);
-			host_ptimer_irq_flags = IRQF_TRIGGER_LOW;
-		}
-
 		err = request_percpu_irq(host_ptimer_irq, kvm_arch_timer_handler,
 					 "kvm guest ptimer", kvm_get_running_vcpus());
 		if (err) {
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 8/9] KVM: arm64: timer: Add support for SW-based deactivation
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (6 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-10 13:48 ` [PATCH v3 9/9] irqchip/apple-aic: Advertise some level of vGICv3 compatibility Marc Zyngier
  2021-05-12 16:22 ` [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Alexandru Elisei
  9 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

In order to deal with the lack of active state, we need to use
the mask/unmask primitives (after all, the active state is just an
additional mask on top of the normal one).

To avoid adding a bunch of ugly conditionals in the timer and vgic
code, let's use a timer-specific irqdomain to deal with the state
conversion. Yes, this is an unexpected use of irqdomains, but
there is no reason not to be just as creative as the designers
of the HW...

This involves overloading the vcpu_affinity, set_irqchip_state
and eoi callbacks so that the rest of the KVM code can continue
ignoring the oddities of the underlying platform.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/arch_timer.c | 100 ++++++++++++++++++++++++++++++++++--
 1 file changed, 96 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 7fa4f446456a..4c24cbb19ba1 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -9,6 +9,7 @@
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/uaccess.h>
 
 #include <clocksource/arm_arch_timer.h>
@@ -973,6 +974,77 @@ static int kvm_timer_dying_cpu(unsigned int cpu)
 	return 0;
 }
 
+static int timer_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
+{
+	if (vcpu)
+		irqd_set_forwarded_to_vcpu(d);
+	else
+		irqd_clr_forwarded_to_vcpu(d);
+
+	return 0;
+}
+
+static int timer_irq_set_irqchip_state(struct irq_data *d,
+				       enum irqchip_irq_state which, bool val)
+{
+	if (which != IRQCHIP_STATE_ACTIVE || !irqd_is_forwarded_to_vcpu(d))
+		return irq_chip_set_parent_state(d, which, val);
+
+	if (val)
+		irq_chip_mask_parent(d);
+	else
+		irq_chip_unmask_parent(d);
+
+	return 0;
+}
+
+static void timer_irq_eoi(struct irq_data *d)
+{
+	if (!irqd_is_forwarded_to_vcpu(d))
+		irq_chip_eoi_parent(d);
+}
+
+static void timer_irq_ack(struct irq_data *d)
+{
+	d = d->parent_data;
+	if (d->chip->irq_ack)
+		d->chip->irq_ack(d);
+}
+
+static struct irq_chip timer_chip = {
+	.name			= "KVM",
+	.irq_ack		= timer_irq_ack,
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= timer_irq_eoi,
+	.irq_set_type		= irq_chip_set_type_parent,
+	.irq_set_vcpu_affinity	= timer_irq_set_vcpu_affinity,
+	.irq_set_irqchip_state	= timer_irq_set_irqchip_state,
+};
+
+static int timer_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs, void *arg)
+{
+	irq_hw_number_t hwirq = (uintptr_t)arg;
+
+	return irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					     &timer_chip, NULL);
+}
+
+static void timer_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+				  unsigned int nr_irqs)
+{
+}
+
+static const struct irq_domain_ops timer_domain_ops = {
+	.alloc	= timer_irq_domain_alloc,
+	.free	= timer_irq_domain_free,
+};
+
+static struct irq_ops arch_timer_irq_ops = {
+	.get_input_level = kvm_arch_timer_get_input_level,
+};
+
 static void kvm_irq_fixup_flags(unsigned int virq, u32 *flags)
 {
 	*flags = irq_get_trigger_type(virq);
@@ -998,9 +1070,33 @@ static int kvm_irq_init(struct arch_timer_kvm_info *info)
 	host_vtimer_irq = info->virtual_irq;
 	kvm_irq_fixup_flags(host_vtimer_irq, &host_vtimer_irq_flags);
 
+	if (kvm_vgic_global_state.no_hw_deactivation) {
+		fwnode = irq_domain_alloc_named_fwnode("kvm-timer");
+		if (!fwnode)
+			return -ENOMEM;
+
+		/* Assume both vtimer and ptimer in the same parent */
+		data = irq_get_irq_data(host_vtimer_irq);
+		domain = irq_domain_create_hierarchy(data->domain, 0,
+						     NR_KVM_TIMERS, fwnode,
+						     &timer_domain_ops, NULL);
+		if (!domain) {
+			irq_domain_free_fwnode(fwnode);
+			return -ENOMEM;
+		}
+
+		arch_timer_irq_ops.flags |= VGIC_IRQ_SW_RESAMPLE;
+		WARN_ON(irq_domain_push_irq(domain, host_vtimer_irq,
+					    (void *)TIMER_VTIMER));
+	}
+
 	if (info->physical_irq > 0) {
 		host_ptimer_irq = info->physical_irq;
 		kvm_irq_fixup_flags(host_ptimer_irq, &host_ptimer_irq_flags);
+
+		if (domain)
+			WARN_ON(irq_domain_push_irq(domain, host_ptimer_irq,
+						    (void *)TIMER_PTIMER));
 	}
 
 	return 0;
@@ -1129,10 +1225,6 @@ bool kvm_arch_timer_get_input_level(int vintid)
 	return kvm_timer_should_fire(timer);
 }
 
-static struct irq_ops arch_timer_irq_ops = {
-	.get_input_level = kvm_arch_timer_get_input_level,
-};
-
 int kvm_timer_enable(struct kvm_vcpu *vcpu)
 {
 	struct arch_timer_cpu *timer = vcpu_timer(vcpu);
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 9/9] irqchip/apple-aic: Advertise some level of vGICv3 compatibility
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (7 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 8/9] KVM: arm64: timer: Add support for SW-based deactivation Marc Zyngier
@ 2021-05-10 13:48 ` Marc Zyngier
  2021-05-12 16:22 ` [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Alexandru Elisei
  9 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 13:48 UTC (permalink / raw)
  To: linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Eric Auger,
	Hector Martin, Mark Rutland, kernel-team

The CPUs in the Apple M1 SoC partially implement a virtual GICv3
CPU interface, although one that is incapable of HW deactivation
of interrupts.

Advertise the support to KVM.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/irqchip/irq-apple-aic.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
index c179e27062fd..a44370c018e2 100644
--- a/drivers/irqchip/irq-apple-aic.c
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -50,6 +50,7 @@
 #include <linux/cpuhotplug.h>
 #include <linux/io.h>
 #include <linux/irqchip.h>
+#include <linux/irqchip/arm-vgic-info.h>
 #include <linux/irqdomain.h>
 #include <linux/limits.h>
 #include <linux/of_address.h>
@@ -787,6 +788,11 @@ static int aic_init_cpu(unsigned int cpu)
 	return 0;
 }
 
+static struct gic_kvm_info vgic_info __initdata = {
+	.type			= GIC_V3,
+	.no_hw_deactivation	= true,
+};
+
 static int __init aic_of_ic_init(struct device_node *node, struct device_node *parent)
 {
 	int i;
@@ -843,6 +849,8 @@ static int __init aic_of_ic_init(struct device_node *node, struct device_node *p
 			  "irqchip/apple-aic/ipi:starting",
 			  aic_init_cpu, NULL);
 
+	vgic_set_kvm_info(&vgic_info);
+
 	pr_info("Initialized with %d IRQs, %d FIQs, %d vIPIs\n",
 		irqc->nr_hw, AIC_NR_FIQ, AIC_NR_SWIPI);
 
-- 
2.29.2


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt
  2021-05-10 13:48 ` [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt Marc Zyngier
@ 2021-05-10 16:19   ` Mark Rutland
  2021-05-10 17:44     ` Marc Zyngier
  0 siblings, 1 reply; 23+ messages in thread
From: Mark Rutland @ 2021-05-10 16:19 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-arm-kernel, kvm, kvmarm, James Morse, Suzuki K Poulose,
	Alexandru Elisei, Eric Auger, Hector Martin, kernel-team

On Mon, May 10, 2021 at 02:48:18PM +0100, Marc Zyngier wrote:
> As it turns out, not all the interrupt controllers are able to
> expose a vGIC maintenance interrupt as a distrete signal.
> And to be fair, it doesn't really matter as all we require is
> for *something* to kick us out of guest mode out way or another.
> 
> On systems that do not expose a maintenance interrupt as such,
> there are two outcomes:
> 
> - either the virtual CPUIF does generate an interrupt, and
>   by the time we are back to the host the interrupt will have long
>   been disabled (as we set ICH_HCR_EL2.EN to 0 on exit). In this case,
>   interrupt latency is as good as it gets.
> 
> - or some other event (physical timer) will take us out of the guest
>   anyway, and the only drawback is a bad interrupt latency.

IIRC we won't have a a guaranteed schedular tick for NO_HZ_FULL, so in
that case we'll either need to set a period software maintenance
interrupt, or reject this combination at runtime (either when trying to
isolate the dynticks CPUs, or when trying to create a VM).

Otherwise, it's very likely that something will take us out of the guest
from time to time, but we won't have a strict guarantee (e.g. if all
guest memory is pinned).

Thanks,
Mark.

> 
> So let's be tolerant to the lack of maintenance interrupt, and just let
> the user know that their mileage may vary...
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 2fdb65529594..9fd23f32aa54 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -524,11 +524,6 @@ int kvm_vgic_hyp_init(void)
>  	if (!gic_kvm_info)
>  		return -ENODEV;
>  
> -	if (!gic_kvm_info->maint_irq) {
> -		kvm_err("No vgic maintenance irq\n");
> -		return -ENXIO;
> -	}
> -
>  	switch (gic_kvm_info->type) {
>  	case GIC_V2:
>  		ret = vgic_v2_probe(gic_kvm_info);
> @@ -552,6 +547,11 @@ int kvm_vgic_hyp_init(void)
>  	if (ret)
>  		return ret;
>  
> +	if (!kvm_vgic_global_state.maint_irq) {
> +		kvm_err("No maintenance interrupt available, fingers crossed...\n");
> +		return 0;
> +	}
> +
>  	ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>  				 vgic_maintenance_handler,
>  				 "vgic", kvm_get_running_vcpus());
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt
  2021-05-10 16:19   ` Mark Rutland
@ 2021-05-10 17:44     ` Marc Zyngier
  2021-05-11 11:13       ` Mark Rutland
  0 siblings, 1 reply; 23+ messages in thread
From: Marc Zyngier @ 2021-05-10 17:44 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-arm-kernel, kvm, kvmarm, James Morse, Suzuki K Poulose,
	Alexandru Elisei, Eric Auger, Hector Martin, kernel-team

On Mon, 10 May 2021 17:19:07 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Mon, May 10, 2021 at 02:48:18PM +0100, Marc Zyngier wrote:
> > As it turns out, not all the interrupt controllers are able to
> > expose a vGIC maintenance interrupt as a distrete signal.
> > And to be fair, it doesn't really matter as all we require is
> > for *something* to kick us out of guest mode out way or another.
> > 
> > On systems that do not expose a maintenance interrupt as such,
> > there are two outcomes:
> > 
> > - either the virtual CPUIF does generate an interrupt, and
> >   by the time we are back to the host the interrupt will have long
> >   been disabled (as we set ICH_HCR_EL2.EN to 0 on exit). In this case,
> >   interrupt latency is as good as it gets.
> > 
> > - or some other event (physical timer) will take us out of the guest
> >   anyway, and the only drawback is a bad interrupt latency.
> 
> IIRC we won't have a a guaranteed schedular tick for NO_HZ_FULL, so in
> that case we'll either need to set a period software maintenance
> interrupt, or reject this combination at runtime (either when trying to
> isolate the dynticks CPUs, or when trying to create a VM).

That's a good point.

On sensible systems, the maintenance interrupt is a standard GIC PPI
that requires enabling, and that is all that KVM requires (the
maintenance interrupt is only used as an exit mechanism and will be
disabled before reaching the handler).

On the M1, owing to the lack of a per-CPU interrupt controller, there
is nothing to enable. The virtual CPU interface will fire at will and
take us out of the guest in a timely manner.

So maybe instead of relaxing the requirement for a maintenance
interrupt, we should only bypass the checks if the root interrupt
controller advertises that it is safe to do so, making it a
M1-specific hack.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt
  2021-05-10 17:44     ` Marc Zyngier
@ 2021-05-11 11:13       ` Mark Rutland
  0 siblings, 0 replies; 23+ messages in thread
From: Mark Rutland @ 2021-05-11 11:13 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: linux-arm-kernel, kvm, kvmarm, James Morse, Suzuki K Poulose,
	Alexandru Elisei, Eric Auger, Hector Martin, kernel-team

On Mon, May 10, 2021 at 06:44:49PM +0100, Marc Zyngier wrote:
> On Mon, 10 May 2021 17:19:07 +0100,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Mon, May 10, 2021 at 02:48:18PM +0100, Marc Zyngier wrote:
> > > As it turns out, not all the interrupt controllers are able to
> > > expose a vGIC maintenance interrupt as a distrete signal.
> > > And to be fair, it doesn't really matter as all we require is
> > > for *something* to kick us out of guest mode out way or another.
> > > 
> > > On systems that do not expose a maintenance interrupt as such,
> > > there are two outcomes:
> > > 
> > > - either the virtual CPUIF does generate an interrupt, and
> > >   by the time we are back to the host the interrupt will have long
> > >   been disabled (as we set ICH_HCR_EL2.EN to 0 on exit). In this case,
> > >   interrupt latency is as good as it gets.
> > > 
> > > - or some other event (physical timer) will take us out of the guest
> > >   anyway, and the only drawback is a bad interrupt latency.
> > 
> > IIRC we won't have a a guaranteed schedular tick for NO_HZ_FULL, so in
> > that case we'll either need to set a period software maintenance
> > interrupt, or reject this combination at runtime (either when trying to
> > isolate the dynticks CPUs, or when trying to create a VM).
> 
> That's a good point.
> 
> On sensible systems, the maintenance interrupt is a standard GIC PPI
> that requires enabling, and that is all that KVM requires (the
> maintenance interrupt is only used as an exit mechanism and will be
> disabled before reaching the handler).
> 
> On the M1, owing to the lack of a per-CPU interrupt controller, there
> is nothing to enable. The virtual CPU interface will fire at will and
> take us out of the guest in a timely manner.

Ah, so the M1 does have a maintenance interrupt, but you can't silence
it at the irqchip level.

> So maybe instead of relaxing the requirement for a maintenance
> interrupt, we should only bypass the checks if the root interrupt
> controller advertises that it is safe to do so, making it a
> M1-specific hack.

That certainly sounds safer than permitting running without any
maintenance interrupt at all.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1
  2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
                   ` (8 preceding siblings ...)
  2021-05-10 13:48 ` [PATCH v3 9/9] irqchip/apple-aic: Advertise some level of vGICv3 compatibility Marc Zyngier
@ 2021-05-12 16:22 ` Alexandru Elisei
  2021-05-12 16:33   ` Marc Zyngier
  9 siblings, 1 reply; 23+ messages in thread
From: Alexandru Elisei @ 2021-05-12 16:22 UTC (permalink / raw)
  To: Marc Zyngier, linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Eric Auger, Hector Martin,
	Mark Rutland, kernel-team

Hi Marc,

On 5/10/21 2:48 PM, Marc Zyngier wrote:
> This is a new version of the series previously posted at [2], reworking
> the vGIC and timer code to cope with the M1 braindead^Wamusing nature.
>
> Hardly any change this time around, mostly rebased on top of upstream
> now that the dependencies have made it in.
>
> Tested with multiple concurrent VMs running from an initramfs.
>
> * From v2:
>   - Rebased on 5.13-rc1
>   - Fixed a couple of nits in the GIC registration code
>
> * From v1 [1]:
>   - Rebased on Hector's v4 posting[0]
>   - Dropped a couple of patches that have been merged in the above series
>   - Fixed irq_ack callback on the timer path
>
> [0] https://lore.kernel.org/r/20210402090542.131194-1-marcan@marcan.st
> [1] https://lore.kernel.org/r/20210316174617.173033-1-maz@kernel.org
> [2] https://lore.kernel.org/r/20210403112931.1043452-1-maz@kernel.org

This looks interesting and I want to take a look. For now, I can only review the
series, but maybe at some point I'll take the leap and try to run Linux on my
Macbook Air.

Can I find something resembling a specification for the Apple interrupt
controller, or the only available documentation is in the Linux driver and patches
on the mailing list?

Thanks,

Alex

>
> Marc Zyngier (9):
>   irqchip/gic: Split vGIC probing information from the GIC code
>   KVM: arm64: Handle physical FIQ as an IRQ while running a guest
>   KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt
>   KVM: arm64: vgic: Let an interrupt controller advertise lack of HW
>     deactivation
>   KVM: arm64: vgic: move irq->get_input_level into an ops structure
>   KVM: arm64: vgic: Implement SW-driven deactivation
>   KVM: arm64: timer: Refactor IRQ configuration
>   KVM: arm64: timer: Add support for SW-based deactivation
>   irqchip/apple-aic: Advertise some level of vGICv3 compatibility
>
>  arch/arm64/kvm/arch_timer.c            | 161 +++++++++++++++++++++----
>  arch/arm64/kvm/hyp/hyp-entry.S         |   6 +-
>  arch/arm64/kvm/vgic/vgic-init.c        |  34 +++++-
>  arch/arm64/kvm/vgic/vgic-v2.c          |  19 ++-
>  arch/arm64/kvm/vgic/vgic-v3.c          |  19 ++-
>  arch/arm64/kvm/vgic/vgic.c             |  14 +--
>  drivers/irqchip/irq-apple-aic.c        |   8 ++
>  drivers/irqchip/irq-gic-common.c       |  13 --
>  drivers/irqchip/irq-gic-common.h       |   2 -
>  drivers/irqchip/irq-gic-v3.c           |   6 +-
>  drivers/irqchip/irq-gic.c              |   6 +-
>  include/kvm/arm_vgic.h                 |  41 +++++--
>  include/linux/irqchip/arm-gic-common.h |  25 +---
>  include/linux/irqchip/arm-vgic-info.h  |  43 +++++++
>  14 files changed, 291 insertions(+), 106 deletions(-)
>  create mode 100644 include/linux/irqchip/arm-vgic-info.h
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1
  2021-05-12 16:22 ` [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Alexandru Elisei
@ 2021-05-12 16:33   ` Marc Zyngier
  0 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-12 16:33 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: linux-arm-kernel, kvm, kvmarm, James Morse, Suzuki K Poulose,
	Eric Auger, Hector Martin, Mark Rutland, kernel-team

Hi Alex,

On Wed, 12 May 2021 17:22:43 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 5/10/21 2:48 PM, Marc Zyngier wrote:
> > This is a new version of the series previously posted at [2], reworking
> > the vGIC and timer code to cope with the M1 braindead^Wamusing nature.
> >
> > Hardly any change this time around, mostly rebased on top of upstream
> > now that the dependencies have made it in.
> >
> > Tested with multiple concurrent VMs running from an initramfs.
> >
> > * From v2:
> >   - Rebased on 5.13-rc1
> >   - Fixed a couple of nits in the GIC registration code
> >
> > * From v1 [1]:
> >   - Rebased on Hector's v4 posting[0]
> >   - Dropped a couple of patches that have been merged in the above series
> >   - Fixed irq_ack callback on the timer path
> >
> > [0] https://lore.kernel.org/r/20210402090542.131194-1-marcan@marcan.st
> > [1] https://lore.kernel.org/r/20210316174617.173033-1-maz@kernel.org
> > [2] https://lore.kernel.org/r/20210403112931.1043452-1-maz@kernel.org
> 
> This looks interesting and I want to take a look. For now, I can
> only review the series, but maybe at some point I'll take the leap
> and try to run Linux on my Macbook Air.

It is a bit involved at the moment, and I haven't tried on a laptop
(the nice thing about the Mini is that you can bury it under a pile of
other machines and still make use of it).

> Can I find something resembling a specification for the Apple
> interrupt controller, or the only available documentation is in the
> Linux driver and patches on the mailing list?

The Asahi wiki has a bunch of RE goodies, but you really don't need to
know much about the HW to follow what this series does. Actually, you
instead need to understand what the GIC guarantees to the guest,
because this is all about the GIC emulation on a non-GIC interrupt
controller.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration
  2021-05-10 13:48 ` [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration Marc Zyngier
@ 2021-05-14 12:46   ` Zenghui Yu
  2021-05-24 17:48     ` Marc Zyngier
  0 siblings, 1 reply; 23+ messages in thread
From: Zenghui Yu @ 2021-05-14 12:46 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-arm-kernel, kvm, kvmarm, kernel-team, Hector Martin

On 2021/5/10 21:48, Marc Zyngier wrote:
> As we are about to add some more things to the timer IRQ
> configuration, move this code out of the main timer init code
> into its own set of functions.
> 
> No functional changes.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arch_timer.c | 61 ++++++++++++++++++++++---------------
>  1 file changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index e2288b6bf435..7fa4f446456a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -973,6 +973,39 @@ static int kvm_timer_dying_cpu(unsigned int cpu)
>  	return 0;
>  }
>  
> +static void kvm_irq_fixup_flags(unsigned int virq, u32 *flags)
> +{
> +	*flags = irq_get_trigger_type(virq);
> +	if (*flags != IRQF_TRIGGER_HIGH && *flags != IRQF_TRIGGER_LOW) {
> +		kvm_err("Invalid trigger for timer IRQ%d, assuming level low\n",
> +			virq);
> +		*flags = IRQF_TRIGGER_LOW;
> +	}
> +}
> +
> +static int kvm_irq_init(struct arch_timer_kvm_info *info)
> +{
> +	struct irq_domain *domain = NULL;
> +	struct fwnode_handle *fwnode;
> +	struct irq_data *data;

Shouldn't this belong to patch #8?

> +
> +	if (info->virtual_irq <= 0) {
> +		kvm_err("kvm_arch_timer: invalid virtual timer IRQ: %d\n",
> +			info->virtual_irq);
> +		return -ENODEV;
> +	}
> +
> +	host_vtimer_irq = info->virtual_irq;
> +	kvm_irq_fixup_flags(host_vtimer_irq, &host_vtimer_irq_flags);
> +
> +	if (info->physical_irq > 0) {
> +		host_ptimer_irq = info->physical_irq;
> +		kvm_irq_fixup_flags(host_ptimer_irq, &host_ptimer_irq_flags);
> +	}
> +
> +	return 0;
> +}

Otherwise this look like a good refactoring.

Zenghui

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code
  2021-05-10 13:48 ` [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code Marc Zyngier
@ 2021-05-18 16:51   ` Alexandru Elisei
  0 siblings, 0 replies; 23+ messages in thread
From: Alexandru Elisei @ 2021-05-18 16:51 UTC (permalink / raw)
  To: Marc Zyngier, linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Eric Auger, Hector Martin,
	Mark Rutland, kernel-team

Hi Marc,

On 5/10/21 2:48 PM, Marc Zyngier wrote:
> The vGIC advertising code is unsurprisingly very much tied to
> the GIC implementations. However, we are about to extend the
> support to lesser implementations.
>
> Let's dissociate the vgic registration from the GIC code and
> move it into KVM, where it makes a bit more sense. This also
> allows us to mark the gic_kvm_info structures as __initdata.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c        | 18 +++++++++--
>  drivers/irqchip/irq-gic-common.c       | 13 --------
>  drivers/irqchip/irq-gic-common.h       |  2 --
>  drivers/irqchip/irq-gic-v3.c           |  6 ++--
>  drivers/irqchip/irq-gic.c              |  6 ++--
>  include/linux/irqchip/arm-gic-common.h | 25 +---------------
>  include/linux/irqchip/arm-vgic-info.h  | 41 ++++++++++++++++++++++++++
>  7 files changed, 63 insertions(+), 48 deletions(-)
>  create mode 100644 include/linux/irqchip/arm-vgic-info.h
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 58cbda00e56d..2fdb65529594 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -482,6 +482,16 @@ static irqreturn_t vgic_maintenance_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static struct gic_kvm_info *gic_kvm_info;
> +
> +void __init vgic_set_kvm_info(const struct gic_kvm_info *info)
> +{
> +	BUG_ON(gic_kvm_info != NULL);
> +	gic_kvm_info = kmalloc(sizeof(*info), GFP_KERNEL);
> +	if (gic_kvm_info)
> +		*gic_kvm_info = *info;
> +}
> +
>  /**
>   * kvm_vgic_init_cpu_hardware - initialize the GIC VE hardware
>   *
> @@ -509,10 +519,8 @@ void kvm_vgic_init_cpu_hardware(void)
>   */
>  int kvm_vgic_hyp_init(void)
>  {
> -	const struct gic_kvm_info *gic_kvm_info;
>  	int ret;
>  
> -	gic_kvm_info = gic_get_kvm_info();
>  	if (!gic_kvm_info)
>  		return -ENODEV;
>  
> @@ -536,10 +544,14 @@ int kvm_vgic_hyp_init(void)
>  		ret = -ENODEV;
>  	}
>  
> +	kvm_vgic_global_state.maint_irq = gic_kvm_info->maint_irq;
> +
> +	kfree(gic_kvm_info);
> +	gic_kvm_info = NULL;

I double checked and gic_kvm_info is not used after this point (vgic_{v2,v3}_probe
make copies of the various fields). And after returning an error (below) this
function cannot be called again.

> +
>  	if (ret)
>  		return ret;
>  
> -	kvm_vgic_global_state.maint_irq = gic_kvm_info->maint_irq;
>  	ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>  				 vgic_maintenance_handler,
>  				 "vgic", kvm_get_running_vcpus());
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index f47b41dfd023..a610821c8ff2 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -12,19 +12,6 @@
>  
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  
> -static const struct gic_kvm_info *gic_kvm_info;
> -
> -const struct gic_kvm_info *gic_get_kvm_info(void)
> -{
> -	return gic_kvm_info;
> -}
> -
> -void gic_set_kvm_info(const struct gic_kvm_info *info)
> -{
> -	BUG_ON(gic_kvm_info != NULL);
> -	gic_kvm_info = info;
> -}
> -
>  void gic_enable_of_quirks(const struct device_node *np,
>  			  const struct gic_quirk *quirks, void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index ccba8b0fe0f5..27e3d4ed4f32 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -28,6 +28,4 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  void gic_enable_of_quirks(const struct device_node *np,
>  			  const struct gic_quirk *quirks, void *data);
>  
> -void gic_set_kvm_info(const struct gic_kvm_info *info);
> -
>  #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 37a23aa6de37..453fc425eede 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -103,7 +103,7 @@ EXPORT_SYMBOL(gic_nonsecure_priorities);
>  /* ppi_nmi_refs[n] == number of cpus having ppi[n + 16] set as NMI */
>  static refcount_t *ppi_nmi_refs;
>  
> -static struct gic_kvm_info gic_v3_kvm_info;
> +static struct gic_kvm_info gic_v3_kvm_info __initdata;
>  static DEFINE_PER_CPU(bool, has_rss);
>  
>  #define MPIDR_RS(mpidr)			(((mpidr) & 0xF0UL) >> 4)
> @@ -1852,7 +1852,7 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  
>  	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
>  	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
> -	gic_set_kvm_info(&gic_v3_kvm_info);
> +	vgic_set_kvm_info(&gic_v3_kvm_info);
>  }
>  
>  static int __init gic_of_init(struct device_node *node, struct device_node *parent)
> @@ -2168,7 +2168,7 @@ static void __init gic_acpi_setup_kvm_info(void)
>  
>  	gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
>  	gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
> -	gic_set_kvm_info(&gic_v3_kvm_info);
> +	vgic_set_kvm_info(&gic_v3_kvm_info);
>  }
>  
>  static int __init
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index b1d9c22caf2e..2de9ec8ece0c 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -119,7 +119,7 @@ static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
>  
>  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
>  
> -static struct gic_kvm_info gic_v2_kvm_info;
> +static struct gic_kvm_info gic_v2_kvm_info __initdata;
>  
>  static DEFINE_PER_CPU(u32, sgi_intid);
>  
> @@ -1451,7 +1451,7 @@ static void __init gic_of_setup_kvm_info(struct device_node *node)
>  		return;
>  
>  	if (static_branch_likely(&supports_deactivate_key))
> -		gic_set_kvm_info(&gic_v2_kvm_info);
> +		vgic_set_kvm_info(&gic_v2_kvm_info);
>  }
>  
>  int __init
> @@ -1618,7 +1618,7 @@ static void __init gic_acpi_setup_kvm_info(void)
>  
>  	gic_v2_kvm_info.maint_irq = irq;
>  
> -	gic_set_kvm_info(&gic_v2_kvm_info);
> +	vgic_set_kvm_info(&gic_v2_kvm_info);
>  }
>  
>  static int __init gic_v2_acpi_init(union acpi_subtable_headers *header,
> diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
> index fa8c0455c352..1177f3a1aed5 100644
> --- a/include/linux/irqchip/arm-gic-common.h
> +++ b/include/linux/irqchip/arm-gic-common.h
> @@ -7,8 +7,7 @@
>  #ifndef __LINUX_IRQCHIP_ARM_GIC_COMMON_H
>  #define __LINUX_IRQCHIP_ARM_GIC_COMMON_H
>  
> -#include <linux/types.h>
> -#include <linux/ioport.h>
> +#include <linux/irqchip/arm-vgic-info.h>
>  
>  #define GICD_INT_DEF_PRI		0xa0
>  #define GICD_INT_DEF_PRI_X4		((GICD_INT_DEF_PRI << 24) |\
> @@ -16,28 +15,6 @@
>  					(GICD_INT_DEF_PRI << 8) |\
>  					GICD_INT_DEF_PRI)
>  
> -enum gic_type {
> -	GIC_V2,
> -	GIC_V3,
> -};
> -
> -struct gic_kvm_info {
> -	/* GIC type */
> -	enum gic_type	type;
> -	/* Virtual CPU interface */
> -	struct resource vcpu;
> -	/* Interrupt number */
> -	unsigned int	maint_irq;
> -	/* Virtual control interface */
> -	struct resource vctrl;
> -	/* vlpi support */
> -	bool		has_v4;
> -	/* rvpeid support */
> -	bool		has_v4_1;
> -};
> -
> -const struct gic_kvm_info *gic_get_kvm_info(void);
> -
>  struct irq_domain;
>  struct fwnode_handle;
>  int gicv2m_init(struct fwnode_handle *parent_handle,
> diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h
> new file mode 100644
> index 000000000000..0319636be928
> --- /dev/null
> +++ b/include/linux/irqchip/arm-vgic-info.h
> @@ -0,0 +1,41 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * include/linux/irqchip/arm-vgic-info.h
> + *
> + * Copyright (C) 2016 ARM Limited, All Rights Reserved.
> + */
> +#ifndef __ARM_VGIC_INFO_H
> +#define __ARM_VGIC_INFO_H

Totally irrelevant nitpick, but the header guards from the other files in this
directory are like __LINUX_IRQCHIP_ARM_VGIC_INFO_H. Regardless, the patch looks
correct to me, the functions are called at the exact moment in the boot flow, only
where the VGIC info is saved is different:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,

Alex

> +
> +#include <linux/types.h>
> +#include <linux/ioport.h>
> +
> +enum gic_type {
> +	/* Full GICv2 */
> +	GIC_V2,
> +	/* Full GICv3, optionally with v2 compat */
> +	GIC_V3,
> +};
> +
> +struct gic_kvm_info {
> +	/* GIC type */
> +	enum gic_type	type;
> +	/* Virtual CPU interface */
> +	struct resource vcpu;
> +	/* Interrupt number */
> +	unsigned int	maint_irq;
> +	/* Virtual control interface */
> +	struct resource vctrl;
> +	/* vlpi support */
> +	bool		has_v4;
> +	/* rvpeid support */
> +	bool		has_v4_1;
> +};
> +
> +#ifdef CONFIG_KVM
> +void vgic_set_kvm_info(const struct gic_kvm_info *info);
> +#else
> +static inline void vgic_set_kvm_info(const struct gic_kvm_info *info) {}
> +#endif
> +
> +#endif

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest
  2021-05-10 13:48 ` [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest Marc Zyngier
@ 2021-05-20 17:46   ` Alexandru Elisei
  0 siblings, 0 replies; 23+ messages in thread
From: Alexandru Elisei @ 2021-05-20 17:46 UTC (permalink / raw)
  To: Marc Zyngier, linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Eric Auger, Hector Martin,
	Mark Rutland, kernel-team

Hi Marc,

On 5/10/21 2:48 PM, Marc Zyngier wrote:
> As we we now entertain the possibility of FIQ being used on the host,
> treat the signalling of a FIQ while running a guest as an IRQ,
> causing an exit instead of a HYP panic.

I've mulling over this, and I can't find anything wrong with it. Any FIQs for
which there is no handler registered by the interrupt controller will panic in the
default_handle_fiq() FIQ handler, similar to the current KVM behaviour. And if
there is a handler registered (only AIC does it for now), then a FIQ will be
handled just like any other interrupt instead of KVM panic'ing when the host can
handle it just fine.

I've briefly considered creating a new return code from __kvm_vcpu_run,
ARM_EXCEPTION_FIQ, but I really don't see any reason for it, since it will serve
the same purpose as ARM_EXCEPTION_IRQ, which is to resume the guest without any
special exit handling.

It makes sense to me for KVM to handle FIQs just like IRQs, now that the kernel
treats them the same:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,

Alex

>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/hyp/hyp-entry.S | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
> index 5f49df4ffdd8..9aa9b73475c9 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -76,6 +76,7 @@ el1_trap:
>  	b	__guest_exit
>  
>  el1_irq:
> +el1_fiq:
>  	get_vcpu_ptr	x1, x0
>  	mov	x0, #ARM_EXCEPTION_IRQ
>  	b	__guest_exit
> @@ -131,7 +132,6 @@ SYM_CODE_END(\label)
>  	invalid_vector	el2t_error_invalid
>  	invalid_vector	el2h_irq_invalid
>  	invalid_vector	el2h_fiq_invalid
> -	invalid_vector	el1_fiq_invalid
>  
>  	.ltorg
>  
> @@ -179,12 +179,12 @@ SYM_CODE_START(__kvm_hyp_vector)
>  
>  	valid_vect	el1_sync		// Synchronous 64-bit EL1
>  	valid_vect	el1_irq			// IRQ 64-bit EL1
> -	invalid_vect	el1_fiq_invalid		// FIQ 64-bit EL1
> +	valid_vect	el1_fiq			// FIQ 64-bit EL1
>  	valid_vect	el1_error		// Error 64-bit EL1
>  
>  	valid_vect	el1_sync		// Synchronous 32-bit EL1
>  	valid_vect	el1_irq			// IRQ 32-bit EL1
> -	invalid_vect	el1_fiq_invalid		// FIQ 32-bit EL1
> +	valid_vect	el1_fiq			// FIQ 32-bit EL1
>  	valid_vect	el1_error		// Error 32-bit EL1
>  SYM_CODE_END(__kvm_hyp_vector)
>  

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation
  2021-05-10 13:48 ` [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation Marc Zyngier
@ 2021-05-21 17:01   ` Alexandru Elisei
  2021-05-24 17:17     ` Marc Zyngier
  0 siblings, 1 reply; 23+ messages in thread
From: Alexandru Elisei @ 2021-05-21 17:01 UTC (permalink / raw)
  To: Marc Zyngier, linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Eric Auger, Hector Martin,
	Mark Rutland, kernel-team

Hi Marc,

On 5/10/21 2:48 PM, Marc Zyngier wrote:
> The vGIC, as architected by ARM, allows a virtual interrupt to
> trigger the deactivation of a physical interrupt. This allows
> the following interrupt to be delivered without requiring an exit.

If I got this right, the AIC doesn't implement/ignores the ICH_LR_EL2.HW bit. Does
it mean that the CPU IF behaves as if HW = 0b0, meaning it asserts a maintenance
interrupt on virtual interrupt deactivation when ICH_LR_EL2.EOI = 0b1? I assume
that's the case, just double checking.

I am wondering what would happen if we come across an interrupt controller where
the CPU IF cannot assert a maintenance interrupt at all and we rely on the EOI bit
to take us out of the guest to deactivate the HW interrupt. I have to say that it
looks a bit strange to start relying on the maintenance interrupt to emulate
interrupt deactivate for hardware interrupts, but at the same timer allowing an
interrupt controller without a maintenance interrupt.

Other than that, this idea sounds like the best thing to do considering the
circumstances, I certainly can't think of anything better.

>
> However, some implementations have choosen not to implement this,
> meaning that we will need some unsavoury workarounds to deal with this.
>
> On detecting such a case, taint the kernel and spit a nastygram.
> We'll deal with this in later patches.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c       | 10 ++++++++++
>  include/kvm/arm_vgic.h                |  3 +++
>  include/linux/irqchip/arm-vgic-info.h |  2 ++
>  3 files changed, 15 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 9fd23f32aa54..5b06a9970a57 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -524,6 +524,16 @@ int kvm_vgic_hyp_init(void)
>  	if (!gic_kvm_info)
>  		return -ENODEV;
>  
> +	/*
> +	 * If we get one of these oddball non-GICs, taint the kernel,
> +	 * as we have no idea of how they *really* behave.
> +	 */
> +	if (gic_kvm_info->no_hw_deactivation) {
> +		kvm_info("Non-architectural vgic, tainting kernel\n");
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> +		kvm_vgic_global_state.no_hw_deactivation = true;
> +	}

IMO, since this means we're going to rely even more on the maintenance interrupt
(not just for software emulation of level sensitive interrupts), I think there
should be some sort of dependency on having something that resembles a working
maintenance interrupt.

Thanks,

Alex

> +
>  	switch (gic_kvm_info->type) {
>  	case GIC_V2:
>  		ret = vgic_v2_probe(gic_kvm_info);
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index ec621180ef09..e45b26e8d479 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -72,6 +72,9 @@ struct vgic_global {
>  	bool			has_gicv4;
>  	bool			has_gicv4_1;
>  
> +	/* Pseudo GICv3 from outer space */
> +	bool			no_hw_deactivation;
> +
>  	/* GIC system register CPU interface */
>  	struct static_key_false gicv3_cpuif;
>  
> diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h
> index 0319636be928..d39d0b591c5a 100644
> --- a/include/linux/irqchip/arm-vgic-info.h
> +++ b/include/linux/irqchip/arm-vgic-info.h
> @@ -30,6 +30,8 @@ struct gic_kvm_info {
>  	bool		has_v4;
>  	/* rvpeid support */
>  	bool		has_v4_1;
> +	/* Deactivation impared, subpar stuff */
> +	bool		no_hw_deactivation;
>  };
>  
>  #ifdef CONFIG_KVM

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation
  2021-05-10 13:48 ` [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation Marc Zyngier
@ 2021-05-24 16:53   ` Alexandru Elisei
  2021-05-24 17:43     ` Marc Zyngier
  0 siblings, 1 reply; 23+ messages in thread
From: Alexandru Elisei @ 2021-05-24 16:53 UTC (permalink / raw)
  To: Marc Zyngier, linux-arm-kernel, kvm, kvmarm
  Cc: James Morse, Suzuki K Poulose, Eric Auger, Hector Martin,
	Mark Rutland, kernel-team

Hi Marc,

Some questions regarding how this is supposed to work.

On 5/10/21 2:48 PM, Marc Zyngier wrote:
> In order to deal with these systems that do not offer HW-based
> deactivation of interrupts, let implement a SW-based approach:
>
> - When the irq is queued into a LR, treat it as a pure virtual
>   interrupt and set the EOI flag in the LR.
>
> - When the interrupt state is read back from the LR, force a
>   deactivation when the state is invalid (neither active nor
>   pending)
>
> Interrupts requiring such treatment get the VGIC_SW_RESAMPLE flag.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-v2.c | 19 +++++++++++++++----
>  arch/arm64/kvm/vgic/vgic-v3.c | 19 +++++++++++++++----
>  include/kvm/arm_vgic.h        | 10 ++++++++++
>  3 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index 11934c2af2f4..2c580204f1dc 100644
> --- a/arch/arm64/kvm/vgic/vgic-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> @@ -108,11 +108,22 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
>  		 * If this causes us to lower the level, we have to also clear
>  		 * the physical active state, since we will otherwise never be
>  		 * told when the interrupt becomes asserted again.
> +		 *
> +		 * Another case is when the interrupt requires a helping hand
> +		 * on deactivation (no HW deactivation, for example).
>  		 */
> -		if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
> -			irq->line_level = vgic_get_phys_line_level(irq);
> +		if (vgic_irq_is_mapped_level(irq)) {
> +			bool resample = false;
> +
> +			if (val & GICH_LR_PENDING_BIT) {
> +				irq->line_level = vgic_get_phys_line_level(irq);
> +				resample = !irq->line_level;
> +			} else if (vgic_irq_needs_resampling(irq) &&
> +				   !(irq->active || irq->pending_latch)) {

So this means that if the IRQ has the special flag, if it's not pending in the LR
or at the software level, and it's not active either, then perform interrupt
deactivation. I don't see where the state of the interrupt is checked again, am I
correct in assuming that we rely on the CPU interface to assert the interrupt to
the host while we run with interrupts enabled in the run loop, and the handler for
the interrupt will mark it pending for kvm_vgic_sync_hw_state->vgic_vx_fold_lr_state?

> +				resample = true;
> +			}
>  
> -			if (!irq->line_level)
> +			if (resample)

This name, "resample", is confusing to me, quite possibly because I'm not familiar
with the irqchip subsystem. It was my impression that "resample" means that at
some point, the physical interrupt state will be checked again, yet I don't see
that happening anywhere when VGIC_IRQ_SW_RESAMPLE is set. Am I mistaken in my
assumptions?

Thanks,

Alex

>  				vgic_irq_set_phys_active(irq, false);
>  		}
>  
> @@ -152,7 +163,7 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (irq->group)
>  		val |= GICH_LR_GROUP1;
>  
> -	if (irq->hw) {
> +	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
>  		val |= GICH_LR_HW;
>  		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
>  		/*
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 41ecf219c333..66004f61cd83 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -101,11 +101,22 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
>  		 * If this causes us to lower the level, we have to also clear
>  		 * the physical active state, since we will otherwise never be
>  		 * told when the interrupt becomes asserted again.
> +		 *
> +		 * Another case is when the interrupt requires a helping hand
> +		 * on deactivation (no HW deactivation, for example).
>  		 */
> -		if (vgic_irq_is_mapped_level(irq) && (val & ICH_LR_PENDING_BIT)) {
> -			irq->line_level = vgic_get_phys_line_level(irq);
> +		if (vgic_irq_is_mapped_level(irq)) {
> +			bool resample = false;
> +
> +			if (val & ICH_LR_PENDING_BIT) {
> +				irq->line_level = vgic_get_phys_line_level(irq);
> +				resample = !irq->line_level;
> +			} else if (vgic_irq_needs_resampling(irq) &&
> +				   !(irq->active || irq->pending_latch)) {
> +				resample = true;
> +			}
>  
> -			if (!irq->line_level)
> +			if (resample)
>  				vgic_irq_set_phys_active(irq, false);
>  		}
>  
> @@ -136,7 +147,7 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  		}
>  	}
>  
> -	if (irq->hw) {
> +	if (irq->hw && !vgic_irq_needs_resampling(irq)) {
>  		val |= ICH_LR_HW;
>  		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
>  		/*
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index e5f06df000f2..e602d848fc1a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -99,6 +99,11 @@ enum vgic_irq_config {
>   * kvm_arm_get_running_vcpu() to get the vcpu pointer for private IRQs.
>   */
>  struct irq_ops {
> +	/* Per interrupt flags for special-cased interrupts */
> +	unsigned long flags;
> +
> +#define VGIC_IRQ_SW_RESAMPLE	BIT(0)	/* Clear the active state for resampling */
> +
>  	/*
>  	 * Callback function pointer to in-kernel devices that can tell us the
>  	 * state of the input level of mapped level-triggered IRQ faster than
> @@ -150,6 +155,11 @@ struct vgic_irq {
>  					   for in-kernel devices. */
>  };
>  
> +static inline bool vgic_irq_needs_resampling(struct vgic_irq *irq)
> +{
> +	return irq->ops && (irq->ops->flags & VGIC_IRQ_SW_RESAMPLE);
> +}
> +
>  struct vgic_register_region;
>  struct vgic_its;
>  

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation
  2021-05-21 17:01   ` Alexandru Elisei
@ 2021-05-24 17:17     ` Marc Zyngier
  0 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-24 17:17 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: linux-arm-kernel, kvm, kvmarm, James Morse, Suzuki K Poulose,
	Eric Auger, Hector Martin, Mark Rutland, kernel-team

Hi Alex,

On Fri, 21 May 2021 18:01:05 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 5/10/21 2:48 PM, Marc Zyngier wrote:
> > The vGIC, as architected by ARM, allows a virtual interrupt to
> > trigger the deactivation of a physical interrupt. This allows
> > the following interrupt to be delivered without requiring an exit.
> 
> If I got this right, the AIC doesn't implement/ignores the

s/AIC/M1 CPU/

> ICH_LR_EL2.HW bit. Does it mean that the CPU IF behaves as if HW =
> 0b0, meaning it asserts a maintenance interrupt on virtual interrupt
> deactivation when ICH_LR_EL2.EOI = 0b1? I assume that's the case,
> just double checking.

Yes, that's what it looks like.

> I am wondering what would happen if we come across an interrupt
> controller where the CPU IF cannot assert a maintenance interrupt at
> all and we rely on the EOI bit to take us out of the guest to
> deactivate the HW interrupt.

That'd be broken, and we wouldn't be able to support such an
implementation, at least not in configuration such as CPU isolation.

> I have to say that it looks a bit strange to start relying on the
> maintenance interrupt to emulate interrupt deactivate for hardware
> interrupts, but at the same timer allowing an interrupt controller
> without a maintenance interrupt.

We are not allowing a vGIC without a maintenance interrupt. We are
actively mandating it. The M1 does have a working per-CPU maintenance
interrupt. It just isn't wired into an interrupt controller, which
means we can't mask it. But it is otherwise perfectly functional.

> Other than that, this idea sounds like the best thing to do
> considering the circumstances, I certainly can't think of anything
> better.
> 
> >
> > However, some implementations have choosen not to implement this,
> > meaning that we will need some unsavoury workarounds to deal with this.
> >
> > On detecting such a case, taint the kernel and spit a nastygram.
> > We'll deal with this in later patches.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/vgic/vgic-init.c       | 10 ++++++++++
> >  include/kvm/arm_vgic.h                |  3 +++
> >  include/linux/irqchip/arm-vgic-info.h |  2 ++
> >  3 files changed, 15 insertions(+)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> > index 9fd23f32aa54..5b06a9970a57 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -524,6 +524,16 @@ int kvm_vgic_hyp_init(void)
> >  	if (!gic_kvm_info)
> >  		return -ENODEV;
> >  
> > +	/*
> > +	 * If we get one of these oddball non-GICs, taint the kernel,
> > +	 * as we have no idea of how they *really* behave.
> > +	 */
> > +	if (gic_kvm_info->no_hw_deactivation) {
> > +		kvm_info("Non-architectural vgic, tainting kernel\n");
> > +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> > +		kvm_vgic_global_state.no_hw_deactivation = true;
> > +	}
> 
> IMO, since this means we're going to rely even more on the
> maintenance interrupt (not just for software emulation of level
> sensitive interrupts), I think there should be some sort of
> dependency on having something that resembles a working maintenance
> interrupt.

But the timer interrupt is exactly a SW emulation of a level sensitive
interrupt in this context. And the maintenance interrupt is still
required to use the vGIC.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation
  2021-05-24 16:53   ` Alexandru Elisei
@ 2021-05-24 17:43     ` Marc Zyngier
  0 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-24 17:43 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: linux-arm-kernel, kvm, kvmarm, James Morse, Suzuki K Poulose,
	Eric Auger, Hector Martin, Mark Rutland, kernel-team

On Mon, 24 May 2021 17:53:04 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> Some questions regarding how this is supposed to work.
> 
> On 5/10/21 2:48 PM, Marc Zyngier wrote:
> > In order to deal with these systems that do not offer HW-based
> > deactivation of interrupts, let implement a SW-based approach:
> >
> > - When the irq is queued into a LR, treat it as a pure virtual
> >   interrupt and set the EOI flag in the LR.
> >
> > - When the interrupt state is read back from the LR, force a
> >   deactivation when the state is invalid (neither active nor
> >   pending)
> >
> > Interrupts requiring such treatment get the VGIC_SW_RESAMPLE flag.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/vgic/vgic-v2.c | 19 +++++++++++++++----
> >  arch/arm64/kvm/vgic/vgic-v3.c | 19 +++++++++++++++----
> >  include/kvm/arm_vgic.h        | 10 ++++++++++
> >  3 files changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> > index 11934c2af2f4..2c580204f1dc 100644
> > --- a/arch/arm64/kvm/vgic/vgic-v2.c
> > +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> > @@ -108,11 +108,22 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
> >  		 * If this causes us to lower the level, we have to also clear
> >  		 * the physical active state, since we will otherwise never be
> >  		 * told when the interrupt becomes asserted again.
> > +		 *
> > +		 * Another case is when the interrupt requires a helping hand
> > +		 * on deactivation (no HW deactivation, for example).
> >  		 */
> > -		if (vgic_irq_is_mapped_level(irq) && (val & GICH_LR_PENDING_BIT)) {
> > -			irq->line_level = vgic_get_phys_line_level(irq);
> > +		if (vgic_irq_is_mapped_level(irq)) {
> > +			bool resample = false;
> > +
> > +			if (val & GICH_LR_PENDING_BIT) {
> > +				irq->line_level = vgic_get_phys_line_level(irq);
> > +				resample = !irq->line_level;
> > +			} else if (vgic_irq_needs_resampling(irq) &&
> > +				   !(irq->active || irq->pending_latch)) {
> 
> So this means that if the IRQ has the special flag, if it's not
> pending in the LR or at the software level, and it's not active
> either, then perform interrupt deactivation.

Correct.

> I don't see where the state of the interrupt is checked again, am I
> correct in assuming that we rely on the CPU interface to assert the
> interrupt to the host while we run with interrupts enabled in the
> run loop, and the handler for the interrupt will mark it pending for
> kvm_vgic_sync_hw_state->vgic_vx_fold_lr_state?

See the vgic_get_phys_line_level() call. This is all about dealing
with an interrupt that was made pending in the LR, that the guest
didn't Ack, but instead decided to disable the timer.

In this case, we need to clear the pending bit and deactivate the
interrupt because nothing will perform the physical deactivation for
us.

What we add in the M1 case is that if the interrupt isn't pending
anymore at the virtual level, we also need to deactivate it at the
physical level, because there is no HW mechanism to enforce it.

> 
> > +				resample = true;
> > +			}
> >  
> > -			if (!irq->line_level)
> > +			if (resample)
> 
> This name, "resample", is confusing to me, quite possibly because
> I'm not familiar with the irqchip subsystem. It was my impression
> that "resample" means that at some point, the physical interrupt
> state will be checked again, yet I don't see that happening anywhere
> when VGIC_IRQ_SW_RESAMPLE is set. Am I mistaken in my assumptions?

The resample is at the HW level. We forcefully tell the interrupt
controller to deliver a pending interrupt (this is implemented as an
unmask under the hood).

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration
  2021-05-14 12:46   ` Zenghui Yu
@ 2021-05-24 17:48     ` Marc Zyngier
  0 siblings, 0 replies; 23+ messages in thread
From: Marc Zyngier @ 2021-05-24 17:48 UTC (permalink / raw)
  To: Zenghui Yu; +Cc: linux-arm-kernel, kvm, kvmarm, kernel-team, Hector Martin

On 2021-05-14 13:46, Zenghui Yu wrote:
> On 2021/5/10 21:48, Marc Zyngier wrote:
>> As we are about to add some more things to the timer IRQ
>> configuration, move this code out of the main timer init code
>> into its own set of functions.
>> 
>> No functional changes.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/arch_timer.c | 61 
>> ++++++++++++++++++++++---------------
>>  1 file changed, 37 insertions(+), 24 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>> index e2288b6bf435..7fa4f446456a 100644
>> --- a/arch/arm64/kvm/arch_timer.c
>> +++ b/arch/arm64/kvm/arch_timer.c
>> @@ -973,6 +973,39 @@ static int kvm_timer_dying_cpu(unsigned int cpu)
>>  	return 0;
>>  }
>>  +static void kvm_irq_fixup_flags(unsigned int virq, u32 *flags)
>> +{
>> +	*flags = irq_get_trigger_type(virq);
>> +	if (*flags != IRQF_TRIGGER_HIGH && *flags != IRQF_TRIGGER_LOW) {
>> +		kvm_err("Invalid trigger for timer IRQ%d, assuming level low\n",
>> +			virq);
>> +		*flags = IRQF_TRIGGER_LOW;
>> +	}
>> +}
>> +
>> +static int kvm_irq_init(struct arch_timer_kvm_info *info)
>> +{
>> +	struct irq_domain *domain = NULL;
>> +	struct fwnode_handle *fwnode;
>> +	struct irq_data *data;
> 
> Shouldn't this belong to patch #8?

Yup. Now moved.

Thanks,

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2021-05-24 17:48 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10 13:48 [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 1/9] irqchip/gic: Split vGIC probing information from the GIC code Marc Zyngier
2021-05-18 16:51   ` Alexandru Elisei
2021-05-10 13:48 ` [PATCH v3 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest Marc Zyngier
2021-05-20 17:46   ` Alexandru Elisei
2021-05-10 13:48 ` [PATCH v3 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt Marc Zyngier
2021-05-10 16:19   ` Mark Rutland
2021-05-10 17:44     ` Marc Zyngier
2021-05-11 11:13       ` Mark Rutland
2021-05-10 13:48 ` [PATCH v3 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation Marc Zyngier
2021-05-21 17:01   ` Alexandru Elisei
2021-05-24 17:17     ` Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 6/9] KVM: arm64: vgic: Implement SW-driven deactivation Marc Zyngier
2021-05-24 16:53   ` Alexandru Elisei
2021-05-24 17:43     ` Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 7/9] KVM: arm64: timer: Refactor IRQ configuration Marc Zyngier
2021-05-14 12:46   ` Zenghui Yu
2021-05-24 17:48     ` Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 8/9] KVM: arm64: timer: Add support for SW-based deactivation Marc Zyngier
2021-05-10 13:48 ` [PATCH v3 9/9] irqchip/apple-aic: Advertise some level of vGICv3 compatibility Marc Zyngier
2021-05-12 16:22 ` [PATCH v3 0/9] KVM: arm64: Initial host support for the Apple M1 Alexandru Elisei
2021-05-12 16:33   ` Marc Zyngier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).