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

[Hongyan Xia <hx242@xxxxxxx>]

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