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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 14.05.14 at 02:55, <mukesh.rathor@oracle.com> wrote:
> 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?

I think scattering around new special casing is the wrong approach;
you may recall that I wasn't in favor of the lots of special case
additions elsewhere by the PVH patches. I think you first of all need
to take a step back and understand what the long term picture here
is supposed to be. Which features can and should be supported,
and which ones should be regarded as useless or unsupportable.

Once you determined that, whether the above is the right thing for
now (with a suitable fixme comment) will fall out almost naturally I
assume.

>> >> 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().

But that's way too late, isn't it? You ought to disallow paging/sharing
(and whatever else you can't support right now) from the beginning,
i.e. it shouldn't even get enabled on a DomU if the controlling domain
is PVH.

Jan