Re: [Qemu-devel] [kvm-unit-tests PATCH v4 09/11] arm/arm64: add initial gicv3 support

[Andrew Jones]

On Wed, Nov 09, 2016 at 12:35:48PM +0000, Andre Przywara wrote:
[...]
> > diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> > new file mode 100644
> > index 000000000000..03321f8c860f
> > --- /dev/null
> > +++ b/lib/arm/asm/gic-v3.h
> > @@ -0,0 +1,92 @@
> > +/*
> > + * All GIC* defines are lifted from include/linux/irqchip/arm-gic-v3.h
> > + *
> > + * Copyright (C) 2016, Red Hat Inc, Andrew Jones <address@hidden>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > + */
> > +#ifndef _ASMARM_GIC_V3_H_
> > +#define _ASMARM_GIC_V3_H_
> > +
> > +#ifndef _ASMARM_GIC_H_
> > +#error Do not directly include <asm/gic-v3.h>. Include <asm/gic.h>
> > +#endif
> > +
> > +#define GICD_CTLR                  0x0000
> > +#define GICD_TYPER                 0x0004
> 
> So if we share the distributor register definition with GICv2, these
> shouldn't be here, but in gic.h.
> But this is the right naming scheme we should use (instead of GIC_DIST_xxx).
> 
> Now this gets interesting with your wish to both share the definitions
> for the GICv2 and GICv3 distributors, but also stick to the names the
> kernel uses (because they differ between the two) ;-)
> So now you loose the greppability for either GIC_DIST_CTR or GICD_TYPER,
> for instance.

Well, we just have the same offset with two names (giving us two
symbols to grep). I put them here, vs. asm/gic.h, because the kernel
only uses theses symbols for gicv3. Now, nothing stops a unit test
from using them with gicv2 tests, though, because unit tests include
gic.h, which includes both gic-v2.h and gic-v3.h, and thus it gets
both. I know, it's sounding messy... Shouldn't we post some churn to
the kernel? :-)

Note, I tried to only add defines to asm/gic.h that are actually
shared in the kernel between v2 and v3, e.g. GIC_DIST_ENABLE_SET.
Actually, GIC_DIST_CTRL and GIC_DIST_CTR may be the only exceptions
we have so far.

> 
> > +#define GICD_IGROUPR                       0x0080
> > +
> > +#define GICD_CTLR_RWP                      (1U << 31)
> > +#define GICD_CTLR_ARE_NS           (1U << 4)
> > +#define GICD_CTLR_ENABLE_G1A               (1U << 1)
> > +#define GICD_CTLR_ENABLE_G1                (1U << 0)
> > +
> > +#define GICR_TYPER                 0x0008
> > +#define GICR_IGROUPR0                      GICD_IGROUPR
> > +#define GICR_TYPER_LAST                    (1U << 4)
> > +
> > +
> > +#include <asm/arch_gicv3.h>
> > +
> > +#ifndef __ASSEMBLY__
> > +#include <asm/setup.h>
> > +#include <asm/smp.h>
> > +#include <asm/processor.h>
> > +#include <asm/io.h>
> > +
> > +struct gicv3_data {
> > +   void *dist_base;
> > +   void *redist_base[NR_CPUS];
> > +   unsigned int irq_nr;
> > +};
> > +extern struct gicv3_data gicv3_data;
> > +
> > +#define gicv3_dist_base()          (gicv3_data.dist_base)
> > +#define gicv3_redist_base()                
> > (gicv3_data.redist_base[smp_processor_id()])
> > +#define gicv3_sgi_base()           
> > (gicv3_data.redist_base[smp_processor_id()] + SZ_64K)
> > +
> > +extern int gicv3_init(void);
> > +extern void gicv3_enable_defaults(void);
> > +
> > +static inline void gicv3_do_wait_for_rwp(void *base)
> > +{
> > +   int count = 100000;     /* 1s */
> > +
> > +   while (readl(base + GICD_CTLR) & GICD_CTLR_RWP) {
> > +           if (!--count) {
> > +                   printf("GICv3: RWP timeout!\n");
> > +                   abort();
> > +           }
> > +           cpu_relax();
> > +           udelay(10);
> > +   };
> > +}
> > +
> > +static inline void gicv3_dist_wait_for_rwp(void)
> > +{
> > +   gicv3_do_wait_for_rwp(gicv3_dist_base());
> > +}
> > +
> > +static inline void gicv3_redist_wait_for_rwp(void)
> > +{
> > +   gicv3_do_wait_for_rwp(gicv3_redist_base());
> > +}
> > +
> > +static inline u32 mpidr_compress(u64 mpidr)
> > +{
> > +   u64 compressed = mpidr & MPIDR_HWID_BITMASK;
> > +
> > +   compressed = (((compressed >> 32) & 0xff) << 24) | compressed;
> > +   return compressed;
> > +}
> > +
> > +static inline u64 mpidr_uncompress(u32 compressed)
> > +{
> > +   u64 mpidr = ((u64)compressed >> 24) << 32;
> > +
> > +   mpidr |= compressed & MPIDR_HWID_BITMASK;
> > +   return mpidr;
> > +}
> > +
> > +#endif /* !__ASSEMBLY__ */
> > +#endif /* _ASMARM_GIC_V3_H_ */
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > index 328e078a9ae1..4897bc592cdd 100644
> > --- a/lib/arm/asm/gic.h
> > +++ b/lib/arm/asm/gic.h
> > @@ -7,6 +7,7 @@
> >  #define _ASMARM_GIC_H_
> >  
> >  #include <asm/gic-v2.h>
> > +#include <asm/gic-v3.h>
> >  
> >  #define GIC_CPU_CTRL                       0x00
> >  #define GIC_CPU_PRIMASK                    0x04
> > diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> > index 91d78c9a0cc2..af58a11ea13e 100644
> > --- a/lib/arm/gic.c
> > +++ b/lib/arm/gic.c
> > @@ -8,9 +8,11 @@
> >  #include <asm/io.h>
> >  
> >  struct gicv2_data gicv2_data;
> > +struct gicv3_data gicv3_data;
> >  
> >  /*
> >   * Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt
> > + * Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.txt
> >   */
> >  static bool
> >  gic_get_dt_bases(const char *compatible, void **base1, void **base2)
> > @@ -48,10 +50,18 @@ int gicv2_init(void)
> >                     &gicv2_data.dist_base, &gicv2_data.cpu_base);
> >  }
> >  
> > +int gicv3_init(void)
> > +{
> > +   return gic_get_dt_bases("arm,gic-v3", &gicv3_data.dist_base,
> > +                   &gicv3_data.redist_base[0]);
> > +}
> > +
> >  int gic_init(void)
> >  {
> >     if (gicv2_init())
> >             return 2;
> > +   else if (gicv3_init())
> > +           return 3;
> >     return 0;
> >  }
> >  
> > @@ -73,3 +83,49 @@ void gicv2_enable_defaults(void)
> >     writel(GICC_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
> >     writel(GICC_ENABLE, cpu_base + GIC_CPU_CTRL);
> >  }
> > +
> > +void gicv3_set_redist_base(void)
> > +{
> > +   u32 aff = mpidr_compress(get_mpidr());
> > +   void *ptr = gicv3_data.redist_base[0];
> > +   u64 typer;
> > +
> > +   do {
> > +           typer = gicv3_read_typer(ptr + GICR_TYPER);
> > +           if ((typer >> 32) == aff) {
> > +                   gicv3_redist_base() = ptr;
> > +                   return;
> > +           }
> > +           ptr += SZ_64K * 2; /* skip RD_base and SGI_base */
> > +   } while (!(typer & GICR_TYPER_LAST));
> > +   assert(0);
> > +}
> > +
> > +void gicv3_enable_defaults(void)
> > +{
> > +   void *dist = gicv3_dist_base();
> > +   void *sgi_base;
> > +   unsigned int i;
> > +
> > +   gicv3_data.irq_nr = GICD_TYPER_IRQS(readl(dist + GICD_TYPER));
> > +   if (gicv3_data.irq_nr > 1020)
> > +           gicv3_data.irq_nr = 1020;
> > +
> > +   writel(GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1,
> > +          dist + GICD_CTLR);
> > +   gicv3_dist_wait_for_rwp();
> > +
> > +   if (!gicv3_redist_base())
> > +           gicv3_set_redist_base();
> > +   sgi_base = gicv3_sgi_base();
> > +
> > +   writel(~0, sgi_base + GICR_IGROUPR0);
> 
> This is mixing redist setup with distributor setup. Is it that what you
> meant with:
> " - simplify enable by not caring if we reinit the distributor [drew]"?

Yes, but, TBH, I wasn't sure I could get away with it. I tested and it
worked, and I figured you'd yell at me if I was wrong :-)

> 
> Also if you set the group for the SGIs, you should set it for SPIs as
> well (like the kernel does). This was done in v3 of the series.

OK, I was also simplifying by removing everything and then adding stuff
back until it worked :-) I can certainly add this back for completeness
though.

> 
> What about you finish the per-CPU setup first, then bail out with:
> 
> if (smp_processor_id() != 0)
>       return;
> 
> and then do the distributor setup (only on the first core).

Sure, if it's necessary. I actually like not having to worry about
a particular core or a particular order/number of times this enable
gets called. Does it hurt to just do it each time?

Thanks,
drew