Re: [V15 PATCH 1/2] pvh dom0: Add and remove foreign pages

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Mon, 26 May 2014 10:24:01 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 24.05.14 at 01:50, <mukesh.rathor@oracle.com> wrote:
> > On Sat, 24 May 2014 01:08:49 +0200
> > Tim Deegan <tim@xen.org> wrote:
> > 
> >> At 15:37 -0700 on 23 May (1400855820), Mukesh Rathor wrote:
> >> > On Fri, 23 May 2014 21:05:34 +0200
> >> > Tim Deegan <tim@xen.org> wrote:
> >> > 
> >> > > At 16:30 -0700 on 22 May (1400772630), Mukesh Rathor wrote:
> >> > > > In this patch, a new function, p2m_add_foreign(), is added
> >> > > > to map pages from a foreign guest into dom0 for various
> >> > > > purposes like domU creation, running xentrace, etc... Such
> >> > > > pages are typed p2m_map_foreign.  Note, it is the nature of
> >> > > > such pages that a refcnt is held during their stay in the
> >> > > > p2m. The refcnt is added and released in the low level ept
> >> > > > function atomic_write_ept_entry. That macro is converted to
> >> > > > a function to allow for such refcounting, which only applies
> >> > > > to leaf entries in the ept. Furthermore, please note that
> >> > > > paging/sharing is disabled if the controlling or hardware
> >> > > > domain is pvh. Any enabling of those features would need to
> >> > > > ensure refcnt are properly maintained for foreign types, or
> >> > > > paging/sharing is skipped for foreign types.
> >> > > > 
> >> > > > Also, we change get_pg_owner so it allows foreign mappings
> >> > > > for pvh.
> >> > > 
> >> > > But you no longer actually call get_pg_owner() for PVH domains,
> >> > > right? So that hunk should go away.  With that done,
> >> > 
> >> > Hi Tim,
> >> > 
> >> > We actually need get_pg_owner for the mmuext call by the
> >> > toolstack when building a PV domain, doing pinning operations on
> >> > the guest table.
> >> 
> >> Ah, I see.  Let's handle that in a separate patch then, since it's
> >> now unrelated to foreign mappings in PVH any more.
> >> 
> >> Having the change where it is seems fine, but I think the correct
> >> test is (is_pv() && paging_mode_translate()) rather than
> >> (!is_pvh() && paging_mode_translate()) -- it's a weakness of the
> >> PV pagetable ops that's being avoided here, rather than any
> >> special treatment for PVH.
> > 
> > Good point, but Jan had a concern on that when I had dropped the if
> > statement completely, that it would allow HVM guests to go thru. 
> > Hence !is_pvh to let hvm guest continue to fail.
> 
> The same would be achieved by using is_pv as Tim suggested.

So sorry, but I don't understand how:

    if ( is_pv_domain(curr) && unlikely(paging_mode_translate(curr)) )
    {
        MEM_LOG("Cannot mix foreign mappings with translated domains");
        goto out;
    }

will cause this error for hvm, which is what happens now without my
change, or will continue to with my proposed change:

    if ( !is_pvh_domain(curr) && unlikely(paging_mode_translate(curr)) )
    {
        MEM_LOG("Cannot mix foreign mappings with translated domains");
..


I understand your suggestion earlier was that hvm should continue to fail.

Also, my understanding is that pv domains are never translated?

thanks
mukesh