Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
On Sun, Jan 25, 2015 at 10:07:00AM -0700, Warner Losh wrote: > > > On Jan 24, 2015, at 8:51 AM, Konstantin Belousov > > wrote: > > > > On Sat, Jan 24, 2015 at 05:42:40PM +0200, Konstantin Belousov wrote: > >> On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote: > >>> On Sat, 2015-01-24 at 12:51 +, 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. > > > > Like this. > > So why a sx lock and not a mutex? I explained this above. uiomove() needs to sleep on fault. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
> On Jan 24, 2015, at 8:51 AM, Konstantin Belousov wrote: > > On Sat, Jan 24, 2015 at 05:42:40PM +0200, Konstantin Belousov wrote: >> On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote: >>> On Sat, 2015-01-24 at 12:51 +, 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. > > Like this. So why a sx lock and not a mutex? Warner > diff --git a/sys/arm/arm/mem.c b/sys/arm/arm/mem.c > index 30d4b1d..58b0d25 100644 > --- a/sys/arm/arm/mem.c > +++ b/sys/arm/arm/mem.c > @@ -55,6 +55,7 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > #include > #include > > @@ -72,6 +73,9 @@ MALLOC_DEFINE(M_MEMDESC, "memdesc", "memory range > descriptors"); > > struct mem_range_softc mem_range_softc; > > +static struct sx tmppt_lock; > +SX_SYSINIT(tmppt, &tmppt_lock, "mem4map"); > + > /* ARGSUSED */ > int > memrw(struct cdev *dev, struct uio *uio, int flags) > @@ -107,6 +111,7 @@ memrw(struct cdev *dev, struct uio *uio, int flags) > } > if (!address_valid) > return (EINVAL); > + sx_xlock(&tmppt_lock); > pmap_kenter((vm_offset_t)_tmppt, v); > o = (int)uio->uio_offset & PAGE_MASK; > c = (u_int)(PAGE_SIZE - ((int)iov->iov_base & > PAGE_MASK)); > @@ -114,6 +119,7 @@ memrw(struct cdev *dev, struct uio *uio, int flags) > c = min(c, (u_int)iov->iov_len); > error = uiomove((caddr_t)&_tmppt[o], (int)c, uio); > pmap_qremove((vm_offset_t)_tmppt, 1); > + sx_xunlock(&tmppt_lock); > continue; > } > else if (dev2unit(dev) == CDEV_MINOR_KMEM) { > ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
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 +, 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 ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
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 +, 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 ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
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 +, 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. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
On Sat, 2015-01-24 at 17:42 +0200, Konstantin Belousov wrote: > On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote: > > On Sat, 2015-01-24 at 12:51 +, 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. I had planned to look into this today or tomorrow, so no need to revert. I'll study the other implementations and see what works well for arm. -- Ian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
On Sat, Jan 24, 2015 at 05:42:40PM +0200, Konstantin Belousov wrote: > On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote: > > On Sat, 2015-01-24 at 12:51 +, 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. Like this. diff --git a/sys/arm/arm/mem.c b/sys/arm/arm/mem.c index 30d4b1d..58b0d25 100644 --- a/sys/arm/arm/mem.c +++ b/sys/arm/arm/mem.c @@ -55,6 +55,7 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include #include #include @@ -72,6 +73,9 @@ MALLOC_DEFINE(M_MEMDESC, "memdesc", "memory range descriptors"); struct mem_range_softc mem_range_softc; +static struct sx tmppt_lock; +SX_SYSINIT(tmppt, &tmppt_lock, "mem4map"); + /* ARGSUSED */ int memrw(struct cdev *dev, struct uio *uio, int flags) @@ -107,6 +111,7 @@ memrw(struct cdev *dev, struct uio *uio, int flags) } if (!address_valid) return (EINVAL); + sx_xlock(&tmppt_lock); pmap_kenter((vm_offset_t)_tmppt, v); o = (int)uio->uio_offset & PAGE_MASK; c = (u_int)(PAGE_SIZE - ((int)iov->iov_base & PAGE_MASK)); @@ -114,6 +119,7 @@ memrw(struct cdev *dev, struct uio *uio, int flags) c = min(c, (u_int)iov->iov_len); error = uiomove((caddr_t)&_tmppt[o], (int)c, uio); pmap_qremove((vm_offset_t)_tmppt, 1); + sx_xunlock(&tmppt_lock); continue; } else if (dev2unit(dev) == CDEV_MINOR_KMEM) { ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
On Sat, Jan 24, 2015 at 07:56:37AM -0700, Ian Lepore wrote: > On Sat, 2015-01-24 at 12:51 +, 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. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
On Sat, 2015-01-24 at 12:51 +, 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? -- Ian ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r277643 - in head/sys: arm/arm dev/mem i386/i386 mips/mips sparc64/sparc64
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. Discussed with: imp, nwhitehorn Sponsored by: The FreeBSD Foundation MFC after:3 weeks Modified: head/sys/arm/arm/mem.c head/sys/dev/mem/memdev.c head/sys/i386/i386/mem.c head/sys/mips/mips/mem.c head/sys/sparc64/sparc64/mem.c Modified: head/sys/arm/arm/mem.c == --- head/sys/arm/arm/mem.c Sat Jan 24 12:43:36 2015(r277642) +++ head/sys/arm/arm/mem.c Sat Jan 24 12:51:15 2015(r277643) @@ -82,8 +82,6 @@ memrw(struct cdev *dev, struct uio *uio, int error = 0; vm_offset_t addr, eaddr; - GIANT_REQUIRED; - while (uio->uio_resid > 0 && error == 0) { iov = uio->uio_iov; if (iov->iov_len == 0) { Modified: head/sys/dev/mem/memdev.c == --- head/sys/dev/mem/memdev.c Sat Jan 24 12:43:36 2015(r277642) +++ head/sys/dev/mem/memdev.c Sat Jan 24 12:51:15 2015(r277643) @@ -52,7 +52,7 @@ static struct cdev *memdev, *kmemdev; static struct cdevsw mem_cdevsw = { .d_version =D_VERSION, - .d_flags = D_MEM|D_NEEDGIANT, + .d_flags = D_MEM, .d_open = memopen, .d_read = memrw, .d_write = memrw, Modified: head/sys/i386/i386/mem.c == --- head/sys/i386/i386/mem.cSat Jan 24 12:43:36 2015(r277642) +++ head/sys/i386/i386/mem.cSat Jan 24 12:51:15 2015(r277643) @@ -86,10 +86,6 @@ memrw(struct cdev *dev, struct uio *uio, int error = 0; vm_offset_t addr; - /* XXX UPS Why ? */ - GIANT_REQUIRED; - - if (dev2unit(dev) != CDEV_MINOR_MEM && dev2unit(dev) != CDEV_MINOR_KMEM) return EIO; Modified: head/sys/mips/mips/mem.c == --- head/sys/mips/mips/mem.cSat Jan 24 12:43:36 2015(r277642) +++ head/sys/mips/mips/mem.cSat Jan 24 12:51:15 2015(r277643) @@ -85,8 +85,6 @@ memrw(struct cdev *dev, struct uio *uio, cnt = 0; error = 0; - GIANT_REQUIRED; - pmap_page_init(&m); while (uio->uio_resid > 0 && !error) { iov = uio->uio_iov; Modified: head/sys/sparc64/sparc64/mem.c == --- head/sys/sparc64/sparc64/mem.c Sat Jan 24 12:43:36 2015 (r277642) +++ head/sys/sparc64/sparc64/mem.c Sat Jan 24 12:51:15 2015 (r277643) @@ -99,8 +99,6 @@ memrw(struct cdev *dev, struct uio *uio, error = 0; ova = 0; - GIANT_REQUIRED; - while (uio->uio_resid > 0 && error == 0) { iov = uio->uio_iov; if (iov->iov_len == 0) { ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"