[PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Marc Zyngier]

This is a new version of the series previously posted at [3], 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.

Until someone shouts loudly now, I'll take this into 5.14 (and in
-next from tomorrow).

* From v3 [3]:
  - Rebased on 5.13-rc4 to match the kvmarm/next base
  - Moved stuff from patch #7 to its logical spot in patch #8
  - Changed the include/linux/irqchip/arm-vgic-info.h guard
  - Collected RBs from Alex, with thanks

* From v2 [2]:
  - 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
[3] https://lore.kernel.org/r/20210510134824.1910399-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
    masking
  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            | 162 +++++++++++++++++++++----
 arch/arm64/kvm/hyp/hyp-entry.S         |   6 +-
 arch/arm64/kvm/vgic/vgic-init.c        |  36 +++++-
 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        |   9 ++
 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  |  45 +++++++
 14 files changed, 299 insertions(+), 104 deletions(-)
 create mode 100644 include/linux/irqchip/arm-vgic-info.h

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/9] irqchip/gic: Split vGIC probing information from the GIC code

[Marc Zyngier]

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.

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
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..a25d4da5697d
--- /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 __LINUX_IRQCHIP_ARM_VGIC_INFO_H
+#define __LINUX_IRQCHIP_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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/9] KVM: arm64: Handle physical FIQ as an IRQ while running a guest

[Marc Zyngier]

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.

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt masking

[Marc Zyngier]

As it turns out, not all the interrupt controllers are able to
expose a vGIC maintenance interrupt that can be independently
enabled/disabled.

And to be fair, it doesn't really matter as all we require is
for the interrupt to kick us out of guest mode out way or another.

To that effect, add gic_kvm_info.no_maint_irq_mask for an interrupt
controller to advertise the lack of masking.

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

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 2fdb65529594..6752d084934d 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -519,12 +519,15 @@ void kvm_vgic_init_cpu_hardware(void)
  */
 int kvm_vgic_hyp_init(void)
 {
+	bool has_mask;
 	int ret;
 
 	if (!gic_kvm_info)
 		return -ENODEV;
 
-	if (!gic_kvm_info->maint_irq) {
+	has_mask = !gic_kvm_info->no_maint_irq_mask;
+
+	if (has_mask && !gic_kvm_info->maint_irq) {
 		kvm_err("No vgic maintenance irq\n");
 		return -ENXIO;
 	}
@@ -552,6 +555,9 @@ int kvm_vgic_hyp_init(void)
 	if (ret)
 		return ret;
 
+	if (!has_mask)
+		return 0;
+
 	ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
 				 vgic_maintenance_handler,
 				 "vgic", kvm_get_running_vcpus());
diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h
index a25d4da5697d..7c0d08ebb82c 100644
--- a/include/linux/irqchip/arm-vgic-info.h
+++ b/include/linux/irqchip/arm-vgic-info.h
@@ -24,6 +24,8 @@ struct gic_kvm_info {
 	struct resource vcpu;
 	/* Interrupt number */
 	unsigned int	maint_irq;
+	/* No interrupt mask, no need to use the above field */
+	bool		no_maint_irq_mask;
 	/* Virtual control interface */
 	struct resource vctrl;
 	/* vlpi support */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt masking

[Alexandru Elisei]

Hi Marc,

On 6/1/21 11:39 AM, Marc Zyngier wrote:
> As it turns out, not all the interrupt controllers are able to
> expose a vGIC maintenance interrupt that can be independently
> enabled/disabled.
>
> And to be fair, it doesn't really matter as all we require is
> for the interrupt to kick us out of guest mode out way or another.
>
> To that effect, add gic_kvm_info.no_maint_irq_mask for an interrupt
> controller to advertise the lack of masking.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c       | 8 +++++++-
>  include/linux/irqchip/arm-vgic-info.h | 2 ++
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 2fdb65529594..6752d084934d 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -519,12 +519,15 @@ void kvm_vgic_init_cpu_hardware(void)
>   */
>  int kvm_vgic_hyp_init(void)
>  {
> +	bool has_mask;
>  	int ret;
>  
>  	if (!gic_kvm_info)
>  		return -ENODEV;
>  
> -	if (!gic_kvm_info->maint_irq) {
> +	has_mask = !gic_kvm_info->no_maint_irq_mask;

This double negative is pretty awkward, I suppose this was done to avoid changes
to the gic drivers, because the default value is 0 (false). Just an idea, maybe
renaming it to maint_irq_unmaskable would be more readable?

Other than that, the patch looks good:

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

Thanks,

Alex

> +
> +	if (has_mask && !gic_kvm_info->maint_irq) {
>  		kvm_err("No vgic maintenance irq\n");
>  		return -ENXIO;
>  	}
> @@ -552,6 +555,9 @@ int kvm_vgic_hyp_init(void)
>  	if (ret)
>  		return ret;
>  
> +	if (!has_mask)
> +		return 0;
> +
>  	ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>  				 vgic_maintenance_handler,
>  				 "vgic", kvm_get_running_vcpus());
> diff --git a/include/linux/irqchip/arm-vgic-info.h b/include/linux/irqchip/arm-vgic-info.h
> index a25d4da5697d..7c0d08ebb82c 100644
> --- a/include/linux/irqchip/arm-vgic-info.h
> +++ b/include/linux/irqchip/arm-vgic-info.h
> @@ -24,6 +24,8 @@ struct gic_kvm_info {
>  	struct resource vcpu;
>  	/* Interrupt number */
>  	unsigned int	maint_irq;
> +	/* No interrupt mask, no need to use the above field */
> +	bool		no_maint_irq_mask;
>  	/* Virtual control interface */
>  	struct resource vctrl;
>  	/* vlpi support */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/9] KVM: arm64: vgic: Be tolerant to the lack of maintenance interrupt masking

[Alexandru Elisei]

Hi Marc,

On 6/11/21 5:38 PM, Alexandru Elisei wrote:
> Hi Marc,
>
> On 6/1/21 11:39 AM, Marc Zyngier wrote:
>> As it turns out, not all the interrupt controllers are able to
>> expose a vGIC maintenance interrupt that can be independently
>> enabled/disabled.
>>
>> And to be fair, it doesn't really matter as all we require is
>> for the interrupt to kick us out of guest mode out way or another.
>>
>> To that effect, add gic_kvm_info.no_maint_irq_mask for an interrupt
>> controller to advertise the lack of masking.
>>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/vgic/vgic-init.c       | 8 +++++++-
>>  include/linux/irqchip/arm-vgic-info.h | 2 ++
>>  2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
>> index 2fdb65529594..6752d084934d 100644
>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>> @@ -519,12 +519,15 @@ void kvm_vgic_init_cpu_hardware(void)
>>   */
>>  int kvm_vgic_hyp_init(void)
>>  {
>> +	bool has_mask;
>>  	int ret;
>>  
>>  	if (!gic_kvm_info)
>>  		return -ENODEV;
>>  
>> -	if (!gic_kvm_info->maint_irq) {
>> +	has_mask = !gic_kvm_info->no_maint_irq_mask;
> This double negative is pretty awkward, I suppose this was done to avoid changes
> to the gic drivers, because the default value is 0 (false). Just an idea, maybe
> renaming it to maint_irq_unmaskable would be more readable?

Actually, after another look, the current name stopped looking awkward to me.

Thanks,

Alex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation

[Marc Zyngier]

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 6752d084934d..340c51d87677 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -532,6 +532,16 @@ int kvm_vgic_hyp_init(void)
 		return -ENXIO;
 	}
 
+	/*
+	 * 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 7c0d08ebb82c..a75b2c7de69d 100644
--- a/include/linux/irqchip/arm-vgic-info.h
+++ b/include/linux/irqchip/arm-vgic-info.h
@@ -32,6 +32,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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation

[Alexandru Elisei]

Hi Marc,

On 6/1/21 11:40 AM, 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.
>
> 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 6752d084934d..340c51d87677 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -532,6 +532,16 @@ int kvm_vgic_hyp_init(void)
>  		return -ENXIO;
>  	}
>  
> +	/*
> +	 * 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);

I'm trying to figure out what are the effects of tainting the kernel, besides
those nasty messages. In Documentation/admin-guide/tainted-kernels.rst, I found
this bit:

[..] the information is mainly of interest once someone wants to investigate some
problem, as its real cause might be the event that got the kernel tainted. That's
why bug reports from tainted kernels will often be ignored by developers, hence
try to reproduce problems with an untainted kernel.

The lack of HW deactivation affects only KVM, I was wondering if we could taint
the kernel the first time a VM created. If the above doc is to go by, someone who
is running Linux on an M1, but not using KVM, might stand a better chance to get
support when something goes wrong in that case.

What do you think?

Thanks,

Alex

> +		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 7c0d08ebb82c..a75b2c7de69d 100644
> --- a/include/linux/irqchip/arm-vgic-info.h
> +++ b/include/linux/irqchip/arm-vgic-info.h
> @@ -32,6 +32,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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/9] KVM: arm64: vgic: Let an interrupt controller advertise lack of HW deactivation

[Marc Zyngier]

On Tue, 15 Jun 2021 15:26:02 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 6/1/21 11:40 AM, 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.
> >
> > 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 6752d084934d..340c51d87677 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -532,6 +532,16 @@ int kvm_vgic_hyp_init(void)
> >  		return -ENXIO;
> >  	}
> >  
> > +	/*
> > +	 * 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);
> 
> I'm trying to figure out what are the effects of tainting the
> kernel, besides those nasty messages. In
> Documentation/admin-guide/tainted-kernels.rst, I found this bit:
> 
> [..] the information is mainly of interest once someone wants to
> investigate some problem, as its real cause might be the event that
> got the kernel tainted. That's why bug reports from tainted kernels
> will often be ignored by developers, hence try to reproduce problems
> with an untainted kernel.
> 
> The lack of HW deactivation affects only KVM, I was wondering if we
> could taint the kernel the first time a VM created. If the above doc
> is to go by, someone who is running Linux on an M1, but not using
> KVM, might stand a better chance to get support when something goes
> wrong in that case.

Unfortunately, by the time we're here, we have already committed to
using stuff that isn't architectural.

For example, this CPU doesn't advertise a virtual GICv3 CPU interface
(because it isn't possible to do so independently of the full-fat
one). And right from the beginning, before any VM is present, we are
going to access ICH_VTR_EL2, because we really need it as part of
initialising KVM.

> What do you think?

I think that if people are bothered by this tainting, they can disable
KVM altogether. And to be fair, we should taint the kernel right when
the first CPU boots, because it isn't implementing the ARM
architecture as defined by the spec.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure

[Marc Zyngier]

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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure

[Alexandru Elisei]

Hi Marc,

On 6/1/21 11:40 AM, Marc Zyngier wrote:
> 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.

I can't figure out what you mean by that. If you are referring to struct vgic_irq,
the change I am seeing is a pointer being replaced by another pointer, which
shouldn't affect its size. Are you referring to something else?

>
> 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,

Since kvm_arch_timer_get_input_level() is used only indirectly, through the
get_input_level field of the static struct, I think we can make
kvm_arch_timer_get_input_level() static and remove the declaration from
include/kvm/arm_arch_timer.h.

Other than that, everything else looks correct.

Thanks,

Alex

> +};
> +
>  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);
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 5/9] KVM: arm64: vgic: move irq->get_input_level into an ops structure

[Marc Zyngier]

On Tue, 15 Jun 2021 15:45:03 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 6/1/21 11:40 AM, Marc Zyngier wrote:
> > 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.
> 
> I can't figure out what you mean by that. If you are referring to
> struct vgic_irq, the change I am seeing is a pointer being replaced
> by another pointer, which shouldn't affect its size. Are you
> referring to something else?

Eventually, we have more than just a pointer (we also get flags in the
same structure), so this saves some space.

> 
> >
> > 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,
> 
> Since kvm_arch_timer_get_input_level() is used only indirectly, through the
> get_input_level field of the static struct, I think we can make
> kvm_arch_timer_get_input_level() static and remove the declaration from
> include/kvm/arm_arch_timer.h.

Ah, good point. I'll stash a patch on top of the existing series (I'm
trying not to change what is currently queued in -next).

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 6/9] KVM: arm64: vgic: Implement SW-driven deactivation

[Marc Zyngier]

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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/9] KVM: arm64: vgic: Implement SW-driven deactivation

[Alexandru Elisei]

Hi Marc,

On 6/1/21 11:40 AM, 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:

Nitpick, but shouldn't that be "let's"?

>
> - 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)) {

I'm having a hard time figuring out when and why a level sensitive can have
pending_latch = true.

I looked kvm_vgic_inject_irq(), and that function sets pending_latch only for edge
triggered interrupts (it sets line_level for level sensitive ones). But
irq_is_pending() looks at **both** pending_latch and line_level for level
sensitive interrupts.

The only place that I've found that sets pending_latch regardless of the interrupt
type is in vgic_mmio_write_spending() (called on a trapped write to
GICD_ISENABLER). vgic_v2_populate_lr() clears pending_latch only for edge
triggered interrupts, so that leaves vgic_v2_fold_lr_state() as the only function
pending_latch is cleared for level sensitive interrupts, when the interrupt has
been handled by the guest. Are we doing all of this to emulate the fact that level
sensitive interrupts (either purely virtual or hw mapped) made pending by a write
to GICD_ISENABLER remain pending until they are handled by the guest?

If that is the case, then I think this is what the code is doing:

- There's no functional change when the irqchip has HW deactivation

- For level sensitive, hw mapped interrupts made pending by a write to
GICD_ISENABLER and not yet handled by the guest (pending_latch == true) we don't
clear the pending state of the interrupt.

- For level sensitive, hw mapped interrupts we clear the pending state in the GIC
and the device will assert the interrupt again if it's still pending at the device
level. I have a question about this. Why don't we sample the interrupt state by
calling vgic_get_phys_line_level()? Because that would be slower than the
alternative that you are proposing here?

> +				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)) {

This looks good, we set the EOI bit in the LR register in the case of purely
virtual level sensitive interrupts or for HW mapped level sensitive on systems
where the GIC doesn't have the mandatory HW deactivation architectural feature.

>  		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)) {

Both changes to the vGICv3 code look identical to the vGICv2 changes.

Thanks,

Alex

>  		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;
>  

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/9] KVM: arm64: vgic: Implement SW-driven deactivation

[Marc Zyngier]

On Thu, 17 Jun 2021 15:58:31 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 6/1/21 11:40 AM, 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:
> 
> Nitpick, but shouldn't that be "let's"?

"Let it be...". ;-) Yup.

> 
> >
> > - 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)) {
> 
> I'm having a hard time figuring out when and why a level sensitive
> can have pending_latch = true.
> 
> I looked kvm_vgic_inject_irq(), and that function sets pending_latch
> only for edge triggered interrupts (it sets line_level for level
> sensitive ones). But irq_is_pending() looks at **both**
> pending_latch and line_level for level sensitive interrupts.

Yes, and that's what an implementation requires.

> The only place that I've found that sets pending_latch regardless of
> the  interrupt type  is in  vgic_mmio_write_spending() (called  on a
> trapped  write  to   GICD_ISENABLER).

Are you sure? It really should be GICD_ISPENDR. I'll assume that this
is what you mean below.

> vgic_v2_populate_lr()  clears
> pending_latch  only for  edge triggered  interrupts, so  that leaves
> vgic_v2_fold_lr_state()  as  the   only  function  pending_latch  is
> cleared for level sensitive interrupts,  when the interrupt has been
> handled by the guest.  Are we doing all of this  to emulate the fact
> that level sensitive interrupts (either purely virtual or hw mapped)
> made pending by a write  to GICD_ISENABLER remain pending until they
> are handled by the guest?

Yes, or cleared by a write to GICD_ICPENDR. You really need to think
of the input into the GIC as some sort of OR gate combining both the
line level and the PEND register. With a latch for edge interrupts.

Have a look at Figure 4-10 ("Logic of the pending status of a
level-sensitive interrupt") in the GICv2 arch spec (ARM IHI 0048B.b)
to see what I actually mean.

> If that is the case, then I think this is what the code is doing:
> 
> - There's no functional change when the irqchip has HW deactivation
> 
> - For level sensitive, hw mapped interrupts made pending by a write
> to GICD_ISENABLER and not yet handled by the guest (pending_latch ==
> true) we don't clear the pending state of the interrupt.
> 
> - For level sensitive, hw mapped interrupts we clear the pending
> state in the GIC and the device will assert the interrupt again if
> it's still pending at the device 1level. I have a question about
> this. Why don't we sample the interrupt state by calling
> vgic_get_phys_line_level()? Because that would be slower than the
> alternative that you are proposing here?

Yes. It is *much* faster to read the timer status register (for
example) than going via an MMIO access to read the (re)distributor
that will return the same value.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/9] KVM: arm64: vgic: Implement SW-driven deactivation

[Alexandru Elisei]

Hi Marc,

On 6/22/21 5:12 PM, Marc Zyngier wrote:
> On Thu, 17 Jun 2021 15:58:31 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> Hi Marc,
>>
>> On 6/1/21 11:40 AM, 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:
>> Nitpick, but shouldn't that be "let's"?
> "Let it be...". ;-) Yup.
>
>>> - 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)) {
>> I'm having a hard time figuring out when and why a level sensitive
>> can have pending_latch = true.
>>
>> I looked kvm_vgic_inject_irq(), and that function sets pending_latch
>> only for edge triggered interrupts (it sets line_level for level
>> sensitive ones). But irq_is_pending() looks at **both**
>> pending_latch and line_level for level sensitive interrupts.
> Yes, and that's what an implementation requires.
>
>> The only place that I've found that sets pending_latch regardless of
>> the  interrupt type  is in  vgic_mmio_write_spending() (called  on a
>> trapped  write  to   GICD_ISENABLER).
> Are you sure? It really should be GICD_ISPENDR. I'll assume that this
> is what you mean below.

Yes, that's what I meant, sorry for the confusion.

>
>> vgic_v2_populate_lr()  clears
>> pending_latch  only for  edge triggered  interrupts, so  that leaves
>> vgic_v2_fold_lr_state()  as  the   only  function  pending_latch  is
>> cleared for level sensitive interrupts,  when the interrupt has been
>> handled by the guest.  Are we doing all of this  to emulate the fact
>> that level sensitive interrupts (either purely virtual or hw mapped)
>> made pending by a write  to GICD_ISENABLER remain pending until they
>> are handled by the guest?
> Yes, or cleared by a write to GICD_ICPENDR. You really need to think
> of the input into the GIC as some sort of OR gate combining both the
> line level and the PEND register. With a latch for edge interrupts.
>
> Have a look at Figure 4-10 ("Logic of the pending status of a
> level-sensitive interrupt") in the GICv2 arch spec (ARM IHI 0048B.b)
> to see what I actually mean.
>
>> If that is the case, then I think this is what the code is doing:
>>
>> - There's no functional change when the irqchip has HW deactivation
>>
>> - For level sensitive, hw mapped interrupts made pending by a write
>> to GICD_ISENABLER and not yet handled by the guest (pending_latch ==
>> true) we don't clear the pending state of the interrupt.
>>
>> - For level sensitive, hw mapped interrupts we clear the pending
>> state in the GIC and the device will assert the interrupt again if
>> it's still pending at the device 1level. I have a question about
>> this. Why don't we sample the interrupt state by calling
>> vgic_get_phys_line_level()? Because that would be slower than the
>> alternative that you are proposing here?
> Yes. It is *much* faster to read the timer status register (for
> example) than going via an MMIO access to read the (re)distributor
> that will return the same value.

Thank you for the explanation, much appreciated. The patch looks to me like it's
doing the right thing.

Thanks,

Alex


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 7/9] KVM: arm64: timer: Refactor IRQ configuration

[Marc Zyngier]

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 | 57 +++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index e2288b6bf435..3cd170388d88 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -973,6 +973,35 @@ 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)
+{
+	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 +1015,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 +1045,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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 7/9] KVM: arm64: timer: Refactor IRQ configuration

[Alexandru Elisei]

Hi Marc,

On 6/1/21 11:40 AM, 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.

That looks to be the case for me:

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

Thanks,

Alex

>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/arch_timer.c | 57 +++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index e2288b6bf435..3cd170388d88 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -973,6 +973,35 @@ 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)
> +{
> +	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 +1015,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 +1045,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) {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 8/9] KVM: arm64: timer: Add support for SW-based deactivation

[Marc Zyngier]

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 | 105 ++++++++++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 3cd170388d88..3df67c127489 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);
@@ -985,6 +1057,8 @@ static void kvm_irq_fixup_flags(unsigned int virq, u32 *flags)
 
 static int kvm_irq_init(struct arch_timer_kvm_info *info)
 {
+	struct irq_domain *domain = NULL;
+
 	if (info->virtual_irq <= 0) {
 		kvm_err("kvm_arch_timer: invalid virtual timer IRQ: %d\n",
 			info->virtual_irq);
@@ -994,9 +1068,36 @@ 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) {
+		struct fwnode_handle *fwnode;
+		struct irq_data *data;
+
+		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;
@@ -1125,10 +1226,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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 9/9] irqchip/apple-aic: Advertise some level of vGICv3 compatibility

[Marc Zyngier]

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

Advertise the support to KVM.

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

diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
index c179e27062fd..b8c06bd8659e 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,12 @@ static int aic_init_cpu(unsigned int cpu)
 	return 0;
 }
 
+static struct gic_kvm_info vgic_info __initdata = {
+	.type			= GIC_V3,
+	.no_maint_irq_mask	= true,
+	.no_hw_deactivation	= true,
+};
+
 static int __init aic_of_ic_init(struct device_node *node, struct device_node *parent)
 {
 	int i;
@@ -843,6 +850,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.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Alexandru Elisei]

Hi Marc,

On 6/1/21 11:39 AM, Marc Zyngier wrote:
> This is a new version of the series previously posted at [3], 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.
>
> Until someone shouts loudly now, I'll take this into 5.14 (and in
> -next from tomorrow).

I am not familiar with irqdomains or with the irqchip infrastructure, so I can't
really comment on patch #8.

I tried testing this with a GICv3 by modifying the driver to set
no_hw_deactivation and no_maint_irq_mask:

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 340c51d87677..d0c6f808d7f4 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -565,8 +565,10 @@ int kvm_vgic_hyp_init(void)
        if (ret)
                return ret;
 
+       /*
        if (!has_mask)
                return 0;
+               */
 
        ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
                                 vgic_maintenance_handler,
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 453fc425eede..9ce4dee20655 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -1850,6 +1850,12 @@ static void __init gic_of_setup_kvm_info(struct device_node
*node)
        if (!ret)
                gic_v3_kvm_info.vcpu = r;
 
+       gic_v3_kvm_info.no_hw_deactivation = true;
+       gic_v3_kvm_info.no_maint_irq_mask = true;
+
+       vgic_set_kvm_info(&gic_v3_kvm_info);
+       return;
+
        gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
        gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
        vgic_set_kvm_info(&gic_v3_kvm_info);

Kept the maintenance irq ID so the IRQ gets enabled at the Redistributor level. I
don't know if I managed to break something with those changes, but when testing on
the model and on a rockpro64 (with the patches cherry-picked on top of v5.13-rc7)
I kept seeing rcu stalls. I assume I did something wrong.

Thanks,

Alex

>
> * From v3 [3]:
>   - Rebased on 5.13-rc4 to match the kvmarm/next base
>   - Moved stuff from patch #7 to its logical spot in patch #8
>   - Changed the include/linux/irqchip/arm-vgic-info.h guard
>   - Collected RBs from Alex, with thanks
>
> * From v2 [2]:
>   - 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
> [3] https://lore.kernel.org/r/20210510134824.1910399-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
>     masking
>   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            | 162 +++++++++++++++++++++----
>  arch/arm64/kvm/hyp/hyp-entry.S         |   6 +-
>  arch/arm64/kvm/vgic/vgic-init.c        |  36 +++++-
>  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        |   9 ++
>  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  |  45 +++++++
>  14 files changed, 299 insertions(+), 104 deletions(-)
>  create mode 100644 include/linux/irqchip/arm-vgic-info.h
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Marc Zyngier]

Hi Alex,

On Tue, 22 Jun 2021 16:39:11 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 6/1/21 11:39 AM, Marc Zyngier wrote:
> > This is a new version of the series previously posted at [3], 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.
> >
> > Until someone shouts loudly now, I'll take this into 5.14 (and in
> > -next from tomorrow).
> 
> I am not familiar with irqdomains or with the irqchip
> infrastructure, so I can't really comment on patch #8.
> 
> I tried testing this with a GICv3 by modifying the driver to set
> no_hw_deactivation and no_maint_irq_mask:
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 340c51d87677..d0c6f808d7f4 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -565,8 +565,10 @@ int kvm_vgic_hyp_init(void)
>         if (ret)
>                 return ret;
>  
> +       /*
>         if (!has_mask)
>                 return 0;
> +               */
>  
>         ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>                                  vgic_maintenance_handler,
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 453fc425eede..9ce4dee20655 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -1850,6 +1850,12 @@ static void __init gic_of_setup_kvm_info(struct device_node
> *node)
>         if (!ret)
>                 gic_v3_kvm_info.vcpu = r;
>  
> +       gic_v3_kvm_info.no_hw_deactivation = true;

Blink...

> +       gic_v3_kvm_info.no_maint_irq_mask = true;
> +
> +       vgic_set_kvm_info(&gic_v3_kvm_info);
> +       return;
> +
>         gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
>         gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
>         vgic_set_kvm_info(&gic_v3_kvm_info);
> 
> Kept the maintenance irq ID so the IRQ gets enabled at the
> Redistributor level. I don't know if I managed to break something
> with those changes, but when testing on the model and on a rockpro64
> (with the patches cherry-picked on top of v5.13-rc7) I kept seeing
> rcu stalls. I assume I did something wrong.

If you do that, the interrupts that are forwarded to the guest
(timers) will never be deactivated, and will be left dangling after
the first injection. This is bound to create havoc, as we will then
use mask/unmask to control the timer delivery (remember that the
Active state is just another form of auto-masking on top of the
standard enable bit)

On the contrary, the AIC only has a single bit to control the timer
(used as a mask), which is what the irqdomain stuff implements to
mimic the active state.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Alexandru Elisei]

Hi Marc,

On 6/22/21 4:51 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On Tue, 22 Jun 2021 16:39:11 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> Hi Marc,
>>
>> On 6/1/21 11:39 AM, Marc Zyngier wrote:
>>> This is a new version of the series previously posted at [3], 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.
>>>
>>> Until someone shouts loudly now, I'll take this into 5.14 (and in
>>> -next from tomorrow).
>> I am not familiar with irqdomains or with the irqchip
>> infrastructure, so I can't really comment on patch #8.
>>
>> I tried testing this with a GICv3 by modifying the driver to set
>> no_hw_deactivation and no_maint_irq_mask:
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
>> index 340c51d87677..d0c6f808d7f4 100644
>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>> @@ -565,8 +565,10 @@ int kvm_vgic_hyp_init(void)
>>         if (ret)
>>                 return ret;
>>  
>> +       /*
>>         if (!has_mask)
>>                 return 0;
>> +               */
>>  
>>         ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>>                                  vgic_maintenance_handler,
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index 453fc425eede..9ce4dee20655 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -1850,6 +1850,12 @@ static void __init gic_of_setup_kvm_info(struct device_node
>> *node)
>>         if (!ret)
>>                 gic_v3_kvm_info.vcpu = r;
>>  
>> +       gic_v3_kvm_info.no_hw_deactivation = true;
> Blink...
>
>> +       gic_v3_kvm_info.no_maint_irq_mask = true;
>> +
>> +       vgic_set_kvm_info(&gic_v3_kvm_info);
>> +       return;
>> +
>>         gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
>>         gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
>>         vgic_set_kvm_info(&gic_v3_kvm_info);
>>
>> Kept the maintenance irq ID so the IRQ gets enabled at the
>> Redistributor level. I don't know if I managed to break something
>> with those changes, but when testing on the model and on a rockpro64
>> (with the patches cherry-picked on top of v5.13-rc7) I kept seeing
>> rcu stalls. I assume I did something wrong.
> If you do that, the interrupts that are forwarded to the guest
> (timers) will never be deactivated, and will be left dangling after
> the first injection. This is bound to create havoc, as we will then
> use mask/unmask to control the timer delivery (remember that the
> Active state is just another form of auto-masking on top of the
> standard enable bit)
>
> On the contrary, the AIC only has a single bit to control the timer
> (used as a mask), which is what the irqdomain stuff implements to
> mimic the active state.

So these patches work **only** with the AIC, not with a standard GICv3 without the
HW bit in the LR registers and with an unmaskable maintenance IRQ? Because from
the commit message from #8 I got the impression that the purpose of the change is
to make timers work on a standard GICv3, sans those required architectural features.

Thanks,

Alex

>
> Thanks,
>
> 	M.
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Marc Zyngier]

On Tue, 22 Jun 2021 17:03:22 +0100,
Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> 
> Hi Marc,
> 
> On 6/22/21 4:51 PM, Marc Zyngier wrote:
> > Hi Alex,
> >
> > On Tue, 22 Jun 2021 16:39:11 +0100,
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >> Hi Marc,
> >>
> >> On 6/1/21 11:39 AM, Marc Zyngier wrote:
> >>> This is a new version of the series previously posted at [3], 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.
> >>>
> >>> Until someone shouts loudly now, I'll take this into 5.14 (and in
> >>> -next from tomorrow).
> >> I am not familiar with irqdomains or with the irqchip
> >> infrastructure, so I can't really comment on patch #8.
> >>
> >> I tried testing this with a GICv3 by modifying the driver to set
> >> no_hw_deactivation and no_maint_irq_mask:
> >>
> >> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> >> index 340c51d87677..d0c6f808d7f4 100644
> >> --- a/arch/arm64/kvm/vgic/vgic-init.c
> >> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> >> @@ -565,8 +565,10 @@ int kvm_vgic_hyp_init(void)
> >>         if (ret)
> >>                 return ret;
> >>  
> >> +       /*
> >>         if (!has_mask)
> >>                 return 0;
> >> +               */
> >>  
> >>         ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
> >>                                  vgic_maintenance_handler,
> >> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> >> index 453fc425eede..9ce4dee20655 100644
> >> --- a/drivers/irqchip/irq-gic-v3.c
> >> +++ b/drivers/irqchip/irq-gic-v3.c
> >> @@ -1850,6 +1850,12 @@ static void __init gic_of_setup_kvm_info(struct device_node
> >> *node)
> >>         if (!ret)
> >>                 gic_v3_kvm_info.vcpu = r;
> >>  
> >> +       gic_v3_kvm_info.no_hw_deactivation = true;
> > Blink...
> >
> >> +       gic_v3_kvm_info.no_maint_irq_mask = true;
> >> +
> >> +       vgic_set_kvm_info(&gic_v3_kvm_info);
> >> +       return;
> >> +
> >>         gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
> >>         gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
> >>         vgic_set_kvm_info(&gic_v3_kvm_info);
> >>
> >> Kept the maintenance irq ID so the IRQ gets enabled at the
> >> Redistributor level. I don't know if I managed to break something
> >> with those changes, but when testing on the model and on a rockpro64
> >> (with the patches cherry-picked on top of v5.13-rc7) I kept seeing
> >> rcu stalls. I assume I did something wrong.
> > If you do that, the interrupts that are forwarded to the guest
> > (timers) will never be deactivated, and will be left dangling after
> > the first injection. This is bound to create havoc, as we will then
> > use mask/unmask to control the timer delivery (remember that the
> > Active state is just another form of auto-masking on top of the
> > standard enable bit)
> >
> > On the contrary, the AIC only has a single bit to control the timer
> > (used as a mask), which is what the irqdomain stuff implements to
> > mimic the active state.
> 
> So these patches work **only** with the AIC, not with a standard
> GICv3 without the HW bit in the LR registers and with an unmaskable
> maintenance IRQ? Because from the commit message from #8 I got the
> impression that the purpose of the change is to make timers work on
> a standard GICv3, sans those required architectural features.

I don't understand what you mean.

The HW bit in the LR and deactivation *are* required, non-negotiable
parts of the GICv3 architecture. Apple did not implement it is a
consequence of the AIC not having an active state that the guest can
manipulate independently of the host.

Either you have both HW bit and active state, and both work together
(normal GICv3), or you have none of that and we rely on the
maintenance interrupt to exit and fix the mess (Apple crap). You
cannot have an intermediate state between the two.

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/9] KVM: arm64: Initial host support for the Apple M1

[Alexandru Elisei]

Hi Marc,

On 6/22/21 5:26 PM, Marc Zyngier wrote:
> On Tue, 22 Jun 2021 17:03:22 +0100,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>> Hi Marc,
>>
>> On 6/22/21 4:51 PM, Marc Zyngier wrote:
>>> Hi Alex,
>>>
>>> On Tue, 22 Jun 2021 16:39:11 +0100,
>>> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>>>> Hi Marc,
>>>>
>>>> On 6/1/21 11:39 AM, Marc Zyngier wrote:
>>>>> This is a new version of the series previously posted at [3], 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.
>>>>>
>>>>> Until someone shouts loudly now, I'll take this into 5.14 (and in
>>>>> -next from tomorrow).
>>>> I am not familiar with irqdomains or with the irqchip
>>>> infrastructure, so I can't really comment on patch #8.
>>>>
>>>> I tried testing this with a GICv3 by modifying the driver to set
>>>> no_hw_deactivation and no_maint_irq_mask:
>>>>
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
>>>> index 340c51d87677..d0c6f808d7f4 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-init.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-init.c
>>>> @@ -565,8 +565,10 @@ int kvm_vgic_hyp_init(void)
>>>>         if (ret)
>>>>                 return ret;
>>>>  
>>>> +       /*
>>>>         if (!has_mask)
>>>>                 return 0;
>>>> +               */
>>>>  
>>>>         ret = request_percpu_irq(kvm_vgic_global_state.maint_irq,
>>>>                                  vgic_maintenance_handler,
>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>>>> index 453fc425eede..9ce4dee20655 100644
>>>> --- a/drivers/irqchip/irq-gic-v3.c
>>>> +++ b/drivers/irqchip/irq-gic-v3.c
>>>> @@ -1850,6 +1850,12 @@ static void __init gic_of_setup_kvm_info(struct device_node
>>>> *node)
>>>>         if (!ret)
>>>>                 gic_v3_kvm_info.vcpu = r;
>>>>  
>>>> +       gic_v3_kvm_info.no_hw_deactivation = true;
>>> Blink...
>>>
>>>> +       gic_v3_kvm_info.no_maint_irq_mask = true;
>>>> +
>>>> +       vgic_set_kvm_info(&gic_v3_kvm_info);
>>>> +       return;
>>>> +
>>>>         gic_v3_kvm_info.has_v4 = gic_data.rdists.has_vlpis;
>>>>         gic_v3_kvm_info.has_v4_1 = gic_data.rdists.has_rvpeid;
>>>>         vgic_set_kvm_info(&gic_v3_kvm_info);
>>>>
>>>> Kept the maintenance irq ID so the IRQ gets enabled at the
>>>> Redistributor level. I don't know if I managed to break something
>>>> with those changes, but when testing on the model and on a rockpro64
>>>> (with the patches cherry-picked on top of v5.13-rc7) I kept seeing
>>>> rcu stalls. I assume I did something wrong.
>>> If you do that, the interrupts that are forwarded to the guest
>>> (timers) will never be deactivated, and will be left dangling after
>>> the first injection. This is bound to create havoc, as we will then
>>> use mask/unmask to control the timer delivery (remember that the
>>> Active state is just another form of auto-masking on top of the
>>> standard enable bit)
>>>
>>> On the contrary, the AIC only has a single bit to control the timer
>>> (used as a mask), which is what the irqdomain stuff implements to
>>> mimic the active state.
>> So these patches work **only** with the AIC, not with a standard
>> GICv3 without the HW bit in the LR registers and with an unmaskable
>> maintenance IRQ? Because from the commit message from #8 I got the
>> impression that the purpose of the change is to make timers work on
>> a standard GICv3, sans those required architectural features.
> I don't understand what you mean.

I think I understand what is happening better now.

With my changes, vgic_set_irq_phys_active(irq,
false)->irq_set_irqchip_state(host_irq, IRQCHIP_STATE_ACTIVE, false) will call
timer_set_irqchip_state()->irqchip_unmask_parent() which doesn't clear the active
state of the timer interrupt at the GIC level. This means that the timer interrupt
acts like it's permanently masked after that first interrupt, like you've said.

From this I understand that the AIC is the only GIC implementation that will work
when no_hw_deactivation = true. To put it another way, a GIC implementation that
is 100% according to the spec with the exception that ICH_LR.HW is hardwired to 0
and the maintenance interrupt enabled and unmaskable will not work with these
patches because vgic_set_irq_phys_active(irq, false) will unmask the interrupt
instead of clearing the active state.

I was confused about that because I didn't find anywhere mentioned in the commit
message or in the code for patch #8 that the patch only works with an AIC, and not
with a generic GICv3 without hardware deactivation.

Thanks,

Alex

> The HW bit in the LR and deactivation *are* required, non-negotiable
> parts of the GICv3 architecture. Apple did not implement it is a
> consequence of the AIC not having an active state that the guest can
> manipulate independently of the host.
>
> Either you have both HW bit and active state, and both work together
> (normal GICv3), or you have none of that and we rely on the
> maintenance interrupt to exit and fix the mess (Apple crap). You
> cannot have an intermediate state between the two.
>
> Thanks,
>
> 	M.
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel