On Fri, Sep 04, 2020 at 08:55:30AM -0600, Theo de Raadt wrote:
> Too much ifdef. Can you try a different approach -- leave the ipi code
> intact, but wrap the call to db_enter() with #ifdef DDB? The ipi won't
> be activated, so it should be fine for it to return 0.
Thanks Theo. I just tried to the follow (what seems to be) the standard
practice across the kernel of segregating all DDB-specific code. Pros
and cons to both approaches, obviously, but I can confirm the less
explicit approach works too:
Index: agintc.c
===================================================================
RCS file: /cvs/src/sys/arch/arm64/dev/agintc.c,v
retrieving revision 1.26
diff -u -p -u -p -r1.26 agintc.c
--- agintc.c 17 Jul 2020 08:07:33 -0000 1.26
+++ agintc.c 5 Sep 2020 01:32:13 -0000
@@ -1121,7 +1121,9 @@ int
agintc_ipi_ddb(void *v)
{
/* XXX */
+#ifdef DDB
db_enter();
+#endif
return 1;
}
Index: ampintc.c
===================================================================
RCS file: /cvs/src/sys/arch/arm64/dev/ampintc.c,v
retrieving revision 1.19
diff -u -p -u -p -r1.19 ampintc.c
--- ampintc.c 17 Jul 2020 08:07:33 -0000 1.19
+++ ampintc.c 5 Sep 2020 01:32:13 -0000
@@ -958,7 +958,9 @@ int
ampintc_ipi_ddb(void *v)
{
/* XXX */
+#ifdef DDB
db_enter();
+#endif
return 1;
}
> Matt Baulch <[email protected]> wrote:
>
> > Currently, it's not possible to build on arm64 without the in-kernel
> > debugger.
> > This patch fixes the two offending files:
> >
> > Index: agintc.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/arm64/dev/agintc.c,v
> > retrieving revision 1.26
> > diff -u -p -u -p -r1.26 agintc.c
> > --- agintc.c 17 Jul 2020 08:07:33 -0000 1.26
> > +++ agintc.c 4 Sep 2020 13:18:33 -0000
> > @@ -156,7 +156,9 @@ struct agintc_softc {
> > struct agintc_dmamem *sc_pend;
> > struct interrupt_controller sc_ic;
> > int sc_ipi_num[2]; /* id for NOP and DDB ipi */
> > +#ifdef DDB
> > int sc_ipi_reason[MAXCPUS]; /* NOP or DDB caused */
> > +#endif
> > void *sc_ipi_irq[2]; /* irqhandle for each ipi */
> > };
> > struct agintc_softc *agintc_sc;
> > @@ -226,7 +228,9 @@ void agintc_wait_rwp(struct agintc_soft
> > void agintc_r_wait_rwp(struct agintc_softc *sc);
> > uint32_t agintc_r_ictlr(void);
> >
> > +#ifdef DDB
> > int agintc_ipi_ddb(void *v);
> > +#endif
> > int agintc_ipi_nop(void *v);
> > int agintc_ipi_combined(void *);
> > void agintc_send_ipi(struct cpu_info *, int);
> > @@ -576,15 +580,19 @@ agintc_attach(struct device *parent, str
> > sc->sc_ipi_irq[0] = agintc_intr_establish(ipiirq[0],
> > IPL_IPI|IPL_MPSAFE, NULL, agintc_ipi_combined, sc, "ipi");
> > sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> > +#ifdef DDB
> > sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[0];
> > +#endif
> > break;
> > case 2:
> > sc->sc_ipi_irq[0] = agintc_intr_establish(ipiirq[0],
> > IPL_IPI|IPL_MPSAFE, NULL, agintc_ipi_nop, sc, "ipinop");
> > sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> > +#ifdef DDB
> > sc->sc_ipi_irq[1] = agintc_intr_establish(ipiirq[1],
> > IPL_IPI|IPL_MPSAFE, NULL, agintc_ipi_ddb, sc, "ipiddb");
> > sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[1];
> > +#endif
> > break;
> > default:
> > panic("nipi unexpected number %d", nipi);
> > @@ -1117,6 +1125,7 @@ agintc_r_wait_rwp(struct agintc_softc *s
> > }
> >
> > #ifdef MULTIPROCESSOR
> > +#ifdef DDB
> > int
> > agintc_ipi_ddb(void *v)
> > {
> > @@ -1124,6 +1133,7 @@ agintc_ipi_ddb(void *v)
> > db_enter();
> > return 1;
> > }
> > +#endif
> >
> > int
> > agintc_ipi_nop(void *v)
> > @@ -1135,14 +1145,16 @@ agintc_ipi_nop(void *v)
> > int
> > agintc_ipi_combined(void *v)
> > {
> > +#ifdef DDB
> > struct agintc_softc *sc = v;
> >
> > if (sc->sc_ipi_reason[cpu_number()] == ARM_IPI_DDB) {
> > sc->sc_ipi_reason[cpu_number()] = ARM_IPI_NOP;
> > return agintc_ipi_ddb(v);
> > - } else {
> > - return agintc_ipi_nop(v);
> > }
> > +#endif
> > +
> > + return agintc_ipi_nop(v);
> > }
> >
> > void
> > @@ -1154,9 +1166,11 @@ agintc_send_ipi(struct cpu_info *ci, int
> > if (ci == curcpu() && id == ARM_IPI_NOP)
> > return;
> >
> > +#ifdef DDB
> > /* never overwrite IPI_DDB with IPI_NOP */
> > if (id == ARM_IPI_DDB)
> > sc->sc_ipi_reason[ci->ci_cpuid] = id;
> > +#endif
> >
> > /* will only send 1 cpu */
> > sendmask = (ci->ci_mpidr & MPIDR_AFF3) << 16;
> > Index: ampintc.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/arm64/dev/ampintc.c,v
> > retrieving revision 1.19
> > diff -u -p -u -p -r1.19 ampintc.c
> > --- ampintc.c 17 Jul 2020 08:07:33 -0000 1.19
> > +++ ampintc.c 4 Sep 2020 13:18:33 -0000
> > @@ -141,7 +141,9 @@ struct ampintc_softc {
> > uint8_t sc_cpu_mask[ICD_ICTR_CPU_M + 1];
> > struct evcount sc_spur;
> > struct interrupt_controller sc_ic;
> > +#ifdef DDB
> > int sc_ipi_reason[ICD_ICTR_CPU_M + 1];
> > +#endif
> > int sc_ipi_num[2];
> > };
> > struct ampintc_softc *ampintc;
> > @@ -196,7 +198,9 @@ void ampintc_intr_barrier(void *);
> >
> > int ampintc_ipi_combined(void *);
> > int ampintc_ipi_nop(void *);
> > +#ifdef DDB
> > int ampintc_ipi_ddb(void *);
> > +#endif
> > void ampintc_send_ipi(struct cpu_info *, int);
> >
> > struct cfattach ampintc_ca = {
> > @@ -347,15 +351,19 @@ ampintc_attach(struct device *parent, st
> > ampintc_intr_establish(ipiirq[0], IST_EDGE_RISING,
> > IPL_IPI|IPL_MPSAFE, NULL, ampintc_ipi_combined, sc, "ipi");
> > sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> > +#ifdef DDB
> > sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[0];
> > +#endif
> > break;
> > case 2:
> > ampintc_intr_establish(ipiirq[0], IST_EDGE_RISING,
> > IPL_IPI|IPL_MPSAFE, NULL, ampintc_ipi_nop, sc, "ipinop");
> > sc->sc_ipi_num[ARM_IPI_NOP] = ipiirq[0];
> > +#ifdef DDB
> > ampintc_intr_establish(ipiirq[1], IST_EDGE_RISING,
> > IPL_IPI|IPL_MPSAFE, NULL, ampintc_ipi_ddb, sc, "ipiddb");
> > sc->sc_ipi_num[ARM_IPI_DDB] = ipiirq[1];
> > +#endif
> > break;
> > default:
> > panic("nipi unexpected number %d", nipi);
> > @@ -954,6 +962,7 @@ ampintc_intr_barrier_msi(void *cookie)
> > }
> >
> > #ifdef MULTIPROCESSOR
> > +#ifdef DDB
> > int
> > ampintc_ipi_ddb(void *v)
> > {
> > @@ -961,6 +970,7 @@ ampintc_ipi_ddb(void *v)
> > db_enter();
> > return 1;
> > }
> > +#endif
> >
> > int
> > ampintc_ipi_nop(void *v)
> > @@ -972,14 +982,16 @@ ampintc_ipi_nop(void *v)
> > int
> > ampintc_ipi_combined(void *v)
> > {
> > +#ifdef DDB
> > struct ampintc_softc *sc = (struct ampintc_softc *)v;
> >
> > if (sc->sc_ipi_reason[cpu_number()] == ARM_IPI_DDB) {
> > sc->sc_ipi_reason[cpu_number()] = ARM_IPI_NOP;
> > return ampintc_ipi_ddb(v);
> > - } else {
> > - return ampintc_ipi_nop(v);
> > }
> > +#endif
> > +
> > + return ampintc_ipi_nop(v);
> > }
> >
> > void
> > @@ -991,9 +1003,11 @@ ampintc_send_ipi(struct cpu_info *ci, in
> > if (ci == curcpu() && id == ARM_IPI_NOP)
> > return;
> >
> > +#ifdef DDB
> > /* never overwrite IPI_DDB with IPI_NOP */
> > if (id == ARM_IPI_DDB)
> > sc->sc_ipi_reason[ci->ci_cpuid] = id;
> > +#endif
> >
> > /* currently will only send to one cpu */
> > sendmask = 1 << (16 + ci->ci_cpuid);
> >
