Re: [PATCH] hypercall/mem: Introduce XENMEM_machphys_compat_mfn_list

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Apr 17, 2014 at 04:53:25PM +0100, Andrew Cooper wrote:
> To correctly migrate a PV guest, the toolstack must remove Xen mappings from
> the guest pagetables.  For 32bit PV guests, the pagetables cannot be walked
> from the top so upon encountering an L2 table, the toolstack must decide
> whether it contains Xen mappings or not, to avoid corrupting L2s without Xen
> mappings.
> 
> The migration code performs this search efficiently by knowing that the Xen
> mappings will start at a known L2e and point to a known mfn, which will be the
> fist mfn in the m2p table.

first
> 
> Unfortunately there are two m2p tables in use; a regular and a compatibility
                                               ^- ":"
> one.  The toolstack looks for the first mfn of its own m2p table in the guest
> pagetables.  This only works if the toolstack is the same bitness as the 32bit
> domain being migrated, and leaves a problem for 64bit toolstacks which will
> never be able to find its regular m2p in a compat guest.
> 
> It appears that this bug for 64bit toolstacks was discovered, but hacked
> around in an unsafe manor.  The code currently shoots any invalid L2es and
                      ^^^^ - manner
> doesn't report a failure for L2 tables in a 32 bit guest, even after the guest
> is paused.  This means that non Xen entries which should fail the migration
> don't, and the guest will resume on the far side with unexpectedly fewer
> present pagetable entries.

Ouch1
> 
> This patch introduces XENMEM_machphys_compat_mfn_list which permits a 64bit
> toolstack to access the compat m2p mfn list, for the purpose of correctly
> identifying Xen entries in a 32bit guest.
> 
> It is worth noting for completeness that 64bit PV guests don't have any of
> these games to play.  The Xen mappings are present at a known location in all
> L4 tables, so can be safely shot by 32 and 64bit toolstacks without looking at
> where the mapping points to.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> ---
> 
> I am happy for this to live as part of my "migration v2" series, but is
> presented here for individual review.
> 
> Changes in v2:
>  * Don't alias other local scope variables in subarch_memory_op
> ---
>  xen/arch/x86/x86_64/compat/mm.c |    1 +
>  xen/arch/x86/x86_64/mm.c        |   30 +++++++++++++++++++++++++++++-
>  xen/include/public/memory.h     |   10 ++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
> index 0a8408b..6d3bc31 100644
> --- a/xen/arch/x86/x86_64/compat/mm.c
> +++ b/xen/arch/x86/x86_64/compat/mm.c
> @@ -146,6 +146,7 @@ int compat_arch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      }
>  
>      case XENMEM_machphys_mfn_list:
> +    case XENMEM_machphys_compat_mfn_list:
>      {
>          unsigned long limit;
>          compat_pfn_t last_mfn;
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 71ae519..ff96997 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -953,7 +953,7 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>      struct xen_machphys_mfn_list xmml;
>      l3_pgentry_t l3e;
>      l2_pgentry_t l2e;
> -    unsigned long v;
> +    unsigned long v, limit;
>      xen_pfn_t mfn, last_mfn;
>      unsigned int i;
>      long rc = 0;
> @@ -1000,6 +1000,34 @@ long subarch_memory_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>          break;
>  
> +    case XENMEM_machphys_compat_mfn_list:
> +        if ( copy_from_guest(&xmml, arg, 1) )
> +            return -EFAULT;
> +
> +        limit = (unsigned long)(compat_machine_to_phys_mapping + max_page);
> +        if ( limit > RDWR_COMPAT_MPT_VIRT_END )
> +            limit = RDWR_COMPAT_MPT_VIRT_END;
> +        for ( i = 0, v = RDWR_COMPAT_MPT_VIRT_START, last_mfn = 0;
> +              (i != xmml.max_extents) && (v < limit);
> +              i++, v += 1 << L2_PAGETABLE_SHIFT )
> +        {
> +            l2e = compat_idle_pg_table_l2[l2_table_offset(v)];
> +            if ( l2e_get_flags(l2e) & _PAGE_PRESENT )
> +                mfn = l2e_get_pfn(l2e);
> +            else
> +                mfn = last_mfn;
> +            ASSERT(mfn);
> +            if ( copy_to_guest_offset(xmml.extent_start, i, &mfn, 1) )
> +                return -EFAULT;
> +            last_mfn = mfn;
> +        }
> +
> +        xmml.nr_extents = i;
> +        if ( __copy_to_guest(arg, &xmml, 1) )
> +            rc = -EFAULT;
> +
> +        break;
> +
>      case XENMEM_get_sharing_freed_pages:
>          return mem_sharing_get_nr_saved_mfns();
>  
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index f19ac14..be49beb 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -465,6 +465,16 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
>   * The zero value is appropiate.
>   */
>  
> +/*
> + * For a compat toolstack domain, this is indentical to
                                             ^^^^^^^^^ - identical

> + * XENMEM_machphys_mfn_list.
> + *
> + * For a non compat toolstack domain, this functions similarly to
> + * XENMEM_machphys_mfn_list, but returns the mfns making up the compatibility
> + * m2p table.
> + */
> +#define XENMEM_machphys_compat_mfn_list     25
> +
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>  
>  #endif /* __XEN_PUBLIC_MEMORY_H__ */
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel