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

[Ian Lepore]

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.

-- Ian