Re: [PATCH RFC 42/55] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table

From: "Nuernberger, Stefan" <snu@amazon.de>
To: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"wei.liu2@citrix.com" <wei.liu2@citrix.com>
Cc: "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"jbeulich@suse.com" <jbeulich@suse.com>,
	"roger.pau@citrix.com" <roger.pau@citrix.com>
Subject: Re: [PATCH RFC 42/55] x86_64/mm: map and unmap page tables in share_hotadd_m2p_table
Date: Tue, 19 Mar 2019 16:45:32 +0000	[thread overview]
Message-ID: <1553013931.6788.128.camel@amazon.de> (raw)
In-Reply-To: <20190207164456.9260-43-wei.liu2@citrix.com>

On Thu, 2019-02-07 at 16:44 +0000, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/x86/x86_64/mm.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 2b88a1af37..597d8e9ed8 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -166,8 +166,8 @@ static int share_hotadd_m2p_table(struct
> mem_hotadd_info *info)
>  {
>      unsigned long i, n, v;
>      mfn_t m2p_start_mfn = INVALID_MFN;
> -    l3_pgentry_t l3e;
> -    l2_pgentry_t l2e;
> +    l3_pgentry_t l3e, *l3t;
> +    l2_pgentry_t l2e, *l2t;
>  
>      /* M2P table is mappable read-only by privileged domains. */
>      for ( v  = RDWR_MPT_VIRT_START;
> @@ -175,14 +175,22 @@ static int share_hotadd_m2p_table(struct
> mem_hotadd_info *info)
>            v += n << PAGE_SHIFT )
>      {
>          n = L2_PAGETABLE_ENTRIES * L1_PAGETABLE_ENTRIES;
> -        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[
> -            l3_table_offset(v)];
> +
> +        l3t = map_xen_pagetable_new(
> +            l4e_get_mfn(idle_pg_table[l4_table_offset(v)]));
> +        l3e = l3t[l3_table_offset(v)];
> Can you elaborate why this is required?
> 
> > 
> +        UNMAP_XEN_PAGETABLE_NEW(l3t);
> +

This pattern of mapping the table, retrieving the entry and then
immediately unmapping the table is repeated a couple of times here and
in later patches. This looks like it could use a convenience wrapper.

- Stefan

>          if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
>              continue;
>          if ( !(l3e_get_flags(l3e) & _PAGE_PSE) )
>          {
>              n = L1_PAGETABLE_ENTRIES;
> -            l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
> +
> +            l2t = map_xen_pagetable_new(l3e_get_mfn(l3e));
> +            l2e = l2t[l2_table_offset(v)];
> +            UNMAP_XEN_PAGETABLE_NEW(l2t);
> +
>              if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) )
>                  continue;
>              m2p_start_mfn = l2e_get_mfn(l2e);
> @@ -203,11 +211,18 @@ static int share_hotadd_m2p_table(struct
> mem_hotadd_info *info)
>            v != RDWR_COMPAT_MPT_VIRT_END;
>            v += 1 << L2_PAGETABLE_SHIFT )
>      {
> -        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(v)])[
> -            l3_table_offset(v)];
> +        l3t = map_xen_pagetable_new(
> +            l4e_get_mfn(idle_pg_table[l4_table_offset(v)]));
> +        l3e = l3t[l3_table_offset(v)];
> +        UNMAP_XEN_PAGETABLE_NEW(l3t);
> +
>          if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
>              continue;
> -        l2e = l3e_to_l2e(l3e)[l2_table_offset(v)];
> +
> +        l2t = map_xen_pagetable_new(l3e_get_mfn(l3e));
> +        l2e = l2t[l2_table_offset(v)];
> +        UNMAP_XEN_PAGETABLE_NEW(l2t);
> +
>          if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) )
>              continue;
>          m2p_start_mfn = l2e_get_mfn(l2e);

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel