[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [Xen-20] Clearing of GICD_ICACTIVER register at boot time.


Please use scripts/get_maintainers.pl to CC all the relevant maintainers on your patch.

Regarding the title, Xen is supporting multiple architecture. So it is always useful to specify the architecture in the title.

Also the title is quite confusing as for clearing interrupts, you need to write 1s in that register.

A better title would be:

xen/arm: gic: Clear active state of IRQs when the GIC is initialized

On 05/06/18 20:20, cdeshpan wrote:
From: Chaitanya Deshpande <chaitanyagd11@xxxxxxxxx>

Please explain in the commit message why you need to clear the interrupts.

Signed-off-by: Chaitanya Deshpande <chaitanyagd11@xxxxxxxxx>
  xen/arch/arm/gic-v2.c            | 6 +++++-
  xen/arch/arm/gic-v3.c            | 5 +++++
  xen/arch/arm/vgic-v2.c           | 5 +++++
  xen/arch/arm/vgic-v3.c           | 5 +++++
  xen/arch/arm/vgic/vgic-mmio-v2.c | 5 +++++
  5 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index d2dcafb..6bd7e43 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -5,7 +5,6 @@
   * Tim Deegan <tim@xxxxxxx>
   * Copyright (c) 2011 Citrix Systems.
- *

Please don't remove unnecessary line.

   * This program is free software; you can redistribute it and/or modify
   * it under the terms of the GNU General Public License as published by
   * the Free Software Foundation; either version 2 of the License, or
@@ -1247,6 +1246,11 @@ static int __init gicv2_init(void)
      uint32_t aliased_offset = 0;

+    /* Clear GIC register at boot time */

This comment seems incorrect.

+    uint32_t *gicd_icactiver;
+    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
+    *gicd_icactiver = 0;

That code is not going to do anything if you write 0. Per the spec, you need to set 1 in order to deactivate interrupts.

Furthermore, this is only covering the first 32 interrupts. You have to clear the active state of all the interrupts. The first 32 interrupts are banked (i.e per-CPU) the others are shared.

The former should be initialized in gicv{2,3}_cpu_init that will be called during each CPU initialization. The latter should be done in gicv{2,3}_dist_init().

Lastly, GICD_ICACTIVER is just a register offset in the distributor. You have to add the base address in order to find to memory address to write. We have an helper for that in the GICv2 driver, see writel_gicd(...). For GICv3, you will have to use writel_relaxed(..., GICD + GICD_ICACTIVER).

I would also recommend to test your patches as very likely the latest error would have been caught.

      if ( acpi_disabled )
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index b2ed0f8..f61d2b3 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1738,6 +1738,11 @@ static int __init gicv3_init(void)
      uint32_t reg;
      unsigned int intid_bits;

+    /* Clear GIC register at boot time */
+    uint32_t *gicd_icactiver;
+    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
+    *gicd_icactiver = 0;

See my remark above.

      if ( !cpu_has_gicv3 )
          dprintk(XENLOG_ERR, "GICv3: driver requires system register 
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 646d1f3..d8b2761 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -727,6 +727,11 @@ static const struct vgic_ops vgic_v2_ops = {

  int vgic_v2_init(struct domain *d, int *mmio_count)
+    /* Clear GIC register at boot time */
+    uint32_t *gicd_icactiver;
+    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
+    *gicd_icactiver = 0;

I don't understand why you are clearing active register in vgic_v{2,3}_init. Those functions are called when creating a domain, so there are no need to mess with the physical interrupts here.

      if ( !vgic_v2_hw.enabled )
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 4b42739..6fe0dac 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1791,6 +1791,11 @@ static const struct vgic_ops v3_ops = {

  int vgic_v3_init(struct domain *d, int *mmio_count)
+    /* Clear GIC register at boot time */
+    uint32_t *gicd_icactiver;
+    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
+    *gicd_icactiver = 0;

See above.

      if ( !vgic_v3_hw.enabled )
diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
index 2e507b1..863b4d6 100644
--- a/xen/arch/arm/vgic/vgic-mmio-v2.c
+++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
@@ -305,6 +305,11 @@ static const struct vgic_register_region 
vgic_v2_dist_registers[] = {

  unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev)
+    /* Clear GIC register at boot time */
+    uint32_t *gicd_icactiver;
+    gicd_icactiver = (uint32_t*) GICD_ICACTIVER;
+    *gicd_icactiver = 0;

See above.

+ >       dev->regions = vgic_v2_dist_registers;
      dev->nr_regions = ARRAY_SIZE(vgic_v2_dist_registers);


This email and any attachments are intended for the sole use of the named 
recipient(s) and contain(s) confidential information that may be proprietary, 
privileged or copyrighted under applicable law. If you are not the intended 
recipient, do not read, copy, or forward this email message or any attachments. 
Delete this email message and any attachments immediately.

I am afraid I am not going to be able to accept any patch containing that paragraph in the signature (see [1]). This is a public mailing list, so everything is public. Please resend your patch without it if it was intended to be merged.

[1] https://www.youtube.com/watch?v=fMeH7wqOwXA&feature=youtu.be&t=13m53s


Julien Grall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.