svn commit: r330780 - in head/sys: amd64/include isa x86/isa

[Konstantin Belousov]

On Sun, Mar 11, 2018 at 02:20:39PM -0600, Ian Lepore wrote:
> On Sun, 2018-03-11 at 21:58 +0200, Konstantin Belousov wrote:
> > On Sun, Mar 11, 2018 at 07:22:58PM +0000, Ian Lepore wrote:
> > > 
> > > Author: ian
> > > Date: Sun Mar 11 19:22:58 2018
> > > New Revision: 330780
> > > URL: https://svnweb.freebsd.org/changeset/base/330780
> > > 
> > > Log:
> > >   Eliminate atrtc_time_lock, and use atrtc_lock for efirtc locking.
> > > 
> > > Modified:
> > >   head/sys/amd64/include/efi.h
> > >   head/sys/isa/rtc.h
> > >   head/sys/x86/isa/atrtc.c
> > > 
> > > Modified: head/sys/amd64/include/efi.h
> > > ==============================================================================
> > > --- head/sys/amd64/include/efi.h	Sun Mar 11 19:14:01 2018	(r330779)
> > > +++ head/sys/amd64/include/efi.h	Sun Mar 11 19:22:58 2018	(r330780)
> > > @@ -48,9 +48,9 @@
> > >  #ifdef _KERNEL
> > >  #include 
> > >  
> > > -#define	EFI_TIME_LOCK()		mtx_lock(&atrtc_time_lock);
> > > -#define	EFI_TIME_UNLOCK()	mtx_unlock(&atrtc_time_lock);
> > > -#define	EFI_TIME_OWNED()	mtx_assert(&atrtc_time_lock, MA_OWNED);
> > > +#define	EFI_TIME_LOCK()		mtx_lock_spin(&atrtc_lock);
> > > +#define	EFI_TIME_UNLOCK()	mtx_unlock_spin(&atrtc_lock);
> > > +#define	EFI_TIME_OWNED()	mtx_assert(&atrtc_lock, MA_OWNED);
> > I do not understand how this can work. You changed EFI_TIME_LOCK() to
> > spinlock.  As result, the spinlock is taken before efi_enter() is called.
> > efi_enter() locks the pmap and efi_lock, both of which are sleepable
> > mutexes.  Also, arch efirt code is allowed to take arch-specific
> > sleepable mutexes and perhaps some spinlocks (but currently does not).
> > 
> > The first context switch when either of the listed sleepable mutexes
> > are contested and the thread is taken off CPU would panic.  I suspect
> > that some invariants check might fire earlier.
> > 
> 
> Oops, sure enough; I reverted it.
> 
> I think I knew that about efi_enter() at one time, but I had completely
> forgotten it did any locking.  I just realized my only efi-capable
> machine that I tested this on has virtually all debugging and witness
> disabled, so I didn't really give it much of a test after all.
> 
> Unfortunately, this reverts one type of wrong locking back to another.
>  The need is to prevent access to the atrtc hardware if efi is
> accessing it, and the locking I just reverted to that uses a sleepable
> mutex only protects against inittodr()/resettodr() access, but not
> against nvram(4) or if the atrtc is being used as an eventtimer (of
> course nobody uses it for that, but the driver supports it).
> 
> I have some pending changes that cause the atrtc driver to just not
> attach at all if the efirtc driver attached, but they're stacked up
> behind some other changes in phab.  And that still doesn't fix the
> nvram(4) part of it.
Not attaching atrtc if efirtc is attached sounds reasonable. But then
you should also disable efirt attach if atrtc is on. One possible issue
is that efirt is typicall loadable, while atrtc is compiled into the
kernel, which means that efirt would become virtually unusable.

For nvram(4), you can take the atrtc_time_lock around accesses in addition
to the atrtc_lock, instead of providing exclusivity on the level of drivers
attach.