Re: [PATCH v7 09/15] efi: use new page table APIs in copy_mapping

[Hongyan Xia <hx242@xxxxxxx>]

On Tue, 2020-07-14 at 14:42 +0200, Jan Beulich wrote:
> On 29.05.2020 13:11, Hongyan Xia wrote:
> > From: Wei Liu <wei.liu2@xxxxxxxxxx>
> > 
> > After inspection ARM doesn't have alloc_xen_pagetable so this
> > function
> > is x86 only, which means it is safe for us to change.
> 
> Well, it sits inside a "#ifndef CONFIG_ARM" section.
> 
> > @@ -1442,29 +1443,42 @@ static __init void copy_mapping(unsigned
> > long mfn, unsigned long end,
> >                                                   unsigned long
> > emfn))
> >  {
> >      unsigned long next;
> > +    l3_pgentry_t *l3src = NULL, *l3dst = NULL;
> >  
> >      for ( ; mfn < end; mfn = next )
> >      {
> >          l4_pgentry_t l4e = efi_l4_pgtable[l4_table_offset(mfn <<
> > PAGE_SHIFT)];
> > -        l3_pgentry_t *l3src, *l3dst;
> >          unsigned long va = (unsigned long)mfn_to_virt(mfn);
> >  
> > +        if ( !((mfn << PAGE_SHIFT) & ((1UL << L4_PAGETABLE_SHIFT)
> > - 1)) )
> 
> To be in line with ...
> 
> > +        {
> > +            UNMAP_DOMAIN_PAGE(l3src);
> > +            UNMAP_DOMAIN_PAGE(l3dst);
> > +        }
> >          next = mfn + (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
> 
> ... this, please avoid the left shift of mfn in the if(). Judgingfrom

What do you mean by "in line" here? It does not look to me that "next
=" can be easily squashed into the if() condition.

Hongyan