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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.14 at 03:02, <mukesh.rathor@xxxxxxxxxx> wrote:
> On Mon, 12 May 2014 11:34:14 +0100
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 10.05.14 at 02:50, <mukesh.rathor@xxxxxxxxxx> 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.

>> > @@ -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
example) subarch_memory_op() -> do_mem_event_op() ->
mem_paging_memop() without spotting any restriction on either
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?

Jan


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