Re: [Xen-devel] [PATCH v2 09/10] xen/arm: Set foreign page type to p2m_map_foreign

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/10/2013 09:37 AM, Ian Campbell wrote:
> On Tue, 2013-12-10 at 01:51 +0000, Julien Grall wrote:
> > On 12/09/2013 04:40 PM, Ian Campbell wrote:
> > > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
> > > > Xen needs to know that the current page belongs to another domain. Also take
> > > > the refcount to this page.
> > > >
> > > > Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> > > >
> > > > ---
> > > >       Changes in v2:
> > > >           - Even if gcc is buggy (see 
> > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501)
> > > >           define p2m type per mapspace to let the compiler warns about 
> > > > unitialized
> > > >           values.
> > > > ---
> > > >    xen/arch/arm/mm.c |   24 ++++++++++++++++--------
> > > >    1 file changed, 16 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > > index ba51f6e..b08ffa0 100644
> > > > --- a/xen/arch/arm/mm.c
> > > > +++ b/xen/arch/arm/mm.c
> > > > @@ -993,6 +993,7 @@ static int xenmem_add_to_physmap_one(
> > > >    {
> > > >        unsigned long mfn = 0;
> > > >        int rc;
> > > > +    p2m_type_t t;
> > > >
> > > >        switch ( space )
> > > >        {
> > > > @@ -1025,18 +1026,22 @@ static int xenmem_add_to_physmap_one(
> > > >
> > > >            d->arch.grant_table_gpfn[idx] = gpfn;
> > > >
> > > > +        t = p2m_ram_rw;
> > > > +
> > > >            spin_unlock(&d->grant_table->lock);
> > > >            break;
> > > >        case XENMAPSPACE_shared_info:
> > > > -        if ( idx == 0 )
> > > > -            mfn = virt_to_mfn(d->shared_info);
> > > > -        else
> > > > +        if ( idx != 0 )
> > > >                return -EINVAL;
> > > > +
> > > > +        mfn = virt_to_mfn(d->shared_info);
> > > > +        t = p2m_ram_rw;
> > > > +
> > > >            break;
> > > >        case XENMAPSPACE_gmfn_foreign:
> > > >        {
> > > > -        paddr_t maddr;
> > > >            struct domain *od;
> > > > +        struct page_info *page;
> > > >            od = rcu_lock_domain_by_any_id(foreign_domid);
> > > >            if ( od == NULL )
> > > >                return -ESRCH;
> > > > @@ -1048,15 +1053,18 @@ static int xenmem_add_to_physmap_one(
> > > >                return rc;
> > > >            }
> > > >
> > > > -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> > > > -        if ( maddr == INVALID_PADDR )
> > > > +        /* Take refcount to the foreign domain page.
> > > "refcount on...". Or "Take a reference to..."
> > >
> > > > +         * Refcount will be release in XENMEM_remove_from_physmap */
> > > and a few other places...
> > >
> > > Like with the unmap it might be best to push this reference manipulation
> > > down into the eventual function which makes the mapping. That would keep
> > > this stuff much more localised.
> > Forget to answer to this part:
> >
> > We need to take the reference with foreign domain in parameter,
> > otherwise we don't really know if the foreign domain has unlikely
> > removed the mapping between the hypercall was called and now.
> Are we not holding the p2m lock at this point?
p2m lock of which domain? For now, neither of foreign and current domain 
lock are taken in this function.
> We probably do need to do the final check and unmap with the lock held.
> Either by moving the unmap within the same lock as the lookup or perhaps
> with some sort of cmpxchg mechanism where we pass in the pte value we
> think we are tearing down. Just checking the type may not be sufficient
> if the guest tears down one foreign mapping and replaces it with
> another.
I'm not sure to follow you, are you talking about remove the foreign 
mapping? If so, I have reworking all the removing code to include the 
put_page (see V3).
--
Julien Grall

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