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

[Jan Beulich <jbeulich@xxxxxxxx>]

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(). Judging from
code further down I also think the zapping of l3src would better be
dependent upon va than upon mfn.

>          if ( !is_valid(mfn, min(next, end)) )
>              continue;
> -        if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
> +        if ( !l3dst )
>          {
> -            l3dst = alloc_xen_pagetable();
> -            BUG_ON(!l3dst);
> -            clear_page(l3dst);
> -            efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)] =
> -                l4e_from_paddr(virt_to_maddr(l3dst), __PAGE_HYPERVISOR);
> +            if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
> +            {
> +                mfn_t l3mfn;
> +
> +                l3dst = alloc_map_clear_xen_pt(&l3mfn);
> +                BUG_ON(!l3dst);
> +                efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)] =
> +                    l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR);
> +            }
> +            else
> +                l3dst = map_l3t_from_l4e(l4e);
>          }
> -        else
> -            l3dst = l4e_to_l3e(l4e);

As for the earlier patch, maybe again neater if you started with

        if ( l3dst )
            /* nothing */;
        else if ...

Would also save a level of indentation as it seems.

Jan