Re: [Xen-devel] Weird altp2m behaviour when switching early to a new view

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Apr 11, 2018 at 12:39 AM, Razvan Cojocaru
<rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 04/09/2018 05:12 PM, George Dunlap wrote:
>> The obvious place to look is the logdirtyvram functionality, which is
>> used to make it easier for QEMU to figure out which bits of the display
>> buffer have been modified.  One of the big reasons the altp2m
>> functionality is still considered "experimental" is that these sorts of
>> interactions were never carefully thought out.
>
> George (and Alexey), thanks for the help!
>
> I've managed to find out why this is happening, though I don't yet have
> a solution for it. After much debugging, it turns out that the
> "p2m_is_ram(p2mt)" test in hvm_hap_nested_page_fault() fails if I switch
> to the new altp2m view fast enough, and that in turn disables the
> logdirty processing gated on it:
>
> /* Spurious fault? PoD and log-dirty also take this path. */
> if ( p2m_is_ram(p2mt) )
> {
>     rc = 1;
>     /*
>      * Page log dirty is always done with order 0. If this mfn resides
>      * in a large page, we do not change other pages type within that
>      * large page.
>      */
>     if ( npfec.write_access )
>     {
>         paging_mark_pfn_dirty(currd, _pfn(gfn));
>         /*
>          * If p2m is really an altp2m, unlock here to avoid lock
>          * ordering violation when the change below is propagated
>          * from host p2m.
>          */
>         if ( ap2m_active )
>             __put_gfn(p2m, gfn);
>         p2m_change_type_one(currd, gfn, p2m_ram_logdirty, p2m_ram_rw);
>         __put_gfn(ap2m_active ? hostp2m : p2m, gfn);
>
>         goto out;
>     }
>     goto out_put_gfn;
> }
>
> I think this is likely because switching to the new EPT view loses
> information stored when previously working with the default one, in this
> case while that work has not finished (if that's the case, creating a
> new altp2m view should also somehow inherit the history of view 0 - when
> doing set_access() on view 0 the changes propagate to all existent
> altp2m views, but I don't think that happens when a new view is created:
> that starts with a clean slate).
>
> Also related to this part of the altp2m design, I've also noticed that
> creating a new EPT view with libxc involves this function:
>
> int xc_altp2m_create_view(xc_interface *handle, uint32_t domid,
>                           xenmem_access_t default_access,
>                           uint16_t *view_id);
>
> However, default_access is completely ignored. If this was meant to
> reset all pages to that access it's trivial to wire it in into the
> hypervisor (we could just iterate through all guest pages from 0 to
> max_gpfn and set the access) - but in light of this issue that's not a
> good thing to do. Or, rather, it was meant to specify the default_access
> for new pages where it is not explicitly specified?

I don't think the function of default_access for an altp2m was
settled, so both of these would sound to me like a valid setup. I
personally would prefer that if the default_access is specified and
it's not rwx it would result in the entire altp2m view getting
populated right there instead of it being done lazily. The whole
lazy-population of the altp2m views has been nothing but a headache.
Add to that the fact that views get completely reset under certain
situations without any indication being given to the application using
it.. might be time we rethink these parts of altp2m to make it more
robust.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel