Re: [Xen-devel] [PATCH v2 3/7] xen/arm: Make gic-v2 code handle hip04-d01 platform

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

> On 11/03/2014 04:46 PM, Frediano Ziglio wrote:
> > The GIC in this platform is mainly compatible with the standard
> > GICv2 beside:
> > - ITARGET is extended to 16 bit to support 16 CPUs;
> > - SGI mask is extended to support 16 CPUs;
> > - maximum supported interrupt is 510.
> >
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> > Signed-off-by: Zoltan Kiss <zoltan.kiss@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/gic-v2.c     | 89
> +++++++++++++++++++++++++++++++++++++++--------
> >  xen/arch/arm/gic.c        |  3 +-
> >  xen/include/asm-arm/gic.h |  4 ++-
> >  3 files changed, 80 insertions(+), 16 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index
> > faad1ff..9461fe3 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -79,16 +79,23 @@ static struct gic_info gicv2_info;
> >   * logical CPU numbering. Let's use mapping as returned by the GIC
> >   * itself
> >   */
> > -static DEFINE_PER_CPU(u8, gic_cpu_id);
> > +static DEFINE_PER_CPU(u16, gic_cpu_id);
> >
> >  /* Maximum cpu interface per GIC */
> > -#define NR_GIC_CPU_IF 8
> > +static unsigned int nr_gic_cpu_if = 8; static unsigned int
> > +gicd_sgi_target_shift = GICD_SGI_TARGET_SHIFT; static unsigned int
> > +gic_cpu_mask = 0xff;
> >
> >  static inline void writeb_gicd(uint8_t val, unsigned int offset)  {
> >      writeb_relaxed(val, gicv2.map_dbase + offset);  }
> >
> > +static inline void writew_gicd(uint16_t val, unsigned int offset) {
> > +    writew_relaxed(val, gicv2.map_dbase + offset); }
> > +
> >  static inline void writel_gicd(uint32_t val, unsigned int offset)  {
> >      writel_relaxed(val, gicv2.map_dbase + offset); @@ -132,7 +139,7
> > @@ static unsigned int gicv2_cpu_mask(const cpumask_t *cpumask)
> >      cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> >      for_each_cpu( cpu, &possible_mask )
> >      {
> > -        ASSERT(cpu < NR_GIC_CPU_IF);
> > +        ASSERT(cpu < nr_gic_cpu_if);
> >          mask |= per_cpu(gic_cpu_id, cpu);
> >      }
> >
> > @@ -203,6 +210,15 @@ static unsigned int gicv2_read_irq(void)
> >      return (readl_gicc(GICC_IAR) & GICC_IA_IRQ);  }
> >
> > +/* Set target CPU mask (RAZ/WI on uniprocessor) */ static void
> > +gicv2_set_irq_mask(int irq, unsigned int mask) {
> > +    if ( nr_gic_cpu_if == 16 )
> 
> This check is very confusing, and even more in patch #5.
> 
> Code executed under this check describes your platform and not a
> generic 16-CPU support (actually there is no spec for it).
> 
> I would introduce a new boolean or hide this check in a macro.
> 

In some cases is not so terrible, as it's the only 16-bit implementation and as 
it's assuming ITARGETSR is 16 bit instead of 8. In other cases (like the 
compatible cases) I fully agree.

I agree a macro should be enough. Something like is_hip04() sounds ok?

> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index
> > 70d10d6..8050a65 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -563,12 +563,13 @@ static void do_sgi(struct cpu_user_regs *regs,
> > enum gic_sgi sgi)  void gic_interrupt(struct cpu_user_regs *regs, int
> > is_fiq)  {
> >      unsigned int irq;
> > +    unsigned int max_irq = gic_hw_ops->info->nr_lines;
> >
> >      do  {
> >          /* Reading IRQ will ACK it */
> >          irq = gic_hw_ops->read_irq();
> >
> > -        if ( likely(irq >= 16 && irq < 1021) )
> > +        if ( likely(irq >= 16 && irq < max_irq) )
> 
> On the previous version I've asked that need to explain in the commit
> message why this change is valid.
> 

Regards,
  Frediano


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