Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1352915 > unrolled thread

[PATCH v3 0/9] arm64: Add support of KVM with ACPI

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/9] arm64: Add support of KVM with ACPI Julien Grall <julien.grall@arm.com> - 2016-03-08 12:40 +0100
    [PATCH v3 4/9] irqchip/gic-v2: Parse and export virtual GIC information Julien Grall <julien.grall@arm.com> - 2016-03-08 12:40 +0100
      Re: [PATCH v3 4/9] irqchip/gic-v2: Parse and export virtual GIC  information Christoffer Dall <christoffer.dall@linaro.org> - 2016-03-09 06:20 +0100
        Re: [PATCH v3 4/9] irqchip/gic-v2: Parse and export virtual GIC  information Julien Grall <julien.grall@arm.com> - 2016-03-14 13:20 +0100
    [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a single structure Julien Grall <julien.grall@arm.com> - 2016-03-08 12:40 +0100
      Re: [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a  single structure Christoffer Dall <christoffer.dall@linaro.org> - 2016-03-09 06:50 +0100
        Re: [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a  single structure Julien Grall <julien.grall@arm.com> - 2016-03-09 07:20 +0100
          Re: [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a  single structure Christoffer Dall <christoffer.dall@linaro.org> - 2016-03-13 19:20 +0100
    [PATCH v3 6/9] irqchip/gic-v3: Parse and export virtual GIC information Julien Grall <julien.grall@arm.com> - 2016-03-08 12:40 +0100
      Re: [PATCH v3 6/9] irqchip/gic-v3: Parse and export virtual GIC  information Christoffer Dall <christoffer.dall@linaro.org> - 2016-03-09 07:00 +0100
    [PATCH v3 7/9] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables Julien Grall <julien.grall@arm.com> - 2016-03-08 12:40 +0100
      Re: [PATCH v3 7/9] KVM: arm/arm64: arch_timer: Rely on the arch  timer to parse the firmware tables Christoffer Dall <christoffer.dall@linaro.org> - 2016-03-09 07:00 +0100

#1352915 — [PATCH v3 0/9] arm64: Add support of KVM with ACPI

Hello,

This patch series allows an ARM64 ACPI based platform to use KVM.

Currently the KVM code has to parse the firmware table to get the necessary
information to setup the virtual timer and virtual GIC.

However the parsing of those tables are already done in the GIC and arch
timer drivers.

This patch series introduces different helpers to retrieve the information
from different drivers avoiding the duplication of the parsing code.

Compare to v2, the serie has been reworked to allow easy merge via the
different trees.

For the other changes see the different patches.

Sincerely yours,

Julien Grall (9):
  clocksource: arm_arch_timer: Gather KVM specific information in a
    structure
  clocksource: arm_arch_timer: Extend arch_timer_kvm_info to get the
    virtual IRQ
  irqchip/gic-v2: Gather ACPI specific data in a single structure
  irqchip/gic-v2: Parse and export virtual GIC information
  irqchip/gic-v3: Gather all ACPI specific data in a single structure
  irqchip/gic-v3: Parse and export virtual GIC information
  KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the
    firmware tables
  KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware
    tables
  clocksource: arm_arch_timer: Remove arch_timer_get_timecounter

 drivers/clocksource/arm_arch_timer.c   |  11 ++-
 drivers/irqchip/irq-gic-common.c       |  13 +++
 drivers/irqchip/irq-gic-common.h       |   3 +
 drivers/irqchip/irq-gic-v3.c           | 145 ++++++++++++++++++++++++++-------
 drivers/irqchip/irq-gic.c              |  91 +++++++++++++++++++--
 include/clocksource/arm_arch_timer.h   |  12 +--
 include/kvm/arm_vgic.h                 |   7 +-
 include/linux/irqchip/arm-gic-common.h |  34 ++++++++
 virt/kvm/arm/arch_timer.c              |  40 +++------
 virt/kvm/arm/vgic-v2.c                 |  67 ++++++---------
 virt/kvm/arm/vgic-v3.c                 |  45 +++-------
 virt/kvm/arm/vgic.c                    |  50 ++++++------
 12 files changed, 345 insertions(+), 173 deletions(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1352918 — [PATCH v3 4/9] irqchip/gic-v2: Parse and export virtual GIC information

For now, the firmware tables are parsed 2 times: once in the GIC
drivers, the other timer when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).

Introduce a new structure and set of helpers to get/set the virtual GIC
information. Also fill up the structure for GICv2.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>

    Changes in v2:
        - Use 0 rather than a negative value to know when the maintenance IRQ
        is not present.
        - Use resource for vcpu and vctrl
---
 drivers/irqchip/irq-gic-common.c       | 13 ++++++
 drivers/irqchip/irq-gic-common.h       |  3 ++
 drivers/irqchip/irq-gic.c              | 80 +++++++++++++++++++++++++++++++++-
 include/linux/irqchip/arm-gic-common.h | 33 ++++++++++++++
 4 files changed, 128 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0..704caf4 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,19 @@
 
 #include "irq-gic-common.h"
 
+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)
+{
+	WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");
+	gic_kvm_info = info;
+}
+
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
 		void *data)
 {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index fff697d..205e5fd 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -19,6 +19,7 @@
 
 #include <linux/of.h>
 #include <linux/irqdomain.h>
+#include <linux/irqchip/arm-gic-common.h>
 
 struct gic_quirk {
 	const char *desc;
@@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
 void gic_enable_quirks(u32 iidr, 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.c b/drivers/irqchip/irq-gic.c
index fbde202..0c58112 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -102,6 +102,8 @@ static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
 
 static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
 
+static struct gic_kvm_info gic_v2_kvm_info;
+
 #ifdef CONFIG_GIC_NON_BANKED
 static void __iomem *gic_get_percpu_base(union gic_base *base)
 {
@@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
 	return true;
 }
 
+static void __init gic_of_setup_kvm_info(struct device_node *node)
+{
+	int ret;
+	struct resource r;
+
+	gic_v2_kvm_info.type = GIC_V2;
+
+	gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
+
+	ret = of_address_to_resource(node, 2, &r);
+	if (!ret)
+		gic_v2_kvm_info.vctrl = r;
+
+	ret = of_address_to_resource(node, 3, &r);
+	if (!ret) {
+		if (!PAGE_ALIGNED(r.start))
+			pr_warn("GICV physical address 0x%llx not page aligned\n",
+				(unsigned long long)r.start);
+		else if (!PAGE_ALIGNED(resource_size(&r)))
+			pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
+				(unsigned long long)resource_size(&r),
+				PAGE_SIZE);
+		else
+			gic_v2_kvm_info.vcpu = r;
+	}
+
+	gic_set_kvm_info(&gic_v2_kvm_info);
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
@@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 
 	__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
 			 &node->fwnode);
-	if (!gic_cnt)
+	if (!gic_cnt) {
 		gic_init_physaddr(node);
+		gic_of_setup_kvm_info(node);
+	}
 
 	if (parent) {
 		irq = irq_of_parse_and_map(node, 0);
@@ -1248,6 +1281,10 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
 static struct
 {
 	phys_addr_t cpu_phy_base;
+	u32 maint_irq;
+	int maint_irq_mode;
+	phys_addr_t vctrl_base;
+	phys_addr_t vcpu_base;
 } acpi_data __initdata;
 
 static int __init
@@ -1272,6 +1309,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 		return -EINVAL;
 
 	acpi_data.cpu_phy_base = gic_cpu_base;
+	acpi_data.maint_irq = processor->vgic_interrupt;
+	acpi_data.maint_irq_mode = (processor->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
+				    ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+	acpi_data.vctrl_base = processor->gich_base_address;
+	acpi_data.vcpu_base = processor->gicv_base_address;
+
 	cpu_base_assigned = 1;
 	return 0;
 }
@@ -1302,6 +1345,39 @@ static bool __init gic_validate_dist(struct acpi_subtable_header *header,
 
 #define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
 #define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
+#define ACPI_GICV2_VCTRL_MEM_SIZE	(SZ_4K)
+#define ACPI_GICV2_VCPU_MEM_SIZE	(SZ_8K)
+
+static void __init gic_acpi_setup_kvm_info(void)
+{
+	int irq;
+
+	gic_v2_kvm_info.type = GIC_V2;
+
+	irq = acpi_register_gsi(NULL, acpi_data.maint_irq,
+				acpi_data.maint_irq_mode,
+				ACPI_ACTIVE_HIGH);
+	if (irq > 0)
+		gic_v2_kvm_info.maint_irq = irq;
+
+	if (acpi_data.vctrl_base) {
+		struct resource *vctrl = &gic_v2_kvm_info.vctrl;
+
+		vctrl->flags = IORESOURCE_MEM;
+		vctrl->start = acpi_data.vctrl_base;
+		vctrl->end = vctrl->start + ACPI_GICV2_VCTRL_MEM_SIZE - 1;
+	}
+
+	if (acpi_data.vcpu_base) {
+		struct resource *vcpu = &gic_v2_kvm_info.vcpu;
+
+		vcpu->flags = IORESOURCE_MEM;
+		vcpu->start = acpi_data.vcpu_base;
+		vcpu->end = vcpu->start + ACPI_GICV2_VCPU_MEM_SIZE - 1;
+	}
+
+	gic_set_kvm_info(&gic_v2_kvm_info);
+}
 
 static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 				   const unsigned long end)
@@ -1359,6 +1435,8 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 	if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
 		gicv2m_init(NULL, gic_data[0].domain);
 
+	gic_acpi_setup_kvm_info();
+
 	return 0;
 }
 IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
new file mode 100644
index 0000000..f7723b9
--- /dev/null
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -0,0 +1,33 @@
+/*
+ * include/linux/irqchip/arm-gic-common.h
+ *
+ * Copyright (C) 2016 ARM Limited, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef __LINUX_IRQCHIP_ARM_GIC_COMMON_H
+#define __LINUX_IRQCHIP_ARM_GIC_COMMON_H
+
+#include <linux/types.h>
+#include <linux/ioport.h>
+
+enum gic_type {
+	GIC_V2,
+};
+
+struct gic_kvm_info {
+	/* GIC type */
+	enum gic_type	type;
+	/* Physical address & size of virtual cpu interface */
+	struct resource vcpu;
+	/* Interrupt number */
+	unsigned int	maint_irq;
+	/* Physical address & size of virtual control interface */
+	struct resource vctrl;
+};
+
+const struct gic_kvm_info *gic_get_kvm_info(void);
+
+#endif /* __LINUX_IRQCHIP_ARM_GIC_COMMON_H */
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1353793 — Re: [PATCH v3 4/9] irqchip/gic-v2: Parse and export virtual GIC information

On Tue, Mar 08, 2016 at 11:29:28AM +0000, Julien Grall wrote:
> For now, the firmware tables are parsed 2 times: once in the GIC
> drivers, the other timer when initializing the vGIC. It means code
> duplication and make more tedious to add the support for another
> firmware table (like ACPI).
> 
> Introduce a new structure and set of helpers to get/set the virtual GIC
> information. Also fill up the structure for GICv2.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
>     Changes in v2:
>         - Use 0 rather than a negative value to know when the maintenance IRQ
>         is not present.
>         - Use resource for vcpu and vctrl
> ---
>  drivers/irqchip/irq-gic-common.c       | 13 ++++++
>  drivers/irqchip/irq-gic-common.h       |  3 ++
>  drivers/irqchip/irq-gic.c              | 80 +++++++++++++++++++++++++++++++++-
>  include/linux/irqchip/arm-gic-common.h | 33 ++++++++++++++
>  4 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/irqchip/arm-gic-common.h
> 
> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> index f174ce0..704caf4 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -21,6 +21,19 @@
>  
>  #include "irq-gic-common.h"
>  
> +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)
> +{
> +	WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");

why do we WARN here?  Wouldn't this be an obvious bug?

> +	gic_kvm_info = info;
> +}
> +
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>  		void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> index fff697d..205e5fd 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -19,6 +19,7 @@
>  
>  #include <linux/of.h>
>  #include <linux/irqdomain.h>
> +#include <linux/irqchip/arm-gic-common.h>
>  
>  struct gic_quirk {
>  	const char *desc;
> @@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
>  void gic_enable_quirks(u32 iidr, 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.c b/drivers/irqchip/irq-gic.c
> index fbde202..0c58112 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -102,6 +102,8 @@ static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
>  
>  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
>  
> +static struct gic_kvm_info gic_v2_kvm_info;
> +
>  #ifdef CONFIG_GIC_NON_BANKED
>  static void __iomem *gic_get_percpu_base(union gic_base *base)
>  {
> @@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>  	return true;
>  }
>  
> +static void __init gic_of_setup_kvm_info(struct device_node *node)
> +{
> +	int ret;
> +	struct resource r;

I would prefer two struct resource variables more akin to how the KVM
code did it already, and then name them something more understandable
than 'r'.

You could also argue that you can then only populate the gic_v2_kvm_info
with all coherent info or nothing, instead of filling it out partially
and then exiting.

> +
> +	gic_v2_kvm_info.type = GIC_V2;
> +
> +	gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
> +
> +	ret = of_address_to_resource(node, 2, &r);
> +	if (!ret)
> +		gic_v2_kvm_info.vctrl = r;
> +
> +	ret = of_address_to_resource(node, 3, &r);

here you're overwriting the error return value if the first call to
of_address_to_resource failed ?

> +	if (!ret) {
> +		if (!PAGE_ALIGNED(r.start))
> +			pr_warn("GICV physical address 0x%llx not page aligned\n",
> +				(unsigned long long)r.start);

how does KVM know that this went bad?

> +		else if (!PAGE_ALIGNED(resource_size(&r)))
> +			pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> +				(unsigned long long)resource_size(&r),
> +				PAGE_SIZE);

same?

> +		else
> +			gic_v2_kvm_info.vcpu = r;
> +	}
> +
> +	gic_set_kvm_info(&gic_v2_kvm_info);

so here you're setting the kvm info even if one of the calls above
fails?

I think this function could leverage much more of the existing KVM
implementation to avoid these kinds of mistakes.

> +}
> +
>  int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
> @@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>  
>  	__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
>  			 &node->fwnode);
> -	if (!gic_cnt)
> +	if (!gic_cnt) {
>  		gic_init_physaddr(node);
> +		gic_of_setup_kvm_info(node);
> +	}
>  
>  	if (parent) {
>  		irq = irq_of_parse_and_map(node, 0);
> @@ -1248,6 +1281,10 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>  static struct
>  {
>  	phys_addr_t cpu_phy_base;
> +	u32 maint_irq;
> +	int maint_irq_mode;
> +	phys_addr_t vctrl_base;
> +	phys_addr_t vcpu_base;
>  } acpi_data __initdata;
>  
>  static int __init
> @@ -1272,6 +1309,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>  		return -EINVAL;
>  
>  	acpi_data.cpu_phy_base = gic_cpu_base;
> +	acpi_data.maint_irq = processor->vgic_interrupt;
> +	acpi_data.maint_irq_mode = (processor->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
> +				    ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;

I can only assume that bit 2 set means edge, and the default is leve,
but I really don't know how ACPI works here.

> +	acpi_data.vctrl_base = processor->gich_base_address;
> +	acpi_data.vcpu_base = processor->gicv_base_address;
> +
>  	cpu_base_assigned = 1;
>  	return 0;
>  }
> @@ -1302,6 +1345,39 @@ static bool __init gic_validate_dist(struct acpi_subtable_header *header,
>  
>  #define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
>  #define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
> +#define ACPI_GICV2_VCTRL_MEM_SIZE	(SZ_4K)
> +#define ACPI_GICV2_VCPU_MEM_SIZE	(SZ_8K)
> +
> +static void __init gic_acpi_setup_kvm_info(void)
> +{
> +	int irq;
> +
> +	gic_v2_kvm_info.type = GIC_V2;
> +
> +	irq = acpi_register_gsi(NULL, acpi_data.maint_irq,
> +				acpi_data.maint_irq_mode,
> +				ACPI_ACTIVE_HIGH);
> +	if (irq > 0)
> +		gic_v2_kvm_info.maint_irq = irq;
> +
> +	if (acpi_data.vctrl_base) {
> +		struct resource *vctrl = &gic_v2_kvm_info.vctrl;
> +
> +		vctrl->flags = IORESOURCE_MEM;
> +		vctrl->start = acpi_data.vctrl_base;
> +		vctrl->end = vctrl->start + ACPI_GICV2_VCTRL_MEM_SIZE - 1;
> +	}
> +
> +	if (acpi_data.vcpu_base) {
> +		struct resource *vcpu = &gic_v2_kvm_info.vcpu;
> +
> +		vcpu->flags = IORESOURCE_MEM;
> +		vcpu->start = acpi_data.vcpu_base;
> +		vcpu->end = vcpu->start + ACPI_GICV2_VCPU_MEM_SIZE - 1;
> +	}
> +
> +	gic_set_kvm_info(&gic_v2_kvm_info);

again, if something fails above, you're still setting the struct
pointer.

how will KVM know?

> +}
>  
>  static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
>  				   const unsigned long end)
> @@ -1359,6 +1435,8 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
>  	if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
>  		gicv2m_init(NULL, gic_data[0].domain);
>  
> +	gic_acpi_setup_kvm_info();
> +
>  	return 0;
>  }
>  IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
> new file mode 100644
> index 0000000..f7723b9
> --- /dev/null
> +++ b/include/linux/irqchip/arm-gic-common.h
> @@ -0,0 +1,33 @@
> +/*
> + * include/linux/irqchip/arm-gic-common.h
> + *
> + * Copyright (C) 2016 ARM Limited, All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef __LINUX_IRQCHIP_ARM_GIC_COMMON_H
> +#define __LINUX_IRQCHIP_ARM_GIC_COMMON_H
> +
> +#include <linux/types.h>
> +#include <linux/ioport.h>
> +
> +enum gic_type {
> +	GIC_V2,
> +};
> +
> +struct gic_kvm_info {
> +	/* GIC type */
> +	enum gic_type	type;
> +	/* Physical address & size of virtual cpu interface */

is this a bitwise '&' or did you want to save two ascii characters by
not writing 'and' ?  If the latter, please write 'and'.

> +	struct resource vcpu;
> +	/* Interrupt number */

if you're going to comment on the fact that this is an interrupt number,
I would be more interested in knowing if it's a Linux IRQ number or the
INTID...

> +	unsigned int	maint_irq;
> +	/* Physical address & size of virtual control interface */

same here

> +	struct resource vctrl;

I'm actually not convinced the comments on this struct help a lot, but
ok.

> +};
> +
> +const struct gic_kvm_info *gic_get_kvm_info(void);
> +
> +#endif /* __LINUX_IRQCHIP_ARM_GIC_COMMON_H */
> -- 
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1357193 — Re: [PATCH v3 4/9] irqchip/gic-v2: Parse and export virtual GIC information

Hi Christoffer,

On 09/03/16 05:14, Christoffer Dall wrote:
> On Tue, Mar 08, 2016 at 11:29:28AM +0000, Julien Grall wrote:
>> For now, the firmware tables are parsed 2 times: once in the GIC
>> drivers, the other timer when initializing the vGIC. It means code
>> duplication and make more tedious to add the support for another
>> firmware table (like ACPI).
>>
>> Introduce a new structure and set of helpers to get/set the virtual GIC
>> information. Also fill up the structure for GICv2.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>
>>      Changes in v2:
>>          - Use 0 rather than a negative value to know when the maintenance IRQ
>>          is not present.
>>          - Use resource for vcpu and vctrl
>> ---
>>   drivers/irqchip/irq-gic-common.c       | 13 ++++++
>>   drivers/irqchip/irq-gic-common.h       |  3 ++
>>   drivers/irqchip/irq-gic.c              | 80 +++++++++++++++++++++++++++++++++-
>>   include/linux/irqchip/arm-gic-common.h | 33 ++++++++++++++
>>   4 files changed, 128 insertions(+), 1 deletion(-)
>>   create mode 100644 include/linux/irqchip/arm-gic-common.h
>>
>> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
>> index f174ce0..704caf4 100644
>> --- a/drivers/irqchip/irq-gic-common.c
>> +++ b/drivers/irqchip/irq-gic-common.c
>> @@ -21,6 +21,19 @@
>>
>>   #include "irq-gic-common.h"
>>
>> +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)
>> +{
>> +	WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");
>
> why do we WARN here?  Wouldn't this be an obvious bug?

Right this is an obvious bug. I will switch to BUG_ON.

>
>> +	gic_kvm_info = info;
>> +}
>> +
>>   void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>>   		void *data)
>>   {
>> diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
>> index fff697d..205e5fd 100644
>> --- a/drivers/irqchip/irq-gic-common.h
>> +++ b/drivers/irqchip/irq-gic-common.h
>> @@ -19,6 +19,7 @@
>>
>>   #include <linux/of.h>
>>   #include <linux/irqdomain.h>
>> +#include <linux/irqchip/arm-gic-common.h>
>>
>>   struct gic_quirk {
>>   	const char *desc;
>> @@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
>>   void gic_enable_quirks(u32 iidr, 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.c b/drivers/irqchip/irq-gic.c
>> index fbde202..0c58112 100644
>> --- a/drivers/irqchip/irq-gic.c
>> +++ b/drivers/irqchip/irq-gic.c
>> @@ -102,6 +102,8 @@ static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
>>
>>   static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
>>
>> +static struct gic_kvm_info gic_v2_kvm_info;
>> +
>>   #ifdef CONFIG_GIC_NON_BANKED
>>   static void __iomem *gic_get_percpu_base(union gic_base *base)
>>   {
>> @@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node *node, void __iomem **base)
>>   	return true;
>>   }
>>
>> +static void __init gic_of_setup_kvm_info(struct device_node *node)
>> +{
>> +	int ret;
>> +	struct resource r;
>
> I would prefer two struct resource variables more akin to how the KVM
> code did it already, and then name them something more understandable
> than 'r'.
>
> You could also argue that you can then only populate the gic_v2_kvm_info
> with all coherent info or nothing, instead of filling it out partially
> and then exiting.
>
>> +
>> +	gic_v2_kvm_info.type = GIC_V2;
>> +
>> +	gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
>> +
>> +	ret = of_address_to_resource(node, 2, &r);
>> +	if (!ret)
>> +		gic_v2_kvm_info.vctrl = r;
>> +
>> +	ret = of_address_to_resource(node, 3, &r);
>
> here you're overwriting the error return value if the first call to
> of_address_to_resource failed ?
>
>> +	if (!ret) {
>> +		if (!PAGE_ALIGNED(r.start))
>> +			pr_warn("GICV physical address 0x%llx not page aligned\n",
>> +				(unsigned long long)r.start);
>
> how does KVM know that this went bad?
>
>> +		else if (!PAGE_ALIGNED(resource_size(&r)))
>> +			pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
>> +				(unsigned long long)resource_size(&r),
>> +				PAGE_SIZE);
>
> same?
>
>> +		else
>> +			gic_v2_kvm_info.vcpu = r;
>> +	}
>> +
>> +	gic_set_kvm_info(&gic_v2_kvm_info);
>
> so here you're setting the kvm info even if one of the calls above
> fails?
>
> I think this function could leverage much more of the existing KVM
> implementation to avoid these kinds of mistakes.

I will rework this function.

>
>> +}
>> +
>>   int __init
>>   gic_of_init(struct device_node *node, struct device_node *parent)
>>   {
>> @@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>>
>>   	__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
>>   			 &node->fwnode);
>> -	if (!gic_cnt)
>> +	if (!gic_cnt) {
>>   		gic_init_physaddr(node);
>> +		gic_of_setup_kvm_info(node);
>> +	}
>>
>>   	if (parent) {
>>   		irq = irq_of_parse_and_map(node, 0);
>> @@ -1248,6 +1281,10 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>>   static struct
>>   {
>>   	phys_addr_t cpu_phy_base;
>> +	u32 maint_irq;
>> +	int maint_irq_mode;
>> +	phys_addr_t vctrl_base;
>> +	phys_addr_t vcpu_base;
>>   } acpi_data __initdata;
>>
>>   static int __init
>> @@ -1272,6 +1309,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>>   		return -EINVAL;
>>
>>   	acpi_data.cpu_phy_base = gic_cpu_base;
>> +	acpi_data.maint_irq = processor->vgic_interrupt;
>> +	acpi_data.maint_irq_mode = (processor->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
>> +				    ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
>
> I can only assume that bit 2 set means edge, and the default is leve,
> but I really don't know how ACPI works here.

See Table 5.62 in ACPI 6.0.

>
>> +	acpi_data.vctrl_base = processor->gich_base_address;
>> +	acpi_data.vcpu_base = processor->gicv_base_address;
>> +
>>   	cpu_base_assigned = 1;
>>   	return 0;
>>   }
>> @@ -1302,6 +1345,39 @@ static bool __init gic_validate_dist(struct acpi_subtable_header *header,
>>
>>   #define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
>>   #define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
>> +#define ACPI_GICV2_VCTRL_MEM_SIZE	(SZ_4K)
>> +#define ACPI_GICV2_VCPU_MEM_SIZE	(SZ_8K)
>> +
>> +static void __init gic_acpi_setup_kvm_info(void)
>> +{
>> +	int irq;
>> +
>> +	gic_v2_kvm_info.type = GIC_V2;
>> +
>> +	irq = acpi_register_gsi(NULL, acpi_data.maint_irq,
>> +				acpi_data.maint_irq_mode,
>> +				ACPI_ACTIVE_HIGH);
>> +	if (irq > 0)
>> +		gic_v2_kvm_info.maint_irq = irq;
>> +
>> +	if (acpi_data.vctrl_base) {
>> +		struct resource *vctrl = &gic_v2_kvm_info.vctrl;
>> +
>> +		vctrl->flags = IORESOURCE_MEM;
>> +		vctrl->start = acpi_data.vctrl_base;
>> +		vctrl->end = vctrl->start + ACPI_GICV2_VCTRL_MEM_SIZE - 1;
>> +	}
>> +
>> +	if (acpi_data.vcpu_base) {
>> +		struct resource *vcpu = &gic_v2_kvm_info.vcpu;
>> +
>> +		vcpu->flags = IORESOURCE_MEM;
>> +		vcpu->start = acpi_data.vcpu_base;
>> +		vcpu->end = vcpu->start + ACPI_GICV2_VCPU_MEM_SIZE - 1;
>> +	}
>> +
>> +	gic_set_kvm_info(&gic_v2_kvm_info);
>
> again, if something fails above, you're still setting the struct
> pointer.
>
> how will KVM know?

I will rework the function.

>
>> +}
>>
>>   static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
>>   				   const unsigned long end)
>> @@ -1359,6 +1435,8 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
>>   	if (IS_ENABLED(CONFIG_ARM_GIC_V2M))
>>   		gicv2m_init(NULL, gic_data[0].domain);
>>
>> +	gic_acpi_setup_kvm_info();
>> +
>>   	return 0;
>>   }
>>   IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>> diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
>> new file mode 100644
>> index 0000000..f7723b9
>> --- /dev/null
>> +++ b/include/linux/irqchip/arm-gic-common.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * include/linux/irqchip/arm-gic-common.h
>> + *
>> + * Copyright (C) 2016 ARM Limited, All Rights Reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef __LINUX_IRQCHIP_ARM_GIC_COMMON_H
>> +#define __LINUX_IRQCHIP_ARM_GIC_COMMON_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/ioport.h>
>> +
>> +enum gic_type {
>> +	GIC_V2,
>> +};
>> +
>> +struct gic_kvm_info {
>> +	/* GIC type */
>> +	enum gic_type	type;
>> +	/* Physical address & size of virtual cpu interface */
>
> is this a bitwise '&' or did you want to save two ascii characters by
> not writing 'and' ?  If the latter, please write 'and'.

I will write 'and'

>
>> +	struct resource vcpu;
>> +	/* Interrupt number */
>
> if you're going to comment on the fact that this is an interrupt number,
> I would be more interested in knowing if it's a Linux IRQ number or the
> INTID...
>
>> +	unsigned int	maint_irq;
>> +	/* Physical address & size of virtual control interface */
>
> same here

Ditto

>
>> +	struct resource vctrl;
>
> I'm actually not convinced the comments on this struct help a lot, but
> ok.

I can rename to "Virtual control interface".

Regards,

-- 
Julien Grall

[toc] | [prev] | [next] | [standalone]

#1352919 — [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a single structure

For now, there is only one member. More member will be added later.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>

    Changes in v2:
        - Patch added
---
 drivers/irqchip/irq-gic.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 8f9ebf7..fbde202 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1245,7 +1245,10 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
 #endif
 
 #ifdef CONFIG_ACPI
-static phys_addr_t cpu_phy_base __initdata;
+static struct
+{
+	phys_addr_t cpu_phy_base;
+} acpi_data __initdata;
 
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
@@ -1265,10 +1268,10 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 	 * All CPU interface addresses have to be the same.
 	 */
 	gic_cpu_base = processor->base_address;
-	if (cpu_base_assigned && gic_cpu_base != cpu_phy_base)
+	if (cpu_base_assigned && gic_cpu_base != acpi_data.cpu_phy_base)
 		return -EINVAL;
 
-	cpu_phy_base = gic_cpu_base;
+	acpi_data.cpu_phy_base = gic_cpu_base;
 	cpu_base_assigned = 1;
 	return 0;
 }
@@ -1316,7 +1319,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
 		return -EINVAL;
 	}
 
-	cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
+	cpu_base = ioremap(acpi_data.cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
 	if (!cpu_base) {
 		pr_err("Unable to map GICC registers\n");
 		return -ENOMEM;
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1353832 — Re: [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a single structure

On Tue, Mar 08, 2016 at 11:29:27AM +0000, Julien Grall wrote:
> For now, there is only one member. More member will be added later.

questionable commit message

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
>     Changes in v2:
>         - Patch added
> ---
>  drivers/irqchip/irq-gic.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 8f9ebf7..fbde202 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1245,7 +1245,10 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>  #endif
>  
>  #ifdef CONFIG_ACPI
> -static phys_addr_t cpu_phy_base __initdata;
> +static struct
> +{
> +	phys_addr_t cpu_phy_base;
> +} acpi_data __initdata;
>  
>  static int __init
>  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> @@ -1265,10 +1268,10 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>  	 * All CPU interface addresses have to be the same.
>  	 */
>  	gic_cpu_base = processor->base_address;
> -	if (cpu_base_assigned && gic_cpu_base != cpu_phy_base)
> +	if (cpu_base_assigned && gic_cpu_base != acpi_data.cpu_phy_base)
>  		return -EINVAL;
>  
> -	cpu_phy_base = gic_cpu_base;
> +	acpi_data.cpu_phy_base = gic_cpu_base;
>  	cpu_base_assigned = 1;
>  	return 0;
>  }
> @@ -1316,7 +1319,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
>  		return -EINVAL;
>  	}
>  
> -	cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
> +	cpu_base = ioremap(acpi_data.cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
>  	if (!cpu_base) {
>  		pr_err("Unable to map GICC registers\n");
>  		return -ENOMEM;
> -- 
> 1.9.1
> 
super nit: I would use cpu_phys_base instead of cpu_phy_base, but I'll
leave it up to you.

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

[toc] | [prev] | [next] | [standalone]

#1353855 — Re: [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a single structure

Hi Christoffer,

On 09/03/2016 12:47, Christoffer Dall wrote:
> On Tue, Mar 08, 2016 at 11:29:27AM +0000, Julien Grall wrote:
>> For now, there is only one member. More member will be added later.
>
> questionable commit message

What about:

"The ACPI code requires to use global variables in order to collect 
information from the tables.

For now only a single global variable is used, but more will be added in 
a subsequent patch. To make clear they are ACPI specific, gather all the 
information in a single structure."

[...]

>> @@ -1316,7 +1319,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
>>   		return -EINVAL;
>>   	}
>>
>> -	cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
>> +	cpu_base = ioremap(acpi_data.cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
>>   	if (!cpu_base) {
>>   		pr_err("Unable to map GICC registers\n");
>>   		return -ENOMEM;
>> --
>> 1.9.1
>>
> super nit: I would use cpu_phys_base instead of cpu_phy_base, but I'll
> leave it up to you.

I will update the commit message, so I will rename the variable too.

>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Cheers,

-- 
Julien Grall

[toc] | [prev] | [next] | [standalone]

#1356712 — Re: [PATCH v3 3/9] irqchip/gic-v2: Gather ACPI specific data in a single structure

On Wed, Mar 09, 2016 at 01:18:14PM +0700, Julien Grall wrote:
> Hi Christoffer,
> 
> On 09/03/2016 12:47, Christoffer Dall wrote:
> >On Tue, Mar 08, 2016 at 11:29:27AM +0000, Julien Grall wrote:
> >>For now, there is only one member. More member will be added later.
> >
> >questionable commit message
> 
> What about:
> 
> "The ACPI code requires to use global variables in order to collect
> information from the tables.
> 
> For now only a single global variable is used, but more will be
> added in a subsequent patch. To make clear they are ACPI specific,
> gather all the information in a single structure."
> 

that's better.

> [...]
> 
> >>@@ -1316,7 +1319,7 @@ static int __init gic_v2_acpi_init(struct acpi_subtable_header *header,
> >>  		return -EINVAL;
> >>  	}
> >>
> >>-	cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
> >>+	cpu_base = ioremap(acpi_data.cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
> >>  	if (!cpu_base) {
> >>  		pr_err("Unable to map GICC registers\n");
> >>  		return -ENOMEM;
> >>--
> >>1.9.1
> >>
> >super nit: I would use cpu_phys_base instead of cpu_phy_base, but I'll
> >leave it up to you.
> 
> I will update the commit message, so I will rename the variable too.
> 

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1352920 — [PATCH v3 6/9] irqchip/gic-v3: Parse and export virtual GIC information

Fill up the recently introduced gic_kvm_info with the virtual GIC
information.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>

---
    Changes in v3:
        - Add ACPI support

    Changes in v2:
        - Use 0 rather than a negative value to know when the maintenance IRQ
        is not present.
        - Use resource for vcpu and vctrl
---
 drivers/irqchip/irq-gic-v3.c           | 85 +++++++++++++++++++++++++++++++++-
 include/linux/irqchip/arm-gic-common.h |  1 +
 2 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 50e87e6..6ae25120 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -28,6 +28,7 @@
 #include <linux/slab.h>
 
 #include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-common.h>
 #include <linux/irqchip/arm-gic-v3.h>
 
 #include <asm/cputype.h>
@@ -56,6 +57,8 @@ struct gic_chip_data {
 static struct gic_chip_data gic_data __read_mostly;
 static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
 
+static struct gic_kvm_info gic_v3_kvm_info;
+
 #define gic_data_rdist()		(this_cpu_ptr(gic_data.rdists.rdist))
 #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
 #define gic_data_rdist_sgi_base()	(gic_data_rdist_rd_base() + SZ_64K)
@@ -901,6 +904,37 @@ static int __init gic_validate_dist_version(void __iomem *dist_base)
 	return 0;
 }
 
+static void __init gic_of_setup_kvm_info(struct device_node *node)
+{
+	int ret;
+	struct resource r;
+	u32 gicv_idx;
+
+	gic_v3_kvm_info.type = GIC_V3;
+
+	gic_v3_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
+
+	if (of_property_read_u32(node, "#redistributor-regions",
+				 &gicv_idx))
+		gicv_idx = 1;
+
+	gicv_idx += 3;	/* Also skip GICD, GICC, GICH */
+	ret = of_address_to_resource(node, gicv_idx, &r);
+	if (!ret) {
+		if (!PAGE_ALIGNED(r.start))
+			pr_warn("GICV physical address 0x%llx not page aligned\n",
+				(unsigned long long)r.start);
+		else if (!PAGE_ALIGNED(resource_size(&r)))
+			pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
+				(unsigned long long)resource_size(&r),
+				PAGE_SIZE);
+		else
+			gic_v3_kvm_info.vcpu = r;
+	}
+
+	gic_set_kvm_info(&gic_v3_kvm_info);
+}
+
 static int __init gic_of_init(struct device_node *node, struct device_node *parent)
 {
 	void __iomem *dist_base;
@@ -952,8 +986,10 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 
 	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
 			     redist_stride, &node->fwnode);
-	if (!err)
+	if (!err) {
+		gic_of_setup_kvm_info(node);
 		return 0;
+	}
 
 out_unmap_rdist:
 	for (i = 0; i < nr_redist_regions; i++)
@@ -974,6 +1010,10 @@ static struct
 	struct redist_region *redist_regs;
 	u32 nr_redist_regions;
 	bool single_redist;
+	u32 maint_irq;
+	int maint_irq_mode;
+	phys_addr_t vctrl_base;
+	phys_addr_t vcpu_base;
 } acpi_data __initdata;
 
 static void __init
@@ -1020,6 +1060,13 @@ gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header,
 		return -ENOMEM;
 
 	gic_acpi_register_redist(gicc->gicr_base_address, redist_base);
+
+	acpi_data.maint_irq = gicc->vgic_interrupt;
+	acpi_data.maint_irq_mode = (gicc->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
+				    ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
+	acpi_data.vctrl_base = gicc->gich_base_address;
+	acpi_data.vcpu_base = gicc->gicv_base_address;
+
 	return 0;
 }
 
@@ -1111,6 +1158,40 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header,
 }
 
 #define ACPI_GICV3_DIST_MEM_SIZE (SZ_64K)
+#define ACPI_GICV2_VCTRL_MEM_SIZE	(SZ_4K)
+#define ACPI_GICV2_VCPU_MEM_SIZE	(SZ_8K)
+
+static void __init gic_acpi_setup_kvm_info(void)
+{
+	int irq;
+
+	gic_v3_kvm_info.type = GIC_V3;
+
+	irq = acpi_register_gsi(NULL, acpi_data.maint_irq,
+				acpi_data.maint_irq_mode,
+				ACPI_ACTIVE_HIGH);
+	if (irq > 0)
+		gic_v3_kvm_info.maint_irq = irq;
+
+	if (acpi_data.vctrl_base) {
+		struct resource *vctrl = &gic_v3_kvm_info.vctrl;
+
+		vctrl->flags = IORESOURCE_MEM;
+		vctrl->start = acpi_data.vctrl_base;
+		vctrl->end = vctrl->start + ACPI_GICV2_VCTRL_MEM_SIZE - 1;
+	}
+
+	if (acpi_data.vcpu_base) {
+		struct resource *vcpu = &gic_v3_kvm_info.vcpu;
+
+		vcpu->flags = IORESOURCE_MEM;
+		vcpu->start = acpi_data.vcpu_base;
+		vcpu->end = vcpu->start + ACPI_GICV2_VCPU_MEM_SIZE - 1;
+	}
+
+	gic_set_kvm_info(&gic_v3_kvm_info);
+}
+
 
 static int __init
 gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
@@ -1159,6 +1240,8 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
 		goto out_fwhandle_free;
 
 	acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
+	gic_acpi_setup_kvm_info();
+
 	return 0;
 
 out_fwhandle_free:
diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
index f7723b9..15d68c6 100644
--- a/include/linux/irqchip/arm-gic-common.h
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -15,6 +15,7 @@
 
 enum gic_type {
 	GIC_V2,
+	GIC_V3,
 };
 
 struct gic_kvm_info {
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1353837 — Re: [PATCH v3 6/9] irqchip/gic-v3: Parse and export virtual GIC information

On Tue, Mar 08, 2016 at 11:29:30AM +0000, Julien Grall wrote:
> Fill up the recently introduced gic_kvm_info with the virtual GIC
> information.

this is not really virtual GIC information, it's information about the
hardware used for virtualization.

> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
> ---
>     Changes in v3:
>         - Add ACPI support
> 
>     Changes in v2:
>         - Use 0 rather than a negative value to know when the maintenance IRQ
>         is not present.
>         - Use resource for vcpu and vctrl
> ---
>  drivers/irqchip/irq-gic-v3.c           | 85 +++++++++++++++++++++++++++++++++-
>  include/linux/irqchip/arm-gic-common.h |  1 +
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 50e87e6..6ae25120 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -28,6 +28,7 @@
>  #include <linux/slab.h>
>  
>  #include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic-common.h>
>  #include <linux/irqchip/arm-gic-v3.h>
>  
>  #include <asm/cputype.h>
> @@ -56,6 +57,8 @@ struct gic_chip_data {
>  static struct gic_chip_data gic_data __read_mostly;
>  static struct static_key supports_deactivate = STATIC_KEY_INIT_TRUE;
>  
> +static struct gic_kvm_info gic_v3_kvm_info;
> +
>  #define gic_data_rdist()		(this_cpu_ptr(gic_data.rdists.rdist))
>  #define gic_data_rdist_rd_base()	(gic_data_rdist()->rd_base)
>  #define gic_data_rdist_sgi_base()	(gic_data_rdist_rd_base() + SZ_64K)
> @@ -901,6 +904,37 @@ static int __init gic_validate_dist_version(void __iomem *dist_base)
>  	return 0;
>  }
>  
> +static void __init gic_of_setup_kvm_info(struct device_node *node)
> +{
> +	int ret;
> +	struct resource r;
> +	u32 gicv_idx;
> +
> +	gic_v3_kvm_info.type = GIC_V3;
> +
> +	gic_v3_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
> +
> +	if (of_property_read_u32(node, "#redistributor-regions",
> +				 &gicv_idx))
> +		gicv_idx = 1;
> +
> +	gicv_idx += 3;	/* Also skip GICD, GICC, GICH */
> +	ret = of_address_to_resource(node, gicv_idx, &r);
> +	if (!ret) {
> +		if (!PAGE_ALIGNED(r.start))
> +			pr_warn("GICV physical address 0x%llx not page aligned\n",
> +				(unsigned long long)r.start);
> +		else if (!PAGE_ALIGNED(resource_size(&r)))
> +			pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> +				(unsigned long long)resource_size(&r),
> +				PAGE_SIZE);
> +		else
> +			gic_v3_kvm_info.vcpu = r;
> +	}
> +
> +	gic_set_kvm_info(&gic_v3_kvm_info);

I have the same error handling concerns as the previous patch here.

> +}
> +
>  static int __init gic_of_init(struct device_node *node, struct device_node *parent)
>  {
>  	void __iomem *dist_base;
> @@ -952,8 +986,10 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
>  
>  	err = gic_init_bases(dist_base, rdist_regs, nr_redist_regions,
>  			     redist_stride, &node->fwnode);
> -	if (!err)
> +	if (!err) {
> +		gic_of_setup_kvm_info(node);
>  		return 0;
> +	}
>  
>  out_unmap_rdist:
>  	for (i = 0; i < nr_redist_regions; i++)
> @@ -974,6 +1010,10 @@ static struct
>  	struct redist_region *redist_regs;
>  	u32 nr_redist_regions;
>  	bool single_redist;
> +	u32 maint_irq;
> +	int maint_irq_mode;
> +	phys_addr_t vctrl_base;
> +	phys_addr_t vcpu_base;
>  } acpi_data __initdata;
>  
>  static void __init
> @@ -1020,6 +1060,13 @@ gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header,
>  		return -ENOMEM;
>  
>  	gic_acpi_register_redist(gicc->gicr_base_address, redist_base);
> +
> +	acpi_data.maint_irq = gicc->vgic_interrupt;
> +	acpi_data.maint_irq_mode = (gicc->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
> +				    ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> +	acpi_data.vctrl_base = gicc->gich_base_address;
> +	acpi_data.vcpu_base = gicc->gicv_base_address;
> +
>  	return 0;
>  }
>  
> @@ -1111,6 +1158,40 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header,
>  }
>  
>  #define ACPI_GICV3_DIST_MEM_SIZE (SZ_64K)
> +#define ACPI_GICV2_VCTRL_MEM_SIZE	(SZ_4K)
> +#define ACPI_GICV2_VCPU_MEM_SIZE	(SZ_8K)
> +
> +static void __init gic_acpi_setup_kvm_info(void)
> +{
> +	int irq;
> +
> +	gic_v3_kvm_info.type = GIC_V3;
> +
> +	irq = acpi_register_gsi(NULL, acpi_data.maint_irq,
> +				acpi_data.maint_irq_mode,
> +				ACPI_ACTIVE_HIGH);
> +	if (irq > 0)
> +		gic_v3_kvm_info.maint_irq = irq;
> +
> +	if (acpi_data.vctrl_base) {
> +		struct resource *vctrl = &gic_v3_kvm_info.vctrl;
> +
> +		vctrl->flags = IORESOURCE_MEM;
> +		vctrl->start = acpi_data.vctrl_base;
> +		vctrl->end = vctrl->start + ACPI_GICV2_VCTRL_MEM_SIZE - 1;
> +	}
> +
> +	if (acpi_data.vcpu_base) {
> +		struct resource *vcpu = &gic_v3_kvm_info.vcpu;
> +
> +		vcpu->flags = IORESOURCE_MEM;
> +		vcpu->start = acpi_data.vcpu_base;
> +		vcpu->end = vcpu->start + ACPI_GICV2_VCPU_MEM_SIZE - 1;
> +	}
> +
> +	gic_set_kvm_info(&gic_v3_kvm_info);

and here.

> +}
> +
>  
>  static int __init
>  gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> @@ -1159,6 +1240,8 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
>  		goto out_fwhandle_free;
>  
>  	acpi_set_irq_model(ACPI_IRQ_MODEL_GIC, domain_handle);
> +	gic_acpi_setup_kvm_info();
> +
>  	return 0;
>  
>  out_fwhandle_free:
> diff --git a/include/linux/irqchip/arm-gic-common.h b/include/linux/irqchip/arm-gic-common.h
> index f7723b9..15d68c6 100644
> --- a/include/linux/irqchip/arm-gic-common.h
> +++ b/include/linux/irqchip/arm-gic-common.h
> @@ -15,6 +15,7 @@
>  
>  enum gic_type {
>  	GIC_V2,
> +	GIC_V3,
>  };
>  
>  struct gic_kvm_info {
> -- 
> 1.9.1
> 

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1352922 — [PATCH v3 7/9] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

The firmware table is currently parsed by the virtual timer code in
order to retrieve the virtual timer interrupt. However, this is already
done by the arch timer driver.

To avoid code duplication, use the newly function arch_timer_get_kvm_info()
which return all the information required by the virtual timer code.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>

    Changes in v3:
        - Patch added
---
 virt/kvm/arm/arch_timer.c | 40 +++++++++++-----------------------------
 1 file changed, 11 insertions(+), 29 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index a9ad4fe..c6a6f8e 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -17,7 +17,6 @@
  */
 
 #include <linux/cpu.h>
-#include <linux/of_irq.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
@@ -409,45 +408,29 @@ static struct notifier_block kvm_timer_cpu_nb = {
 	.notifier_call = kvm_timer_cpu_notify,
 };
 
-static const struct of_device_id arch_timer_of_match[] = {
-	{ .compatible	= "arm,armv7-timer",	},
-	{ .compatible	= "arm,armv8-timer",	},
-	{},
-};
-
 int kvm_timer_hyp_init(void)
 {
-	struct device_node *np;
-	unsigned int ppi;
+	struct arch_timer_kvm_info *info;
 	int err;
 
-	timecounter = arch_timer_get_timecounter();
-	if (!timecounter)
-		return -ENODEV;
+	info = arch_timer_get_kvm_info();
+	timecounter = &info->timecounter;
 
-	np = of_find_matching_node(NULL, arch_timer_of_match);
-	if (!np) {
-		kvm_err("kvm_arch_timer: can't find DT node\n");
+	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;
 
-	ppi = irq_of_parse_and_map(np, 2);
-	if (!ppi) {
-		kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
-		err = -EINVAL;
-		goto out;
-	}
-
-	err = request_percpu_irq(ppi, kvm_arch_timer_handler,
+	err = request_percpu_irq(host_vtimer_irq, kvm_arch_timer_handler,
 				 "kvm guest timer", kvm_get_running_vcpus());
 	if (err) {
 		kvm_err("kvm_arch_timer: can't request interrupt %d (%d)\n",
-			ppi, err);
+			host_vtimer_irq, err);
 		goto out;
 	}
 
-	host_vtimer_irq = ppi;
-
 	err = __register_cpu_notifier(&kvm_timer_cpu_nb);
 	if (err) {
 		kvm_err("Cannot register timer CPU notifier\n");
@@ -460,14 +443,13 @@ int kvm_timer_hyp_init(void)
 		goto out_free;
 	}
 
-	kvm_info("%s IRQ%d\n", np->name, ppi);
+	kvm_info("virtual timer IRQ%d\n", host_vtimer_irq);
 	on_each_cpu(kvm_timer_init_interrupt, NULL, 1);
 
 	goto out;
 out_free:
-	free_percpu_irq(ppi, kvm_get_running_vcpus());
+	free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus());
 out:
-	of_node_put(np);
 	return err;
 }
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1353838 — Re: [PATCH v3 7/9] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

On Tue, Mar 08, 2016 at 11:29:31AM +0000, Julien Grall wrote:
> The firmware table is currently parsed by the virtual timer code in
> order to retrieve the virtual timer interrupt. However, this is already
> done by the arch timer driver.
> 
> To avoid code duplication, use the newly function arch_timer_get_kvm_info()
> which return all the information required by the virtual timer code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web