It doesn't seem necessary to do that
duplicated calculation of mfn to addr and l4 table index
in the copy_mapping loop.

Signed-off-by: Paran Lee <p4ranlee@gmail.com>
---
 xen/common/efi/boot.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ac1b235372..7da4269c32 100644
--- 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;
+        unsigned long l4_table_idx = l4_table_offset(addr);
+        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);
         }
         else
@@ -1497,7 +1499,7 @@ static __init void copy_mapping(unsigned long mfn, unsigned long end,
 
         if ( !l3src )
             l3src = map_l3t_from_l4e(idle_pg_table[l4_table_offset(va)]);
-        l3dst[l3_table_offset(mfn << PAGE_SHIFT)] = l3src[l3_table_offset(va)];
+        l3dst[l3_table_offset(addr)] = l3src[l3_table_offset(va)];
     }
 
     unmap_domain_page(l3src);
-- 
2.25.1

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

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.
> 
> Signed-off-by: Paran Lee <p4ranlee@gmail.com>

Oh, one more thing: Please submit patches _To_ the list, with maintainers
on _Cc_ (not the other way around).

Jan