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

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

On Fri, 25 Apr 2014 10:55:19 +0200
Tim Deegan <tim@xen.org> wrote:

> At 19:09 -0700 on 24 Apr (1398362965), Mukesh Rathor wrote:
> > On Thu, 24 Apr 2014 11:46:41 +0200
> > Tim Deegan <tim@xen.org> wrote:
> > 
> > > At 19:21 -0700 on 23 Apr (1398277311), Mukesh Rathor wrote:
> > > > On Thu, 17 Apr 2014 14:58:42 +0100
> > > > "Jan Beulich" <JBeulich@suse.com> wrote:
> > > > 
> > > > > >>> On 17.04.14 at 14:36, <tim@xen.org> wrote:
> > > > > > At 07:50 +0100 on 17 Apr (1397717440), Jan Beulich wrote:
> > > > > >> >>> On 17.04.14 at 03:37, <mukesh.rathor@oracle.com> wrote:
> > > > > >> > On Wed, 16 Apr 2014 17:00:35 +0100
> > > > > >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > ......
> > > > > > 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.
> > 
> > Correct, altho, at present I've checks in p2m paths to not allow
> > foreign types to come down to such calls.
> > 
> > > 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.
> > 
> > Ok, how about something like shown further below?  (I think
> > it would be more simpler to have one atomic_write ops, instead of
> > two)
> 
> I like this approach.  I think the test for foreignness needs to check
> that it's dealing with a leaf node, e.g. by checking that bit .sp == 1

Right, right. My bad, somehow my brain got fixated thinking that sp ==0
meant we were on leaf node. 

> and setting that bit in all leaf entries.  At the moment we clear .sp
> in 4k entries but the docs say it's ignored by hardware so we could
> set it, I think -- it would need an adjustment of ept_next_level() to
> detect superpages by checking the level.

Right, that's an option. The bit is ignored for L1 entries. But would
be super confusing to anyone looking at it, me first. By convention it's 
been the superpage bit.... Let me see if I can come up with alternate 
solutions/proposoals then, now that I (finally) understand whats going
on :). 

thanks
Mukesh