Date: Thu, 05 Nov 2015 10:21:43 -0700 From: Ian Lepore <ian@freebsd.org> To: Jason Harmening <jason.harmening@gmail.com>, Svatopluk Kraus <onwahe@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r289759 - in head/sys/arm: arm include Message-ID: <1446744103.91534.399.camel@freebsd.org> In-Reply-To: <CAM=8qanoZotHXsiDyUCKe5jszzPsLE07uxM1vnOBnKowvvSvYQ@mail.gmail.com> References: <201510221638.t9MGc1cc053885@repo.freebsd.org> <CAGtf9xOx_iSNqsPT7OdTmAy-ER_wFzOC9YxFigpQ%2B0wLj7=ytQ@mail.gmail.com> <56348FF8.3010602@gmail.com> <CAM=8qa=4EqPvRoUnMytQBe32GtvPRZRE_s34tOzmVNCLe8c=Fw@mail.gmail.com> <1446394311.91534.236.camel@freebsd.org> <CAM=8qa=GX3yZmcXmMZz-nfmwOFhXyyKh_6%2BLVR=y7P5pURYvpQ@mail.gmail.com> <CAFHCsPWE3YLcO_GKL7pGkB5wU342WDZ5T695S_DPG7eBQNmqaQ@mail.gmail.com> <CAM=8qakYSkY-8_z7D%2Bg=ctip8h9qrb-k_dLRyZVYLg5AvTZbEw@mail.gmail.com> <CAFHCsPX2cGn5EFg1M9mnpsnq=ikUfnAb%2BhxF1zRx8ckDsivR_w@mail.gmail.com> <CAM=8qanoZotHXsiDyUCKe5jszzPsLE07uxM1vnOBnKowvvSvYQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Just as an FYI for you guys, since we're all splashing around in busdma code lately.... The things on my near-term (next several weeks) to-do list for busdma are: Add "small bounce" support to help reduce the bounce preallocation overhead for handling bounces due to cacheline alignment. Almost all arm and mips bounces are triggered by cache alignment restrictions and involve buffers smaller than a page (usually 256 bytes or less), and we currently allocate full pages for bouncing. Instead we can use the same small-buffer uma pools that are used for bus_dmamem_alloc(). The armv6 busdma implementation can be shared by armv4, armv6, and mips. The only thing blocking this right now is the small-bounce stuff; armv4 and mips boards often have only 32-128MB of ram, and they can't afford the bounce-page allocation overhead of the current armv6 implementation. Once the small-bounce support is in place, we can use this implementation for all these platforms (and basically for any platform that comes along that has a similar requirement for software -assisted cache coherency for DMA). -- Ian On Thu, 2015-11-05 at 08:08 -0800, Jason Harmening wrote: > Userspace buffers in load_buffer() also need temporary mappings, so > it > might be nice to keep the panic/KASSERT there for completeness. I > don't > see how anything coming from userspace would be outside > vm_page_array > though, so that is up to you and Michal. > > Since Michal's already made the initial patch, I think he should make > the > commit. That seems only right, plus I'm still busy trying to get > everything set up at my new place. I'd like to be on the phabricator > review, though. > > On Thu, Nov 5, 2015 at 3:58 AM, Svatopluk Kraus <onwahe@gmail.com> > wrote: > > > On Thu, Nov 5, 2015 at 1:11 AM, Jason Harmening > > <jason.harmening@gmail.com> wrote: > > > > > > On Wed, Nov 4, 2015 at 2:17 PM, Svatopluk Kraus <onwahe@gmail.com > > > > > > wrote: > > > > > > > > On Tue, Nov 3, 2015 at 8:45 AM, Jason Harmening > > > > <jason.harmening@gmail.com> wrote: > > > > > > > > > > On Sun, Nov 1, 2015 at 8:11 AM, Ian Lepore <ian@freebsd.org> > > > > > wrote: > > > > > > > > > > > > > > > > > > It's almost certainly not related to sysinit ordering. > > > > > > This > > exception > > > > > > is happening during mmc probing after interrupts are > > > > > > enabled. > > > > > > > > > > > > It appears that the problem is the faulting code is running > > > > > > on one of > > > > > > the very early pre-allocated kernel stacks (perhaps in an > > > > > > interrupt > > > > > > handler on an idle thread stack), and these stacks are not > > > > > > in memory > > > > > > represented in the vm page arrays. The mmc code is using a > > > > > > 64-byte > > > > > > buffer on the stack and mapping it for DMA. Normally that > > > > > > causes a > > > > > > bounce for cacheline alignment, but unluckily in this case > > > > > > that > > buffer > > > > > > on the stack just happened to be aligned to a cacheline > > > > > > boundary and > > a > > > > > > multiple of the cacheline size, so no bounce. That causes > > > > > > the new > > sync > > > > > > logic that is based on keeping vm_page_t pointers and > > > > > > offsets to get > > a > > > > > > NULL pointer back from PHYS_TO_VM_PAGE when mapping, then > > > > > > it dies at > > > > > > sync time trying to dereference that. It used to work > > > > > > because the > > sync > > > > > > logic used to use the vaddr, not a page pointer. > > > > > > > > > > > > Michal was working on a patch yesterday. > > > > > > > > > > > > > > > > Ah, thanks for pointing that out Ian. I was left scratching > > > > > my head > > > > > (admittedly on the road and w/o easy access to the code) > > > > > wondering > > what > > > > > on > > > > > earth would be trying to do DMA during SI_SUB_CPU. > > > > > > > > > > > > > > > > > Using of fictitious pages is not so easy here as in case > > > > pointed by > > > > kib@ where they are allocated and freed inside one function. > > > > For sync > > > > list sake, they must be allocated when a buffer is loaded and > > > > freed > > > > when is unloaded. > > > > > > > > Michal uses pmap_kextract() in case when KVA of buffer is not > > > > zero in > > > > his proof-of-concept patch: > > > > https://gist.github.com/strejda/d5ca3ed202427a2e0557 > > > > When KVA of buffer is not zero, temporary mapping is not used > > > > at all, > > > > so vm_page_t array is not needed too. > > > > > > > > IMO, buffer's physical address can be saved in sync list to be > > > > used > > > > when its KVA is not zero. Thus pmap_kextract() won't be called > > > > in > > > > dma_dcache_sync(). > > > > > > > > > I like the idea of storing off the physical address. If you want > > > to save > > > space in the sync list, I think you can place busaddr and pages > > > in a > > union, > > > using vaddr == 0 to select which field to use. Some people frown > > > upon > > use > > > of unions, though. > > > > > > Any reason the panics on PHYS_TO_VM_PAGE in load_buffer() and > > > load_phys() > > > shouldn't be KASSERTs instead? > > > > > > KASSERTs are fine. I have looked at Michal's patch again and only > > KASSERT should be in load_phys() as such buffers are unmapped, > > temporary mapping must be used and so, they must be in > > vm_page_array. > > And maybe some inline function for getting PA from sync list would > > be > > nice too. I would like to review final patch if you are going to > > make > > it. And it would be nice if Ganbold will test it. Phabricator? > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1446744103.91534.399.camel>