[PATCH v2 0/6] arm64: Add support of KVM with ACPI

[PATCH v2 0/6] arm64: Add support of KVM with ACPI

[Julien Grall]

Hello,

This small 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 to duplicate the parsing code.

Note there is patch series ([1] and [2]) adding support of KVM on ACPI,
although the approach chosen is completely different. The code to parse
the firmware tables are duplicated which I think make more complex to
support new firmware tables.

See the changes since v1 in the different patches.

Regards,

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018482.html
[2] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018355.html

Julien Grall (6):
  KVM: arm/arm64: arch_timer: Gather KVM specific information in a
    structure
  KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the
    firmware tables
  irqchip/gic-v2: Gather ACPI specific data in a single structure
  irqchip/gic-v2: Parse and export virtual GIC information
  irqchip/gic-v3: Parse and export virtual GIC information
  KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware
    tables

 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           | 36 ++++++++++++++
 drivers/irqchip/irq-gic.c              | 91 ++++++++++++++++++++++++++++++++--
 include/clocksource/arm_arch_timer.h   | 13 ++---
 include/kvm/arm_vgic.h                 |  7 +--
 include/linux/irqchip/arm-gic-common.h | 34 +++++++++++++
 virt/kvm/arm/arch_timer.c              | 39 ++++-----------
 virt/kvm/arm/vgic-v2.c                 | 67 +++++++++----------------
 virt/kvm/arm/vgic-v3.c                 | 45 +++++------------
 virt/kvm/arm/vgic.c                    | 50 ++++++++++---------
 12 files changed, 264 insertions(+), 145 deletions(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

-- 
1.9.1

[PATCH v2 1/6] KVM: arm/arm64: arch_timer: Gather KVM specific information in a structure

[Julien Grall]

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.

This is also dropping arch_timer_get_timecounter as it was only used by
the KVM code. Furthermore, a stub for the new helper hasn't been
introduced because KVM is requiring the arch timer for both ARM64 and
ARM32.

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

---
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/clocksource/arm_arch_timer.c |  9 +++++----
 include/clocksource/arm_arch_timer.h | 12 ++++++------
 virt/kvm/arm/arch_timer.c            |  6 +++---
 3 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index c64d543..6eb2c5d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -447,11 +447,11 @@ static struct cyclecounter cyclecounter = {
 	.mask	= CLOCKSOURCE_MASK(56),
 };
 
-static struct timecounter timecounter;
+static struct arch_timer_kvm_info arch_timer_kvm_info;
 
-struct timecounter *arch_timer_get_timecounter(void)
+struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
 {
-	return &timecounter;
+	return &arch_timer_kvm_info;
 }
 
 static void __init arch_counter_register(unsigned type)
@@ -479,7 +479,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..4d487f8 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
 
@@ -67,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
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 69bca18..a669c6a 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -385,11 +385,11 @@ 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) {
-- 
1.9.1

Re: [PATCH v2 1/6] KVM: arm/arm64: arch_timer: Gather KVM specific information in a structure

[Wei Huang]

On 02/11/2016 09:33 AM, 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.
> 
> This is also dropping arch_timer_get_timecounter as it was only used by
> the KVM code. Furthermore, a stub for the new helper hasn't been
> introduced because KVM is requiring the arch timer for both ARM64 and
> ARM32.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  drivers/clocksource/arm_arch_timer.c |  9 +++++----
>  include/clocksource/arm_arch_timer.h | 12 ++++++------
>  virt/kvm/arm/arch_timer.c            |  6 +++---
>  3 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index c64d543..6eb2c5d 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -447,11 +447,11 @@ static struct cyclecounter cyclecounter = {
>  	.mask	= CLOCKSOURCE_MASK(56),
>  };
>  
> -static struct timecounter timecounter;
> +static struct arch_timer_kvm_info arch_timer_kvm_info;
>  
> -struct timecounter *arch_timer_get_timecounter(void)
> +struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
>  {
> -	return &timecounter;
> +	return &arch_timer_kvm_info;
>  }
>  
>  static void __init arch_counter_register(unsigned type)
> @@ -479,7 +479,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..4d487f8 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
>  
> @@ -67,11 +72,6 @@ static inline u64 arch_timer_read_counter(void)
>  	return 0;
>  }
>  
> -static inline struct timecounter *arch_timer_get_timecounter(void)
> -{
> -	return NULL;
> -}
> -

Most parts are OK. Regarding removing this function from the #else area,
is there a possibility to have CONFIG_ARM_ARCH_TIMER=n and CONFIG_KVM=y.
If so, will the compilation fails here?

-Wei

>  #endif
>  
>  #endif
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 69bca18..a669c6a 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -385,11 +385,11 @@ 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) {
> 

Re: [PATCH v2 1/6] KVM: arm/arm64: arch_timer: Gather KVM specific information in a structure

[Julien Grall]

Hello Wei,

On 19/02/16 07:24, Wei Huang wrote:
> On 02/11/2016 09:33 AM, Julien Grall wrote:

[...]

>> This is also dropping arch_timer_get_timecounter as it was only used by
>> the KVM code. Furthermore, a stub for the new helper hasn't been
>> introduced because KVM is requiring the arch timer for both ARM64 and
>> ARM32.


[...]

>> -static inline struct timecounter *arch_timer_get_timecounter(void)
>> -{
>> -    return NULL;
>> -}
>> -
>
> Most parts are OK. Regarding removing this function from the #else area,
> is there a possibility to have CONFIG_ARM_ARCH_TIMER=n and CONFIG_KVM=y.
> If so, will the compilation fails here?

This patch is removing arch_timer_get_timecounter. So it's normal to
drop the stub in the #else area.

Furthermore, as mentioned in the commit message, I didn't add a stub for
the new function because KVM mandates the arch timer for both ARM64 and
ARM32.

Regards,

--
Julien Grall
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH v2 1/6] KVM: arm/arm64: arch_timer: Gather KVM specific information in a structure

[Julien Grall]

On 19/02/16 10:53, Julien Grall wrote:
> --
> Julien Grall
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy
> the information in any medium. Thank you.

Apologies, this footer shouldn't have been there.

-- 
Julien Grall

Re: [PATCH v2 1/6] KVM: arm/arm64: arch_timer: Gather KVM specific information in a structure

[Marc Zyngier]

On 19/02/16 07:24, Wei Huang wrote:
> 
> 
> On 02/11/2016 09:33 AM, 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.
>>
>> This is also dropping arch_timer_get_timecounter as it was only used by
>> the KVM code. Furthermore, a stub for the new helper hasn't been
>> introduced because KVM is requiring the arch timer for both ARM64 and
>> ARM32.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Christoffer Dall <christoffer.dall@linaro.org>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Gleb Natapov <gleb@kernel.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  drivers/clocksource/arm_arch_timer.c |  9 +++++----
>>  include/clocksource/arm_arch_timer.h | 12 ++++++------
>>  virt/kvm/arm/arch_timer.c            |  6 +++---
>>  3 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index c64d543..6eb2c5d 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -447,11 +447,11 @@ static struct cyclecounter cyclecounter = {
>>  	.mask	= CLOCKSOURCE_MASK(56),
>>  };
>>  
>> -static struct timecounter timecounter;
>> +static struct arch_timer_kvm_info arch_timer_kvm_info;
>>  
>> -struct timecounter *arch_timer_get_timecounter(void)
>> +struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
>>  {
>> -	return &timecounter;
>> +	return &arch_timer_kvm_info;
>>  }
>>  
>>  static void __init arch_counter_register(unsigned type)
>> @@ -479,7 +479,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..4d487f8 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
>>  
>> @@ -67,11 +72,6 @@ static inline u64 arch_timer_read_counter(void)
>>  	return 0;
>>  }
>>  
>> -static inline struct timecounter *arch_timer_get_timecounter(void)
>> -{
>> -	return NULL;
>> -}
>> -
> 
> Most parts are OK. Regarding removing this function from the #else area,
> is there a possibility to have CONFIG_ARM_ARCH_TIMER=n and CONFIG_KVM=y.
> If so, will the compilation fails here?

On arm64, arch timers are not optional (see the "select ARM_ARCH_TIMER"
in arch/arm64/Kconfig). On 32bit, we have "depends on ARM_VIRT_EXT &&
ARM_LPAE && ARM_ARCH_TIMER", which nails it as well.

Thanks,

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

[PATCH v2 2/6] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

[Julien Grall]

The firmware table is currently parsed by the virt timer code in order
to retrieve the virtual interrupt. However, this is already done by the
arch timer driver and therefore make the parsing code duplicated twice.

Introduce an helper to get the virtual interrupt number and use it in
the virtual timer code.

With this changes, the support for ACPI is coming free.

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

---
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Gleb Natapov <gleb@kernel.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 drivers/clocksource/arm_arch_timer.c |  2 ++
 include/clocksource/arm_arch_timer.h |  1 +
 virt/kvm/arm/arch_timer.c            | 33 +++++++--------------------------
 3 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 6eb2c5d..3cdbefd 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -451,6 +451,8 @@ static struct arch_timer_kvm_info arch_timer_kvm_info;
 
 struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
 {
+	arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
+
 	return &arch_timer_kvm_info;
 }
 
diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
index 4d487f8..8890552 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
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index a669c6a..e4de549 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>
@@ -375,45 +374,28 @@ 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;
 
 	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");
+	host_vtimer_irq = info->virtual_irq;
+	if (host_vtimer_irq <= 0) {
+		kvm_err("kvm_arch_timer: can't find virtual timer info or config virtual timer interrupt\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;
-	}
-
-	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");
@@ -426,14 +408,13 @@ int kvm_timer_hyp_init(void)
 		goto out_free;
 	}
 
-	kvm_info("%s IRQ%d\n", np->name, ppi);
+	kvm_info("vtimer 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

Re: [PATCH v2 2/6] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

[Christoffer Dall]

On Thu, Feb 11, 2016 at 03:33:20PM +0000, Julien Grall wrote:
> The firmware table is currently parsed by the virt timer code in order
> to retrieve the virtual interrupt. However, this is already done by the
> arch timer driver and therefore make the parsing code duplicated twice.
> 
> Introduce an helper to get the virtual interrupt number and use it in
> the virtual timer code.
> 
> With this changes, the support for ACPI is coming free.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Gleb Natapov <gleb@kernel.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  drivers/clocksource/arm_arch_timer.c |  2 ++
>  include/clocksource/arm_arch_timer.h |  1 +
>  virt/kvm/arm/arch_timer.c            | 33 +++++++--------------------------
>  3 files changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 6eb2c5d..3cdbefd 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -451,6 +451,8 @@ static struct arch_timer_kvm_info arch_timer_kvm_info;
>  
>  struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
>  {
> +	arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
> +

it feels weird that we set this member on every retrieval of the pointer
to the struct?

wouldn't it be more natural to intialize the struct somewhere during
init and then this function just returns a pointer to the struct?

>  	return &arch_timer_kvm_info;
>  }
>  
> diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h
> index 4d487f8..8890552 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
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index a669c6a..e4de549 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>
> @@ -375,45 +374,28 @@ 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;
>  
>  	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");
> +	host_vtimer_irq = info->virtual_irq;
> +	if (host_vtimer_irq <= 0) {
> +		kvm_err("kvm_arch_timer: can't find virtual timer info or config virtual timer interrupt\n");

I don't understand the difference between the two cases in this error
message.

How about: "Invalid virtual timer IRQ: %d\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;
> -	}
> -
> -	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");
> @@ -426,14 +408,13 @@ int kvm_timer_hyp_init(void)
>  		goto out_free;
>  	}
>  
> -	kvm_info("%s IRQ%d\n", np->name, ppi);
> +	kvm_info("vtimer IRQ%d\n", host_vtimer_irq);

is "vtimer" a known concept anywhere?  If not, can you use some known
terms intead?

>  	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
> 

Thanks,
-Christoffer

Re: [PATCH v2 2/6] KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the firmware tables

[Julien Grall]

Hi Christoffer,

On 03/03/16 19:38, Christoffer Dall wrote:
> On Thu, Feb 11, 2016 at 03:33:20PM +0000, Julien Grall wrote:

[...]

>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 6eb2c5d..3cdbefd 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -451,6 +451,8 @@ static struct arch_timer_kvm_info arch_timer_kvm_info;
>>
>>   struct arch_timer_kvm_info *arch_timer_get_kvm_info(void)
>>   {
>> +	arch_timer_kvm_info.virtual_irq = arch_timer_ppi[VIRT_PPI];
>> +
>
> it feels weird that we set this member on every retrieval of the pointer
> to the struct?
>
> wouldn't it be more natural to intialize the struct somewhere during
> init and then this function just returns a pointer to the struct?

True, I will move the initialization into arch_timer_common_init.

>
>>   	return &arch_timer_kvm_info;
>>   }
>>

[...]

>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index a669c6a..e4de549 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>
>> @@ -375,45 +374,28 @@ 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;
>>
>>   	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");
>> +	host_vtimer_irq = info->virtual_irq;
>> +	if (host_vtimer_irq <= 0) {
>> +		kvm_err("kvm_arch_timer: can't find virtual timer info or config virtual timer interrupt\n");
>
> I don't understand the difference between the two cases in this error
> message.
>
> How about: "Invalid virtual timer IRQ: %d\n" ?

I will update the error message.

>
>>   		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;
>> -	}
>> -
>> -	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");
>> @@ -426,14 +408,13 @@ int kvm_timer_hyp_init(void)
>>   		goto out_free;
>>   	}
>>
>> -	kvm_info("%s IRQ%d\n", np->name, ppi);
>> +	kvm_info("vtimer IRQ%d\n", host_vtimer_irq);
>
> is "vtimer" a known concept anywhere?  If not, can you use some known
> terms intead?

I will use "virtual timer".

>
>>   	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;
>>   }

Regards,

-- 
Julien Grall

[PATCH v2 3/6] irqchip/gic-v2: Gather ACPI specific data in a single structure

[Julien Grall]

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

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

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

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

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 911758c..d6131c4 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1246,7 +1246,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,
@@ -1266,10 +1269,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;
 }
@@ -1317,7 +1320,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

[PATCH v2 4/6] irqchip/gic-v2: Parse and export virtual GIC information

[Julien Grall]

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

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

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

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

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

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0..704caf4 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,19 @@
 
 #include "irq-gic-common.h"
 
+static const struct gic_kvm_info *gic_kvm_info;
+
+const struct gic_kvm_info *gic_get_kvm_info(void)
+{
+	return gic_kvm_info;
+}
+
+void gic_set_kvm_info(const struct gic_kvm_info *info)
+{
+	WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");
+	gic_kvm_info = info;
+}
+
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
 		void *data)
 {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index fff697d..205e5fd 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -19,6 +19,7 @@
 
 #include <linux/of.h>
 #include <linux/irqdomain.h>
+#include <linux/irqchip/arm-gic-common.h>
 
 struct gic_quirk {
 	const char *desc;
@@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void (*sync_access)(void));
 void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
 		void *data);
 
+void gic_set_kvm_info(const struct gic_kvm_info *info);
+
 #endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index d6131c4..b794fd0 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)
 {
@@ -1190,6 +1192,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)
 {
@@ -1219,8 +1250,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);
@@ -1249,6 +1282,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
@@ -1273,6 +1310,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;
 }
@@ -1303,6 +1346,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)
@@ -1360,6 +1436,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

[PATCH v2 5/6] irqchip/gic-v3: Parse and export virtual GIC information

[Julien Grall]

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

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

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

    Changes in 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           | 36 ++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-common.h |  1 +
 2 files changed, 37 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index d7be6dd..9992600 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -26,6 +26,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>
@@ -53,6 +54,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)
@@ -811,6 +814,37 @@ static void gicv3_enable_quirks(void)
 #endif
 }
 
+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;
@@ -908,6 +942,8 @@ static int __init gic_of_init(struct device_node *node, struct device_node *pare
 	gic_cpu_init();
 	gic_cpu_pm_init();
 
+	gic_of_setup_kvm_info(node);
+
 	return 0;
 
 out_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

[PATCH v2 6/6] KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware tables

[Julien Grall]

For now, 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 retrieve
information about the virtual GIC.

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

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

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

    Changes in 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 043032c..748cb2f 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

Re: [PATCH v2 0/6] arm64: Add support of KVM with ACPI

[Wei Huang]

On 02/11/2016 09:33 AM, Julien Grall wrote:
> Hello,
> 
> This small 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 to duplicate the parsing code.
> 
> Note there is patch series ([1] and [2]) adding support of KVM on ACPI,
> although the approach chosen is completely different. The code to parse
> the firmware tables are duplicated which I think make more complex to
> support new firmware tables.

I backported these patches to my internal tree. It booted on an ARM64
machine. Even though I haven't got the chance to test it on an GICv3
machine (will update later), I think you can add my name as Tested-by if
needed.

-Wei

> 
> See the changes since v1 in the different patches.
> 
> Regards,
> 
> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018482.html
> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018355.html
> 
> Julien Grall (6):
>   KVM: arm/arm64: arch_timer: Gather KVM specific information in a
>     structure
>   KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the
>     firmware tables
>   irqchip/gic-v2: Gather ACPI specific data in a single structure
>   irqchip/gic-v2: Parse and export virtual GIC information
>   irqchip/gic-v3: Parse and export virtual GIC information
>   KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware
>     tables
> 
>  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           | 36 ++++++++++++++
>  drivers/irqchip/irq-gic.c              | 91 ++++++++++++++++++++++++++++++++--
>  include/clocksource/arm_arch_timer.h   | 13 ++---
>  include/kvm/arm_vgic.h                 |  7 +--
>  include/linux/irqchip/arm-gic-common.h | 34 +++++++++++++
>  virt/kvm/arm/arch_timer.c              | 39 ++++-----------
>  virt/kvm/arm/vgic-v2.c                 | 67 +++++++++----------------
>  virt/kvm/arm/vgic-v3.c                 | 45 +++++------------
>  virt/kvm/arm/vgic.c                    | 50 ++++++++++---------
>  12 files changed, 264 insertions(+), 145 deletions(-)
>  create mode 100644 include/linux/irqchip/arm-gic-common.h
> 

Re: [PATCH v2 0/6] arm64: Add support of KVM with ACPI

[Huang Shijie]

On Thu, Feb 11, 2016 at 03:33:18PM +0000, Julien Grall wrote:
> Hello,
>
> This small 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 to duplicate the parsing code.
>
> Note there is patch series ([1] and [2]) adding support of KVM on ACPI,
> although the approach chosen is completely different. The code to parse
> the firmware tables are duplicated which I think make more complex to
> support new firmware tables.
>
> See the changes since v1 in the different patches.
>
> Regards,
>
> [1] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018482.html
> [2] https://lists.cs.columbia.edu/pipermail/kvmarm/2016-February/018355.html
>
> Julien Grall (6):
>   KVM: arm/arm64: arch_timer: Gather KVM specific information in a
>     structure
>   KVM: arm/arm64: arch_timer: Rely on the arch timer to parse the
>     firmware tables
>   irqchip/gic-v2: Gather ACPI specific data in a single structure
>   irqchip/gic-v2: Parse and export virtual GIC information
>   irqchip/gic-v3: Parse and export virtual GIC information
Beside this patch (I do not have the gic-v3 hardware now), I tested this patch set with my JUNo-r1 board
with kvmtool. It works fine.

Tested-by: Huang Shijie <shijie.huang@arm.com>



>   KVM: arm/arm64: vgic: Rely on the GIC driver to parse the firmware
>     tables
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.