diff mbox series

mem_sharing: map shared_info page to same gfn during fork

Message ID 08d022bbca06c59624817ac9e23ddcb288121763.1588004969.git.tamas.lengyel@intel.com (mailing list archive)
State Superseded
Headers show
Series mem_sharing: map shared_info page to same gfn during fork | expand

Commit Message

Tamas K Lengyel April 27, 2020, 4:36 p.m. UTC
During a VM fork we copy the shared_info page; however, we also need to ensure
that the page is mapped into the same GFN in the fork as its in the parent.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
Suggested-by: Roger Pau Monne <roger.pau@citrix.com>
---
 xen/arch/x86/mm/mem_sharing.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Roger Pau Monné April 28, 2020, 10 a.m. UTC | #1
On Mon, Apr 27, 2020 at 09:36:05AM -0700, Tamas K Lengyel wrote:
> During a VM fork we copy the shared_info page; however, we also need to ensure
> that the page is mapped into the same GFN in the fork as its in the parent.
> 
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
> Suggested-by: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  xen/arch/x86/mm/mem_sharing.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 344a5bfb3d..acbf21b22c 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1656,6 +1656,7 @@ static void copy_tsc(struct domain *cd, struct domain *d)
>  static int copy_special_pages(struct domain *cd, struct domain *d)
>  {
>      mfn_t new_mfn, old_mfn;
> +    gfn_t old_gfn;
>      struct p2m_domain *p2m = p2m_get_hostp2m(cd);
>      static const unsigned int params[] =
>      {
> @@ -1701,6 +1702,34 @@ static int copy_special_pages(struct domain *cd, struct domain *d)
>      new_mfn = _mfn(virt_to_mfn(cd->shared_info));
>      copy_domain_page(new_mfn, old_mfn);
>  
> +    old_gfn = _gfn(get_gpfn_from_mfn(mfn_x(old_mfn)));
> +
> +    if ( !gfn_eq(old_gfn, INVALID_GFN) )

I think you need to compare the parent shared info gfn against the
child shared info gfn, in case the child has mapped the shared info
but the parent doesn't have it mapped. In which case you want to
remove the mapping when doing a fork reset.

> +    {
> +        /* let's make sure shared_info is mapped to the same gfn */
> +        gfn_t new_gfn = _gfn(get_gpfn_from_mfn(mfn_x(new_mfn)));
> +
> +        if ( !gfn_eq(new_gfn, INVALID_GFN) && !gfn_eq(old_gfn, new_gfn) )

You can then remove the last condition in this if.

> +        {
> +            /* if shared info is mapped to a different gfn just remove it */
> +            rc = p2m->set_entry(p2m, new_gfn, INVALID_MFN, PAGE_ORDER_4K,
> +                                p2m_invalid, p2m->default_access, -1);
> +            if ( rc )
> +                return rc;
> +
> +            new_gfn = INVALID_GFN;
> +        }
> +
> +        if ( gfn_eq(new_gfn, INVALID_GFN) )

And this should then be if ( !gfn_eq(old_gfn, INVALID_GFN) ) ...

Thanks, Roger.
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 344a5bfb3d..acbf21b22c 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1656,6 +1656,7 @@  static void copy_tsc(struct domain *cd, struct domain *d)
 static int copy_special_pages(struct domain *cd, struct domain *d)
 {
     mfn_t new_mfn, old_mfn;
+    gfn_t old_gfn;
     struct p2m_domain *p2m = p2m_get_hostp2m(cd);
     static const unsigned int params[] =
     {
@@ -1701,6 +1702,34 @@  static int copy_special_pages(struct domain *cd, struct domain *d)
     new_mfn = _mfn(virt_to_mfn(cd->shared_info));
     copy_domain_page(new_mfn, old_mfn);
 
+    old_gfn = _gfn(get_gpfn_from_mfn(mfn_x(old_mfn)));
+
+    if ( !gfn_eq(old_gfn, INVALID_GFN) )
+    {
+        /* let's make sure shared_info is mapped to the same gfn */
+        gfn_t new_gfn = _gfn(get_gpfn_from_mfn(mfn_x(new_mfn)));
+
+        if ( !gfn_eq(new_gfn, INVALID_GFN) && !gfn_eq(old_gfn, new_gfn) )
+        {
+            /* if shared info is mapped to a different gfn just remove it */
+            rc = p2m->set_entry(p2m, new_gfn, INVALID_MFN, PAGE_ORDER_4K,
+                                p2m_invalid, p2m->default_access, -1);
+            if ( rc )
+                return rc;
+
+            new_gfn = INVALID_GFN;
+        }
+
+        if ( gfn_eq(new_gfn, INVALID_GFN) )
+        {
+            /* if shared info is not currently mapped then map it */
+            rc = p2m->set_entry(p2m, old_gfn, new_mfn, PAGE_ORDER_4K,
+                                p2m_ram_rw, p2m->default_access, -1);
+            if ( rc )
+                return rc;
+        }
+    }
+
     return 0;
 }