diff mbox

[v4,08/11] xen/arm: Handle remove foreign mapping

Message ID 1386963461-6520-9-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall Dec. 13, 2013, 7:37 p.m. UTC 
Introduce p2m_remove_foreign to remove foreign mapping. This function will
will release the reference taken when the mapping is added in the p2m.

As the function is not yet implemented on x86, add stubs.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com> [common/memory.c]
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>

---
    Changes in v4:
        - Split patch #6 from dom0 pvh series v6.2 to retrieve only common
        code.
        - Rework commit title
        - Rename xen_rem_foreign_from_p2m in p2m_remove_foreign
        - Get the mfn from the pte. We are not sure that maddr given in
        parameters is valid
    Changes in v3:
        - Move put_page in create_p2m_entries
        - Move xenmem_rem_foreign_from_p2m in arch/arm/p2m.c
    Changes in v2:
        - Introduce the patch
---
 xen/arch/arm/p2m.c        |   25 +++++++++++++++++++++++--
 xen/common/memory.c       |   12 +++++++++++-
 xen/include/asm-arm/p2m.h |    2 ++
 xen/include/asm-x86/p2m.h |    6 ++++++
 4 files changed, 42 insertions(+), 3 deletions(-)

Comments

Tim Deegan Dec. 16, 2013, 11:51 a.m. UTC | #1 
At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
> @@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>              return rc;
>          }
>  
> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> +        /*
> +         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
> +         * from foreign domain by the user space tool during domain creation.
> +         * We need to check for that, free it up from the p2m, and release
> +         * refcnt on it. In such a case, page would be NULL and the following
> +         * call would not have refcnt'd the page.
> +         */
> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
>          if ( page )
>          {
>              guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
>              put_page(page);
>          }
> +        else if ( p2m_is_foreign(p2mt) )
> +            rc = p2m_remove_foreign(d, xrfp.gpfn);

This doesn't seem like the right interface -- having special cases
like this in the callers is something we slipped into in x86 for a lot
of the paging/sharing code and it's not nice.  I think maybe we can
just have get_page_from_gfn() DTRT for foreign (and grant) entries.

Also, the comment will have been out of data by the time the x86
version of this code is finished, as we won't be handling the refcount
at this level. :)

Tim.
Ian Campbell Dec. 16, 2013, 11:55 a.m. UTC | #2 
On Mon, 2013-12-16 at 12:51 +0100, Tim Deegan wrote:
> At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
> > @@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >              return rc;
> >          }
> >  
> > -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> > +        /*
> > +         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
> > +         * from foreign domain by the user space tool during domain creation.
> > +         * We need to check for that, free it up from the p2m, and release
> > +         * refcnt on it. In such a case, page would be NULL and the following
> > +         * call would not have refcnt'd the page.
> > +         */
> > +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> >          if ( page )
> >          {
> >              guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> >              put_page(page);
> >          }
> > +        else if ( p2m_is_foreign(p2mt) )
> > +            rc = p2m_remove_foreign(d, xrfp.gpfn);
> 
> This doesn't seem like the right interface -- having special cases
> like this in the callers is something we slipped into in x86 for a lot
> of the paging/sharing code and it's not nice.  I think maybe we can
> just have get_page_from_gfn() DTRT for foreign (and grant) entries.

DYM guest_physmap_remove_page?

I asked Mukesh a few times to make get_page_from_gfn handle the foreign
page refcounting and return a valid struct page_info *.

> Also, the comment will have been out of data by the time the x86
> version of this code is finished, as we won't be handling the refcount
> at this level. :)
Tim Deegan Dec. 16, 2013, 3:26 p.m. UTC | #3 
At 11:55 +0000 on 16 Dec (1387191318), Ian Campbell wrote:
> On Mon, 2013-12-16 at 12:51 +0100, Tim Deegan wrote:
> > At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
> > > @@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> > >              return rc;
> > >          }
> > >  
> > > -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> > > +        /*
> > > +         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
> > > +         * from foreign domain by the user space tool during domain creation.
> > > +         * We need to check for that, free it up from the p2m, and release
> > > +         * refcnt on it. In such a case, page would be NULL and the following
> > > +         * call would not have refcnt'd the page.
> > > +         */
> > > +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> > >          if ( page )
> > >          {
> > >              guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> > >              put_page(page);
> > >          }
> > > +        else if ( p2m_is_foreign(p2mt) )
> > > +            rc = p2m_remove_foreign(d, xrfp.gpfn);
> > 
> > This doesn't seem like the right interface -- having special cases
> > like this in the callers is something we slipped into in x86 for a lot
> > of the paging/sharing code and it's not nice.  I think maybe we can
> > just have get_page_from_gfn() DTRT for foreign (and grant) entries.
> 
> DYM guest_physmap_remove_page?

I think I mean both get_page_from_gfn and guest_physmap_remove_page.

> I asked Mukesh a few times to make get_page_from_gfn handle the foreign
> page refcounting and return a valid struct page_info *.

Yes, that sounds like a better place for it, if it can be done
safely.  It would need a bit of care at the existing callers to make
sure they're not going to break when handed a foreign-owned page (but
they should be audited anyway for this new case).

Tim.
Julien Grall Dec. 16, 2013, 3:34 p.m. UTC | #4 
On 12/16/2013 11:51 AM, Tim Deegan wrote:
> At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
>> @@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>               return rc;
>>           }
>>
>> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
>> +        /*
>> +         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
>> +         * from foreign domain by the user space tool during domain creation.
>> +         * We need to check for that, free it up from the p2m, and release
>> +         * refcnt on it. In such a case, page would be NULL and the following
>> +         * call would not have refcnt'd the page.
>> +         */
>> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
>>           if ( page )
>>           {
>>               guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
>>               put_page(page);
>>           }
>> +        else if ( p2m_is_foreign(p2mt) )
>> +            rc = p2m_remove_foreign(d, xrfp.gpfn);
>
> This doesn't seem like the right interface -- having special cases
> like this in the callers is something we slipped into in x86 for a lot
> of the paging/sharing code and it's not nice.  I think maybe we can
> just have get_page_from_gfn() DTRT for foreign (and grant) entries.
>
> Also, the comment will have been out of data by the time the x86
> version of this code is finished, as we won't be handling the refcount
> at this level. :)

I will remove the comment and modify get_page_from_gfn to handle foreign 
mapping.
Ian Campbell Dec. 16, 2013, 3:40 p.m. UTC | #5 
On Mon, 2013-12-16 at 15:34 +0000, Julien Grall wrote:
> 
> On 12/16/2013 11:51 AM, Tim Deegan wrote:
> > At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
> >> @@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>               return rc;
> >>           }
> >>
> >> -        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
> >> +        /*
> >> +         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
> >> +         * from foreign domain by the user space tool during domain creation.
> >> +         * We need to check for that, free it up from the p2m, and release
> >> +         * refcnt on it. In such a case, page would be NULL and the following
> >> +         * call would not have refcnt'd the page.
> >> +         */
> >> +        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
> >>           if ( page )
> >>           {
> >>               guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
> >>               put_page(page);
> >>           }
> >> +        else if ( p2m_is_foreign(p2mt) )
> >> +            rc = p2m_remove_foreign(d, xrfp.gpfn);
> >
> > This doesn't seem like the right interface -- having special cases
> > like this in the callers is something we slipped into in x86 for a lot
> > of the paging/sharing code and it's not nice.  I think maybe we can
> > just have get_page_from_gfn() DTRT for foreign (and grant) entries.
> >
> > Also, the comment will have been out of data by the time the x86
> > version of this code is finished, as we won't be handling the refcount
> > at this level. :)
> 
> I will remove the comment and modify get_page_from_gfn to handle foreign 
> mapping.

You'll want to coordinate with Mukesh on that latter I think.

Ian.
diff mbox

Patch

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 39d8a03..cfdc19c 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -317,10 +317,21 @@  static int create_p2m_entries(struct domain *d,
                 break;
             case REMOVE:
                 {
-                    lpae_t pte;
+                    lpae_t pte = third[third_table_offset(addr)];
+                    unsigned long mfn;
+
+                    maddr = (pte.bits & PADDR_MASK & PAGE_MASK);
+                    mfn = paddr_to_pfn(maddr);
+
+                    /* TODO: Handle other p2m type */
+                    if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
+                    {
+                        ASSERT(mfn_valid(mfn));
+                        put_page(mfn_to_page(mfn));
+                    }
+
                     memset(&pte, 0x00, sizeof(pte));
                     write_pte(&third[third_table_offset(addr)], pte);
-                    maddr += PAGE_SIZE;
                 }
                 break;
         }
@@ -380,6 +391,16 @@  void guest_physmap_remove_page(struct domain *d,
                        pfn_to_paddr(mfn), MATTR_MEM, p2m_invalid);
 }
 
+int p2m_remove_foreign(struct domain *d, unsigned long gpfn)
+{
+    unsigned long mfn = gmfn_to_mfn(d, gpfn);
+
+    ASSERT(mfn_valid(mfn));
+    guest_physmap_remove_page(d, gpfn, mfn, 0);
+
+    return 0;
+}
+
 int p2m_alloc_table(struct domain *d)
 {
     struct p2m_domain *p2m = &d->arch.p2m;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index eb7b72b..61791a4 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -678,6 +678,7 @@  long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct xen_remove_from_physmap xrfp;
         struct page_info *page;
         struct domain *d;
+        p2m_type_t p2mt;
 
         if ( copy_from_guest(&xrfp, arg, 1) )
             return -EFAULT;
@@ -693,12 +694,21 @@  long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return rc;
         }
 
-        page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
+        /*
+         * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
+         * from foreign domain by the user space tool during domain creation.
+         * We need to check for that, free it up from the p2m, and release
+         * refcnt on it. In such a case, page would be NULL and the following
+         * call would not have refcnt'd the page.
+         */
+        page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
         if ( page )
         {
             guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
             put_page(page);
         }
+        else if ( p2m_is_foreign(p2mt) )
+            rc = p2m_remove_foreign(d, xrfp.gpfn);
         else
             rc = -ENOENT;
 
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0eb07a8..844ef9d 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -144,6 +144,8 @@  static inline int get_page_and_type(struct page_info *page,
     return rc;
 }
 
+int p2m_remove_foreign(struct domain *d, unsigned long gpfn);
+
 #endif /* _XEN_P2M_H */
 
 /*
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index d5d6391..9288043 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -515,6 +515,12 @@  int clear_mmio_p2m_entry(struct domain *d, unsigned long gfn);
 /* Set foreign mfn in the current guest's p2m table. */
 int set_foreign_p2m_entry(struct domain *domp, unsigned long gfn, mfn_t mfn);
 
+/* Remove foreign mapping from the guest's p2m table. */
+static inline int p2m_remove_foreign(struct domain *d, unsigned long gpfn)
+{
+    return -ENOSYS;
+}
+
 /* 
  * Populate-on-demand
  */