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

Re: [Xen-devel] [PATCH v5 18/22] xen/arm: ITS: Add domain specific ITS initialization



Hi Vijay,

This patch doesn't only do initialization but also destruction of vGIC info.

Although, a part of the domain initialization is done in part in #8 and #8. It's very confusing to see the initialization split in 3 different patch.

I would prefer to see the initialization/destruction in a single patch. It would help to catch whether you forgot to see you forgot some bits.

On 27/07/2015 04:12, vijay.kilari@xxxxxxxxx wrote:
From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>

Add Domain and vcpu specific ITS initialization

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
---
  xen/arch/arm/vgic-v3-its.c    |   10 ++++++++++
  xen/arch/arm/vgic-v3.c        |   10 ++++++++++
  xen/arch/arm/vgic.c           |    3 +++
  xen/include/asm-arm/gic-its.h |    2 ++
  xen/include/asm-arm/vgic.h    |    3 +++
  5 files changed, 28 insertions(+)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 5323192..e182cee 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -44,6 +44,7 @@
  #define GITS_PIDR4_VAL               (0x04)
  #define GITS_BASER_INIT_VAL          ((1UL << GITS_BASER_TYPE_SHIFT) | \
                                       (0x7UL << GITS_BASER_ENTRY_SIZE_SHIFT))
+//#define DEBUG_ITS

Why here?


  #ifdef DEBUG_ITS
  # define DPRINTK(fmt, args...) dprintk(XENLOG_DEBUG, fmt, ##args)
  #else
@@ -1122,6 +1123,15 @@ int vits_domain_init(struct domain *d)
      return 0;
  }

+void vits_domain_free(struct domain *d)
+{
+   free_xenheap_pages(d->arch.vgic.vits->prop_page,
+                       get_order_from_bytes(d->arch.vgic.vits->prop_size));
+   xfree(d->arch.vgic.pending_lpis);
+   xfree(d->arch.vgic.vits->collections);
+   xfree(d->arch.vgic.vits);

For instance, you add the initialization of these fields in patch #8 and #10 but free everything here. I have to go in the others patch to get if you forgot something or not.

+}
+
  /*
   * Local variables:
   * mode: C
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 9e6e3ff..a09ba36 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1299,12 +1299,22 @@ static int vgic_v3_domain_init(struct domain *d)

      d->arch.vgic.ctlr = VGICD_CTLR_DEFAULT;

+    if ( is_hardware_domain(d) && gic_lpi_supported() )

I think gic_lpi_supported is badly name. vITS should only be supported when ITS is present, not when LPIs is present.

And I know that you unset LPIs when ITS is not present. But this confuse the reader to do this.

+        vits_domain_init(d);
+
      return 0;
  }

+void vgic_v3_domain_free(struct domain *d)
+{
+    if ( is_hardware_domain(d) && gic_lpi_supported() )
+        vits_domain_free(d);
+}
+
  static const struct vgic_ops v3_ops = {
      .vcpu_init   = vgic_v3_vcpu_init,
      .domain_init = vgic_v3_domain_init,
+    .domain_free = vgic_v3_domain_free,
      .get_irq_priority = vgic_v3_get_irq_priority,
      .get_target_vcpu  = vgic_v3_get_target_vcpu,
      .emulate_sysreg  = vgic_v3_emulate_sysreg,
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 57c0f52..e2bfdb6 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -166,6 +166,9 @@ void domain_vgic_free(struct domain *d)
      xfree(d->arch.vgic.shared_irqs);
      xfree(d->arch.vgic.pending_irqs);
      xfree(d->arch.vgic.allocated_irqs);
+
+    if ( !d->arch.vgic.handler->domain_free )

The test is wrong. If domain_free is NULL you will end up to dereference the pointer and crash.

+        d->arch.vgic.handler->domain_free(d);
  }

  int vcpu_vgic_init(struct vcpu *v)
diff --git a/xen/include/asm-arm/gic-its.h b/xen/include/asm-arm/gic-its.h
index 870c9a8..da689a4 100644
--- a/xen/include/asm-arm/gic-its.h
+++ b/xen/include/asm-arm/gic-its.h
@@ -367,6 +367,8 @@ int its_cpu_init(void);
  void its_set_lpi_properties(struct irq_desc *desc,
                              const cpumask_t *cpu_mask,
                              unsigned int priority);
+int vits_domain_init(struct domain *d);
+void vits_domain_free(struct domain *d);
  int vits_get_vitt_entry(struct domain *d, uint32_t devid,
                          uint32_t event, struct vitt *entry);
  int vits_get_vdevice_entry(struct domain *d, uint32_t devid,
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index b11faa0..853df04 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -114,6 +114,8 @@ struct vgic_ops {
      int (*vcpu_init)(struct vcpu *v);
      /* Domain specific initialization of vGIC */
      int (*domain_init)(struct domain *d);
+    /* Free domain specific resources */
+    void (*domain_free)(struct domain *d);
      /* Get priority for a given irq stored in vgic structure */
      int (*get_irq_priority)(struct vcpu *v, unsigned int irq);
      /* Get the target vcpu for a given virq. The rank lock is already taken
@@ -191,6 +193,7 @@ enum gic_sgi_mode;
  #define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)

  extern int domain_vgic_init(struct domain *d, unsigned int nr_spis);
+extern void vgic_its_init(void);

Where does it come from?

  extern void domain_vgic_free(struct domain *d);
  extern int vcpu_vgic_init(struct vcpu *v);
  extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);


Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

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