Re: [patch 1/3] uml: share page bits handling between 2 and 3 level pagetables

[Jeff Dike <jdike@addtoit.com>]

On Wed, Aug 10, 2005 at 09:37:28PM +0200, Blaisorblade wrote:
> Also look, on the "set_pte" theme, at the attached patch. 

+       WARN_ON(!pte_young(*pte) || pte_write(*pte) && !pte_dirty(*pte));

This one has been firing on me, and I decided to figure out why.  The
culprit is this code in do_no_page:

	if (pte_none(*page_table)) {
		if (!PageReserved(new_page))
			inc_mm_counter(mm, rss);

		flush_icache_page(vma, new_page);
		entry = mk_pte(new_page, vma->vm_page_prot);
		if (write_access)
			entry = maybe_mkwrite(pte_mkdirty(entry), vma);
		set_pte_at(mm, address, page_table, entry);

The first mk_pte immediately sets the pte to the protection limits of
the VMA, regardless of the access type.  So, if it's a read access on
a writeable page, we get a writeable, but not dirty pte, since the
mkdirty never happens.  The exercises the warning you added.

This seems somewhat bogus to me.  If we set the pte protection to its
limits, then the maybe_mkwrite is unneccesary.  

If we are the process in this address space, and we have a write
access, then the maybe_mkwrite doesn't do anything because the pte is
already writeable because the VMA has to be writeable, or we would
have been faulted already.

If we are a debugger changing the process memory, then the vma may be
read-only, and maybe_mkwrite is explicitly a no-op in this case.

This doesn't seem to harm our dirty bit emulation.  fix_range_common
checks the dirty and accessed bits and disables read and write
protection as appropriate.

So, it seems like the warning could be dropped, or perhaps made more
selective, like checking for is_write == 0 and VM_WRITE, but then the
test is getting complicated.

				Heff