Re: panic booting with if_tap_load="YES" in loader.conf

On Sat, May 18, 2019, 9:48 AM Mark Johnston <markj_at_freebsd.org> wrote:

> On Sat, May 18, 2019 at 05:45:47PM +0300, Konstantin Belousov wrote:
> > On Sat, May 18, 2019 at 05:38:15PM +0300, Konstantin Belousov wrote:
> > > On Sat, May 18, 2019 at 10:24:36AM -0400, Mark Johnston wrote:
> > > > On Sat, May 18, 2019 at 11:55:46AM +0300, Konstantin Belousov wrote:
> > > > > On Sat, May 18, 2019 at 01:33:28AM -0400, Mark Johnston wrote:
> > > > > > On Fri, May 17, 2019 at 10:18:57PM -0600, Rebecca Cran wrote:
> > > > > > > I just updated from r346856 to r347950 and ran into a new
> panic, caused
> > > > > > > by having if_tap_load="YES" in /boot/loader.conf - because
> it's already
> > > > > > > built-in to the kernel:
> > > > > >
> > > > > > I think this patch should fix the panic, but I only
> compile-tested it.
> > > > > > I considered having the logic live in preload_delete_name()
> instead, but
> > > > > > the boot-time ucode code must still defer the deletion somewhat.
> > > > >
> > > > > Try this instead.  I will revert r347931 after this landed, or
> could keep
> > > > > it alone.
> > > >
> > > > I have no strong feeling either way.
> > > >
> > > > > diff --git a/sys/amd64/amd64/machdep.c b/sys/amd64/amd64/machdep.c
> > > > > index 1cf09dc5cb7..03fe8a5d096 100644
> > > > > --- a/sys/amd64/amd64/machdep.c
> > > > > +++ b/sys/amd64/amd64/machdep.c
> > > > > _at__at_ -1616,6 +1616,13 _at__at_ hammer_time(u_int64_t modulep, u_int64_t
> physfree)
> > > > >         bzero((void *)thread0.td_kstack, kstack0_sz);
> > > > >         physfree += kstack0_sz;
> > > > >
> > > > > +       /*
> > > > > +        * Initialize enough of thread0 for delayed invalidation to
> > > > > +        * work very early.  Rely on thread0.td_base_pri
> > > > > +        * zero-initialization, it is reset to PVM at proc0_init().
> > > > > +        */
> > > > > +       pmap_thread_init_invl_gen(&thread0);
> > > > > +
> > > >
> > > > I think pmap_thread_init_invl_gen() also needs to initialize
> > > > invl_gen->saved_pri to 0.
> > > It is zero-initialized, same as td_base_pri. The call to
> > > pmap_thread_init_invl_gen() is needed because _INVALID bit must be set,
> > > which is cleared by default.
> > I now think that you mean something else, that invl_gen.saved_pri must
> > be set on each entry to invl_start_u().  Otherwise invl_finish_u() might
> > act on the priority from the previous DI block.
>
> That is not what I was thinking about, but I agree.  thread0's saved_pri
> should be zero-initialized, but I don't see how any other thread's
> saved_pri is initialized - td_md is not in the zero or copy region of
> the thread structure.  Your patch should address this as well, but maybe
> I am still missing something about how saved_pri gets initialized.  The
> patch looks right to me.
>

Could this crash on other architectures? Do we need fixes there as well...

Warner

> diff --git a/sys/amd64/amd64/machdep.c b/sys/amd64/amd64/machdep.c
> > index 1cf09dc5cb7..03fe8a5d096 100644
> > --- a/sys/amd64/amd64/machdep.c
> > +++ b/sys/amd64/amd64/machdep.c
> > _at__at_ -1616,6 +1616,13 _at__at_ hammer_time(u_int64_t modulep, u_int64_t physfree)
> >       bzero((void *)thread0.td_kstack, kstack0_sz);
> >       physfree += kstack0_sz;
> >
> > +     /*
> > +      * Initialize enough of thread0 for delayed invalidation to
> > +      * work very early.  Rely on thread0.td_base_pri
> > +      * zero-initialization, it is reset to PVM at proc0_init().
> > +      */
> > +     pmap_thread_init_invl_gen(&thread0);
> > +
> >       /*
> >        * make gdt memory segments
> >        */
> > diff --git a/sys/amd64/amd64/pmap.c b/sys/amd64/amd64/pmap.c
> > index 7997a9f65dc..89ba9da19d8 100644
> > --- a/sys/amd64/amd64/pmap.c
> > +++ b/sys/amd64/amd64/pmap.c
> > _at__at_ -700,16 +700,17 _at__at_ pmap_delayed_invl_start_u(void)
> >       invl_gen = &td->td_md.md_invl_gen;
> >       PMAP_ASSERT_NOT_IN_DI();
> >       lock_delay_arg_init(&lda, &di_delay);
> > -     thread_lock(td);
> > +     invl_gen->saved_pri = 0;
> >       pri = td->td_base_pri;
> > -     if (pri < PVM) {
> > -             invl_gen->saved_pri = 0;
> > -     } else {
> > -             invl_gen->saved_pri = pri;
> > -             sched_prio(td, PVM);
> > +     if (pri > PVM) {
> > +             thread_lock(td);
> > +             pri = td->td_base_pri;
> > +             if (pri > PVM) {
> > +                     invl_gen->saved_pri = pri;
> > +                     sched_prio(td, PVM);
> > +             }
> > +             thread_unlock(td);
> >       }
> > -     thread_unlock(td);
> > -
> >  again:
> >       PV_STAT(i = 0);
> >       for (p = &pmap_invl_gen_head;; p = prev.next) {
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"
>