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

[Tim Deegan <tim@xxxxxxx>]

At 19:09 -0700 on 24 Apr (1398362965), Mukesh Rathor wrote:
> On Thu, 24 Apr 2014 11:46:41 +0200
> Tim Deegan <tim@xxxxxxx> wrote:
> 
> > 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:
> ......
> > > > > 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
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.

> 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?

Yep, an ASSERT to catch foreign superpage mappings is useful.

> Finally, I can leave other callers of atomic_write_ept_entry as is, or 
> add ASSERTs for rc == 0. LMK.

I would be inclined to ASSERT().

Cheers,

Tim.


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