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

Groups > linux.kernel > #1364377 > unrolled thread

[PATCH v4 0/9] arm64: Add support for KVM with ACPI

Started byJulien Grall <julien.grall@arm.com>
First post2016-03-24 19:00 +0100
Last post2016-03-30 12:00 +0200
Articles 10 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/9] arm64: Add support for KVM with ACPI Julien Grall <julien.grall@arm.com> - 2016-03-24 19:00 +0100
    [PATCH v4 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-24 19:00 +0100
    [PATCH v4 8/9] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables Julien Grall <julien.grall@arm.com> - 2016-03-24 19:00 +0100
      Re: [PATCH v4 8/9] KVM: arm/arm64: vgic: Rely on the GIC driver to  parse the firmware tables Christoffer Dall <christoffer.dall@linaro.org> - 2016-04-01 12:40 +0200
    [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure Julien Grall <julien.grall@arm.com> - 2016-03-24 19:00 +0100
      Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific  information in a structure Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-03-29 19:20 +0200
        Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific  information in a structure Marc Zyngier <marc.zyngier@arm.com> - 2016-03-29 19:40 +0200
          Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific  information in a structure Christoffer Dall <christoffer.dall@linaro.org> - 2016-03-30 11:10 +0200
            Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific  information in a structure Marc Zyngier <marc.zyngier@arm.com> - 2016-03-30 11:20 +0200
              Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific  information in a structure Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-03-30 12:00 +0200

#1364377 — [PATCH v4 0/9] arm64: Add support for KVM with ACPI

FromJulien Grall <julien.grall@arm.com>
Date2016-03-24 19:00 +0100
Subject[PATCH v4 0/9] arm64: Add support for KVM with ACPI
Message-ID<rghI5-6aA-3@gated-at.bofh.it>
Hello,

This patch series allows KVM to work with ACPI on ARM64.

Currently, the firmware tables are parsed by the virtual timer and virtual GIC
code in order to configure them correctly.

However, the parsing of those tables is already done in the GIC and arch
timer drivers. This patch series introduces new helpers to retrieve the
information from the different drivers in order to avoid duplication of the
parsing code.

To make the merge easier via the different trees, each patch modifies
a single subsystem.

For all the changes see the different patches.

Yours sincerely,

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           | 183 ++++++++++++++++++++++++++++-----
 drivers/irqchip/irq-gic.c              |  87 +++++++++++++++-
 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                 |  61 +++++------
 virt/kvm/arm/vgic-v3.c                 |  47 +++------
 virt/kvm/arm/vgic.c                    |  50 ++++-----
 12 files changed, 384 insertions(+), 164 deletions(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

-- 
1.9.1

[toc] | [next] | [standalone]

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

FromJulien Grall <julien.grall@arm.com>
Date2016-03-24 19:00 +0100
Subject[PATCH v4 7/9] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables
Message-ID<rghI8-6aA-31@gated-at.bofh.it>
In reply to#1364377
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>

---
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 v4:
        - Add Christoffer's acked-by

    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]

#1364381 — [PATCH v4 8/9] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables

FromJulien Grall <julien.grall@arm.com>
Date2016-03-24 19:00 +0100
Subject[PATCH v4 8/9] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables
Message-ID<rghI7-6aA-25@gated-at.bofh.it>
In reply to#1364377
Currently, the firmware tables are parsed 2 times: once in the GIC
drivers, the other time when initializing the vGIC. It means code
duplication and make more tedious to add the support for another
firmware table (like ACPI).

Use the recently introduced helper gic_get_kvm_info() to get
information about the virtual GIC.

With this change, the virtual GIC becomes agnostic to the firmware
table and KVM will be able to initialize the vGIC on ACPI.

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 v4:
        - Remove validation check as they are already done during
        parsing.
        - Move the alignement check from the parsing to the vGIC code.
        - Fix typo in the commit message

    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.
---
 include/kvm/arm_vgic.h |  7 +++---
 virt/kvm/arm/vgic-v2.c | 61 +++++++++++++++++++++-----------------------------
 virt/kvm/arm/vgic-v3.c | 47 +++++++++++++-------------------------
 virt/kvm/arm/vgic.c    | 50 ++++++++++++++++++++++-------------------
 4 files changed, 73 insertions(+), 92 deletions(-)

diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 281caf8..be6037a 100644
--- a/include/kvm/arm_vgic.h
+++ b/include/kvm/arm_vgic.h
@@ -25,6 +25,7 @@
 #include <linux/spinlock.h>
 #include <linux/types.h>
 #include <kvm/iodev.h>
+#include <linux/irqchip/arm-gic-common.h>
 
 #define VGIC_NR_IRQS_LEGACY	256
 #define VGIC_NR_SGIS		16
@@ -353,15 +354,15 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
 #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
 #define vgic_ready(k)		((k)->arch.vgic.ready)
 
-int vgic_v2_probe(struct device_node *vgic_node,
+int vgic_v2_probe(const struct gic_kvm_info *gic_kvm_info,
 		  const struct vgic_ops **ops,
 		  const struct vgic_params **params);
 #ifdef CONFIG_KVM_ARM_VGIC_V3
-int vgic_v3_probe(struct device_node *vgic_node,
+int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
 		  const struct vgic_ops **ops,
 		  const struct vgic_params **params);
 #else
-static inline int vgic_v3_probe(struct device_node *vgic_node,
+static inline int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
 				const struct vgic_ops **ops,
 				const struct vgic_params **params)
 {
diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
index 67ec334..7e826c9 100644
--- a/virt/kvm/arm/vgic-v2.c
+++ b/virt/kvm/arm/vgic-v2.c
@@ -20,9 +20,6 @@
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
 
 #include <linux/irqchip/arm-gic.h>
 
@@ -186,38 +183,39 @@ static void vgic_cpu_init_lrs(void *params)
 }
 
 /**
- * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
- * @node:	pointer to the DT node
- * @ops: 	address of a pointer to the GICv2 operations
- * @params:	address of a pointer to HW-specific parameters
+ * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
+ * @gic_kvm_info:	pointer to the GIC description
+ * @ops:		address of a pointer to the GICv2 operations
+ * @params:		address of a pointer to HW-specific parameters
  *
  * Returns 0 if a GICv2 has been found, with the low level operations
  * in *ops and the HW parameters in *params. Returns an error code
  * otherwise.
  */
-int vgic_v2_probe(struct device_node *vgic_node,
-		  const struct vgic_ops **ops,
-		  const struct vgic_params **params)
+int vgic_v2_probe(const struct gic_kvm_info *gic_kvm_info,
+		   const struct vgic_ops **ops,
+		   const struct vgic_params **params)
 {
 	int ret;
-	struct resource vctrl_res;
-	struct resource vcpu_res;
 	struct vgic_params *vgic = &vgic_v2_params;
+	const struct resource *vctrl_res = &gic_kvm_info->vctrl;
+	const struct resource *vcpu_res = &gic_kvm_info->vcpu;
 
-	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
-	if (!vgic->maint_irq) {
-		kvm_err("error getting vgic maintenance irq from DT\n");
+	if (!gic_kvm_info->maint_irq) {
+		kvm_err("error getting vgic maintenance irq\n");
 		ret = -ENXIO;
 		goto out;
 	}
+	vgic->maint_irq = gic_kvm_info->maint_irq;
 
-	ret = of_address_to_resource(vgic_node, 2, &vctrl_res);
-	if (ret) {
-		kvm_err("Cannot obtain GICH resource\n");
+	if (!gic_kvm_info->vctrl.start) {
+		kvm_err("GICH not present in the firmware table\n");
+		ret = -ENXIO;
 		goto out;
 	}
 
-	vgic->vctrl_base = of_iomap(vgic_node, 2);
+	vgic->vctrl_base = ioremap(gic_kvm_info->vctrl.start,
+				   resource_size(&gic_kvm_info->vctrl));
 	if (!vgic->vctrl_base) {
 		kvm_err("Cannot ioremap GICH\n");
 		ret = -ENOMEM;
@@ -228,29 +226,23 @@ int vgic_v2_probe(struct device_node *vgic_node,
 	vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
 
 	ret = create_hyp_io_mappings(vgic->vctrl_base,
-				     vgic->vctrl_base + resource_size(&vctrl_res),
-				     vctrl_res.start);
+				     vgic->vctrl_base + resource_size(vctrl_res),
+				     vctrl_res->start);
 	if (ret) {
 		kvm_err("Cannot map VCTRL into hyp\n");
 		goto out_unmap;
 	}
 
-	if (of_address_to_resource(vgic_node, 3, &vcpu_res)) {
-		kvm_err("Cannot obtain GICV resource\n");
-		ret = -ENXIO;
-		goto out_unmap;
-	}
-
-	if (!PAGE_ALIGNED(vcpu_res.start)) {
+	if (!PAGE_ALIGNED(vcpu_res->start)) {
 		kvm_err("GICV physical address 0x%llx not page aligned\n",
-			(unsigned long long)vcpu_res.start);
+			(unsigned long long)vcpu_res->start);
 		ret = -ENXIO;
 		goto out_unmap;
 	}
 
-	if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
+	if (!PAGE_ALIGNED(resource_size(vcpu_res))) {
 		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
-			(unsigned long long)resource_size(&vcpu_res),
+			(unsigned long long)resource_size(vcpu_res),
 			PAGE_SIZE);
 		ret = -ENXIO;
 		goto out_unmap;
@@ -259,10 +251,10 @@ int vgic_v2_probe(struct device_node *vgic_node,
 	vgic->can_emulate_gicv2 = true;
 	kvm_register_device_ops(&kvm_arm_vgic_v2_ops, KVM_DEV_TYPE_ARM_VGIC_V2);
 
-	vgic->vcpu_base = vcpu_res.start;
+	vgic->vcpu_base = vcpu_res->start;
 
-	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
-		 vctrl_res.start, vgic->maint_irq);
+	kvm_info("GICH base=0x%llx, GICV base=0x%llx, IRQ=%d\n",
+		 gic_kvm_info->vctrl.start, vgic->vcpu_base, vgic->maint_irq);
 
 	vgic->type = VGIC_V2;
 	vgic->max_gic_vcpus = VGIC_V2_MAX_CPUS;
@@ -276,6 +268,5 @@ int vgic_v2_probe(struct device_node *vgic_node,
 out_unmap:
 	iounmap(vgic->vctrl_base);
 out:
-	of_node_put(vgic_node);
 	return ret;
 }
diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
index 999bdc6..c02a1b1 100644
--- a/virt/kvm/arm/vgic-v3.c
+++ b/virt/kvm/arm/vgic-v3.c
@@ -20,11 +20,9 @@
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
 
 #include <linux/irqchip/arm-gic-v3.h>
+#include <linux/irqchip/arm-gic-common.h>
 
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_arm.h>
@@ -222,30 +220,24 @@ static void vgic_cpu_init_lrs(void *params)
 }
 
 /**
- * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
- * @node:	pointer to the DT node
- * @ops: 	address of a pointer to the GICv3 operations
- * @params:	address of a pointer to HW-specific parameters
+ * vgic_v3_probe - probe for a GICv3 compatible interrupt controller
+ * @gic_kvm_info:	pointer to the GIC description
+ * @ops:		address of a pointer to the GICv3 operations
+ * @params:		address of a pointer to HW-specific parameters
  *
  * Returns 0 if a GICv3 has been found, with the low level operations
  * in *ops and the HW parameters in *params. Returns an error code
  * otherwise.
  */
-int vgic_v3_probe(struct device_node *vgic_node,
+int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
 		  const struct vgic_ops **ops,
 		  const struct vgic_params **params)
 {
 	int ret = 0;
-	u32 gicv_idx;
-	struct resource vcpu_res;
 	struct vgic_params *vgic = &vgic_v3_params;
+	const struct resource *vcpu_res = &gic_kvm_info->vcpu;
 
-	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
-	if (!vgic->maint_irq) {
-		kvm_err("error getting vgic maintenance irq from DT\n");
-		ret = -ENXIO;
-		goto out;
-	}
+	vgic->maint_irq = gic_kvm_info->maint_irq;
 
 	ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
 
@@ -256,24 +248,19 @@ int vgic_v3_probe(struct device_node *vgic_node,
 	vgic->nr_lr = (ich_vtr_el2 & 0xf) + 1;
 	vgic->can_emulate_gicv2 = false;
 
-	if (of_property_read_u32(vgic_node, "#redistributor-regions", &gicv_idx))
-		gicv_idx = 1;
-
-	gicv_idx += 3; /* Also skip GICD, GICC, GICH */
-	if (of_address_to_resource(vgic_node, gicv_idx, &vcpu_res)) {
+	if (!vcpu_res->start) {
 		kvm_info("GICv3: no GICV resource entry\n");
 		vgic->vcpu_base = 0;
-	} else if (!PAGE_ALIGNED(vcpu_res.start)) {
+	} else if (!PAGE_ALIGNED(vcpu_res->start)) {
 		pr_warn("GICV physical address 0x%llx not page aligned\n",
-			(unsigned long long)vcpu_res.start);
+			(unsigned long long)vcpu_res->start);
 		vgic->vcpu_base = 0;
-	} else if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
+	} else if (!PAGE_ALIGNED(resource_size(vcpu_res))) {
 		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
-			(unsigned long long)resource_size(&vcpu_res),
+			(unsigned long long)resource_size(vcpu_res),
 			PAGE_SIZE);
-		vgic->vcpu_base = 0;
 	} else {
-		vgic->vcpu_base = vcpu_res.start;
+		vgic->vcpu_base = vcpu_res->start;
 		vgic->can_emulate_gicv2 = true;
 		kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
 					KVM_DEV_TYPE_ARM_VGIC_V2);
@@ -286,15 +273,13 @@ int vgic_v3_probe(struct device_node *vgic_node,
 	vgic->type = VGIC_V3;
 	vgic->max_gic_vcpus = VGIC_V3_MAX_CPUS;
 
-	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
-		 vcpu_res.start, vgic->maint_irq);
+	kvm_info("GICV base=0x%llx, IRQ=%d\n",
+		 vgic->vcpu_base, vgic->maint_irq);
 
 	on_each_cpu(vgic_cpu_init_lrs, vgic, 1);
 
 	*ops = &vgic_v3_ops;
 	*params = vgic;
 
-out:
-	of_node_put(vgic_node);
 	return ret;
 }
diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 00429b3..60668a7 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -21,9 +21,7 @@
 #include <linux/kvm_host.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
+#include <linux/irq.h>
 #include <linux/rculist.h>
 #include <linux/uaccess.h>
 
@@ -33,6 +31,7 @@
 #include <trace/events/kvm.h>
 #include <asm/kvm.h>
 #include <kvm/iodev.h>
+#include <linux/irqchip/arm-gic-common.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace.h"
@@ -2389,33 +2388,38 @@ static struct notifier_block vgic_cpu_nb = {
 	.notifier_call = vgic_cpu_notify,
 };
 
-static const struct of_device_id vgic_ids[] = {
-	{ .compatible = "arm,cortex-a15-gic",	.data = vgic_v2_probe, },
-	{ .compatible = "arm,cortex-a7-gic",	.data = vgic_v2_probe, },
-	{ .compatible = "arm,gic-400",		.data = vgic_v2_probe, },
-	{ .compatible = "arm,gic-v3",		.data = vgic_v3_probe, },
-	{},
-};
-
-int kvm_vgic_hyp_init(void)
+static int kvm_vgic_probe(void)
 {
-	const struct of_device_id *matched_id;
-	const int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
-				const struct vgic_params **);
-	struct device_node *vgic_node;
+	const struct gic_kvm_info *gic_kvm_info;
 	int ret;
 
-	vgic_node = of_find_matching_node_and_match(NULL,
-						    vgic_ids, &matched_id);
-	if (!vgic_node) {
-		kvm_err("error: no compatible GIC node found\n");
+	gic_kvm_info = gic_get_kvm_info();
+	if (!gic_kvm_info)
 		return -ENODEV;
+
+	switch (gic_kvm_info->type) {
+	case GIC_V2:
+		ret = vgic_v2_probe(gic_kvm_info, &vgic_ops, &vgic);
+		break;
+	case GIC_V3:
+		ret = vgic_v3_probe(gic_kvm_info, &vgic_ops, &vgic);
+		break;
+	default:
+		ret = -ENODEV;
 	}
 
-	vgic_probe = matched_id->data;
-	ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
-	if (ret)
+	return ret;
+}
+
+int kvm_vgic_hyp_init(void)
+{
+	int ret;
+
+	ret = kvm_vgic_probe();
+	if (ret) {
+		kvm_err("error: KVM vGIC probing failed\n");
 		return ret;
+	}
 
 	ret = request_percpu_irq(vgic->maint_irq, vgic_maintenance_handler,
 				 "vgic", kvm_get_running_vcpus());
-- 
1.9.1

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

#1369173 — Re: [PATCH v4 8/9] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-04-01 12:40 +0200
SubjectRe: [PATCH v4 8/9] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables
Message-ID<rj4EG-2X4-9@gated-at.bofh.it>
In reply to#1364381
On Thu, Mar 24, 2016 at 05:53:42PM +0000, Julien Grall wrote:
> Currently, the firmware tables are parsed 2 times: once in the GIC
> drivers, the other time when initializing the vGIC. It means code
> duplication and make more tedious to add the support for another
> firmware table (like ACPI).
> 
> Use the recently introduced helper gic_get_kvm_info() to get
> information about the virtual GIC.
> 
> With this change, the virtual GIC becomes agnostic to the firmware
> table and KVM will be able to initialize the vGIC on ACPI.
> 
> 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 v4:
>         - Remove validation check as they are already done during
>         parsing.
>         - Move the alignement check from the parsing to the vGIC code.
>         - Fix typo in the commit message
> 
>     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.
> ---
>  include/kvm/arm_vgic.h |  7 +++---
>  virt/kvm/arm/vgic-v2.c | 61 +++++++++++++++++++++-----------------------------
>  virt/kvm/arm/vgic-v3.c | 47 +++++++++++++-------------------------
>  virt/kvm/arm/vgic.c    | 50 ++++++++++++++++++++++-------------------
>  4 files changed, 73 insertions(+), 92 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 281caf8..be6037a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -25,6 +25,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
>  #include <kvm/iodev.h>
> +#include <linux/irqchip/arm-gic-common.h>
>  
>  #define VGIC_NR_IRQS_LEGACY	256
>  #define VGIC_NR_SGIS		16
> @@ -353,15 +354,15 @@ bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
>  #define vgic_initialized(k)	(!!((k)->arch.vgic.nr_cpus))
>  #define vgic_ready(k)		((k)->arch.vgic.ready)
>  
> -int vgic_v2_probe(struct device_node *vgic_node,
> +int vgic_v2_probe(const struct gic_kvm_info *gic_kvm_info,
>  		  const struct vgic_ops **ops,
>  		  const struct vgic_params **params);
>  #ifdef CONFIG_KVM_ARM_VGIC_V3
> -int vgic_v3_probe(struct device_node *vgic_node,
> +int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
>  		  const struct vgic_ops **ops,
>  		  const struct vgic_params **params);
>  #else
> -static inline int vgic_v3_probe(struct device_node *vgic_node,
> +static inline int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
>  				const struct vgic_ops **ops,
>  				const struct vgic_params **params)
>  {
> diff --git a/virt/kvm/arm/vgic-v2.c b/virt/kvm/arm/vgic-v2.c
> index 67ec334..7e826c9 100644
> --- a/virt/kvm/arm/vgic-v2.c
> +++ b/virt/kvm/arm/vgic-v2.c
> @@ -20,9 +20,6 @@
>  #include <linux/kvm_host.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/of_irq.h>
>  
>  #include <linux/irqchip/arm-gic.h>
>  
> @@ -186,38 +183,39 @@ static void vgic_cpu_init_lrs(void *params)
>  }
>  
>  /**
> - * vgic_v2_probe - probe for a GICv2 compatible interrupt controller in DT
> - * @node:	pointer to the DT node
> - * @ops: 	address of a pointer to the GICv2 operations
> - * @params:	address of a pointer to HW-specific parameters
> + * vgic_v2_probe - probe for a GICv2 compatible interrupt controller
> + * @gic_kvm_info:	pointer to the GIC description
> + * @ops:		address of a pointer to the GICv2 operations
> + * @params:		address of a pointer to HW-specific parameters
>   *
>   * Returns 0 if a GICv2 has been found, with the low level operations
>   * in *ops and the HW parameters in *params. Returns an error code
>   * otherwise.
>   */
> -int vgic_v2_probe(struct device_node *vgic_node,
> -		  const struct vgic_ops **ops,
> -		  const struct vgic_params **params)
> +int vgic_v2_probe(const struct gic_kvm_info *gic_kvm_info,
> +		   const struct vgic_ops **ops,
> +		   const struct vgic_params **params)
>  {
>  	int ret;
> -	struct resource vctrl_res;
> -	struct resource vcpu_res;
>  	struct vgic_params *vgic = &vgic_v2_params;
> +	const struct resource *vctrl_res = &gic_kvm_info->vctrl;
> +	const struct resource *vcpu_res = &gic_kvm_info->vcpu;
>  
> -	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
> -	if (!vgic->maint_irq) {
> -		kvm_err("error getting vgic maintenance irq from DT\n");
> +	if (!gic_kvm_info->maint_irq) {
> +		kvm_err("error getting vgic maintenance irq\n");
>  		ret = -ENXIO;
>  		goto out;
>  	}
> +	vgic->maint_irq = gic_kvm_info->maint_irq;
>  
> -	ret = of_address_to_resource(vgic_node, 2, &vctrl_res);
> -	if (ret) {
> -		kvm_err("Cannot obtain GICH resource\n");
> +	if (!gic_kvm_info->vctrl.start) {
> +		kvm_err("GICH not present in the firmware table\n");
> +		ret = -ENXIO;
>  		goto out;
>  	}
>  
> -	vgic->vctrl_base = of_iomap(vgic_node, 2);
> +	vgic->vctrl_base = ioremap(gic_kvm_info->vctrl.start,
> +				   resource_size(&gic_kvm_info->vctrl));
>  	if (!vgic->vctrl_base) {
>  		kvm_err("Cannot ioremap GICH\n");
>  		ret = -ENOMEM;
> @@ -228,29 +226,23 @@ int vgic_v2_probe(struct device_node *vgic_node,
>  	vgic->nr_lr = (vgic->nr_lr & 0x3f) + 1;
>  
>  	ret = create_hyp_io_mappings(vgic->vctrl_base,
> -				     vgic->vctrl_base + resource_size(&vctrl_res),
> -				     vctrl_res.start);
> +				     vgic->vctrl_base + resource_size(vctrl_res),
> +				     vctrl_res->start);
>  	if (ret) {
>  		kvm_err("Cannot map VCTRL into hyp\n");
>  		goto out_unmap;
>  	}
>  
> -	if (of_address_to_resource(vgic_node, 3, &vcpu_res)) {
> -		kvm_err("Cannot obtain GICV resource\n");
> -		ret = -ENXIO;
> -		goto out_unmap;
> -	}
> -
> -	if (!PAGE_ALIGNED(vcpu_res.start)) {
> +	if (!PAGE_ALIGNED(vcpu_res->start)) {
>  		kvm_err("GICV physical address 0x%llx not page aligned\n",
> -			(unsigned long long)vcpu_res.start);
> +			(unsigned long long)vcpu_res->start);
>  		ret = -ENXIO;
>  		goto out_unmap;
>  	}
>  
> -	if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
> +	if (!PAGE_ALIGNED(resource_size(vcpu_res))) {
>  		kvm_err("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> -			(unsigned long long)resource_size(&vcpu_res),
> +			(unsigned long long)resource_size(vcpu_res),
>  			PAGE_SIZE);
>  		ret = -ENXIO;
>  		goto out_unmap;
> @@ -259,10 +251,10 @@ int vgic_v2_probe(struct device_node *vgic_node,
>  	vgic->can_emulate_gicv2 = true;
>  	kvm_register_device_ops(&kvm_arm_vgic_v2_ops, KVM_DEV_TYPE_ARM_VGIC_V2);
>  
> -	vgic->vcpu_base = vcpu_res.start;
> +	vgic->vcpu_base = vcpu_res->start;
>  
> -	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
> -		 vctrl_res.start, vgic->maint_irq);
> +	kvm_info("GICH base=0x%llx, GICV base=0x%llx, IRQ=%d\n",
> +		 gic_kvm_info->vctrl.start, vgic->vcpu_base, vgic->maint_irq);
>  
>  	vgic->type = VGIC_V2;
>  	vgic->max_gic_vcpus = VGIC_V2_MAX_CPUS;
> @@ -276,6 +268,5 @@ int vgic_v2_probe(struct device_node *vgic_node,
>  out_unmap:
>  	iounmap(vgic->vctrl_base);
>  out:
> -	of_node_put(vgic_node);
>  	return ret;
>  }
> diff --git a/virt/kvm/arm/vgic-v3.c b/virt/kvm/arm/vgic-v3.c
> index 999bdc6..c02a1b1 100644
> --- a/virt/kvm/arm/vgic-v3.c
> +++ b/virt/kvm/arm/vgic-v3.c
> @@ -20,11 +20,9 @@
>  #include <linux/kvm_host.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/of_irq.h>
>  
>  #include <linux/irqchip/arm-gic-v3.h>
> +#include <linux/irqchip/arm-gic-common.h>
>  
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_arm.h>
> @@ -222,30 +220,24 @@ static void vgic_cpu_init_lrs(void *params)
>  }
>  
>  /**
> - * vgic_v3_probe - probe for a GICv3 compatible interrupt controller in DT
> - * @node:	pointer to the DT node
> - * @ops: 	address of a pointer to the GICv3 operations
> - * @params:	address of a pointer to HW-specific parameters
> + * vgic_v3_probe - probe for a GICv3 compatible interrupt controller
> + * @gic_kvm_info:	pointer to the GIC description
> + * @ops:		address of a pointer to the GICv3 operations
> + * @params:		address of a pointer to HW-specific parameters
>   *
>   * Returns 0 if a GICv3 has been found, with the low level operations
>   * in *ops and the HW parameters in *params. Returns an error code
>   * otherwise.
>   */
> -int vgic_v3_probe(struct device_node *vgic_node,
> +int vgic_v3_probe(const struct gic_kvm_info *gic_kvm_info,
>  		  const struct vgic_ops **ops,
>  		  const struct vgic_params **params)
>  {
>  	int ret = 0;
> -	u32 gicv_idx;
> -	struct resource vcpu_res;
>  	struct vgic_params *vgic = &vgic_v3_params;
> +	const struct resource *vcpu_res = &gic_kvm_info->vcpu;
>  
> -	vgic->maint_irq = irq_of_parse_and_map(vgic_node, 0);
> -	if (!vgic->maint_irq) {
> -		kvm_err("error getting vgic maintenance irq from DT\n");
> -		ret = -ENXIO;
> -		goto out;
> -	}
> +	vgic->maint_irq = gic_kvm_info->maint_irq;
>  
>  	ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>  
> @@ -256,24 +248,19 @@ int vgic_v3_probe(struct device_node *vgic_node,
>  	vgic->nr_lr = (ich_vtr_el2 & 0xf) + 1;
>  	vgic->can_emulate_gicv2 = false;
>  
> -	if (of_property_read_u32(vgic_node, "#redistributor-regions", &gicv_idx))
> -		gicv_idx = 1;
> -
> -	gicv_idx += 3; /* Also skip GICD, GICC, GICH */
> -	if (of_address_to_resource(vgic_node, gicv_idx, &vcpu_res)) {
> +	if (!vcpu_res->start) {
>  		kvm_info("GICv3: no GICV resource entry\n");
>  		vgic->vcpu_base = 0;
> -	} else if (!PAGE_ALIGNED(vcpu_res.start)) {
> +	} else if (!PAGE_ALIGNED(vcpu_res->start)) {
>  		pr_warn("GICV physical address 0x%llx not page aligned\n",
> -			(unsigned long long)vcpu_res.start);
> +			(unsigned long long)vcpu_res->start);
>  		vgic->vcpu_base = 0;
> -	} else if (!PAGE_ALIGNED(resource_size(&vcpu_res))) {
> +	} else if (!PAGE_ALIGNED(resource_size(vcpu_res))) {
>  		pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
> -			(unsigned long long)resource_size(&vcpu_res),
> +			(unsigned long long)resource_size(vcpu_res),
>  			PAGE_SIZE);
> -		vgic->vcpu_base = 0;

See my comments in patch 6.  Perhaps I'm missing something, but I still
don't see a clear split between KVM vs. GIC error checking.

>  	} else {
> -		vgic->vcpu_base = vcpu_res.start;
> +		vgic->vcpu_base = vcpu_res->start;
>  		vgic->can_emulate_gicv2 = true;
>  		kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
>  					KVM_DEV_TYPE_ARM_VGIC_V2);
> @@ -286,15 +273,13 @@ int vgic_v3_probe(struct device_node *vgic_node,
>  	vgic->type = VGIC_V3;
>  	vgic->max_gic_vcpus = VGIC_V3_MAX_CPUS;
>  
> -	kvm_info("%s@%llx IRQ%d\n", vgic_node->name,
> -		 vcpu_res.start, vgic->maint_irq);
> +	kvm_info("GICV base=0x%llx, IRQ=%d\n",
> +		 vgic->vcpu_base, vgic->maint_irq);
>  
>  	on_each_cpu(vgic_cpu_init_lrs, vgic, 1);
>  
>  	*ops = &vgic_v3_ops;
>  	*params = vgic;
>  
> -out:
> -	of_node_put(vgic_node);
>  	return ret;
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 00429b3..60668a7 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -21,9 +21,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/of_irq.h>
> +#include <linux/irq.h>
>  #include <linux/rculist.h>
>  #include <linux/uaccess.h>
>  
> @@ -33,6 +31,7 @@
>  #include <trace/events/kvm.h>
>  #include <asm/kvm.h>
>  #include <kvm/iodev.h>
> +#include <linux/irqchip/arm-gic-common.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include "trace.h"
> @@ -2389,33 +2388,38 @@ static struct notifier_block vgic_cpu_nb = {
>  	.notifier_call = vgic_cpu_notify,
>  };
>  
> -static const struct of_device_id vgic_ids[] = {
> -	{ .compatible = "arm,cortex-a15-gic",	.data = vgic_v2_probe, },
> -	{ .compatible = "arm,cortex-a7-gic",	.data = vgic_v2_probe, },
> -	{ .compatible = "arm,gic-400",		.data = vgic_v2_probe, },
> -	{ .compatible = "arm,gic-v3",		.data = vgic_v3_probe, },
> -	{},
> -};
> -
> -int kvm_vgic_hyp_init(void)
> +static int kvm_vgic_probe(void)
>  {
> -	const struct of_device_id *matched_id;
> -	const int (*vgic_probe)(struct device_node *,const struct vgic_ops **,
> -				const struct vgic_params **);
> -	struct device_node *vgic_node;
> +	const struct gic_kvm_info *gic_kvm_info;
>  	int ret;
>  
> -	vgic_node = of_find_matching_node_and_match(NULL,
> -						    vgic_ids, &matched_id);
> -	if (!vgic_node) {
> -		kvm_err("error: no compatible GIC node found\n");
> +	gic_kvm_info = gic_get_kvm_info();
> +	if (!gic_kvm_info)
>  		return -ENODEV;
> +
> +	switch (gic_kvm_info->type) {
> +	case GIC_V2:
> +		ret = vgic_v2_probe(gic_kvm_info, &vgic_ops, &vgic);
> +		break;
> +	case GIC_V3:
> +		ret = vgic_v3_probe(gic_kvm_info, &vgic_ops, &vgic);
> +		break;
> +	default:
> +		ret = -ENODEV;
>  	}
>  
> -	vgic_probe = matched_id->data;
> -	ret = vgic_probe(vgic_node, &vgic_ops, &vgic);
> -	if (ret)
> +	return ret;
> +}
> +
> +int kvm_vgic_hyp_init(void)
> +{
> +	int ret;
> +
> +	ret = kvm_vgic_probe();
> +	if (ret) {
> +		kvm_err("error: KVM vGIC probing failed\n");
>  		return ret;
> +	}
>  
>  	ret = request_percpu_irq(vgic->maint_irq, vgic_maintenance_handler,
>  				 "vgic", kvm_get_running_vcpus());
> -- 
> 1.9.1
>

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

#1364382 — [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

FromJulien Grall <julien.grall@arm.com>
Date2016-03-24 19:00 +0100
Subject[PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure
Message-ID<rghI8-6aA-35@gated-at.bofh.it>
In reply to#1364377
Introduce a structure which are filled up by the arch timer driver and
used by the virtual timer in KVM.

The first member of this structure will be the timecounter. More members
will be added later.

A stub for the new helper isn't introduced because KVM requires the arch
timer for both ARM64 and ARM32.

The function arch_timer_get_timecounter is kept for the time being and
will be dropped in a subsequent patch.

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

---
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>

    Changes in v3:
        - Rename the patch
        - Move the KVM changes and removal of arch_timer_get_timecounter
        in separate patches.
---
 drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
 include/clocksource/arm_arch_timer.h |  5 +++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5152b38..62bdfe7 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
 	.mask	= CLOCKSOURCE_MASK(56),
 };
 
-static struct timecounter timecounter;
+static struct arch_timer_kvm_info arch_timer_kvm_info;
+
+struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
+{
+	return &arch_timer_kvm_info;
+}
 
 struct timecounter *arch_timer_get_timecounter(void)
 {
-	return &timecounter;
+	return &arch_timer_kvm_info.timecounter;
 }
 
 static void __init arch_counter_register(unsigned type)
@@ -500,7 +505,8 @@ static void __init arch_counter_register(unsigned type)
 	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
 	cyclecounter.mult = clocksource_counter.mult;
 	cyclecounter.shift = clocksource_counter.shift;
-	timecounter_init(&timecounter, &cyclecounter, start_count);
+	timecounter_init(&arch_timer_kvm_info.timecounter,
+			 &cyclecounter, start_count);
 
 	/* 56 bits minimum, so we assume worst case rollover */
 	sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 25d0914..9101ed6b 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -49,11 +49,16 @@ enum arch_timer_reg {
 
 #define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
 
+struct arch_timer_kvm_info {
+	struct timecounter timecounter;
+};
+
 #ifdef CONFIG_ARM_ARCH_TIMER
 
 extern u32 arch_timer_get_rate(void);
 extern u64 (*arch_timer_read_counter)(void);
 extern struct timecounter *arch_timer_get_timecounter(void);
+extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
 
 #else
 
-- 
1.9.1

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

#1366488 — Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-03-29 19:20 +0200
SubjectRe: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure
Message-ID<ri5t8-Xc-11@gated-at.bofh.it>
In reply to#1364382
On 03/24/2016 06:53 PM, Julien Grall wrote:
> Introduce a structure which are filled up by the arch timer driver and
> used by the virtual timer in KVM.
>
> The first member of this structure will be the timecounter. More members
> will be added later.
>
> A stub for the new helper isn't introduced because KVM requires the arch
> timer for both ARM64 and ARM32.
>
> The function arch_timer_get_timecounter is kept for the time being and
> will be dropped in a subsequent patch.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
>
>      Changes in v3:
>          - Rename the patch
>          - Move the KVM changes and removal of arch_timer_get_timecounter
>          in separate patches.
> ---
>   drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
>   include/clocksource/arm_arch_timer.h |  5 +++++
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5152b38..62bdfe7 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
>   	.mask	= CLOCKSOURCE_MASK(56),
>   };
>
> -static struct timecounter timecounter;
> +static struct arch_timer_kvm_info arch_timer_kvm_info;

This structure is statically defined in this subsystem but not used in 
this file and a couple of a accessors is added to let another subsystem 
to access it.

That sounds there is something wrong here with the design of the current 
code, virt/phys are mixed.

It isn't possible to split the virt/phys timer code respectively in 
virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?

At least, 'struct arch_timer_kvm_info' should belong to 
virt/kvm/arm/arch_timer.c.

> +struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
> +{
> +	return &arch_timer_kvm_info;
> +}
>
>   struct timecounter *arch_timer_get_timecounter(void)
>   {
> -	return &timecounter;
> +	return &arch_timer_kvm_info.timecounter;
>   }
>
>   static void __init arch_counter_register(unsigned type)
> @@ -500,7 +505,8 @@ static void __init arch_counter_register(unsigned type)
>   	clocksource_register_hz(&clocksource_counter, arch_timer_rate);
>   	cyclecounter.mult = clocksource_counter.mult;
>   	cyclecounter.shift = clocksource_counter.shift;
> -	timecounter_init(&timecounter, &cyclecounter, start_count);
> +	timecounter_init(&arch_timer_kvm_info.timecounter,
> +			 &cyclecounter, start_count);
>
>   	/* 56 bits minimum, so we assume worst case rollover */
>   	sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate);
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 25d0914..9101ed6b 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -49,11 +49,16 @@ enum arch_timer_reg {
>
>   #define ARCH_TIMER_EVT_STREAM_FREQ	10000	/* 100us */
>
> +struct arch_timer_kvm_info {
> +	struct timecounter timecounter;
> +};
> +
>   #ifdef CONFIG_ARM_ARCH_TIMER
>
>   extern u32 arch_timer_get_rate(void);
>   extern u64 (*arch_timer_read_counter)(void);
>   extern struct timecounter *arch_timer_get_timecounter(void);
> +extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void);
>
>   #else
>
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

#1366511 — Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-03-29 19:40 +0200
SubjectRe: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure
Message-ID<ri5Mu-14H-21@gated-at.bofh.it>
In reply to#1366488
Daniel,

On 29/03/16 18:13, Daniel Lezcano wrote:
> On 03/24/2016 06:53 PM, Julien Grall wrote:
>> Introduce a structure which are filled up by the arch timer driver and
>> used by the virtual timer in KVM.
>>
>> The first member of this structure will be the timecounter. More members
>> will be added later.
>>
>> A stub for the new helper isn't introduced because KVM requires the arch
>> timer for both ARM64 and ARM32.
>>
>> The function arch_timer_get_timecounter is kept for the time being and
>> will be dropped in a subsequent patch.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>
>>      Changes in v3:
>>          - Rename the patch
>>          - Move the KVM changes and removal of arch_timer_get_timecounter
>>          in separate patches.
>> ---
>>   drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
>>   include/clocksource/arm_arch_timer.h |  5 +++++
>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 5152b38..62bdfe7 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
>>   	.mask	= CLOCKSOURCE_MASK(56),
>>   };
>>
>> -static struct timecounter timecounter;
>> +static struct arch_timer_kvm_info arch_timer_kvm_info;
> 
> This structure is statically defined in this subsystem but not used in 
> this file and a couple of a accessors is added to let another subsystem 
> to access it.
> 
> That sounds there is something wrong here with the design of the current 
> code, virt/phys are mixed.
> 
> It isn't possible to split the virt/phys timer code respectively in 
> virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?

No, that'd be the wrong thing to do. The kernel uses *either* the virt
or phys timer depending on how it has been booted, and both counters are
in use.

What KVM (or any other hypervisor) needs from the timer subsystem is:
- an interrupt (so that it can force a guest exit when the timer fires),
- a way to convert the values programmed into the HW into a timer event
(which is what the time counter structure is for).

That allows the hypervisor to *emulate* a timer for the guest, and
that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
separation of what is driving the HW vs what is emulating it, and I'm
quite eager to preserve that.

> At least, 'struct arch_timer_kvm_info' should belong to 
> virt/kvm/arm/arch_timer.c.

At the cost of mandating separate storage in the arm_arch_timer driver.
I do not find that much nicer, but if you prefer that, fine by me.

Thanks,

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

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

#1367028 — Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

FromChristoffer Dall <christoffer.dall@linaro.org>
Date2016-03-30 11:10 +0200
SubjectRe: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure
Message-ID<rikit-37e-11@gated-at.bofh.it>
In reply to#1366511
On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
> Daniel,
> 
> On 29/03/16 18:13, Daniel Lezcano wrote:
> > On 03/24/2016 06:53 PM, Julien Grall wrote:
> >> Introduce a structure which are filled up by the arch timer driver and
> >> used by the virtual timer in KVM.
> >>
> >> The first member of this structure will be the timecounter. More members
> >> will be added later.
> >>
> >> A stub for the new helper isn't introduced because KVM requires the arch
> >> timer for both ARM64 and ARM32.
> >>
> >> The function arch_timer_get_timecounter is kept for the time being and
> >> will be dropped in a subsequent patch.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> > 
> >> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Marc Zyngier <marc.zyngier@arm.com>
> >>
> >>      Changes in v3:
> >>          - Rename the patch
> >>          - Move the KVM changes and removal of arch_timer_get_timecounter
> >>          in separate patches.
> >> ---
> >>   drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
> >>   include/clocksource/arm_arch_timer.h |  5 +++++
> >>   2 files changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> >> index 5152b38..62bdfe7 100644
> >> --- a/drivers/clocksource/arm_arch_timer.c
> >> +++ b/drivers/clocksource/arm_arch_timer.c
> >> @@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
> >>   	.mask	= CLOCKSOURCE_MASK(56),
> >>   };
> >>
> >> -static struct timecounter timecounter;
> >> +static struct arch_timer_kvm_info arch_timer_kvm_info;
> > 
> > This structure is statically defined in this subsystem but not used in 
> > this file and a couple of a accessors is added to let another subsystem 
> > to access it.
> > 
> > That sounds there is something wrong here with the design of the current 
> > code, virt/phys are mixed.
> > 
> > It isn't possible to split the virt/phys timer code respectively in 
> > virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
> 
> No, that'd be the wrong thing to do. The kernel uses *either* the virt
> or phys timer depending on how it has been booted, and both counters are
> in use.
> 
> What KVM (or any other hypervisor) needs from the timer subsystem is:
> - an interrupt (so that it can force a guest exit when the timer fires),
> - a way to convert the values programmed into the HW into a timer event
> (which is what the time counter structure is for).
> 
> That allows the hypervisor to *emulate* a timer for the guest, and
> that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
> separation of what is driving the HW vs what is emulating it, and I'm
> quite eager to preserve that.
> 
> > At least, 'struct arch_timer_kvm_info' should belong to 
> > virt/kvm/arm/arch_timer.c.
> 
> At the cost of mandating separate storage in the arm_arch_timer driver.
> I do not find that much nicer, but if you prefer that, fine by me.
> 
If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
do you want to make it globally accessible and populated by this code or
make it static to the KVM code and populate it with accessor functions?

To me the natural thing is that the arch timer driver maintains data
about the device it drives, and consumers of that data can ask the arch
timer driver for the details.

Thanks,
-Christoffer

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

#1367033 — Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

FromMarc Zyngier <marc.zyngier@arm.com>
Date2016-03-30 11:20 +0200
SubjectRe: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure
Message-ID<riksb-3aA-17@gated-at.bofh.it>
In reply to#1367028
On 30/03/16 10:06, Christoffer Dall wrote:
> On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
>> Daniel,
>>
>> On 29/03/16 18:13, Daniel Lezcano wrote:
>>> On 03/24/2016 06:53 PM, Julien Grall wrote:
>>>> Introduce a structure which are filled up by the arch timer driver and
>>>> used by the virtual timer in KVM.
>>>>
>>>> The first member of this structure will be the timecounter. More members
>>>> will be added later.
>>>>
>>>> A stub for the new helper isn't introduced because KVM requires the arch
>>>> timer for both ARM64 and ARM32.
>>>>
>>>> The function arch_timer_get_timecounter is kept for the time being and
>>>> will be dropped in a subsequent patch.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>
>>>>      Changes in v3:
>>>>          - Rename the patch
>>>>          - Move the KVM changes and removal of arch_timer_get_timecounter
>>>>          in separate patches.
>>>> ---
>>>>   drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
>>>>   include/clocksource/arm_arch_timer.h |  5 +++++
>>>>   2 files changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>>>> index 5152b38..62bdfe7 100644
>>>> --- a/drivers/clocksource/arm_arch_timer.c
>>>> +++ b/drivers/clocksource/arm_arch_timer.c
>>>> @@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
>>>>   	.mask	= CLOCKSOURCE_MASK(56),
>>>>   };
>>>>
>>>> -static struct timecounter timecounter;
>>>> +static struct arch_timer_kvm_info arch_timer_kvm_info;
>>>
>>> This structure is statically defined in this subsystem but not used in 
>>> this file and a couple of a accessors is added to let another subsystem 
>>> to access it.
>>>
>>> That sounds there is something wrong here with the design of the current 
>>> code, virt/phys are mixed.
>>>
>>> It isn't possible to split the virt/phys timer code respectively in 
>>> virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
>>
>> No, that'd be the wrong thing to do. The kernel uses *either* the virt
>> or phys timer depending on how it has been booted, and both counters are
>> in use.
>>
>> What KVM (or any other hypervisor) needs from the timer subsystem is:
>> - an interrupt (so that it can force a guest exit when the timer fires),
>> - a way to convert the values programmed into the HW into a timer event
>> (which is what the time counter structure is for).
>>
>> That allows the hypervisor to *emulate* a timer for the guest, and
>> that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
>> separation of what is driving the HW vs what is emulating it, and I'm
>> quite eager to preserve that.
>>
>>> At least, 'struct arch_timer_kvm_info' should belong to 
>>> virt/kvm/arm/arch_timer.c.
>>
>> At the cost of mandating separate storage in the arm_arch_timer driver.
>> I do not find that much nicer, but if you prefer that, fine by me.
>>
> If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
> do you want to make it globally accessible and populated by this code or
> make it static to the KVM code and populate it with accessor functions?

That'd be the latter, as I'm really not fond of global data.

> To me the natural thing is that the arch timer driver maintains data
> about the device it drives, and consumers of that data can ask the arch
> timer driver for the details.

That was my approach too, and that's the way the code proposed by Julien
works. Daniel seems to have a different take on it though.

Thanks,

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

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

#1367062 — Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

FromDaniel Lezcano <daniel.lezcano@linaro.org>
Date2016-03-30 12:00 +0200
SubjectRe: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure
Message-ID<ril4T-3q2-33@gated-at.bofh.it>
In reply to#1367033
On 03/30/2016 11:12 AM, Marc Zyngier wrote:
> On 30/03/16 10:06, Christoffer Dall wrote:
>> On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
>>> Daniel,
>>>
>>> On 29/03/16 18:13, Daniel Lezcano wrote:
>>>> On 03/24/2016 06:53 PM, Julien Grall wrote:
>>>>> Introduce a structure which are filled up by the arch timer driver and
>>>>> used by the virtual timer in KVM.
>>>>>
>>>>> The first member of this structure will be the timecounter. More members
>>>>> will be added later.
>>>>>
>>>>> A stub for the new helper isn't introduced because KVM requires the arch
>>>>> timer for both ARM64 and ARM32.
>>>>>
>>>>> The function arch_timer_get_timecounter is kept for the time being and
>>>>> will be dropped in a subsequent patch.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>>>
>>>>>       Changes in v3:
>>>>>           - Rename the patch
>>>>>           - Move the KVM changes and removal of arch_timer_get_timecounter
>>>>>           in separate patches.
>>>>> ---
>>>>>    drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
>>>>>    include/clocksource/arm_arch_timer.h |  5 +++++
>>>>>    2 files changed, 14 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>>>>> index 5152b38..62bdfe7 100644
>>>>> --- a/drivers/clocksource/arm_arch_timer.c
>>>>> +++ b/drivers/clocksource/arm_arch_timer.c
>>>>> @@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
>>>>>    	.mask	= CLOCKSOURCE_MASK(56),
>>>>>    };
>>>>>
>>>>> -static struct timecounter timecounter;
>>>>> +static struct arch_timer_kvm_info arch_timer_kvm_info;
>>>>
>>>> This structure is statically defined in this subsystem but not used in
>>>> this file and a couple of a accessors is added to let another subsystem
>>>> to access it.
>>>>
>>>> That sounds there is something wrong here with the design of the current
>>>> code, virt/phys are mixed.
>>>>
>>>> It isn't possible to split the virt/phys timer code respectively in
>>>> virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
>>>
>>> No, that'd be the wrong thing to do. The kernel uses *either* the virt
>>> or phys timer depending on how it has been booted, and both counters are
>>> in use.
>>>
>>> What KVM (or any other hypervisor) needs from the timer subsystem is:
>>> - an interrupt (so that it can force a guest exit when the timer fires),
>>> - a way to convert the values programmed into the HW into a timer event
>>> (which is what the time counter structure is for).
>>>
>>> That allows the hypervisor to *emulate* a timer for the guest, and
>>> that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
>>> separation of what is driving the HW vs what is emulating it, and I'm
>>> quite eager to preserve that.
>>>
>>>> At least, 'struct arch_timer_kvm_info' should belong to
>>>> virt/kvm/arm/arch_timer.c.
>>>
>>> At the cost of mandating separate storage in the arm_arch_timer driver.
>>> I do not find that much nicer, but if you prefer that, fine by me.
>>>
>> If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
>> do you want to make it globally accessible and populated by this code or
>> make it static to the KVM code and populate it with accessor functions?
>
> That'd be the latter, as I'm really not fond of global data.
>
>> To me the natural thing is that the arch timer driver maintains data
>> about the device it drives, and consumers of that data can ask the arch
>> timer driver for the details.
>
> That was my approach too, and that's the way the code proposed by Julien
> works. Daniel seems to have a different take on it though.

Well, I'm not against Julien's changes. The arm_arch_timer is complex 
and I don't have all the knowledge for the virt side. So I am just 
asking if everything is clearly separated which seems to be the case 
regarding your previous email.

What sounds strange to me is we have a static global function which is 
not used (except at init time) by the timer and then we add accessors 
function to retrieve it. I would have expected arch_timer to pass a 
structure at init time to the timer driver and this one fills it. Then 
the arch timer can directly use its own structure.

Anyway, perhaps I am splitting hairs. So up to you if you want to keep 
the current approach.

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web