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
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 <[email protected]>
---
Cc: Daniel Lezcano <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
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 ffe9d1c..b7ab588 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -456,11 +456,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)
@@ -488,7 +493,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
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 <[email protected]>
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
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
Currenlty, 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 <[email protected]>
---
Cc: Christoffer Dall <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Gleb Natapov <[email protected]>
Cc: Paolo Bonzini <[email protected]>
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 | 67 ++++++++++++++++++--------------------------------
virt/kvm/arm/vgic-v3.c | 45 ++++++++++-----------------------
virt/kvm/arm/vgic.c | 50 ++++++++++++++++++++-----------------
4 files changed, 68 insertions(+), 101 deletions(-)
diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
index 13a3d53..ed62772 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
@@ -357,15 +358,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 ff02f08..3598cd4 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>
@@ -177,38 +174,38 @@ static const struct vgic_ops vgic_v2_ops = {
static struct vgic_params vgic_v2_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;
+ resource_size_t vctrl_size = resource_size(&gic_kvm_info->vctrl);
- 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;
@@ -219,30 +216,15 @@ 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 + vctrl_size,
+ gic_kvm_info->vctrl.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)) {
- kvm_err("GICV physical address 0x%llx not page aligned\n",
- (unsigned long long)vcpu_res.start);
- ret = -ENXIO;
+ kvm_err("Cannot map VCTLR into hyp\n");
goto out_unmap;
}
- 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),
- PAGE_SIZE);
+ if (!gic_kvm_info->vcpu.start) {
+ kvm_err("GICV not present in the firmware table\n");
ret = -ENXIO;
goto out_unmap;
}
@@ -250,10 +232,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 = gic_kvm_info->vcpu.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;
@@ -264,6 +246,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 453eafd..92c1f04 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>
@@ -217,30 +215,28 @@ static const struct vgic_ops vgic_v3_ops = {
static struct vgic_params vgic_v3_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;
- 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;
ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
@@ -251,24 +247,11 @@ 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 (!gic_kvm_info->vcpu.start) {
kvm_info("GICv3: no GICV resource entry\n");
vgic->vcpu_base = 0;
- } else if (!PAGE_ALIGNED(vcpu_res.start)) {
- pr_warn("GICV physical address 0x%llx not page aligned\n",
- (unsigned long long)vcpu_res.start);
- vgic->vcpu_base = 0;
- } 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),
- PAGE_SIZE);
- vgic->vcpu_base = 0;
} else {
- vgic->vcpu_base = vcpu_res.start;
+ vgic->vcpu_base = gic_kvm_info->vcpu.start;
vgic->can_emulate_gicv2 = true;
kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
KVM_DEV_TYPE_ARM_VGIC_V2);
@@ -281,13 +264,11 @@ 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);
*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
Fill up the recently introduced gic_kvm_info with the virtual GIC
information.
Signed-off-by: Julien Grall <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
---
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
The only call of arch_timer_get_timecounter (in KVM) has been removed.
Signed-off-by: Julien Grall <[email protected]>
---
Cc: Daniel Lezcano <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Changes in v3:
- Patch added
---
drivers/clocksource/arm_arch_timer.c | 5 -----
include/clocksource/arm_arch_timer.h | 6 ------
2 files changed, 11 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index d8887f3..94696d3 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -463,11 +463,6 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
return &arch_timer_kvm_info;
}
-struct timecounter *arch_timer_get_timecounter(void)
-{
- return &arch_timer_kvm_info.timecounter;
-}
-
static void __init arch_counter_register(unsigned type)
{
u64 start_count;
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 9dd996a..caedb74 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -58,7 +58,6 @@ struct arch_timer_kvm_info {
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
@@ -73,11 +72,6 @@ static inline u64 arch_timer_read_counter(void)
return 0;
}
-static inline struct timecounter *arch_timer_get_timecounter(void)
-{
- return NULL;
-}
-
#endif
#endif
--
1.9.1
Even though all the variables aren't marked with __initdata, they are
only used during initialization. So the structure is marked with
__initdata.
Signed-off-by: Julien Grall <[email protected]>
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
Changes in v3:
- Patch added
---
drivers/irqchip/irq-gic-v3.c | 60 ++++++++++++++++++++++++--------------------
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index 5b7d3c2..50e87e6 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -968,19 +968,22 @@ out_unmap_dist:
IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
#ifdef CONFIG_ACPI
-static void __iomem *dist_base;
-static struct redist_region *redist_regs __initdata;
-static u32 nr_redist_regions __initdata;
-static bool single_redist;
+static struct
+{
+ void __iomem *dist_base;
+ struct redist_region *redist_regs;
+ u32 nr_redist_regions;
+ bool single_redist;
+} acpi_data __initdata;
static void __init
gic_acpi_register_redist(phys_addr_t phys_base, void __iomem *redist_base)
{
static int count = 0;
- redist_regs[count].phys_base = phys_base;
- redist_regs[count].redist_base = redist_base;
- redist_regs[count].single_redist = single_redist;
+ acpi_data.redist_regs[count].phys_base = phys_base;
+ acpi_data.redist_regs[count].redist_base = redist_base;
+ acpi_data.redist_regs[count].single_redist = acpi_data.single_redist;
count++;
}
@@ -1008,7 +1011,7 @@ gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header,
{
struct acpi_madt_generic_interrupt *gicc =
(struct acpi_madt_generic_interrupt *)header;
- u32 reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
+ u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
void __iomem *redist_base;
@@ -1025,7 +1028,7 @@ static int __init gic_acpi_collect_gicr_base(void)
acpi_tbl_entry_handler redist_parser;
enum acpi_madt_type type;
- if (single_redist) {
+ if (acpi_data.single_redist) {
type = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
redist_parser = gic_acpi_parse_madt_gicc;
} else {
@@ -1076,14 +1079,14 @@ static int __init gic_acpi_count_gicr_regions(void)
count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
gic_acpi_match_gicr, 0);
if (count > 0) {
- single_redist = false;
+ acpi_data.single_redist = false;
return count;
}
count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
gic_acpi_match_gicc, 0);
if (count > 0)
- single_redist = true;
+ acpi_data.single_redist = true;
return count;
}
@@ -1103,7 +1106,7 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header,
if (count <= 0)
return false;
- nr_redist_regions = count;
+ acpi_data.nr_redist_regions = count;
return true;
}
@@ -1114,25 +1117,28 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
{
struct acpi_madt_generic_distributor *dist;
struct fwnode_handle *domain_handle;
+ size_t size;
int i, err;
/* Get distributor base address */
dist = (struct acpi_madt_generic_distributor *)header;
- dist_base = ioremap(dist->base_address, ACPI_GICV3_DIST_MEM_SIZE);
- if (!dist_base) {
+ acpi_data.dist_base = ioremap(dist->base_address,
+ ACPI_GICV3_DIST_MEM_SIZE);
+ if (!acpi_data.dist_base) {
pr_err("Unable to map GICD registers\n");
return -ENOMEM;
}
- err = gic_validate_dist_version(dist_base);
+ err = gic_validate_dist_version(acpi_data.dist_base);
if (err) {
- pr_err("No distributor detected at @%p, giving up", dist_base);
+ pr_err("No distributor detected at @%p, giving up",
+ acpi_data.dist_base);
goto out_dist_unmap;
}
- redist_regs = kzalloc(sizeof(*redist_regs) * nr_redist_regions,
- GFP_KERNEL);
- if (!redist_regs) {
+ size = sizeof(*acpi_data.redist_regs) * acpi_data.nr_redist_regions;
+ acpi_data.redist_regs = kzalloc(size, GFP_KERNEL);
+ if (!acpi_data.redist_regs) {
err = -ENOMEM;
goto out_dist_unmap;
}
@@ -1141,14 +1147,14 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
if (err)
goto out_redist_unmap;
- domain_handle = irq_domain_alloc_fwnode(dist_base);
+ domain_handle = irq_domain_alloc_fwnode(acpi_data.dist_base);
if (!domain_handle) {
err = -ENOMEM;
goto out_redist_unmap;
}
- err = gic_init_bases(dist_base, redist_regs, nr_redist_regions, 0,
- domain_handle);
+ err = gic_init_bases(acpi_data.dist_base, acpi_data.redist_regs,
+ acpi_data.nr_redist_regions, 0, domain_handle);
if (err)
goto out_fwhandle_free;
@@ -1158,12 +1164,12 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
out_fwhandle_free:
irq_domain_free_fwnode(domain_handle);
out_redist_unmap:
- for (i = 0; i < nr_redist_regions; i++)
- if (redist_regs[i].redist_base)
- iounmap(redist_regs[i].redist_base);
- kfree(redist_regs);
+ for (i = 0; i < acpi_data.nr_redist_regions; i++)
+ if (acpi_data.redist_regs[i].redist_base)
+ iounmap(acpi_data.redist_regs[i].redist_base);
+ kfree(acpi_data.redist_regs);
out_dist_unmap:
- iounmap(dist_base);
+ iounmap(acpi_data.dist_base);
return err;
}
IRQCHIP_ACPI_DECLARE(gic_v3, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
--
1.9.1
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 <[email protected]>
---
Cc: Christoffer Dall <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Gleb Natapov <[email protected]>
Cc: Paolo Bonzini <[email protected]>
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
For now, there is only one member. More member will be added later.
Signed-off-by: Julien Grall <[email protected]>
---
Cc: Thomas Gleixner <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Marc Zyngier <[email protected]>
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
Currently, the firmware table is 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, extend arch_timer_kvm_info to get the virtual
IRQ.
Note that the KVM code will be modified in a subsequent patch.
Signed-off-by: Julien Grall <[email protected]>
---
Cc: Daniel Lezcano <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
Changes in v3:
- Move the KVM changes in a separate patch and rename the patch
- Move the initialization of the virtual_irq to
arch_timer_common_init
---
drivers/clocksource/arm_arch_timer.c | 2 ++
include/clocksource/arm_arch_timer.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index b7ab588..d8887f3 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -701,6 +701,8 @@ static void __init arch_timer_common_init(void)
arch_timer_banner(arch_timers_present);
arch_counter_register(arch_timers_present);
arch_timer_arch_init();
+
+ arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
}
static void __init arch_timer_init(void)
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 9101ed6b..9dd996a 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -51,6 +51,7 @@ enum arch_timer_reg {
struct arch_timer_kvm_info {
struct timecounter timecounter;
+ int virtual_irq;
};
#ifdef CONFIG_ARM_ARCH_TIMER
--
1.9.1
On Tue, Mar 08, 2016 at 11:29:25AM +0000, 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 <[email protected]>
>
> ---
> Cc: Daniel Lezcano <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> 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 ffe9d1c..b7ab588 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -456,11 +456,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)
borderline bikeshedding question:
does it make sense that the info the arch timer code exports is labeled
to be kvm-specific? In other words, could we imagine another subsystem
using this timer info some time and is there a more generic term that
would be more appropriate?
otherwise it looks fine to me.
-Christoffer
> +{
> + 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)
> @@ -488,7 +493,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
>
On Tue, Mar 08, 2016 at 11:29:26AM +0000, Julien Grall wrote:
> Currently, the firmware table is 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, extend arch_timer_kvm_info to get the virtual
> IRQ.
>
> Note that the KVM code will be modified in a subsequent patch.
>
> Signed-off-by: Julien Grall <[email protected]>
>
> ---
> Cc: Daniel Lezcano <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> Changes in v3:
> - Move the KVM changes in a separate patch and rename the patch
> - Move the initialization of the virtual_irq to
> arch_timer_common_init
> ---
> drivers/clocksource/arm_arch_timer.c | 2 ++
> include/clocksource/arm_arch_timer.h | 1 +
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index b7ab588..d8887f3 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -701,6 +701,8 @@ static void __init arch_timer_common_init(void)
> arch_timer_banner(arch_timers_present);
> arch_counter_register(arch_timers_present);
> arch_timer_arch_init();
> +
> + arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
why is this in common_init and not just in init?
> }
>
> static void __init arch_timer_init(void)
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 9101ed6b..9dd996a 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -51,6 +51,7 @@ enum arch_timer_reg {
>
> struct arch_timer_kvm_info {
> struct timecounter timecounter;
> + int virtual_irq;
> };
>
> #ifdef CONFIG_ARM_ARCH_TIMER
> --
> 1.9.1
>
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 <[email protected]>
>
> ---
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> 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
>
Hi Christoffer,
On 09/03/2016 10:23, Christoffer Dall wrote:
> On Tue, Mar 08, 2016 at 11:29:25AM +0000, Julien Grall wrote:
>> -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)
>
> borderline bikeshedding question:
>
> does it make sense that the info the arch timer code exports is labeled
> to be kvm-specific? In other words, could we imagine another subsystem
> using this timer info some time and is there a more generic term that
> would be more appropriate?
I can't see any other usage. I would keep the function name KVM specific
until someone really need similar information.
Cheers,
--
Julien Grall
On Tue, Mar 08, 2016 at 11:29:29AM +0000, Julien Grall wrote:
> Even though all the variables aren't marked with __initdata, they are
> only used during initialization. So the structure is marked with
> __initdata.
Not sure I understand this commit message.
As I see it, this commit includes two changes:
1. Mark the variables only used during init with __initdata
2. Move the variables into a structure
If I get that right, can you argue for both changes?
Thanks,
-Christoffer
>
> Signed-off-by: Julien Grall <[email protected]>
>
> ---
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> Changes in v3:
> - Patch added
> ---
> drivers/irqchip/irq-gic-v3.c | 60 ++++++++++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5b7d3c2..50e87e6 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -968,19 +968,22 @@ out_unmap_dist:
> IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init);
>
> #ifdef CONFIG_ACPI
> -static void __iomem *dist_base;
> -static struct redist_region *redist_regs __initdata;
> -static u32 nr_redist_regions __initdata;
> -static bool single_redist;
> +static struct
> +{
> + void __iomem *dist_base;
> + struct redist_region *redist_regs;
> + u32 nr_redist_regions;
> + bool single_redist;
> +} acpi_data __initdata;
>
> static void __init
> gic_acpi_register_redist(phys_addr_t phys_base, void __iomem *redist_base)
> {
> static int count = 0;
>
> - redist_regs[count].phys_base = phys_base;
> - redist_regs[count].redist_base = redist_base;
> - redist_regs[count].single_redist = single_redist;
> + acpi_data.redist_regs[count].phys_base = phys_base;
> + acpi_data.redist_regs[count].redist_base = redist_base;
> + acpi_data.redist_regs[count].single_redist = acpi_data.single_redist;
> count++;
> }
>
> @@ -1008,7 +1011,7 @@ gic_acpi_parse_madt_gicc(struct acpi_subtable_header *header,
> {
> struct acpi_madt_generic_interrupt *gicc =
> (struct acpi_madt_generic_interrupt *)header;
> - u32 reg = readl_relaxed(dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> + u32 reg = readl_relaxed(acpi_data.dist_base + GICD_PIDR2) & GIC_PIDR2_ARCH_MASK;
> u32 size = reg == GIC_PIDR2_ARCH_GICv4 ? SZ_64K * 4 : SZ_64K * 2;
> void __iomem *redist_base;
>
> @@ -1025,7 +1028,7 @@ static int __init gic_acpi_collect_gicr_base(void)
> acpi_tbl_entry_handler redist_parser;
> enum acpi_madt_type type;
>
> - if (single_redist) {
> + if (acpi_data.single_redist) {
> type = ACPI_MADT_TYPE_GENERIC_INTERRUPT;
> redist_parser = gic_acpi_parse_madt_gicc;
> } else {
> @@ -1076,14 +1079,14 @@ static int __init gic_acpi_count_gicr_regions(void)
> count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
> gic_acpi_match_gicr, 0);
> if (count > 0) {
> - single_redist = false;
> + acpi_data.single_redist = false;
> return count;
> }
>
> count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> gic_acpi_match_gicc, 0);
> if (count > 0)
> - single_redist = true;
> + acpi_data.single_redist = true;
>
> return count;
> }
> @@ -1103,7 +1106,7 @@ static bool __init acpi_validate_gic_table(struct acpi_subtable_header *header,
> if (count <= 0)
> return false;
>
> - nr_redist_regions = count;
> + acpi_data.nr_redist_regions = count;
> return true;
> }
>
> @@ -1114,25 +1117,28 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> {
> struct acpi_madt_generic_distributor *dist;
> struct fwnode_handle *domain_handle;
> + size_t size;
> int i, err;
>
> /* Get distributor base address */
> dist = (struct acpi_madt_generic_distributor *)header;
> - dist_base = ioremap(dist->base_address, ACPI_GICV3_DIST_MEM_SIZE);
> - if (!dist_base) {
> + acpi_data.dist_base = ioremap(dist->base_address,
> + ACPI_GICV3_DIST_MEM_SIZE);
> + if (!acpi_data.dist_base) {
> pr_err("Unable to map GICD registers\n");
> return -ENOMEM;
> }
>
> - err = gic_validate_dist_version(dist_base);
> + err = gic_validate_dist_version(acpi_data.dist_base);
> if (err) {
> - pr_err("No distributor detected at @%p, giving up", dist_base);
> + pr_err("No distributor detected at @%p, giving up",
> + acpi_data.dist_base);
> goto out_dist_unmap;
> }
>
> - redist_regs = kzalloc(sizeof(*redist_regs) * nr_redist_regions,
> - GFP_KERNEL);
> - if (!redist_regs) {
> + size = sizeof(*acpi_data.redist_regs) * acpi_data.nr_redist_regions;
> + acpi_data.redist_regs = kzalloc(size, GFP_KERNEL);
> + if (!acpi_data.redist_regs) {
> err = -ENOMEM;
> goto out_dist_unmap;
> }
> @@ -1141,14 +1147,14 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> if (err)
> goto out_redist_unmap;
>
> - domain_handle = irq_domain_alloc_fwnode(dist_base);
> + domain_handle = irq_domain_alloc_fwnode(acpi_data.dist_base);
> if (!domain_handle) {
> err = -ENOMEM;
> goto out_redist_unmap;
> }
>
> - err = gic_init_bases(dist_base, redist_regs, nr_redist_regions, 0,
> - domain_handle);
> + err = gic_init_bases(acpi_data.dist_base, acpi_data.redist_regs,
> + acpi_data.nr_redist_regions, 0, domain_handle);
> if (err)
> goto out_fwhandle_free;
>
> @@ -1158,12 +1164,12 @@ gic_acpi_init(struct acpi_subtable_header *header, const unsigned long end)
> out_fwhandle_free:
> irq_domain_free_fwnode(domain_handle);
> out_redist_unmap:
> - for (i = 0; i < nr_redist_regions; i++)
> - if (redist_regs[i].redist_base)
> - iounmap(redist_regs[i].redist_base);
> - kfree(redist_regs);
> + for (i = 0; i < acpi_data.nr_redist_regions; i++)
> + if (acpi_data.redist_regs[i].redist_base)
> + iounmap(acpi_data.redist_regs[i].redist_base);
> + kfree(acpi_data.redist_regs);
> out_dist_unmap:
> - iounmap(dist_base);
> + iounmap(acpi_data.dist_base);
> return err;
> }
> IRQCHIP_ACPI_DECLARE(gic_v3, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> --
> 1.9.1
>
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 <[email protected]>
>
> ---
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> 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 <[email protected]>
Hi Christoffer,
On 09/03/2016 10:27, Christoffer Dall wrote:
> On Tue, Mar 08, 2016 at 11:29:26AM +0000, Julien Grall wrote:
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index b7ab588..d8887f3 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -701,6 +701,8 @@ static void __init arch_timer_common_init(void)
>> arch_timer_banner(arch_timers_present);
>> arch_counter_register(arch_timers_present);
>> arch_timer_arch_init();
>> +
>> + arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
>
> why is this in common_init and not just in init?
I thought we wanted to initialize virtual_irq for both the system
registers timer and the memory timer. Although, as talked IRL, KVM
mandates system registers timer. So I will initialize the virtual_irq in
arch_timer_init.
Cheers,
--
Julien Grall
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 <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> ---
> 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
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 <[email protected]>
>
Acked-by: Christoffer Dall <[email protected]>
On Tue, Mar 08, 2016 at 11:29:32AM +0000, Julien Grall wrote:
> Currenlty, the firmware tables are parsed 2 times: once in the GIC
Currently,
> 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 <[email protected]>
>
> ---
> Cc: Christoffer Dall <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Gleb Natapov <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
>
> 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 | 67 ++++++++++++++++++--------------------------------
> virt/kvm/arm/vgic-v3.c | 45 ++++++++++-----------------------
> virt/kvm/arm/vgic.c | 50 ++++++++++++++++++++-----------------
> 4 files changed, 68 insertions(+), 101 deletions(-)
>
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 13a3d53..ed62772 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
> @@ -357,15 +358,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 ff02f08..3598cd4 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>
>
> @@ -177,38 +174,38 @@ static const struct vgic_ops vgic_v2_ops = {
> static struct vgic_params vgic_v2_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;
> + resource_size_t vctrl_size = resource_size(&gic_kvm_info->vctrl);
>
> - 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;
> @@ -219,30 +216,15 @@ 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 + vctrl_size,
> + gic_kvm_info->vctrl.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)) {
> - kvm_err("GICV physical address 0x%llx not page aligned\n",
> - (unsigned long long)vcpu_res.start);
> - ret = -ENXIO;
> + kvm_err("Cannot map VCTLR into hyp\n");
> goto out_unmap;
> }
>
> - 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),
> - PAGE_SIZE);
> + if (!gic_kvm_info->vcpu.start) {
> + kvm_err("GICV not present in the firmware table\n");
> ret = -ENXIO;
> goto out_unmap;
> }
> @@ -250,10 +232,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 = gic_kvm_info->vcpu.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;
> @@ -264,6 +246,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 453eafd..92c1f04 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>
> @@ -217,30 +215,28 @@ static const struct vgic_ops vgic_v3_ops = {
> static struct vgic_params vgic_v3_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;
>
> - 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;
>
> ich_vtr_el2 = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
>
> @@ -251,24 +247,11 @@ 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 (!gic_kvm_info->vcpu.start) {
> kvm_info("GICv3: no GICV resource entry\n");
> vgic->vcpu_base = 0;
> - } else if (!PAGE_ALIGNED(vcpu_res.start)) {
> - pr_warn("GICV physical address 0x%llx not page aligned\n",
> - (unsigned long long)vcpu_res.start);
> - vgic->vcpu_base = 0;
> - } 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),
> - PAGE_SIZE);
> - vgic->vcpu_base = 0;
> } else {
> - vgic->vcpu_base = vcpu_res.start;
> + vgic->vcpu_base = gic_kvm_info->vcpu.start;
> vgic->can_emulate_gicv2 = true;
> kvm_register_device_ops(&kvm_arm_vgic_v2_ops,
> KVM_DEV_TYPE_ARM_VGIC_V2);
> @@ -281,13 +264,11 @@ 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);
> *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
>
Acked-by: Christoffer Dall <[email protected]>
On Tue, Mar 08, 2016 at 11:29:33AM +0000, Julien Grall wrote:
> The only call of arch_timer_get_timecounter (in KVM) has been removed.
>
> Signed-off-by: Julien Grall <[email protected]>
>
Acked-by: Christoffer Dall <[email protected]>
> ---
> Cc: Daniel Lezcano <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
>
> Changes in v3:
> - Patch added
> ---
> drivers/clocksource/arm_arch_timer.c | 5 -----
> include/clocksource/arm_arch_timer.h | 6 ------
> 2 files changed, 11 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index d8887f3..94696d3 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -463,11 +463,6 @@ struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
> return &arch_timer_kvm_info;
> }
>
> -struct timecounter *arch_timer_get_timecounter(void)
> -{
> - return &arch_timer_kvm_info.timecounter;
> -}
> -
> static void __init arch_counter_register(unsigned type)
> {
> u64 start_count;
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 9dd996a..caedb74 100644
> --- a/include/clocksource/arm_arch_timer.h
> +++ b/include/clocksource/arm_arch_timer.h
> @@ -58,7 +58,6 @@ struct arch_timer_kvm_info {
>
> 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
> @@ -73,11 +72,6 @@ static inline u64 arch_timer_read_counter(void)
> return 0;
> }
>
> -static inline struct timecounter *arch_timer_get_timecounter(void)
> -{
> - return NULL;
> -}
> -
> #endif
>
> #endif
> --
> 1.9.1
>
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 <[email protected]>
Cheers,
--
Julien Grall
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
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 <[email protected]>
>>
>> ---
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Jason Cooper <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>>
>> 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
Hi Christoffer,
On 09/03/16 05:39, Christoffer Dall wrote:
> On Tue, Mar 08, 2016 at 11:29:29AM +0000, Julien Grall wrote:
>> Even though all the variables aren't marked with __initdata, they are
>> only used during initialization. So the structure is marked with
>> __initdata.
>
> Not sure I understand this commit message.
>
> As I see it, this commit includes two changes:
>
> 1. Mark the variables only used during init with __initdata
>
> 2. Move the variables into a structure
>
> If I get that right, can you argue for both changes?
What about:
"The ACPI code requires to use global variables in order to collect
information from the tables.
To make clear those variables are ACPI specific, gather all of them in a
single structure.
Furthermore, even if some of the variables are not marked with
__initdata, they are all only used during the initialization. Therefore,
the new variable, which hold the structure, can be marked __initdata."
Cheers,
--
Julien Grall
On Tue, Mar 15, 2016 at 12:26:29PM +0000, Julien Grall wrote:
> Hi Christoffer,
>
> On 09/03/16 05:39, Christoffer Dall wrote:
> >On Tue, Mar 08, 2016 at 11:29:29AM +0000, Julien Grall wrote:
> >>Even though all the variables aren't marked with __initdata, they are
> >>only used during initialization. So the structure is marked with
> >>__initdata.
> >
> >Not sure I understand this commit message.
> >
> >As I see it, this commit includes two changes:
> >
> >1. Mark the variables only used during init with __initdata
> >
> >2. Move the variables into a structure
> >
> >If I get that right, can you argue for both changes?
>
> What about:
>
> "The ACPI code requires to use global variables in order to collect
> information from the tables.
>
> To make clear those variables are ACPI specific, gather all of them
> in a single structure.
>
> Furthermore, even if some of the variables are not marked with
> __initdata, they are all only used during the initialization.
> Therefore, the new variable, which hold the structure, can be marked
> __initdata."
>
Looks ok (the part about some of the variables not marked with
__initdata is confusing but I think I understand what you mean).
Thanks,
-Christoffer
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.
>
> Signed-off-by: Julien Grall <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Marc Zyngier <[email protected]>
>
> ---
> 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;
> }
Placing this here means that it will not collect the info in the case
where there is a Generic Interrupt Redistributor structure in the MADT.
I guess collecting this info should not be a side effect of happening to
pass GICC for redistributor base. Which unfortuneately probably means we
do need to parse the GICCs specifically for this information.
Graeme
>
> @@ -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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Graeme,
On 22/03/16 11:27, Graeme Gregory wrote:
> On Tue, Mar 08, 2016 at 11:29:30AM +0000, Julien Grall wrote:
>> @@ -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;
>> }
>
> Placing this here means that it will not collect the info in the case
> where there is a Generic Interrupt Redistributor structure in the MADT.
>
> I guess collecting this info should not be a side effect of happening to
> pass GICC for redistributor base. Which unfortuneately probably means we
> do need to parse the GICCs specifically for this information.
I noticed it while testing on some platform. It's now fixed and will be
in the next version.
Regards,
--
Julien Grall