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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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:
> 
>> >>> On 16.04.14 at 02:12, <mukesh.rathor@xxxxxxxxxx> wrote:
>> > In this patch, a new function, p2m_add_foreign(), is added
>> > to map pages from foreign guest into dom0 for domU creation.
>> > Such pages are typed p2m_map_foreign.  Note, it is the nature
>> > of such pages that a refcnt is held during their stay in the p2m.
>> > The refcnt is added and released in the low level ept function
>> > atomic_write_ept_entry for convenience.
>> > Also, p2m_teardown is changed to add a call to allow for the
>> > cleanup of foreign pages during it's destruction.
>> > Finally, get_pg_owner is changed to allow pvh to map foreign
>> > mappings, and is made public to be used by p2m_add_foreign().
>> 
>> Do you really need to do this at this low a layer? I'm afraid that's
>> going to be rather limiting when wanting to use the bits used for the
>> p2m type for different purposes in intermediate table entries. I.e. I
>> think this special casing should only be done for leaf entries, and
>> hence at least one layer further up, or introduce something named
>> e.g. atomic_write_epte_leaf().
> 
> Well, Tim and I went back and forth several times on this over the
> last several months (you were cc'd :) ). 

I know, but having worked a lot on the P2M code recently my
perspective may have changed.

> Since a refcnt absolutely needs to be taken and released for such 
> pages while they are mapped, the best way to ensure that was to do
> it at the lowest level. This was Tim's suggestion, my earlier patches
> did at higher levels. At present, there is a single purpose for foreign
> types. But future uses, unlikely at this point, can make any relevant
> enhancements.

For starters I suggest you go look at what extra uses of the macro/
function got added between the commit this series was based on
and current tip. And then, with future uses I wasn't referring to
future uses of foreign types, but future uses of fields in the EPT
entries (and namely in intermediate ones, where they are really
unused at present). I say this because one of the failed attempts in
dealing with the preemption/propagation needs was to make use of
these fields already. But yes, it's only a "would be nice", not a strict
requirement to avoid collisions with eventual future uses.

>> > @@ -4583,6 +4583,11 @@ int xenmem_add_to_physmap_one(
>> >              page = mfn_to_page(mfn);
>> >              break;
>> >          }
>> > +        case XENMAPSPACE_gmfn_foreign:
>> > +        {
>> > +            rc = p2m_add_foreign(d, idx, gpfn, foreign_domid);
>> > +            return rc;
>> > +        }
>> 
>> No pointless braces please.
> 
> Often, case statement get sooo long that having braces makes it easier
> to find matching code, specially when indentation is weird for case 
> statements, and uglier still, switch statements within switch statements 
> are allowed. 
> 
> But I'll remove it here anyways rather than argue, even tho the prev
> case has {}.

The fundamental thought here is: "Yes, there are bad examples, but
please let's not add further ones."

>> > +    {
>> > +        int rc;
>> > +        struct page_info *page;
>> > +        struct domain *fdom;
>> > +
>> > +        ASSERT(mfn_valid(new->mfn));
>> > +        page = mfn_to_page(new->mfn);
>> > +        fdom = page_get_owner(page);
>> > +        ASSERT(fdom);
>> > +        rc = get_page(page, fdom);
>> > +        ASSERT(rc);
>> 
>> I'm afraid you can't rely on this to succeed: A PV guest could have
>> mapped the page so many times that you can't get another ref.
> 
> foreign pages are valid for PVH/HVM only, PV would never get here.

Am I wrong in thinking that the mapping domain can only be PVH/HVM,
but the foreign domain can well be PV? Or how do you deal with PVH
Dom0 managing PV guests?

> I can return rc instead of ASSERT. In the prev version review, you
> had asked for an ASSERT here.

Iirc all I said is that you won't get away without looking at the error.
And that I left it open for you to determine whether an assertion
would be valid, saving you from handling the error (which involves
more than just returning an rc: the function isn't currently expected
to have a way to fail - one more reason to reconsider moving this up
a layer).

>> > @@ -451,6 +456,10 @@ void p2m_teardown(struct p2m_domain *p2m)
>> >      d = p2m->domain;
>> >  
>> >      p2m_lock(p2m);
>> > +    /* pvh: we must release refcnt on all foreign pages */
>> > +    if ( is_pvh_domain(d) )
>> > +        p2m_change_entry_type_global(d, p2m_map_foreign,
>> > p2m_invalid);
>> 
>> Ah, here it comes. But no, this is a no-go. The function currently is
>> running for unbounded time, which I'm about to change (pending
>> some more testing; seems to generally work). I can't, however, see
> 
> I believe Tim has a patch to make this pre-emptible and OK'd this hook
> here.

Has he? I'm sure he would have told me, so I could have avoided
working towards the (almost) same during the last several weeks.

> This path for foreign pages is only for destroy of p2m in case 
> of domain crash, which at present would only be dom0 (or controlling
> domains in future). Normally, these pages are un-mapped by guest via the 
> remove from physmap hcall. 
> 
> Moreover, the number of foreign pages is relatively small.

That's missing the point: No matter how small their count, you still
need to iterate through the entire page table hierarchy. Plus - what
makes you think that count is small? Qemu, iirc, keeps quite a few
pages mapped - are they not mapped using this mechanism? Plus
their count multiplies by the number of domains managed (arguably
that count should be greater than 1 only for non-Dom0 control
domains, and Dom0 won't make it here anyway - which raises the
question whether this change is of any practical use under the
topic on the patch series, i.e. enabling PVH Dom0).

>> > +{
>> > +    p2m_type_t p2mt, p2mt_prev;
>> > +    unsigned long prev_mfn, mfn;
>> > +    struct page_info *page;
>> > +    int rc = -EINVAL;
>> > +    struct domain *fdom = NULL;
>> > +
>> > +    if ( fdid == DOMID_SELF )
>> > +        goto out;
>> > +
>> > +    rc = -ESRCH;
>> > +    fdom = get_pg_owner(fdid);
>> > +    if ( fdom == NULL )
>> > +        goto out;
>> > +
>> > +    rc = -EINVAL;
>> > +    if ( tdom == NULL || tdom == fdom || !is_pvh_domain(tdom) )
>> 
>> Is tdom == NULL a reasonable thing to expect here. I.e. can't you
>> rather ASSERT(tdom)?
> 
> Could. Why is it a big deal to check for NULL rather than ASSERT?

It serves documentation purposes: If you return an error upon seeing
NULL, you expect someone could call you that way. If you assert, you
make clear that callers have to make sure they pass a valid pointer.

Jan

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