[Xen-devel] [PATCH 1/3] x86/EFI: don't apply relocations to l{2, 3}_bootmap
Other than claimed in commit 2ce5963727's ("x86: construct the {l2,l3}_bootmap at compile time") the initialization of the two page tables doesn't take care of everything without furher adjustment: The compile time initialization obviously requires base relocations, and those get processed after efi_arch_memory_setup(). Hence without additional care the correctly initialized values may then get wrongly "adjusted" again. Except the two table from being subject to base relocation. Signed-off-by: Jan Beulich --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -47,11 +47,23 @@ static void __init efi_arch_relocate_ima for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) { -unsigned int i, n; +unsigned int i = 0, n; n = (base_relocs->size - sizeof(*base_relocs)) / sizeof(*base_relocs->entries); -for ( i = 0; i < n; ++i ) + +/* + * Relevant l{2,3}_bootmap entries get initialized explicitly in + * efi_arch_memory_setup(), so we must not apply relocations there. + * l2_identmap's first slot, otoh, should be handled normally, as + * efi_arch_memory_setup() won't touch it (xen_phys_start should + * never be zero). + */ +if ( xen_phys_start + base_relocs->rva == (unsigned long)l3_bootmap || + xen_phys_start + base_relocs->rva == (unsigned long)l2_bootmap ) +i = n; + +for ( ; i < n; ++i ) { unsigned long addr = xen_phys_start + base_relocs->rva + (base_relocs->entries[i] & 0xfff); x86/EFI: don't apply relocations to l{2,3}_bootmap Other than claimed in commit 2ce5963727's ("x86: construct the {l2,l3}_bootmap at compile time") the initialization of the two page tables doesn't take care of everything without furher adjustment: The compile time initialization obviously requires base relocations, and those get processed after efi_arch_memory_setup(). Hence without additional care the correctly initialized values may then get wrongly "adjusted" again. Except the two table from being subject to base relocation. Signed-off-by: Jan Beulich --- a/xen/arch/x86/efi/efi-boot.h +++ b/xen/arch/x86/efi/efi-boot.h @@ -47,11 +47,23 @@ static void __init efi_arch_relocate_ima for ( base_relocs = __base_relocs_start; base_relocs < __base_relocs_end; ) { -unsigned int i, n; +unsigned int i = 0, n; n = (base_relocs->size - sizeof(*base_relocs)) / sizeof(*base_relocs->entries); -for ( i = 0; i < n; ++i ) + +/* + * Relevant l{2,3}_bootmap entries get initialized explicitly in + * efi_arch_memory_setup(), so we must not apply relocations there. + * l2_identmap's first slot, otoh, should be handled normally, as + * efi_arch_memory_setup() won't touch it (xen_phys_start should + * never be zero). + */ +if ( xen_phys_start + base_relocs->rva == (unsigned long)l3_bootmap || + xen_phys_start + base_relocs->rva == (unsigned long)l2_bootmap ) +i = n; + +for ( ; i < n; ++i ) { unsigned long addr = xen_phys_start + base_relocs->rva + (base_relocs->entries[i] & 0xfff); ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/3] x86/EFI: don't apply relocations to l{2, 3}_bootmap
On 19/08/16 08:50, Jan Beulich wrote: > Other than claimed in commit 2ce5963727's ("x86: construct the > {l2,l3}_bootmap at compile time") the initialization of the two page > tables doesn't take care of everything without furher adjustment: The > compile time initialization obviously requires base relocations, and > those get processed after efi_arch_memory_setup(). Hence without > additional care the correctly initialized values may then get wrongly > "adjusted" again. Except the two table from being subject to base > relocation. Do you mean Exempt? "Except the two tables" doesn't parse. > > Signed-off-by: Jan Beulich > > --- a/xen/arch/x86/efi/efi-boot.h > +++ b/xen/arch/x86/efi/efi-boot.h > @@ -47,11 +47,23 @@ static void __init efi_arch_relocate_ima > > for ( base_relocs = __base_relocs_start; base_relocs < > __base_relocs_end; ) > { > -unsigned int i, n; > +unsigned int i = 0, n; > > n = (base_relocs->size - sizeof(*base_relocs)) / > sizeof(*base_relocs->entries); > -for ( i = 0; i < n; ++i ) > + > +/* > + * Relevant l{2,3}_bootmap entries get initialized explicitly in > + * efi_arch_memory_setup(), so we must not apply relocations there. > + * l2_identmap's first slot, otoh, should be handled normally, as And l3 surely? Reviewed-by: Andrew Cooper > + * efi_arch_memory_setup() won't touch it (xen_phys_start should > + * never be zero). > + */ > +if ( xen_phys_start + base_relocs->rva == (unsigned long)l3_bootmap > || > + xen_phys_start + base_relocs->rva == (unsigned long)l2_bootmap ) > +i = n; > + > +for ( ; i < n; ++i ) > { > unsigned long addr = xen_phys_start + base_relocs->rva + > (base_relocs->entries[i] & 0xfff); > > > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/3] x86/EFI: don't apply relocations to l{2, 3}_bootmap
>>> On 19.08.16 at 12:34, wrote: > On 19/08/16 08:50, Jan Beulich wrote: >> Other than claimed in commit 2ce5963727's ("x86: construct the >> {l2,l3}_bootmap at compile time") the initialization of the two page >> tables doesn't take care of everything without furher adjustment: The >> compile time initialization obviously requires base relocations, and >> those get processed after efi_arch_memory_setup(). Hence without >> additional care the correctly initialized values may then get wrongly >> "adjusted" again. Except the two table from being subject to base >> relocation. > > Do you mean Exempt? "Except the two tables" doesn't parse. Hmm, my dictionary tells me that "except" can be used as a verb, and it also tells me that I rather don't mean "exempt". >> Signed-off-by: Jan Beulich >> >> --- a/xen/arch/x86/efi/efi-boot.h >> +++ b/xen/arch/x86/efi/efi-boot.h >> @@ -47,11 +47,23 @@ static void __init efi_arch_relocate_ima >> >> for ( base_relocs = __base_relocs_start; base_relocs < >> __base_relocs_end; ) >> { >> -unsigned int i, n; >> +unsigned int i = 0, n; >> >> n = (base_relocs->size - sizeof(*base_relocs)) / >> sizeof(*base_relocs->entries); >> -for ( i = 0; i < n; ++i ) >> + >> +/* >> + * Relevant l{2,3}_bootmap entries get initialized explicitly in >> + * efi_arch_memory_setup(), so we must not apply relocations there. >> + * l2_identmap's first slot, otoh, should be handled normally, as > > And l3 surely? Generally yes, but the comment refers to efi_arch_memory_setup(), which only touches l2_identmap[]. I.e. I'm trying to answer the reasonably to raise question why it doesn't need special casing here despite that function touching it. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel