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@virtualopensystems.com
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@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm