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

2016-03-14 Thread Julien Grall 

Hi Christoffer,

On 09/03/16 05:14, Christoffer Dall wrote:

On Tue, Mar 08, 2016 at 11:29:28AM +, 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 

---
Cc: Thomas Gleixner 
Cc: Jason Cooper 
Cc: Marc Zyngier 

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

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0..704caf4 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,19 @@

  #include "irq-gic-common.h"

+static const struct gic_kvm_info *gic_kvm_info;
+
+const struct gic_kvm_info *gic_get_kvm_info(void)
+{
+   return gic_kvm_info;
+}
+
+void gic_set_kvm_info(const struct gic_kvm_info *info)
+{
+   WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");


why do we WARN here?  Wouldn't this be an obvious bug?


Right this is an obvious bug. I will switch to BUG_ON.




+   gic_kvm_info = info;
+}
+
  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data)
  {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index fff697d..205e5fd 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -19,6 +19,7 @@

  #include 
  #include 
+#include 

  struct gic_quirk {
const char *desc;
@@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void 
(*sync_access)(void));
  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data);

+void gic_set_kvm_info(const struct gic_kvm_info *info);
+
  #endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index fbde202..0c58112 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -102,6 +102,8 @@ static struct static_key supports_deactivate = 
STATIC_KEY_INIT_TRUE;

  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;

+static struct gic_kvm_info gic_v2_kvm_info;
+
  #ifdef CONFIG_GIC_NON_BANKED
  static void __iomem *gic_get_percpu_base(union gic_base *base)
  {
@@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node *node, 
void __iomem **base)
return true;
  }

+static void __init gic_of_setup_kvm_info(struct device_node *node)
+{
+   int ret;
+   struct resource r;


I would prefer two struct resource variables more akin to how the KVM
code did it already, and then name them something more understandable
than 'r'.

You could also argue that you can then only populate the gic_v2_kvm_info
with all coherent info or nothing, instead of filling it out partially
and then exiting.


+
+   gic_v2_kvm_info.type = GIC_V2;
+
+   gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
+
+   ret = of_address_to_resource(node, 2, );
+   if (!ret)
+   gic_v2_kvm_info.vctrl = r;
+
+   ret = of_address_to_resource(node, 3, );


here you're overwriting the error return value if the first call to
of_address_to_resource failed ?


+   if (!ret) {
+   if (!PAGE_ALIGNED(r.start))
+   pr_warn("GICV physical address 0x%llx not page 
aligned\n",
+   (unsigned long long)r.start);


how does KVM know that this went bad?


+   else if (!PAGE_ALIGNED(resource_size()))
+   pr_warn("GICV size 0x%llx not a multiple of page size 
0x%lx\n",
+   (unsigned long long)resource_size(),
+   PAGE_SIZE);


same?


+   else
+   gic_v2_kvm_info.vcpu = r;
+   }
+
+   gic_set_kvm_info(_v2_kvm_info);


so here you're setting the kvm info even if one of the calls above
fails?

I think this function could leverage much more of the existing KVM
implementation to avoid these kinds of mistakes.


I will rework this function.




+}
+
  int __init
  gic_of_init(struct device_node *node, struct device_node *parent)
  {
@@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct device_node 
*parent)

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

2016-03-14 Thread Julien Grall 

Hi Christoffer,

On 09/03/16 05:14, Christoffer Dall wrote:

On Tue, Mar 08, 2016 at 11:29:28AM +, 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 

---
Cc: Thomas Gleixner 
Cc: Jason Cooper 
Cc: Marc Zyngier 

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

diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index f174ce0..704caf4 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -21,6 +21,19 @@

  #include "irq-gic-common.h"

+static const struct gic_kvm_info *gic_kvm_info;
+
+const struct gic_kvm_info *gic_get_kvm_info(void)
+{
+   return gic_kvm_info;
+}
+
+void gic_set_kvm_info(const struct gic_kvm_info *info)
+{
+   WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");


why do we WARN here?  Wouldn't this be an obvious bug?


Right this is an obvious bug. I will switch to BUG_ON.




+   gic_kvm_info = info;
+}
+
  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data)
  {
diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
index fff697d..205e5fd 100644
--- a/drivers/irqchip/irq-gic-common.h
+++ b/drivers/irqchip/irq-gic-common.h
@@ -19,6 +19,7 @@

  #include 
  #include 
+#include 

  struct gic_quirk {
const char *desc;
@@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void 
(*sync_access)(void));
  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
void *data);

+void gic_set_kvm_info(const struct gic_kvm_info *info);
+
  #endif /* _IRQ_GIC_COMMON_H */
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index fbde202..0c58112 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -102,6 +102,8 @@ static struct static_key supports_deactivate = 
STATIC_KEY_INIT_TRUE;

  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;

+static struct gic_kvm_info gic_v2_kvm_info;
+
  #ifdef CONFIG_GIC_NON_BANKED
  static void __iomem *gic_get_percpu_base(union gic_base *base)
  {
@@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node *node, 
void __iomem **base)
return true;
  }

+static void __init gic_of_setup_kvm_info(struct device_node *node)
+{
+   int ret;
+   struct resource r;


I would prefer two struct resource variables more akin to how the KVM
code did it already, and then name them something more understandable
than 'r'.

You could also argue that you can then only populate the gic_v2_kvm_info
with all coherent info or nothing, instead of filling it out partially
and then exiting.


+
+   gic_v2_kvm_info.type = GIC_V2;
+
+   gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
+
+   ret = of_address_to_resource(node, 2, );
+   if (!ret)
+   gic_v2_kvm_info.vctrl = r;
+
+   ret = of_address_to_resource(node, 3, );


here you're overwriting the error return value if the first call to
of_address_to_resource failed ?


+   if (!ret) {
+   if (!PAGE_ALIGNED(r.start))
+   pr_warn("GICV physical address 0x%llx not page 
aligned\n",
+   (unsigned long long)r.start);


how does KVM know that this went bad?


+   else if (!PAGE_ALIGNED(resource_size()))
+   pr_warn("GICV size 0x%llx not a multiple of page size 
0x%lx\n",
+   (unsigned long long)resource_size(),
+   PAGE_SIZE);


same?


+   else
+   gic_v2_kvm_info.vcpu = r;
+   }
+
+   gic_set_kvm_info(_v2_kvm_info);


so here you're setting the kvm info even if one of the calls above
fails?

I think this function could leverage much more of the existing KVM
implementation to avoid these kinds of mistakes.


I will rework this function.




+}
+
  int __init
  gic_of_init(struct device_node *node, struct device_node *parent)
  {
@@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct device_node 
*parent)

__gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
 >fwnode);
-

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

2016-03-08 Thread Christoffer Dall 
On Tue, Mar 08, 2016 at 11:29:28AM +, 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 
> 
> ---
> Cc: Thomas Gleixner 
> Cc: Jason Cooper 
> Cc: Marc Zyngier 
> 
> Changes in v2:
> - Use 0 rather than a negative value to know when the maintenance IRQ
> is not present.
> - Use resource for vcpu and vctrl
> ---
>  drivers/irqchip/irq-gic-common.c   | 13 ++
>  drivers/irqchip/irq-gic-common.h   |  3 ++
>  drivers/irqchip/irq-gic.c  | 80 
> +-
>  include/linux/irqchip/arm-gic-common.h | 33 ++
>  4 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/irqchip/arm-gic-common.h
> 
> diff --git a/drivers/irqchip/irq-gic-common.c 
> b/drivers/irqchip/irq-gic-common.c
> index f174ce0..704caf4 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -21,6 +21,19 @@
>  
>  #include "irq-gic-common.h"
>  
> +static const struct gic_kvm_info *gic_kvm_info;
> +
> +const struct gic_kvm_info *gic_get_kvm_info(void)
> +{
> + return gic_kvm_info;
> +}
> +
> +void gic_set_kvm_info(const struct gic_kvm_info *info)
> +{
> + WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");

why do we WARN here?  Wouldn't this be an obvious bug?

> + gic_kvm_info = info;
> +}
> +
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>   void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h 
> b/drivers/irqchip/irq-gic-common.h
> index fff697d..205e5fd 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -19,6 +19,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  struct gic_quirk {
>   const char *desc;
> @@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void 
> (*sync_access)(void));
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>   void *data);
>  
> +void gic_set_kvm_info(const struct gic_kvm_info *info);
> +
>  #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index fbde202..0c58112 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -102,6 +102,8 @@ static struct static_key supports_deactivate = 
> STATIC_KEY_INIT_TRUE;
>  
>  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
>  
> +static struct gic_kvm_info gic_v2_kvm_info;
> +
>  #ifdef CONFIG_GIC_NON_BANKED
>  static void __iomem *gic_get_percpu_base(union gic_base *base)
>  {
> @@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node 
> *node, void __iomem **base)
>   return true;
>  }
>  
> +static void __init gic_of_setup_kvm_info(struct device_node *node)
> +{
> + int ret;
> + struct resource r;

I would prefer two struct resource variables more akin to how the KVM
code did it already, and then name them something more understandable
than 'r'.

You could also argue that you can then only populate the gic_v2_kvm_info
with all coherent info or nothing, instead of filling it out partially
and then exiting.

> +
> + gic_v2_kvm_info.type = GIC_V2;
> +
> + gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
> +
> + ret = of_address_to_resource(node, 2, );
> + if (!ret)
> + gic_v2_kvm_info.vctrl = r;
> +
> + ret = of_address_to_resource(node, 3, );

here you're overwriting the error return value if the first call to
of_address_to_resource failed ?

> + if (!ret) {
> + if (!PAGE_ALIGNED(r.start))
> + pr_warn("GICV physical address 0x%llx not page 
> aligned\n",
> + (unsigned long long)r.start);

how does KVM know that this went bad?

> + else if (!PAGE_ALIGNED(resource_size()))
> + pr_warn("GICV size 0x%llx not a multiple of page size 
> 0x%lx\n",
> + (unsigned long long)resource_size(),
> + PAGE_SIZE);

same?

> + else
> + gic_v2_kvm_info.vcpu = r;
> + }
> +
> + gic_set_kvm_info(_v2_kvm_info);

so here you're setting the kvm info even if one of the calls above
fails?

I think this function could leverage much more of the existing KVM
implementation to avoid these kinds of mistakes.

> +}
> +
>  int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
> @@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct 
> device_node *parent)
>  
>

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

2016-03-08 Thread Christoffer Dall 
On Tue, Mar 08, 2016 at 11:29:28AM +, 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 
> 
> ---
> Cc: Thomas Gleixner 
> Cc: Jason Cooper 
> Cc: Marc Zyngier 
> 
> Changes in v2:
> - Use 0 rather than a negative value to know when the maintenance IRQ
> is not present.
> - Use resource for vcpu and vctrl
> ---
>  drivers/irqchip/irq-gic-common.c   | 13 ++
>  drivers/irqchip/irq-gic-common.h   |  3 ++
>  drivers/irqchip/irq-gic.c  | 80 
> +-
>  include/linux/irqchip/arm-gic-common.h | 33 ++
>  4 files changed, 128 insertions(+), 1 deletion(-)
>  create mode 100644 include/linux/irqchip/arm-gic-common.h
> 
> diff --git a/drivers/irqchip/irq-gic-common.c 
> b/drivers/irqchip/irq-gic-common.c
> index f174ce0..704caf4 100644
> --- a/drivers/irqchip/irq-gic-common.c
> +++ b/drivers/irqchip/irq-gic-common.c
> @@ -21,6 +21,19 @@
>  
>  #include "irq-gic-common.h"
>  
> +static const struct gic_kvm_info *gic_kvm_info;
> +
> +const struct gic_kvm_info *gic_get_kvm_info(void)
> +{
> + return gic_kvm_info;
> +}
> +
> +void gic_set_kvm_info(const struct gic_kvm_info *info)
> +{
> + WARN(gic_kvm_info != NULL, "gic_kvm_info already set\n");

why do we WARN here?  Wouldn't this be an obvious bug?

> + gic_kvm_info = info;
> +}
> +
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>   void *data)
>  {
> diff --git a/drivers/irqchip/irq-gic-common.h 
> b/drivers/irqchip/irq-gic-common.h
> index fff697d..205e5fd 100644
> --- a/drivers/irqchip/irq-gic-common.h
> +++ b/drivers/irqchip/irq-gic-common.h
> @@ -19,6 +19,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  struct gic_quirk {
>   const char *desc;
> @@ -35,4 +36,6 @@ void gic_cpu_config(void __iomem *base, void 
> (*sync_access)(void));
>  void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
>   void *data);
>  
> +void gic_set_kvm_info(const struct gic_kvm_info *info);
> +
>  #endif /* _IRQ_GIC_COMMON_H */
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index fbde202..0c58112 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -102,6 +102,8 @@ static struct static_key supports_deactivate = 
> STATIC_KEY_INIT_TRUE;
>  
>  static struct gic_chip_data gic_data[CONFIG_ARM_GIC_MAX_NR] __read_mostly;
>  
> +static struct gic_kvm_info gic_v2_kvm_info;
> +
>  #ifdef CONFIG_GIC_NON_BANKED
>  static void __iomem *gic_get_percpu_base(union gic_base *base)
>  {
> @@ -1189,6 +1191,35 @@ static bool gic_check_eoimode(struct device_node 
> *node, void __iomem **base)
>   return true;
>  }
>  
> +static void __init gic_of_setup_kvm_info(struct device_node *node)
> +{
> + int ret;
> + struct resource r;

I would prefer two struct resource variables more akin to how the KVM
code did it already, and then name them something more understandable
than 'r'.

You could also argue that you can then only populate the gic_v2_kvm_info
with all coherent info or nothing, instead of filling it out partially
and then exiting.

> +
> + gic_v2_kvm_info.type = GIC_V2;
> +
> + gic_v2_kvm_info.maint_irq = irq_of_parse_and_map(node, 0);
> +
> + ret = of_address_to_resource(node, 2, );
> + if (!ret)
> + gic_v2_kvm_info.vctrl = r;
> +
> + ret = of_address_to_resource(node, 3, );

here you're overwriting the error return value if the first call to
of_address_to_resource failed ?

> + if (!ret) {
> + if (!PAGE_ALIGNED(r.start))
> + pr_warn("GICV physical address 0x%llx not page 
> aligned\n",
> + (unsigned long long)r.start);

how does KVM know that this went bad?

> + else if (!PAGE_ALIGNED(resource_size()))
> + pr_warn("GICV size 0x%llx not a multiple of page size 
> 0x%lx\n",
> + (unsigned long long)resource_size(),
> + PAGE_SIZE);

same?

> + else
> + gic_v2_kvm_info.vcpu = r;
> + }
> +
> + gic_set_kvm_info(_v2_kvm_info);

so here you're setting the kvm info even if one of the calls above
fails?

I think this function could leverage much more of the existing KVM
implementation to avoid these kinds of mistakes.

> +}
> +
>  int __init
>  gic_of_init(struct device_node *node, struct device_node *parent)
>  {
> @@ -1218,8 +1249,10 @@ gic_of_init(struct device_node *node, struct 
> device_node *parent)
>  
>   __gic_init_bases(gic_cnt, -1, dist_base, cpu_base, percpu_offset,
>