svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64

[Nathan Whitehorn]

On 01/24/15 10:53, Ian Lepore wrote:
> 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.

sparc64 also does not allow mmap() of device memory through /dev/mem for 
this reason. This leads to a whole bunch of #ifdef in X drivers.
-Nathan