svn commit: r289759 - in head/sys/arm: arm include

[Svatopluk Kraus]

On Thu, Nov 5, 2015 at 1:11 AM, Jason Harmening
<jason.harmening at gmail.com> wrote:
>
> On Wed, Nov 4, 2015 at 2:17 PM, Svatopluk Kraus <onwahe at gmail.com> wrote:
>>
>> On Tue, Nov 3, 2015 at 8:45 AM, Jason Harmening
>> <jason.harmening at gmail.com> wrote:
>> >
>> > On Sun, Nov 1, 2015 at 8:11 AM, Ian Lepore <ian at 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?