Re: [PATCH] mem_sharing: map shared_info page to same gfn during fork

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxx>
> Suggested-by: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> ---
>  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.