Re: [Xen-devel] [V9 PATCH 6/8] pvh dom0: Add and remove foreign pages

[Tim Deegan <tim@xxxxxxx>]

At 07:50 +0100 on 17 Apr (1397717440), Jan Beulich wrote:
> >>> On 17.04.14 at 03:37, <mukesh.rathor@xxxxxxxxxx> wrote:
> > On Wed, 16 Apr 2014 17:00:35 +0100
> > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> > 
> >> >>> On 16.04.14 at 02:12, <mukesh.rathor@xxxxxxxxxx> wrote:
> >> > In this patch, a new function, p2m_add_foreign(), is added
> >> > to map pages from foreign guest into dom0 for domU creation.
> >> > 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 for convenience.
> >> > Also, p2m_teardown is changed to add a call to allow for the
> >> > cleanup of foreign pages during it's destruction.
> >> > Finally, get_pg_owner is changed to allow pvh to map foreign
> >> > mappings, and is made public to be used by p2m_add_foreign().
> >> 
> >> Do you really need to do this at this low a layer? I'm afraid that's
> >> going to be rather limiting when wanting to use the bits used for the
> >> p2m type for different purposes in intermediate table entries. I.e. I
> >> think this special casing should only be done for leaf entries, and
> >> hence at least one layer further up, or introduce something named
> >> e.g. atomic_write_epte_leaf().
> > 
> > Well, Tim and I went back and forth several times on this over the
> > last several months (you were cc'd :) ). 
> 
> I know, but having worked a lot on the P2M code recently my
> perspective may have changed.

[I'm assuming the objection here is to having ther refcounts updated
in atomic_write_ept_entry, which was the change I requested.]
My opinion is still very strongly that reference counting must be done
when the entries change.  Trying to get this kind of thing right in
the callers is an _enormous_ PITA, as I learned working on the shadow
pagetables.  It would get very messy (see, e.g. the myriad places
where p2m op callers individually check for paged/shared entries) and
it'd be nigh impossible to debug where in several hours of operation
something changed a p2m entry from foreign to something else without
dropping a page ref.

That said, it should be easy enough only to refcount on leaf entries,
right?  I can't see how that would be incompatible with the
intermediate-node changes that Jan is working on.

> >> > @@ -451,6 +456,10 @@ void p2m_teardown(struct p2m_domain *p2m)
> >> >      d = p2m->domain;
> >> >  
> >> >      p2m_lock(p2m);
> >> > +    /* pvh: we must release refcnt on all foreign pages */
> >> > +    if ( is_pvh_domain(d) )
> >> > +        p2m_change_entry_type_global(d, p2m_map_foreign,
> >> > p2m_invalid);
> >> 
> >> Ah, here it comes. But no, this is a no-go. The function currently is
> >> running for unbounded time, which I'm about to change (pending
> >> some more testing; seems to generally work). I can't, however, see
> > 
> > I believe Tim has a patch to make this pre-emptible and OK'd this hook
> > here.
> 
> Has he? I'm sure he would have told me, so I could have avoided
> working towards the (almost) same during the last several weeks.

I don't have a patch -- only ever had good intentions of working
on it in my Copious Free Time.  But since we're going to have to
make a preemptible teardown anyway, I was happy to let this pass on
the grounds that it would get folded into the preemptible version in
time. 

> Plus
> their count multiplies by the number of domains managed (arguably
> that count should be greater than 1 only for non-Dom0 control
> domains, and Dom0 won't make it here anyway - which raises the
> question whether this change is of any practical use under the
> topic on the patch series, i.e. enabling PVH Dom0).

I think this is my fault again -- under the same rules of refcounting
hygiene that demand the ref get/put go right beside the datastructure
update, I asked Mukesh to make sure that the teardown operation
correctly dismantled its refcounts.  

Tim.

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