Re: [PATCH v9 10/13] x86/smpboot: switch clone_mapping() to new APIs

2021-04-21 Thread Hongyan Xia 
On Tue, 2021-04-20 at 14:32 +0200, Jan Beulich wrote:
> On 06.04.2021 13:05, Hongyan Xia wrote:
> > @@ -742,51 +742,58 @@ static int clone_mapping(const void *ptr,
> > root_pgentry_t *rpt)
> >  }
> >  }
> >  
> > +UNMAP_DOMAIN_PAGE(pl1e);
> > +UNMAP_DOMAIN_PAGE(pl2e);
> > +UNMAP_DOMAIN_PAGE(pl3e);
> 
> Just one minor remark: A pedantic(?) compiler might warn about the
> setting to NULL of pl3e here, when
> 
> >  if ( !(root_get_flags(rpt[root_table_offset(linear)]) &
> > _PAGE_PRESENT) )
> >  {
> > -pl3e = alloc_xen_pagetable();
> > +mfn_t l3mfn;
> > +
> > +pl3e = alloc_map_clear_xen_pt(&l3mfn);
> >  rc = -ENOMEM;
> >  if ( !pl3e )
> >  goto out;
> > -clear_page(pl3e);
> >  l4e_write(&rpt[root_table_offset(linear)],
> > -  l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
> > +  l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR));
> >  }
> >  else
> > -pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]);
> > +pl3e = map_l3t_from_l4e(rpt[root_table_offset(linear)]);
> 
> ... it is guaranteed to get initialized again before any further
> consumption. IOW strictly speaking the last of those three would
> want to be unmap_domain_page(), just like you have ...
> 
> > @@ -802,6 +809,9 @@ static int clone_mapping(const void *ptr,
> > root_pgentry_t *rpt)
> >  
> >  rc = 0;
> >   out:
> > +unmap_domain_page(pl1e);
> > +unmap_domain_page(pl2e);
> > +unmap_domain_page(pl3e);
> >  return rc;
> >  }
> 
> ... here.

True. I will switch to lower case for pl3e just in case.

Hongyan

Re: [PATCH v9 10/13] x86/smpboot: switch clone_mapping() to new APIs

2021-04-20 Thread Jan Beulich 
On 06.04.2021 13:05, Hongyan Xia wrote:
> @@ -742,51 +742,58 @@ static int clone_mapping(const void *ptr, 
> root_pgentry_t *rpt)
>  }
>  }
>  
> +UNMAP_DOMAIN_PAGE(pl1e);
> +UNMAP_DOMAIN_PAGE(pl2e);
> +UNMAP_DOMAIN_PAGE(pl3e);

Just one minor remark: A pedantic(?) compiler might warn about the
setting to NULL of pl3e here, when

>  if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) )
>  {
> -pl3e = alloc_xen_pagetable();
> +mfn_t l3mfn;
> +
> +pl3e = alloc_map_clear_xen_pt(&l3mfn);
>  rc = -ENOMEM;
>  if ( !pl3e )
>  goto out;
> -clear_page(pl3e);
>  l4e_write(&rpt[root_table_offset(linear)],
> -  l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
> +  l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR));
>  }
>  else
> -pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]);
> +pl3e = map_l3t_from_l4e(rpt[root_table_offset(linear)]);

... it is guaranteed to get initialized again before any further
consumption. IOW strictly speaking the last of those three would
want to be unmap_domain_page(), just like you have ...

> @@ -802,6 +809,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
> *rpt)
>  
>  rc = 0;
>   out:
> +unmap_domain_page(pl1e);
> +unmap_domain_page(pl2e);
> +unmap_domain_page(pl3e);
>  return rc;
>  }

... here.

Jan

[PATCH v9 10/13] x86/smpboot: switch clone_mapping() to new APIs

2021-04-06 Thread Hongyan Xia 
From: Wei Liu 

Signed-off-by: Wei Liu 
Signed-off-by: Hongyan Xia 
Reviewed-by: Jan Beulich 

---
Changed in v7:
- change patch title
- remove initialiser of pl3e.
- combine the initialisation of pl3e into a single assignment.
- use the new alloc_map_clear() helper.
- use the normal map_domain_page() in the error path.
---
 xen/arch/x86/smpboot.c | 44 ++
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index e90c4dfa8a88..9c5323977b25 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -694,8 +694,8 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
*rpt)
 unsigned long linear = (unsigned long)ptr, pfn;
 unsigned int flags;
 l3_pgentry_t *pl3e;
-l2_pgentry_t *pl2e;
-l1_pgentry_t *pl1e;
+l2_pgentry_t *pl2e = NULL;
+l1_pgentry_t *pl1e = NULL;
 int rc = 0;
 
 /*
@@ -710,7 +710,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
*rpt)
  (linear >= XEN_VIRT_END && linear < DIRECTMAP_VIRT_START) )
 return -EINVAL;
 
-pl3e = l4e_to_l3e(idle_pg_table[root_table_offset(linear)]) +
+pl3e = map_l3t_from_l4e(idle_pg_table[root_table_offset(linear)]) +
 l3_table_offset(linear);
 
 flags = l3e_get_flags(*pl3e);
@@ -723,7 +723,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
*rpt)
 }
 else
 {
-pl2e = l3e_to_l2e(*pl3e) + l2_table_offset(linear);
+pl2e = map_l2t_from_l3e(*pl3e) + l2_table_offset(linear);
 flags = l2e_get_flags(*pl2e);
 ASSERT(flags & _PAGE_PRESENT);
 if ( flags & _PAGE_PSE )
@@ -734,7 +734,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
*rpt)
 }
 else
 {
-pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
+pl1e = map_l1t_from_l2e(*pl2e) + l1_table_offset(linear);
 flags = l1e_get_flags(*pl1e);
 if ( !(flags & _PAGE_PRESENT) )
 goto out;
@@ -742,51 +742,58 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
*rpt)
 }
 }
 
+UNMAP_DOMAIN_PAGE(pl1e);
+UNMAP_DOMAIN_PAGE(pl2e);
+UNMAP_DOMAIN_PAGE(pl3e);
+
 if ( !(root_get_flags(rpt[root_table_offset(linear)]) & _PAGE_PRESENT) )
 {
-pl3e = alloc_xen_pagetable();
+mfn_t l3mfn;
+
+pl3e = alloc_map_clear_xen_pt(&l3mfn);
 rc = -ENOMEM;
 if ( !pl3e )
 goto out;
-clear_page(pl3e);
 l4e_write(&rpt[root_table_offset(linear)],
-  l4e_from_paddr(__pa(pl3e), __PAGE_HYPERVISOR));
+  l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR));
 }
 else
-pl3e = l4e_to_l3e(rpt[root_table_offset(linear)]);
+pl3e = map_l3t_from_l4e(rpt[root_table_offset(linear)]);
 
 pl3e += l3_table_offset(linear);
 
 if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) )
 {
-pl2e = alloc_xen_pagetable();
+mfn_t l2mfn;
+
+pl2e = alloc_map_clear_xen_pt(&l2mfn);
 rc = -ENOMEM;
 if ( !pl2e )
 goto out;
-clear_page(pl2e);
-l3e_write(pl3e, l3e_from_paddr(__pa(pl2e), __PAGE_HYPERVISOR));
+l3e_write(pl3e, l3e_from_mfn(l2mfn, __PAGE_HYPERVISOR));
 }
 else
 {
 ASSERT(!(l3e_get_flags(*pl3e) & _PAGE_PSE));
-pl2e = l3e_to_l2e(*pl3e);
+pl2e = map_l2t_from_l3e(*pl3e);
 }
 
 pl2e += l2_table_offset(linear);
 
 if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) )
 {
-pl1e = alloc_xen_pagetable();
+mfn_t l1mfn;
+
+pl1e = alloc_map_clear_xen_pt(&l1mfn);
 rc = -ENOMEM;
 if ( !pl1e )
 goto out;
-clear_page(pl1e);
-l2e_write(pl2e, l2e_from_paddr(__pa(pl1e), __PAGE_HYPERVISOR));
+l2e_write(pl2e, l2e_from_mfn(l1mfn, __PAGE_HYPERVISOR));
 }
 else
 {
 ASSERT(!(l2e_get_flags(*pl2e) & _PAGE_PSE));
-pl1e = l2e_to_l1e(*pl2e);
+pl1e = map_l1t_from_l2e(*pl2e);
 }
 
 pl1e += l1_table_offset(linear);
@@ -802,6 +809,9 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
*rpt)
 
 rc = 0;
  out:
+unmap_domain_page(pl1e);
+unmap_domain_page(pl2e);
+unmap_domain_page(pl3e);
 return rc;
 }
 
-- 
2.23.3