Date: Sat, 24 Jan 2015 11:53:43 -0700 From: Ian Lepore <ian@freebsd.org> To: Alan Cox <alc@rice.edu> Cc: Konstantin Belousov <kostikbel@gmail.com>, svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64 Message-ID: <1422125623.1038.68.camel@freebsd.org> In-Reply-To: <54C3E563.4070903@rice.edu> References: <201501241251.t0OCpGa8053192@svn.freebsd.org> <1422111397.1038.53.camel@freebsd.org> <20150124154240.GV42409@kib.kiev.ua> <54C3E563.4070903@rice.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 2015-01-24 at 12:33 -0600, Alan Cox wrote: > On 01/24/2015 09:42, Konstantin Belousov wrote: > > On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote: > >> On Sat, 2015-01-24 at 12:51 +0000, Konstantin Belousov wrote: > >>> Author: kib > >>> Date: Sat Jan 24 12:51:15 2015 > >>> New Revision: 277643 > >>> URL: https://svnweb.freebsd.org/changeset/base/277643 > >>> > >>> Log: > >>> Remove Giant from /dev/mem and /dev/kmem. It is definitely not needed > >>> for i386, and from the code inspection, nothing in the > >>> arm/mips/sparc64 implementations depends on it. > >>> > >> I'm not sure I agree with that. On arm the memrw() implementation uses > >> a single statically-allocated page of kva space into which it maps each > >> physical page in turn in the main loop. What prevents preemption or > >> multicore access to /dev/mem from trying to use that single page for > >> multiple operations at once? > > I see, thank you for noting this. > > > > But, I do not think that Giant is a solution for the problem. uiomove() > > call accesses userspace, which may fault and cause sleep. If the > > thread sleeps, the Giant is automatically dropped, so there is no real > > protection. > > > > I think dump exclusive sx around whole memrw() should be enough. > > > > I can revert the commit for now, or I can leave it as is while > > writing the patch with sx and waiting for somebody review. What > > would you prefer ? > > > > P.S. mips uses uiomove_fromphys(), avoiding transient mapping, > > and sparc allocates KVA when needed. > > > > > > While we're here, it's worth noting that the arm version of /dev/mem is > not functionally equivalent to that of amd64 or i386. Arm disallows > access to non-DRAM addresses through /dev/mem. That's true for the read/write interface, but not for mmap(). In fact, we have users insisting that mmap() on /dev/mem should provide userland access to memory-mapped devices, and we have ARM architecture rules that say you can't map the same physical address multiple times with different attributes, such as being Device memory in the kernel and Strongly Ordered when mapped into userland. But if the memory isn't mapped S-O for userland, they have no hope of usefully accessing the devices (because they don't have access to cache and buffer maintenance). Even "normal" memory has a variety of attributes that make the temporary mappings done in memrw() a bit iffy, although I'm not sure we're doing anything right now that could lead to trouble. Trouble lurks though if we ever start using some of the more subtle features of the arm memory architecture, such as turning off the sharable attribute on pages that are inherently per-cpu to avoid the overhead of hardware cache coherence when we know only one core can access the pages. -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1422125623.1038.68.camel>