Re: [patch] pagetable cleanups, next version

[Gerd Knorr <kraxel@bytesex.org>]

> > -    unsigned long *pt = map_domain_mem(pt_mfn << PAGE_SHIFT);
> > -    unsigned long match =
> > -        (readonly_gmfn << PAGE_SHIFT) | _PAGE_RW | _PAGE_PRESENT;
> > -    unsigned long mask = PAGE_MASK | _PAGE_RW | _PAGE_PRESENT;
> > +    l1_pgentry_t *pt = map_domain_mem(pt_mfn << PAGE_SHIFT);
> > +    l1_pgentry_t match;
> 
> I don't quite understand how this is equivalent.

Well, I admit it's a bit confusing because multiple things are changed
at the same time ...

> How does the match work now?

I quote a few more lines important lines from the patch:

> > +    unsigned long flags = _PAGE_RW | _PAGE_PRESENT;
> > +    match = l1e_create_pfn(readonly_gmfn, flags);

> > -#define MATCH_ENTRY(_i) (((pt[_i] ^ match) & mask) == 0)

> > -    if ( MATCH_ENTRY(readonly_gpfn & (L1_PAGETABLE_ENTRIES - 1)) &&
> > -         fix_entry(readonly_gpfn & (L1_PAGETABLE_ENTRIES - 1)) )
> > +    i = readonly_gpfn & (L1_PAGETABLE_ENTRIES - 1);
> > +    if ( !l1e_has_changed(&pt[i], &match, flags) && fix_entry(i) )

There is the new l?e_has_changed() function which can be used to compare
two page table entries.  It takes ptr's to two page table entries and a
bitmask with the page flags it should care about.  The function will
also deal with PAGE_MASK, so this is gone from the functions calling
into l?e_has_changed().

In that particular function I also ditched the MATCH_ENTRY() macro which
did something simliar to l?e_has_changed() in a less generic way.  That
makes the code look even more different, although it _should_ be the
same logic and it IMHO is also more readable.  I admit that I havn't
tested shadow mode though ...

CC'ing xen-devel again so others can have a look as well.

> Looks like create plus has changed is the new way?

Huh?  I lost you here ...

  Gerd

-- 
#define printk(args...) fprintf(stderr, ## args)