[3/5] irqchip/gic-v2: Parse and export virtual GIC information
diff mbox

Message ID 1454950049-741-4-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall Feb. 8, 2016, 4:47 p.m. UTC
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>

 drivers/irqchip/irq-gic-common.c       | 13 ++++++
 drivers/irqchip/irq-gic-common.h       |  3 ++
 drivers/irqchip/irq-gic.c              | 78 +++++++++++++++++++++++++++++++++-
 include/linux/irqchip/arm-gic-common.h | 34 +++++++++++++++
 4 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/irqchip/arm-gic-common.h

Comments

Marc Zyngier Feb. 8, 2016, 6:30 p.m. UTC | #1
Julien,

On 08/02/16 16:47, 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 <julien.grall@arm.com>
> ---
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
>  drivers/irqchip/irq-gic-common.c       | 13 ++++++
>  drivers/irqchip/irq-gic-common.h       |  3 ++
>  drivers/irqchip/irq-gic.c              | 78 +++++++++++++++++++++++++++++++++-
>  include/linux/irqchip/arm-gic-common.h | 34 +++++++++++++++
>  4 files changed, 127 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 911758c..d3a09a4 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,44 @@ 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;
> +	unsigned int irq;
> +
> +	gic_v2_kvm_info.type = GIC_V2;
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (!irq)
> +		gic_v2_kvm_info.maint_irq = -1;

Please don't do that. 0 *is* the value for an invalid interrupt, and
this is what you should expose here. Same for GICv3.

> +	else
> +		gic_v2_kvm_info.maint_irq = irq;
> +
> +	ret = of_address_to_resource(node, 2, &r);
> +	if (!ret) {
> +		gic_v2_kvm_info.vctrl_base = r.start;
> +		gic_v2_kvm_info.vctrl_size = resource_size(&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_base = r.start;
> +			gic_v2_kvm_info.vcpu_size = resource_size(&r);

This tends to make me think that this should actually be a proper
resource, and not a set of arbitrary fields.

> +		}
> +	}
> +
> +	gic_set_kvm_info(&gic_v2_kvm_info);
> +}
> +
>  int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
> @@ -1219,8 +1259,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);
> @@ -1247,6 +1289,32 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>  
>  #ifdef CONFIG_ACPI
>  static phys_addr_t cpu_phy_base __initdata;
> +static struct
> +{
> +	u32 maint_irq;
> +	int maint_irq_mode;
> +	phys_addr_t vctrl_base;
> +	phys_addr_t vcpu_base;
> +} acpi_data __initdata;
> +
> +static void __init gic_acpi_setup_kvm_info(void)
> +{
> +	gic_v2_kvm_info.type = GIC_V2;
> +
> +	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
> +						      acpi_data.maint_irq,
> +						      acpi_data.maint_irq_mode,
> +						      ACPI_ACTIVE_HIGH);
> +	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
> +	if (gic_v2_kvm_info.vctrl_base)
> +		gic_v2_kvm_info.vctrl_size = SZ_8K;
> +
> +	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
> +	if (gic_v2_kvm_info.vcpu_base)
> +		gic_v2_kvm_info.vcpu_size = SZ_8K;
> +
> +	gic_set_kvm_info(&gic_v2_kvm_info);
> +}
>  
>  static int __init
>  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> @@ -1270,6 +1338,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>  		return -EINVAL;
>  
>  	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;
> +

Maybe you can now move all the ACPI data into this acpi_data structure?
This would allow for slightly less clutter...

>  	cpu_base_assigned = 1;
>  	return 0;
>  }
> @@ -1357,6 +1431,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..30972b1
> --- /dev/null
> +++ b/include/linux/irqchip/arm-gic-common.h
> @@ -0,0 +1,34 @@
> +/*
> + * 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>
> +
> +enum gic_type {
> +	GIC_V2,
> +};
> +
> +struct gic_kvm_info {
> +	/* GIC type */
> +	enum gic_type	type;
> +	/* Physical address & size of virtual cpu interface */
> +	phys_addr_t	vcpu_base;
> +	resource_size_t	vcpu_size;
> +	/* Interrupt number */
> +	int		maint_irq;
> +	/* Physical address & size of virtual control interface */
> +	phys_addr_t	vctrl_base;
> +	resource_size_t	vctrl_size;
> +};
> +
> +const struct gic_kvm_info *gic_get_kvm_info(void);
> +
> +#endif /* __LINUX_IRQCHIP_ARM_GIC_COMMON_H */
> 

Thanks,

	M.
Julien Grall Feb. 9, 2016, 11:23 a.m. UTC | #2
On 08/02/16 18:30, Marc Zyngier wrote:
> Julien,

Hi Marc,

> On 08/02/16 16:47, Julien Grall wrote:

[...]

>> +static void __init gic_of_setup_kvm_info(struct device_node *node)
>> +{
>> +	int ret;
>> +	struct resource r;
>> +	unsigned int irq;
>> +
>> +	gic_v2_kvm_info.type = GIC_V2;
>> +
>> +	irq = irq_of_parse_and_map(node, 0);
>> +	if (!irq)
>> +		gic_v2_kvm_info.maint_irq = -1;
>
> Please don't do that. 0 *is* the value for an invalid interrupt, and
> this is what you should expose here. Same for GICv3.

I decided to use -1, because the function acpi_register_gsi is returning 
a negative value when an error occurred.

AFAICT, returning 0 would be a valid value for acpi_register_gsi.

>> +	else
>> +		gic_v2_kvm_info.maint_irq = irq;
>> +
>> +	ret = of_address_to_resource(node, 2, &r);
>> +	if (!ret) {
>> +		gic_v2_kvm_info.vctrl_base = r.start;
>> +		gic_v2_kvm_info.vctrl_size = resource_size(&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_base = r.start;
>> +			gic_v2_kvm_info.vcpu_size = resource_size(&r);
>
> This tends to make me think that this should actually be a proper
> resource, and not a set of arbitrary fields.

I will give a look.

[..]

>> @@ -1270,6 +1338,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>>   		return -EINVAL;
>>
>>   	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;
>> +
>
> Maybe you can now move all the ACPI data into this acpi_data structure?
> This would allow for slightly less clutter...

Good idea, I will do it in the next version.

[...]

>
> Thanks,

Regards,
Marc Zyngier Feb. 9, 2016, 11:31 a.m. UTC | #3
On 09/02/16 11:23, Julien Grall wrote:
> On 08/02/16 18:30, Marc Zyngier wrote:
>> Julien,
> 
> Hi Marc,
> 
>> On 08/02/16 16:47, Julien Grall wrote:
> 
> [...]
> 
>>> +static void __init gic_of_setup_kvm_info(struct device_node *node)
>>> +{
>>> +	int ret;
>>> +	struct resource r;
>>> +	unsigned int irq;
>>> +
>>> +	gic_v2_kvm_info.type = GIC_V2;
>>> +
>>> +	irq = irq_of_parse_and_map(node, 0);
>>> +	if (!irq)
>>> +		gic_v2_kvm_info.maint_irq = -1;
>>
>> Please don't do that. 0 *is* the value for an invalid interrupt, and
>> this is what you should expose here. Same for GICv3.
> 
> I decided to use -1, because the function acpi_register_gsi is returning 
> a negative value when an error occurred.
> 
> AFAICT, returning 0 would be a valid value for acpi_register_gsi.

No really. It either returns -EINVAL, or the result of
irq_create_fwspec_mapping, which is either going to return a virtual
interrupt number (strictly positive), or zero if the mapping was impossible.

So anything <= 0 is an error, and you can use 0 to indicate it. In
general, there is no such thing as IRQ0 (if you're not convinced, please
argue with Linus: http://yarchive.net/comp/linux/zero.html).

>>> +	else
>>> +		gic_v2_kvm_info.maint_irq = irq;
>>> +
>>> +	ret = of_address_to_resource(node, 2, &r);
>>> +	if (!ret) {
>>> +		gic_v2_kvm_info.vctrl_base = r.start;
>>> +		gic_v2_kvm_info.vctrl_size = resource_size(&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_base = r.start;
>>> +			gic_v2_kvm_info.vcpu_size = resource_size(&r);
>>
>> This tends to make me think that this should actually be a proper
>> resource, and not a set of arbitrary fields.
> 
> I will give a look.
> 
> [..]
> 
>>> @@ -1270,6 +1338,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
>>>   		return -EINVAL;
>>>
>>>   	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;
>>> +
>>
>> Maybe you can now move all the ACPI data into this acpi_data structure?
>> This would allow for slightly less clutter...
> 
> Good idea, I will do it in the next version.

Thanks,

	M.
Christoffer Dall Feb. 9, 2016, 8:49 p.m. UTC | #4
On Mon, Feb 08, 2016 at 04:47:27PM +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 <julien.grall@arm.com>
> ---
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> 
>  drivers/irqchip/irq-gic-common.c       | 13 ++++++
>  drivers/irqchip/irq-gic-common.h       |  3 ++
>  drivers/irqchip/irq-gic.c              | 78 +++++++++++++++++++++++++++++++++-
>  include/linux/irqchip/arm-gic-common.h | 34 +++++++++++++++
>  4 files changed, 127 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 911758c..d3a09a4 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,44 @@ 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;
> +	unsigned int irq;
> +
> +	gic_v2_kvm_info.type = GIC_V2;
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (!irq)
> +		gic_v2_kvm_info.maint_irq = -1;
> +	else
> +		gic_v2_kvm_info.maint_irq = irq;
> +
> +	ret = of_address_to_resource(node, 2, &r);
> +	if (!ret) {
> +		gic_v2_kvm_info.vctrl_base = r.start;
> +		gic_v2_kvm_info.vctrl_size = resource_size(&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_base = r.start;
> +			gic_v2_kvm_info.vcpu_size = resource_size(&r);
> +		}
> +	}
> +
> +	gic_set_kvm_info(&gic_v2_kvm_info);
> +}
> +
>  int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
> @@ -1219,8 +1259,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);
> @@ -1247,6 +1289,32 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>  
>  #ifdef CONFIG_ACPI
>  static phys_addr_t cpu_phy_base __initdata;
> +static struct
> +{
> +	u32 maint_irq;
> +	int maint_irq_mode;
> +	phys_addr_t vctrl_base;
> +	phys_addr_t vcpu_base;
> +} acpi_data __initdata;
> +
> +static void __init gic_acpi_setup_kvm_info(void)
> +{
> +	gic_v2_kvm_info.type = GIC_V2;
> +
> +	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
> +						      acpi_data.maint_irq,
> +						      acpi_data.maint_irq_mode,
> +						      ACPI_ACTIVE_HIGH);
> +	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
> +	if (gic_v2_kvm_info.vctrl_base)
> +		gic_v2_kvm_info.vctrl_size = SZ_8K;
> +
> +	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
> +	if (gic_v2_kvm_info.vcpu_base)
> +		gic_v2_kvm_info.vcpu_size = SZ_8K;

why are the sizes hard-coded to 8K in this case?

Thanks,
-Christoffer
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wei Huang Feb. 9, 2016, 9:57 p.m. UTC | #5
On 02/09/2016 02:49 PM, Christoffer Dall wrote:
> On Mon, Feb 08, 2016 at 04:47:27PM +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 <julien.grall@arm.com>
>> ---
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Jason Cooper <jason@lakedaemon.net>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>
>>  drivers/irqchip/irq-gic-common.c       | 13 ++++++
>>  drivers/irqchip/irq-gic-common.h       |  3 ++
>>  drivers/irqchip/irq-gic.c              | 78 +++++++++++++++++++++++++++++++++-
>>  include/linux/irqchip/arm-gic-common.h | 34 +++++++++++++++
>>  4 files changed, 127 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 911758c..d3a09a4 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,44 @@ 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;
>> +	unsigned int irq;
>> +
>> +	gic_v2_kvm_info.type = GIC_V2;
>> +
>> +	irq = irq_of_parse_and_map(node, 0);
>> +	if (!irq)
>> +		gic_v2_kvm_info.maint_irq = -1;
>> +	else
>> +		gic_v2_kvm_info.maint_irq = irq;
>> +
>> +	ret = of_address_to_resource(node, 2, &r);
>> +	if (!ret) {
>> +		gic_v2_kvm_info.vctrl_base = r.start;
>> +		gic_v2_kvm_info.vctrl_size = resource_size(&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_base = r.start;
>> +			gic_v2_kvm_info.vcpu_size = resource_size(&r);
>> +		}
>> +	}
>> +
>> +	gic_set_kvm_info(&gic_v2_kvm_info);
>> +}
>> +
>>  int __init
>>  gic_of_init(struct device_node *node, struct device_node *parent)
>>  {
>> @@ -1219,8 +1259,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);
>> @@ -1247,6 +1289,32 @@ IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
>>  
>>  #ifdef CONFIG_ACPI
>>  static phys_addr_t cpu_phy_base __initdata;
>> +static struct
>> +{
>> +	u32 maint_irq;
>> +	int maint_irq_mode;
>> +	phys_addr_t vctrl_base;
>> +	phys_addr_t vcpu_base;
>> +} acpi_data __initdata;
>> +
>> +static void __init gic_acpi_setup_kvm_info(void)
>> +{
>> +	gic_v2_kvm_info.type = GIC_V2;
>> +
>> +	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
>> +						      acpi_data.maint_irq,
>> +						      acpi_data.maint_irq_mode,
>> +						      ACPI_ACTIVE_HIGH);
>> +	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
>> +	if (gic_v2_kvm_info.vctrl_base)
>> +		gic_v2_kvm_info.vctrl_size = SZ_8K;
>> +
>> +	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
>> +	if (gic_v2_kvm_info.vcpu_base)
>> +		gic_v2_kvm_info.vcpu_size = SZ_8K;
> 
> why are the sizes hard-coded to 8K in this case?

ACPI doesn't provide the size info for GICH and GICV resources. So we
have to pick a default one, which is 8K.

> 
> Thanks,
> -Christoffer
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julien Grall Feb. 10, 2016, 2:19 p.m. UTC | #6
Hi Christoffer,

On 09/02/16 20:49, Christoffer Dall wrote:
>> +static void __init gic_acpi_setup_kvm_info(void)
>> +{
>> +	gic_v2_kvm_info.type = GIC_V2;
>> +
>> +	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
>> +						      acpi_data.maint_irq,
>> +						      acpi_data.maint_irq_mode,
>> +						      ACPI_ACTIVE_HIGH);
>> +	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
>> +	if (gic_v2_kvm_info.vctrl_base)
>> +		gic_v2_kvm_info.vctrl_size = SZ_8K;
>> +
>> +	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
>> +	if (gic_v2_kvm_info.vcpu_base)
>> +		gic_v2_kvm_info.vcpu_size = SZ_8K;
>
> why are the sizes hard-coded to 8K in this case?

The MADT only provides the base addresses and not the size. The default 
value has been chosen based on the GICv2 spec (ARM IHI 0048B.b)
	* GICV: See 5.5
	* GICH: I can't find again the section about it. But the example 
bindings in 
Documents/devicetree/bindings/interrupt-controller/arm,gic.txt uses 8K.

I will add a comment in the code explaining where the 8K come from.

Cheers,
Marc Zyngier Feb. 10, 2016, 2:46 p.m. UTC | #7
On 10/02/16 14:19, Julien Grall wrote:
> Hi Christoffer,
> 
> On 09/02/16 20:49, Christoffer Dall wrote:
>>> +static void __init gic_acpi_setup_kvm_info(void)
>>> +{
>>> +	gic_v2_kvm_info.type = GIC_V2;
>>> +
>>> +	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
>>> +						      acpi_data.maint_irq,
>>> +						      acpi_data.maint_irq_mode,
>>> +						      ACPI_ACTIVE_HIGH);
>>> +	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
>>> +	if (gic_v2_kvm_info.vctrl_base)
>>> +		gic_v2_kvm_info.vctrl_size = SZ_8K;
>>> +
>>> +	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
>>> +	if (gic_v2_kvm_info.vcpu_base)
>>> +		gic_v2_kvm_info.vcpu_size = SZ_8K;
>>
>> why are the sizes hard-coded to 8K in this case?
> 
> The MADT only provides the base addresses and not the size. The default 
> value has been chosen based on the GICv2 spec (ARM IHI 0048B.b)
> 	* GICV: See 5.5
> 	* GICH: I can't find again the section about it. But the example 
> bindings in 
> Documents/devicetree/bindings/interrupt-controller/arm,gic.txt uses 8K.
> 
> I will add a comment in the code explaining where the 8K come from.

The GICH size can be found in the GIC400 TRM:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/CHDBJDCB.html

The first 4kB are banked per CPU, while the next 4kB are exposing all
CPUs, each in a 512 bytes window. We don't give a damn about the second
page, but hey, it is there...

Of course, this is GIC400, not the architecture spec. So maybe
considering a 4kB size would be better, just in case someone is
braindead enough to produce another GICv2 implementation without the
aliases...

	M.
Julien Grall Feb. 10, 2016, 3:22 p.m. UTC | #8
Hi Marc,

On 10/02/16 14:46, Marc Zyngier wrote:
> On 10/02/16 14:19, Julien Grall wrote:
>> On 09/02/16 20:49, Christoffer Dall wrote:
>>>> +static void __init gic_acpi_setup_kvm_info(void)
>>>> +{
>>>> +	gic_v2_kvm_info.type = GIC_V2;
>>>> +
>>>> +	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
>>>> +						      acpi_data.maint_irq,
>>>> +						      acpi_data.maint_irq_mode,
>>>> +						      ACPI_ACTIVE_HIGH);
>>>> +	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
>>>> +	if (gic_v2_kvm_info.vctrl_base)
>>>> +		gic_v2_kvm_info.vctrl_size = SZ_8K;
>>>> +
>>>> +	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
>>>> +	if (gic_v2_kvm_info.vcpu_base)
>>>> +		gic_v2_kvm_info.vcpu_size = SZ_8K;
>>>
>>> why are the sizes hard-coded to 8K in this case?
>>
>> The MADT only provides the base addresses and not the size. The default
>> value has been chosen based on the GICv2 spec (ARM IHI 0048B.b)
>> 	* GICV: See 5.5
>> 	* GICH: I can't find again the section about it. But the example
>> bindings in
>> Documents/devicetree/bindings/interrupt-controller/arm,gic.txt uses 8K.
>>
>> I will add a comment in the code explaining where the 8K come from.
>
> The GICH size can be found in the GIC400 TRM:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0471b/CHDBJDCB.html
>
> The first 4kB are banked per CPU, while the next 4kB are exposing all
> CPUs, each in a 512 bytes window. We don't give a damn about the second
> page, but hey, it is there...
>
> Of course, this is GIC400, not the architecture spec. So maybe
> considering a 4kB size would be better, just in case someone is
> braindead enough to produce another GICv2 implementation without the
> aliases...

Ok, I will change the GICH to 4KB in the next version.

Cheers,

Patch
diff mbox

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 911758c..d3a09a4 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,44 @@  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;
+	unsigned int irq;
+
+	gic_v2_kvm_info.type = GIC_V2;
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (!irq)
+		gic_v2_kvm_info.maint_irq = -1;
+	else
+		gic_v2_kvm_info.maint_irq = irq;
+
+	ret = of_address_to_resource(node, 2, &r);
+	if (!ret) {
+		gic_v2_kvm_info.vctrl_base = r.start;
+		gic_v2_kvm_info.vctrl_size = resource_size(&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_base = r.start;
+			gic_v2_kvm_info.vcpu_size = resource_size(&r);
+		}
+	}
+
+	gic_set_kvm_info(&gic_v2_kvm_info);
+}
+
 int __init
 gic_of_init(struct device_node *node, struct device_node *parent)
 {
@@ -1219,8 +1259,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);
@@ -1247,6 +1289,32 @@  IRQCHIP_DECLARE(pl390, "arm,pl390", gic_of_init);
 
 #ifdef CONFIG_ACPI
 static phys_addr_t cpu_phy_base __initdata;
+static struct
+{
+	u32 maint_irq;
+	int maint_irq_mode;
+	phys_addr_t vctrl_base;
+	phys_addr_t vcpu_base;
+} acpi_data __initdata;
+
+static void __init gic_acpi_setup_kvm_info(void)
+{
+	gic_v2_kvm_info.type = GIC_V2;
+
+	gic_v2_kvm_info.maint_irq = acpi_register_gsi(NULL,
+						      acpi_data.maint_irq,
+						      acpi_data.maint_irq_mode,
+						      ACPI_ACTIVE_HIGH);
+	gic_v2_kvm_info.vctrl_base = acpi_data.vctrl_base;
+	if (gic_v2_kvm_info.vctrl_base)
+		gic_v2_kvm_info.vctrl_size = SZ_8K;
+
+	gic_v2_kvm_info.vcpu_base = acpi_data.vcpu_base;
+	if (gic_v2_kvm_info.vcpu_base)
+		gic_v2_kvm_info.vcpu_size = SZ_8K;
+
+	gic_set_kvm_info(&gic_v2_kvm_info);
+}
 
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
@@ -1270,6 +1338,12 @@  gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
 		return -EINVAL;
 
 	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;
 }
@@ -1357,6 +1431,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..30972b1
--- /dev/null
+++ b/include/linux/irqchip/arm-gic-common.h
@@ -0,0 +1,34 @@ 
+/*
+ * 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>
+
+enum gic_type {
+	GIC_V2,
+};
+
+struct gic_kvm_info {
+	/* GIC type */
+	enum gic_type	type;
+	/* Physical address & size of virtual cpu interface */
+	phys_addr_t	vcpu_base;
+	resource_size_t	vcpu_size;
+	/* Interrupt number */
+	int		maint_irq;
+	/* Physical address & size of virtual control interface */
+	phys_addr_t	vctrl_base;
+	resource_size_t	vctrl_size;
+};
+
+const struct gic_kvm_info *gic_get_kvm_info(void);
+
+#endif /* __LINUX_IRQCHIP_ARM_GIC_COMMON_H */