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

[Tim Deegan <tim@xxxxxxx>]

At 19:21 -0700 on 23 Apr (1398277311), Mukesh Rathor wrote:
> On Thu, 17 Apr 2014 14:58:42 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
> > >>> On 17.04.14 at 14:36, <tim@xxxxxxx> wrote:
> > > 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:
> .......
> > >> > 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.
> > 
> > Right - keeping the macro as is and introducing a derived function to
> > handle the extra requirements on leaf entries would seem quite okay,
> > so long as error propagation can be done properly.
> 
> Ok, how about something like the following? In case of get_page failure,
> not sure EINVAL is the best one to return, EBUSY?

This goes back to having refcounts open-coded.  Having the refcounts
open-coded around the atomic_write_ept_entry() in ept_set_entry()
means there are now places where the epte can change without
maintaining the refcount invariants: ept_change_entry_type_page(), for
example.

I would _much_ prefer to have atomic_write_ept_entry() DTRT -- it
would have to know the difference between leaf and non-leaf entries,
and return an error code.  I'd also be OK with having two
atomic_write ops, one for leaf and one for non-leaf, with appropriate
ASSERT()s on the contents.

Cheers,

Tim.


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