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

[Tim Deegan <tim@xxxxxxx>]

At 07:50 +0100 on 28 May (1401259810), Jan Beulich wrote:
> >>> On 28.05.14 at 02:51, <mukesh.rathor@xxxxxxxxxx> wrote:
> > On Mon, 26 May 2014 10:24:01 +0100
> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >> >>> On 24.05.14 at 01:50, <mukesh.rathor@xxxxxxxxxx> wrote:
> >> > On Sat, 24 May 2014 01:08:49 +0200
> >> > Tim Deegan <tim@xxxxxxx> wrote:
> >> >> 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?
> 
> Hmm, indeed, good point. I wonder whether the whole condition
> shouldn't simply become !is_hvm() then (and perhaps the log
> message adjusted accordingly). Tim?

I'll have to trawl the history to see where this came from, but 
AFAIK the worry is for old-style PV+translate, where if we let a
guest put foreign mappings directly in its pagetables they would
fail p2m lookups in the shadow code when trying to translate them.

So we could: 
 - check (is_pv && paging_mode_translate), as I suggested before; or
 - check (paging_mode_translate && !paging_mode_refcounts), which
   is a bit more idiomatic and more closely describes the problem; or
 - not check at all, since PVH has entirely replaced even the partial
   support for PV+translate that was there before.

In any case, I don't think we should be using is_pvh or is_hvm here,
since they've got effectively the same memory management.  But I could
live with it as an intermediate measure to defer checking e.g. shadow
pagetable support...

For further confusion, get_pg_owner is really for foreign mappings but
is reused on the mmuext op path, where the caller is adjusting the
foreign domain's pagetables and not its own.  There this test is
pointless since the state of the _caller's_ pagetables doesn't matter.
And _that's_ the case that Mukesh needs to relax for PVH dom0.  So it
would also be OK (if a bit ugly) to move this check into the other two
callers of the function and just not have it for mmuext_op.

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel