Revisited Alexander's patches, I felt that the approach of
if-DT-else-ACPI approach isn't necessary. More importantly this whole DT
parsing in ./virt/kvm/arm can really be avoided. My opinions are:
1. All DT-ACPI should happen in respective drivers. In this case, they
should be in clocksource/arm_arch_timer.c and irqchip/irq-gic* only. The
driver will expose necessary info to KVM via public interfaces.
2. KVM doesn't have to care about DT/ACPI. Instead it only uses the info
provided by the interface from device drivers.
The attached patch demos how to do it in arch_timer. GIC can follow the
same approach (though trickier than arch_timer, due to GICv2 and GICv3).
Any though?
-Wei
On 09/15/2014 04:41 PM, Wei Huang wrote:
On 09/12/2014 05:56 PM, Alexander Spyridakis wrote:
With ACPI enabled, kvm_timer_hyp_init can't access any device tree
information. Although registration of the virtual timer interrupt
already happened when architected timers were initialized, we need to
point KVM to the interrupt line used.
Signed-off-by: Alexander Spyridakis <a.spyridakis@xxxxxxxxxxxxxxxxxxxxxx>
---
virt/kvm/arm/arch_timer.c | 48
+++++++++++++++++++++++++++++++++++------------
1 file changed, 36 insertions(+), 12 deletions(-)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 22fa819..bca7175 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -21,6 +21,7 @@
#include <linux/kvm.h>
#include <linux/kvm_host.h>
#include <linux/interrupt.h>
+#include <linux/acpi.h>
#include <clocksource/arm_arch_timer.h>
#include <asm/arch_timer.h>
@@ -244,6 +245,15 @@ static const struct of_device_id
arch_timer_of_match[] = {
{},
};
+#ifdef CONFIG_ACPI
+struct acpi_table_gtdt *gtdt_acpi;
+
+static void arch_timer_acpi_parse(struct acpi_table_header *table)
+{
+ gtdt_acpi = container_of(table, struct acpi_table_gtdt, header);
+}
+#endif
+
int kvm_timer_hyp_init(void)
{
struct device_node *np;
@@ -254,17 +264,32 @@ int kvm_timer_hyp_init(void)
if (!timecounter)
return -ENODEV;
- np = of_find_matching_node(NULL, arch_timer_of_match);
- if (!np) {
- kvm_err("kvm_arch_timer: can't find DT node\n");
- return -ENODEV;
- }
-
- ppi = irq_of_parse_and_map(np, 2);
- if (!ppi) {
- kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
- err = -EINVAL;
- goto out;
+ if (acpi_disabled) {
+ np = of_find_matching_node(NULL, arch_timer_of_match);
+ if (!np) {
+ kvm_err("kvm_arch_timer: can't find DT node\n");
+ return -ENODEV;
+ }
+
+ ppi = irq_of_parse_and_map(np, 2);
+ if (!ppi) {
+ kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
+ err = -EINVAL;
+ goto out;
+ }
+ kvm_info("%s IRQ%d\n", np->name, ppi);
+ } else {
+ /* The virtual timer interrupt was already
+ * registered during initialization with ACPI.
+ * Get the interrupt number from the tables
+ * and point there.
+ */
+ acpi_table_parse(ACPI_SIG_GTDT,
+ (acpi_tbl_table_handler)arch_timer_acpi_parse);
+ if (!gtdt_acpi->virtual_timer_interrupt)
+ return -EINVAL;
+ ppi = gtdt_acpi->virtual_timer_interrupt;
I am thinking that we need to get the registered IRQ here. In other
words, should we use ppi from arch_timer_ppi[VIRT_PPI] here?
+ kvm_info("timer IRQ%d\n", ppi);
}
Could we merge the two kvm_info() into one? kvm_info("timer IRQ%d\n",
ppi) can serve the purpose well for both cases.
err = request_percpu_irq(ppi, kvm_arch_timer_handler,
@@ -289,7 +314,6 @@ int kvm_timer_hyp_init(void)
goto out_free;
}
- kvm_info("%s IRQ%d\n", np->name, ppi);
on_each_cpu(kvm_timer_init_interrupt, NULL, 1);
goto out;
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
commit 855ccb8292f0a6d5d8c83b8c95cd7939992ebc97
Author: Wei Huang <wei@xxxxxxxxxx>
Date: Mon Oct 6 11:43:07 2014 -0400
ARM/KVM: remove DT parsing in KVMfor arch_timer virt PPI
Current KVM parses arch_timer virt PPI from device tree. But such
info has been made available by arm_arch_timer driver during booting.
This patch retrieves PPI from arm_arch_timer driver to avoid
calling DT functions again.
More importantly this patch eliminates the dependency on DT or ACPI.
So when ACPI code is added to arm_arch_timer, KVM code doesn't need
change.
Signed-off-by: Wei Huang <wei@xxxxxxxxxx>
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5163ec1..6d3225e 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -53,22 +53,22 @@ struct arch_timer {
static u32 arch_timer_rate;
-enum ppi_nr {
- PHYS_SECURE_PPI,
- PHYS_NONSECURE_PPI,
- VIRT_PPI,
- HYP_PPI,
- MAX_TIMER_PPI
-};
-
-static int arch_timer_ppi[MAX_TIMER_PPI];
-
static struct clock_event_device __percpu *arch_timer_evt;
static bool arch_timer_use_virtual = true;
static bool arch_timer_c3stop;
static bool arch_timer_mem_use_virtual;
+static int arch_timer_ppi[MAX_TIMER_PPI];
+
+int arch_timer_get_ppi(enum ppi_nr idx)
+{
+ if (idx < MAX_TIMER_PPI)
+ return arch_timer_ppi[idx];
+
+ return 0;
+}
+
/*
* Architected system timer support.
*/
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 6d26b40..6efb4b4 100644
--- a/include/clocksource/arm_arch_timer.h
+++ b/include/clocksource/arm_arch_timer.h
@@ -28,6 +28,14 @@ enum arch_timer_reg {
ARCH_TIMER_REG_TVAL,
};
+enum ppi_nr {
+ PHYS_SECURE_PPI,
+ PHYS_NONSECURE_PPI,
+ VIRT_PPI,
+ HYP_PPI,
+ MAX_TIMER_PPI
+};
+
#define ARCH_TIMER_PHYS_ACCESS 0
#define ARCH_TIMER_VIRT_ACCESS 1
#define ARCH_TIMER_MEM_PHYS_ACCESS 2
@@ -48,6 +56,7 @@ enum arch_timer_reg {
extern u32 arch_timer_get_rate(void);
extern u64 (*arch_timer_read_counter)(void);
extern struct timecounter *arch_timer_get_timecounter(void);
+extern int arch_timer_get_ppi(enum ppi_nr idx);
#else
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 22fa819..7e0cc81 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>
@@ -238,15 +237,8 @@ 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;
int err;
@@ -254,13 +246,7 @@ int kvm_timer_hyp_init(void)
if (!timecounter)
return -ENODEV;
- np = of_find_matching_node(NULL, arch_timer_of_match);
- if (!np) {
- kvm_err("kvm_arch_timer: can't find DT node\n");
- return -ENODEV;
- }
-
- ppi = irq_of_parse_and_map(np, 2);
+ ppi = arch_timer_get_ppi(VIRT_PPI);
if (!ppi) {
kvm_err("kvm_arch_timer: no virtual timer interrupt\n");
err = -EINVAL;
@@ -289,14 +275,13 @@ int kvm_timer_hyp_init(void)
goto out_free;
}
- kvm_info("%s IRQ%d\n", np->name, ppi);
+ kvm_info("timer IRQ%d\n", ppi);
on_each_cpu(kvm_timer_init_interrupt, NULL, 1);
goto out;
out_free:
free_percpu_irq(ppi, kvm_get_running_vcpus());
out:
- of_node_put(np);
return err;
}
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
