Re: [PATCH] xen/efi: boot fix duplicated index, offset calculate operation in the copy_mapping loop

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.04.2022 16:44, Paran Lee wrote:
> It doesn't seem necessary to do that
> duplicated calculation of mfn to addr and l4 table index
> in the copy_mapping loop.

I'm not convinced this is an improvement. If the compiler sees fit, it
can CSE things like this, but it may see reasons not to (register
pressure, for example). Furthermore ...

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1470,7 +1470,9 @@ static __init void copy_mapping(unsigned long mfn, 
> unsigned long end,
>  
>      for ( ; mfn < end; mfn = next )
>      {
> -        l4_pgentry_t l4e = efi_l4t[l4_table_offset(mfn << PAGE_SHIFT)];
> +        unsigned long addr = mfn << PAGE_SHIFT;

... this isn't 32-bit clean (we build EFI for 64-bit architectures
only right now, but this should not result in there being issues if
anyone wanted to enable the code for 32-bit as well).

> +        unsigned long l4_table_idx = l4_table_offset(addr);

There's no reason I can see for this to be wider than unsigned int.

> +        l4_pgentry_t l4e = efi_l4t[l4_table_idx];
>          unsigned long va = (unsigned long)mfn_to_virt(mfn);
>  
>          if ( !(mfn & ((1UL << (L4_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)) )
> @@ -1489,7 +1491,7 @@ static __init void copy_mapping(unsigned long mfn, 
> unsigned long end,
>  
>              l3dst = alloc_mapped_pagetable(&l3mfn);
>              BUG_ON(!l3dst);
> -            efi_l4t[l4_table_offset(mfn << PAGE_SHIFT)] =
> +            efi_l4t[l4_table_idx] =
>                  l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR);

_If_ your change was to be taken, you'd want to unwrap this statement
now that it first on a single line.

Jan