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

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

On Thu, 24 Apr 2014 19:09:25 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> 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:
> > 
> > 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)
> 
> One thing, on returning error code, since at present there are no
> paths allowing superpages for foreign types, it appears I'd not need 
> to worry about undoing the ept_split_super_page(), so I added
> assert in there. Sound right?
> 
> Finally, I can leave other callers of atomic_write_ept_entry as is,
> or add ASSERTs for rc == 0. LMK.
> 
> thanks for patience,
> mukesh


Tim/Jan,

Ok, so far:

option 1, Jan's suggestion: 
  - Use my latest patch, but add explicit set of p2m type for intermediate
    entries in ept_set_middle_entry() like Jan said.

    Good: we won't need to then worry about checking for leaf entries.
    Bad: future code can't use the bits for interm entries.

option 2, Tim's suggestion:
  - Use my latest patch, but add sp = 1 in leaf entries. Then use level
    to figure if we are in superpage. Will work too, but will change the
    definition of sp bit, and superpage check will always require checking
    for level. 

option 3:
  - Pass level to atomic_write_ept_entry() and check for foreign only in
    case of level == 0, ie leaf entries where foreign can be set.

    It appears that information is available at all call sites, and would
    be least intrusive in terms of using bits for non-leaf entries.

option 4:
  - each call site checks for level and calls either macro for leaf entry
    or macro for non-leaf entry. same as 3, but the check is in call site.


I'll think of more options over the weekend, but I think I like option 3 
the best. LMK, and I will work on it.

thanks
mukesh