svn commit: r299944 - in head/sys: arm64/arm64 conf
Zbigniew Bodek
zbb at semihalf.com
Wed May 18 10:03:18 UTC 2016
Hello Andrew,
Thanks for the comments. Please check in-line below.
Kind regards
zbb
2016-05-17 14:19 GMT+02:00 Andrew Turner <andrew at fubar.geek.nz>:
> On Mon, 16 May 2016 18:08:49 +0200
> Zbigniew Bodek <zbb at semihalf.com> wrote:
>
> > Hello Andrew,
> >
> > I think committing this code should be preceded by at least brief
> > review. Few remarks to the code found on the first glance below.
>
> See below for comments.
>
> >
> > Kind regards
> > zbb
> >
> > 2016-05-16 16:07 GMT+02:00 Andrew Turner <andrew at freebsd.org>:
> >
> > > Author: andrew
> > > Date: Mon May 16 14:07:43 2016
> > > New Revision: 299944
> > > URL: https://svnweb.freebsd.org/changeset/base/299944
> > >
> > > Log:
> > > Add intrng support to the GICv3 driver. It lacks ITS support so
> > > won't handle
> > > MSI or MSI-X interrupts, however this is enought to boot FreeBSD
> > > under the
> > > ARM Foundation Model with a GICv3 interrupt controller.
> > >
> > > Approved by: ABT Systems Ltd
> > > Relnotes: yes
> > > Sponsored by: The FreeBSD Foundation
> ...
> > > +#ifdef INTRNG
> > > +int
> > > +arm_gic_v3_intr(void *arg)
> > > +{
> > > + struct gic_v3_softc *sc = arg;
> > > + struct gic_v3_irqsrc *gi;
> > > + uint64_t active_irq;
> > > + struct trapframe *tf;
> > > + bool first;
> > > +
> > > + first = true;
> > > +
> > > + while (1) {
> > > + if (CPU_MATCH_ERRATA_CAVIUM_THUNDER_1_1) {
> > > + /*
> > > + * Hardware: Cavium ThunderX
> > > + * Chip revision: Pass 1.0 (early
> > > version)
> > > + * Pass 1.1
> > > (production)
> > > + * ERRATUM: 22978, 23154
> > > + */
> > > + __asm __volatile(
> > > + "nop;nop;nop;nop;nop;nop;nop;nop; \n"
> > > + "mrs %0, ICC_IAR1_EL1 \n"
> > > + "nop;nop;nop;nop; \n"
> > > + "dsb sy \n"
> > > + : "=&r" (active_irq));
> > > + } else {
> > > + active_irq = gic_icc_read(IAR1);
> > > + }
> > > +
> > > + if (__predict_false(active_irq >= sc->gic_nirqs))
> > > + return (FILTER_HANDLED);
> > >
> >
> > IMHO this is not true. Active IRQ could be much bigger than number of
> > supported IRQs. We are asking for debugging in the future when we
> > enable ITS.
>
> It is correct, the ITS change to this file is missing so we are unable
> to enable ITS interrupts.
>
>
In general it is not correct in terms of GICv3 but in that particular case.
So it would be good to have a "ARM64TODO" comment here.
> ...
> > > +
> > > +#ifdef FDT
> > > +static int
> > > +gic_map_fdt(device_t dev, u_int ncells, pcell_t *cells, u_int
> > > *irqp,
> > > + enum intr_polarity *polp, enum intr_trigger *trigp)
> > >
> >
> > All other functions are called gic_v3 and this one is just gic_
> > Why can't we move as much FDT code to the dedicated file which is
> > gic_v3_fdt.c?
>
> Unfortunately due to the current code in subr_intr.c this is needed for
> simplicity. It it my understanding there is work to fix this, so for
> now I would prefer to keep this.
>
OK.
>
> ...
> > > + /* Set the trigger and polarity */
> > > + if (irq <= GIC_LAST_PPI)
> > > + reg = gic_r_read(sc, 4,
> > > + GICR_SGI_BASE_SIZE + GICD_ICFGR(irq));
> > > + else
> > > + reg = gic_d_read(sc, 4, GICD_ICFGR(irq));
> > > + if (trig == INTR_TRIGGER_LEVEL)
> > > + reg &= ~(2 << ((irq % 16) * 2));
> > > + else
> > > + reg |= 2 << ((irq % 16) * 2);
> > >
> >
> > The rule of not using magic numbers does not apply here ;-) ?
>
> This is partially why this is still disabled by default, the code still
> needs a little polish, however it will be needed to help with a future
> review for changes to subr_intr.c.
>
So why do we integrate this WIP code now? Is there anyone else using INTRNG
on ARM64 beside you who need to test it?
I guess this is a matter of opinion but it would be much more convenient
(at least for us) if we get INTRNG in a single patch, review it, test it
and then change the existing code.
> ...
> > > +static void
> > > +gic_v3_disable_intr(device_t dev, struct intr_irqsrc *isrc)
> > > +{
> > > + struct gic_v3_softc *sc;
> > > + struct gic_v3_irqsrc *gi;
> > > + u_int irq;
> > > +
> > > + sc = device_get_softc(dev);
> > > + gi = (struct gic_v3_irqsrc *)isrc;
> > > + irq = gi->gi_irq;
> > > +
> > > + if (irq <= GIC_LAST_PPI) {
> > > + /* SGIs and PPIs in corresponding Re-Distributor */
>
BTW. Apart from the panic string changed the comments are moved a line
below.
> > > + gic_r_write(sc, 4, GICR_SGI_BASE_SIZE +
> > > GICD_ICENABLER(irq),
> > > + GICD_I_MASK(irq));
> > > + gic_v3_wait_for_rwp(sc, REDIST);
> > > + } else if (irq >= GIC_FIRST_SPI && irq <= GIC_LAST_SPI) {
> > > + /* SPIs in distributor */
> > > + gic_d_write(sc, 4, GICD_ICENABLER(irq),
> > > GICD_I_MASK(irq));
> > > + gic_v3_wait_for_rwp(sc, DIST);
> > >
> >
> > In gic_v3_setup_intr() we need spin lock and here we don't ?
>
> The locking was based on the existing intrng gic driver, I think they
> are both bogus.
>
I also think they are unnecessary so it would be good to remove them from
here.
>
> ...
>
> > > +static void
> > > +gic_v3_ipi_send(device_t dev, struct intr_irqsrc *isrc, cpuset_t
> > > cpus,
> > > + u_int ipi)
> > >
> >
> > What exactly is the functional difference between the current
> > implementation and this one?
> > Maybe we should exchange an old one to this or the opposite way
> > instead of having two different implementations?
>
> This one only needs to loop through the list of cpus once. In the
> common case I would expect this to be optimal as FreeBSD cpuids will
> align with the hardware clusters so a contiguous groups of CPUs will be
> on a single cluster. The only hardware I know where this isn't the case
> is the ARM Juno where we boot on hardware CPU 2.
>
OK. So this could be used instead of the current implementation and when we
have INTRNG ready the diff would be smaller to apply.
> >
> >
> > > +{
> > > + struct gic_v3_irqsrc *gi = (struct gic_v3_irqsrc *)isrc;
> > > + uint64_t aff, val, irq;
> > > + int i;
> > > +
> > > +#define GIC_AFF_MASK (CPU_AFF3_MASK | CPU_AFF2_MASK |
> > > CPU_AFF1_MASK)
> > > +#define GIC_AFFINITY(i) (CPU_AFFINITY(i) & GIC_AFF_MASK)
> > > + aff = GIC_AFFINITY(0);
> > > + irq = gi->gi_irq;
> > > + val = 0;
> > > +
> > > + /* Iterate through all CPUs in set */
> > > + for (i = 0; i < mp_ncpus; i++) {
> > > + /* Move to the next affinity group */
> > > + if (aff != GIC_AFFINITY(i)) {
> > > + /* Send the IPI */
> > > + if (val != 0) {
> > > + gic_icc_write(SGI1R, val);
> > > + val = 0;
> > > + }
> > > + aff = GIC_AFFINITY(i);
> > > + }
> > > +
> > > + /* Send the IPI to this cpu */
> > > + if (CPU_ISSET(i, &cpus)) {
> > > +#define
> > > ICC_SGI1R_AFFINITY(aff) \
> > > + (((uint64_t)CPU_AFF3(aff) << ICC_SGI1R_EL1_AFF3_SHIFT) | \
> > > + ((uint64_t)CPU_AFF2(aff) << ICC_SGI1R_EL1_AFF2_SHIFT) | \
> > > + ((uint64_t)CPU_AFF1(aff) << ICC_SGI1R_EL1_AFF1_SHIFT))
> > > + /* Set the affinity when the first at this
> > > level */
> > > + if (val == 0)
> > > + val = ICC_SGI1R_AFFINITY(aff) |
> > > + irq <<
> > > ICC_SGI1R_EL1_SGIID_SHIFT;
> > > + /* Set the bit to send the IPI to te CPU */
> > > + val |= 1 << CPU_AFF0(CPU_AFFINITY(i));
> > > + }
> > > + }
> > > +
> > > + /* Send the IPI to the last cpu affinity group */
> > > + if (val != 0)
> > > + gic_icc_write(SGI1R, val);
> > > +#undef GIC_AFF_MASK
> > > +#undef GIC_AFFINITY
> > >
> >
> > Couldn't we just use variables instead of defining and undefinining
> > those ugly macros here?
> > It really looks like they are here just to look different than it was
> > in the previous implementation.
>
> What's ugly about them? They never change so I'm unsure why a variable
> is needed.
>
Again, this is matter of opinion but defining two macros, one of which is 4
lines in length just to use them the line below and undefine is less
readable than just doing what that macro does on a variable. The only gain
here is that it is different than what was already done.
>
> Andrew
>
More information about the svn-src-head
mailing list