From owner-svn-src-all@freebsd.org Wed May 18 10:03:18 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 58E84B3E78F for ; Wed, 18 May 2016 10:03:18 +0000 (UTC) (envelope-from zbb@semihalf.com) Received: from mail-lb0-x22d.google.com (mail-lb0-x22d.google.com [IPv6:2a00:1450:4010:c04::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id AE9951238 for ; Wed, 18 May 2016 10:03:17 +0000 (UTC) (envelope-from zbb@semihalf.com) Received: by mail-lb0-x22d.google.com with SMTP id jj5so15539828lbc.0 for ; Wed, 18 May 2016 03:03:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=/5edjcFC3QyOU2WIGx8t8JDGX2AnNQCpXS+Vi86ApsM=; b=fZ4mgkTzFjCPORYWJU5cyLFB4Y5VuK6ir5fH1lWeDcblqmSQE09IB91JRDQvfKeAFZ iZMbjbJT3orz8FGXgh+r4CZtwxCtlMnkIBF5D4F4opP/jXmDXCnuVDCPPczalnW74Wbx +I44BLb78d1nC+V1583XqT1QGa2Ebx9NgqrgwTMgBJ1nrsn9VJfFuXwwpN9lvgWYk8as k6oWXEdIiJ9yY8n31MtD0UWC1Yc6pKL5G0Ax7qZi55hGMstJlnCEAon6xlJJ5hbqjFcp TRci1n5S1LihJVKL+GNjUzkom/aKz00z3c4saYzyAEaRVW7Pz87wQUpBTduxmOXjd1tV qXsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=/5edjcFC3QyOU2WIGx8t8JDGX2AnNQCpXS+Vi86ApsM=; b=aCabLS1yNvKDM372muSTQnA/j4lx1fTLKG+rDCogk4qhbKxz3Xj3PYQ0WAbr5vS4tS F2u1chopfOPp5DdBdqozyGG7yPeDq9W+g5eCIC/XABuP4RmklLzqAq622zVjmx3QON1s aKCVR1yGHJnO88W/0QdLWONUGZsnIwJ0VNk8+ZQgmYtlPFlMBmE8DLKAJS37MC2AUsY2 zkG2IMax/WymKy9csqaWhfE82xNBL9+E9/OwhTS4jIrMF/kKRwbQhWhly1AeKZKe+++H nbLtOddPAkm4N3h1G6HPPDknBe/U81ZNNUZ5y+aTHW+77gqCUHYeSKvy2RFjKAMWP118 tiXg== X-Gm-Message-State: AOPr4FXonn8znWrfPeUBlVobvZY1PVisTZLVhpNwfYC0NVMymriDntuaO+98ZHW1dsQtjAbK3S/8CZDwWk1xTw== X-Received: by 10.112.58.232 with SMTP id u8mr1664474lbq.42.1463565795590; Wed, 18 May 2016 03:03:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.157.68 with HTTP; Wed, 18 May 2016 03:02:55 -0700 (PDT) In-Reply-To: <20160517131945.65cd9ccc@zapp> References: <201605161407.u4GE7h9n002600@repo.freebsd.org> <20160517131945.65cd9ccc@zapp> From: Zbigniew Bodek Date: Wed, 18 May 2016 12:02:55 +0200 Message-ID: Subject: Re: svn commit: r299944 - in head/sys: arm64/arm64 conf To: Andrew Turner Cc: Andrew Turner , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 May 2016 10:03:18 -0000 Hello Andrew, Thanks for the comments. Please check in-line below. Kind regards zbb 2016-05-17 14:19 GMT+02:00 Andrew Turner : > On Mon, 16 May 2016 18:08:49 +0200 > Zbigniew Bodek 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 : > > > > > 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 >