Date: Thu, 5 Nov 2015 08:08:36 -0800 From: Jason Harmening <jason.harmening@gmail.com> To: Svatopluk Kraus <onwahe@gmail.com> Cc: Ian Lepore <ian@freebsd.org>, Ganbold Tsagaankhuu <ganbold@gmail.com>, 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: <CAM=8qanoZotHXsiDyUCKe5jszzPsLE07uxM1vnOBnKowvvSvYQ@mail.gmail.com> In-Reply-To: <CAFHCsPX2cGn5EFg1M9mnpsnq=ikUfnAb%2BhxF1zRx8ckDsivR_w@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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?CAM=8qanoZotHXsiDyUCKe5jszzPsLE07uxM1vnOBnKowvvSvYQ>