Re: [Xen-devel] Re: Making pages writable again in paging_log_dirty mode

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

Hi, 

At 18:09 -0500 on 04 Jan (1231092582), Mike Sun wrote:
> >> (XEN) sh error: sh_remove_shadows(): can't find all shadows of mfn
> >> 02673 (shadow_flags=00000008)
> 
> Premature declaration of success.  I'm still getting the panic above
> from this piece of code.
> 
>            SHADOW_FOREACH_L1E(sl1mfn, sl1e, 0, done_l1,
>            {
>                flags_l1 = shadow_l1e_get_flags(*sl1e);
>                if ((flags_l1 & _PAGE_PRESENT) && !(flags_l1 & _PAGE_RW))
>                {
>                    mfn = shadow_l1e_get_mfn(*sl1e);
>                    pfn = mfn_to_gfn(v->domain, mfn);
>                    log_dirty_lock(v->domain);
>                    if (mfn_valid(mfn) && VALID_M2P(pfn) &&
>                        sh_mfn_is_dirty(v->domain, mfn))
>                    {
>                        /* hack: because of the external mapping condition
>                         * ref count not incremented when this is called from
>                         * hypercall originating from dom0, need to do it
>                         * manually
>                         */
>                        struct page_info *page = mfn_to_page(mfn);
>                        if (((page->u.inuse.type_info & PGT_type_mask)
>                                == PGT_writable_page)
>                            && ((page->u.inuse.type_info & PGT_count_mask) == 
> 0)

That looks quite strange to me; it will select every dirty page that is
not currently writeable

 - regardless of whether the guest asked for a writeable mapping
   (e.g. user-mode read-only maps of kernel structures); and
 - regardless of whether it's currently shadowed!

If you make a shadowed page writeable this way then the backstop check
in get_page_type() (which you asked about in another email) will force
it to be unshadowed immediately.  If you do that to the currently in-use
top-level pagetable, you'll get the crash above.

I think you need to check for !sh_mfn_is_a_page_table(mfn) before trying
to make this writeable.

>                            && get_page_type(page, PGT_writable_page))
>                        {
>                            shadow_l1e_t rw_sl1e =
>                                shadow_l1e_add_flags(*sl1e, _PAGE_RW);

Did you by any chance make this shadow_l1e_add_flags() operator by
copying l1e_add_flags from asm/page.h?  That's a macro which modifies
its arguments, which you are definitely not allowed to do to a live
shadow pagetable. :)

Cheers,

Tim.

>                            shadow_write_entries(sl1e, &rw_sl1e, 1, sl1mfn);
>                            cow.made_rw_count++;
>                        }
>                    }
>                    log_dirty_unlock(v->domain);
>                }
>            });
> 
> Am I failing to update the page count somewhere?  Or are there more
> conditions I need to check for the page that I'm adding a RW mapping
> to?
> 
> Thanks,
> Mike

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel