Hi Russell,
From: "ext Russell King - ARM Linux" <linux@xxxxxxxxxxxxxxxx>
Subject: Re: [PATCH 9/9] ARM: OMAP: Add MMU framework
Date: Thu, 6 Dec 2007 09:23:26 +0000
> On Wed, Dec 05, 2007 at 01:14:15PM -0800, Tony Lindgren wrote:
> > +static void omap_mmu_clear_pte(struct omap_mmu *mmu, unsigned long virt)
> > +{
> > + pte_t *ptep, *end;
> > + pmd_t *pmdp;
> > + struct mm_struct *mm = mmu->twl_mm;
> > +
> > + spin_lock(&mm->page_table_lock);
> > +
> > + pmdp = pmd_offset(pgd_offset(mm, virt), virt);
> > +
> > + if (pmd_none(*pmdp))
> > + goto out;
>
> This covers the all zero case.
>
> > +
> > + if (!pmd_table(*pmdp))
> > + goto invalidate_pmd;
>
> This covers the least significant two bits not being '01'. However, the
> classic detection for non-page tables is to use pmd_bad(), or rather to
> use pmd_none_or_clear_bad().
Thanks. I will fix it.
> > +
> > + ptep = pte_offset_kernel(pmdp, virt);
> > + pte_clear(mm, virt, ptep);
> > + flush_pmd_entry((pmd_t *)ptep);
>
> No. That's wrong, very wrong, and that cast just screams that it's wrong.
> Since pte_clear() calls set_pte_ext(), which calls the per-processor
> implementation, which in turn handles the cache issues, this flush is
> just not required.
Thanks. I will fix it.
> > +
> > + /* zap pte */
> > + end = pmd_page_vaddr(*pmdp);
> > + ptep = end - PTRS_PER_PTE;
> > + while (ptep < end) {
> > + if (!pte_none(*ptep))
> > + goto out;
> > + ptep++;
> > + }
> > + pte_free_kernel(pmd_page_vaddr(*pmdp));
>
> So you free the PMD table.
>
> > +
> > + invalidate_pmd:
> > + pmd_clear(pmdp);
> > + flush_pmd_entry(pmdp);
>
> and then remove it from the page tables. Two problems - if the entries
> have been copied to another task, you'll be leaving the PMD table present
> there. Secondly, if you're using this with SMP it's hellishly racy.
> (which is why we don't support section mappings in ioremap on SMP.)
For the first case, this pagetable is dedicated only to each
coprocessor and the coprocessor's MMU is independent of ARM side MMU
from H/W point of view. This code just manages the consistency between
the kernel pagetable(init_mm) and the coprocessor's pagetable against
real physical address at the same time. So this coprocessor's
pagetable itself isn't duplicated even when forking. An user process
also can map/unmap it on its own pagetable through the device driver's
"mmap()/munmap()", looking up the coprocessor's pagetable, where "use
count" is provided to check if this mapping can be free'ed or not when
these pages are going to be free'd.
For example, DSP mmap implementation can be found below:
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=blob;f=drivers/dsp/dspgateway/task.c;h=e5ee8e0e2169f02fdba094094ee1d9ff222b12fe;hb=d23b6a447c9cd1d7376c5ec109b4be015a121eec
So I think that the consistency of this use count is covered here at
least, although I think that, ideally, it may be better to use process
address space management feature more directly without manipulating
the kernel pagetable. If you have any better way for this, please let
me know.
For the second case, I haven't thought that case because OMAP doesn't
have SMP so far.
> Haven't finished reviewing the rest of the patch.
Thank you for taking your time.
Hiroshi DOYU
-
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
