Re: [V12 PATCH 3/4] pvh dom0: Add and remove foreign pages

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

On Tue, 13 May 2014 08:09:42 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 13.05.14 at 03:02, <mukesh.rathor@oracle.com> wrote:
> > On Mon, 12 May 2014 11:34:14 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 10.05.14 at 02:50, <mukesh.rathor@oracle.com> wrote:
> >> > +static int atomic_write_ept_entry(ept_entry_t *entryptr,
> >> > ept_entry_t new,
> >> > +                                  int level)
> >> > +{
> >> > +    unsigned long oldmfn = INVALID_MFN;
> >> > +    bool_t skip_foreign = (new.mfn == entryptr->mfn &&
> >> > +                           new.sa_p2mt == entryptr->sa_p2mt);
> >> 
> >> This still seems too weak to me: Shouldn't you also consider
> >> whether the old and new entries respectively are present (also
> >> further down)?
> > 
> > Not sure I understand why. skip_foreign is combined with
> > p2m_is_foreign: 
> > 
> >     if ( unlikely(p2m_is_foreign(new.sa_p2mt)) && !skip_foreign )
> >     {
> >        ...
> > so checking for invalid entry seems redundant based on my 
> > understanding that invalid entries have sa_p2mt == 1, or are 
> > zeroed, in which case sa_p2mt == 0.
> 
> I didn't say invalid, I said present (i.e. at least one of r, w, or x
> set). For example, it needs to be carefully considered whether the
> second of the two switch() statements in ept_p2m_type_to_flags() could
> have any effect that would require references to be dropped when
> all three flags end up being clear.

That is a valid concern. Here, it's no different than if the type was grant.
But looking at the callers, there's no such case where for grant/foreign
such would happen. However, if it pleases the court, I think it would 
be a good idea to add:

bool_t no_clear_p2mt = type == p2m_grant_map_rw || type == p2m_grant_map_ro || 
                       type == p2m_map_foreign;
...

Then, after second switch:
    BUG_ON(no_clear_p2mt && (entry->r | entry->w | entry->x == 0));

what do you think?


> >> > @@ -292,7 +332,7 @@ static bool_t ept_invalidate_emt(mfn_t mfn,
> >> > bool_t recalc) e.emt = MTRR_NUM_TYPES;
> >> >          if ( recalc )
> >> >              e.recalc = 1;
> >> > -        atomic_write_ept_entry(&epte[i], e);
> >> > +        atomic_write_ept_entry(&epte[i], e, level);
> >> 
> >> I'm afraid you mustn't ever ignore this function failing (i.e.
> >> unless you're in places where you know the non-leaf shortcut is
> >> always going to be taken, but even there I think you'd be better
> >> off documenting this via ASSERT()), for security reasons. And yes,
> >> I realize that this isn't going to be trivial in some cases,
> >> especially if you want to do better than domain_crash().
> > 
> > Hmmm... since p2m type can only change via ept_set_entry, all other 
> > callers are guaranteed success, or IOW, the function is effectively
> > same as before for other callers. As such, an ASSERT combined with
> > printk should be acceptable IMO. Please see below.
> 
> Okay, I agree with this argumentation for the moment (where
> changes to/from p2m_foreign can only be explicit, and indeed only
> through that one path). But the issue raised next may already
> yield this invalid once addressed.
> 
> >> And a more general question: How is the insertion of p2m_foreign
> >> entries working together with the controlled domain (i.e. the one
> >> owning the page) being subject to paging/sharing? I only recall
> >> fixme-s having got added for the two features presently not being
> >> supported for PVH domains...
> > 
> > Right, the two features are not supported presently, the caller will
> > get -EINVAL if attempted. No further progress. 
> 
> Will it? Where is that being enforced? I just went down (as an

In p2m_add_foreign() we return -EINVAL if the foreign gfn is not one of:
       ram_rw | ram_logdirty | ram_ro | paging_out.

Also, patch 8ff5c1d added checks in set_typed_p2m_entry() and
p2m_change_type_one().

> example) subarch_memory_op() -> do_mem_event_op() ->
> mem_paging_memop() without spotting any restriction on either

Going down further one level, each of the subcalls enforce p2m types: 

  XENMEM_paging_op_nominate -> p2m_mem_paging_nominate():
       if ( !p2m_is_pageable(p2mt) )
           goto out;
etc...

> d or current->domain. (I'm anyway surprised by the rather small
> amount of matches 'grep -ir "pvh.*fixme"' turns up - did some get
> lost in the many patch iterations?

I don't think so, I was made to address lot of them :).. There are
couple in this patch.

thanks
mukesh