| Message ID | 20220424144414.GA17868@DESKTOP-NK4TH6S.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/efi: boot fix duplicated index, offset calculate operation in the copy_mapping loop | expand |
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
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);
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(-)