[PATCH 0/3] x86/EFI: boot adjustments

[PATCH 0/3] x86/EFI: boot adjustments

[Jan Beulich]

1: don't apply relocations to l{2,3}_bootmap
2: be cautious about being handed control with CR4.PGE enabled
3: don't accept 64-bit base relocations on page tables

Signed-off-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/3] x86/EFI: don't apply relocations to l{2, 3}_bootmap

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

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 <jbeulich@suse.com>

--- 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);




[-- Attachment #2: x86-EFI-lN_bootmap.patch --]
[-- Type: text/plain, Size: 1760 bytes --]

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 <jbeulich@suse.com>

--- 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);

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] x86/EFI: don't apply relocations to l{2, 3}_bootmap

[Andrew Cooper]

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 <jbeulich@suse.com>
>
> --- 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 <andrew.cooper@citrix.com>

> +         * 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: [PATCH 1/3] x86/EFI: don't apply relocations to l{2, 3}_bootmap

[Jan Beulich]

>>> On 19.08.16 at 12:34, <andrew.cooper3@citrix.com> 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 <jbeulich@suse.com>
>>
>> --- 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

[PATCH 2/3] x86/EFI: be cautious about being handed control with CR4.PGE enabled

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

To effect proper TLB flushing in that case we should clear CR4.PGE
before loading the new page tables.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -228,7 +228,7 @@ static void __init efi_arch_pre_exit_boo
 
 static void __init noreturn efi_arch_post_exit_boot(void)
 {
-    u64 efer;
+    u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer;
 
     efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
     memcpy((void *)trampoline_phys, trampoline_start, cfg.size);
@@ -244,6 +244,10 @@ static void __init noreturn efi_arch_pos
               X86_CR0_AM | X86_CR0_PG);
     asm volatile ( "mov    %[cr4], %%cr4\n\t"
                    "mov    %[cr3], %%cr3\n\t"
+#if XEN_MINIMAL_CR4 & X86_CR4_PGE
+                   "or     $"__stringify(X86_CR4_PGE)", %[cr4]\n\t"
+                   "mov    %[cr4], %%cr4\n\t"
+#endif
                    "movabs $__start_xen, %[rip]\n\t"
                    "lgdt   gdt_descr(%%rip)\n\t"
                    "mov    stack_start(%%rip), %%rsp\n\t"
@@ -255,9 +259,9 @@ static void __init noreturn efi_arch_pos
                    "movl   %[cs], 8(%%rsp)\n\t"
                    "mov    %[rip], (%%rsp)\n\t"
                    "lretq  %[stkoff]-16"
-                   : [rip] "=&r" (efer/* any dead 64-bit variable */)
+                   : [rip] "=&r" (efer/* any dead 64-bit variable */),
+                     [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
-                     [cr4] "r" (mmu_cr4_features),
                      [cs] "ir" (__HYPERVISOR_CS),
                      [ds] "r" (__HYPERVISOR_DS),
                      [stkoff] "i" (STACK_SIZE - sizeof(struct cpu_info)),




[-- Attachment #2: x86-EFI-CR4-PGE.patch --]
[-- Type: text/plain, Size: 1837 bytes --]

x86/EFI: be cautious about being handed control with CR4.PGE enabled

To effect proper TLB flushing in that case we should clear CR4.PGE
before loading the new page tables.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -228,7 +228,7 @@ static void __init efi_arch_pre_exit_boo
 
 static void __init noreturn efi_arch_post_exit_boot(void)
 {
-    u64 efer;
+    u64 cr4 = XEN_MINIMAL_CR4 & ~X86_CR4_PGE, efer;
 
     efi_arch_relocate_image(__XEN_VIRT_START - xen_phys_start);
     memcpy((void *)trampoline_phys, trampoline_start, cfg.size);
@@ -244,6 +244,10 @@ static void __init noreturn efi_arch_pos
               X86_CR0_AM | X86_CR0_PG);
     asm volatile ( "mov    %[cr4], %%cr4\n\t"
                    "mov    %[cr3], %%cr3\n\t"
+#if XEN_MINIMAL_CR4 & X86_CR4_PGE
+                   "or     $"__stringify(X86_CR4_PGE)", %[cr4]\n\t"
+                   "mov    %[cr4], %%cr4\n\t"
+#endif
                    "movabs $__start_xen, %[rip]\n\t"
                    "lgdt   gdt_descr(%%rip)\n\t"
                    "mov    stack_start(%%rip), %%rsp\n\t"
@@ -255,9 +259,9 @@ static void __init noreturn efi_arch_pos
                    "movl   %[cs], 8(%%rsp)\n\t"
                    "mov    %[rip], (%%rsp)\n\t"
                    "lretq  %[stkoff]-16"
-                   : [rip] "=&r" (efer/* any dead 64-bit variable */)
+                   : [rip] "=&r" (efer/* any dead 64-bit variable */),
+                     [cr4] "+&r" (cr4)
                    : [cr3] "r" (idle_pg_table),
-                     [cr4] "r" (mmu_cr4_features),
                      [cs] "ir" (__HYPERVISOR_CS),
                      [ds] "r" (__HYPERVISOR_DS),
                      [stkoff] "i" (STACK_SIZE - sizeof(struct cpu_info)),

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] x86/EFI: be cautious about being handed control with CR4.PGE enabled

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 252 bytes --]

On 19/08/16 08:51, Jan Beulich wrote:
> To effect proper TLB flushing in that case we should clear CR4.PGE
> before loading the new page tables.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[-- Attachment #1.2: Type: text/html, Size: 805 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

Page tables get pre-populated with physical addresses which, due to
living inside the Xen image, will never exceed 32 bits in width. That
in turn results in the tool generating the relocations to produce
32-bit relocations for them instead of the 64-bit ones needed for
relocating virtual addresses. Hence instead of special casing page
tables in the processing of 64-bit relocations, let's be more rigid
and refuse them (as being indicative of something else having gone
wrong in the build process).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -81,12 +81,9 @@ static void __init efi_arch_relocate_ima
                 }
                 break;
             case PE_BASE_RELOC_DIR64:
-                if ( delta )
-                {
-                    *(u64 *)addr += delta;
-                    if ( in_page_tables(addr) )
-                        *(intpte_t *)addr += xen_phys_start;
-                }
+                if ( in_page_tables(addr) )
+                    blexit(L"Unexpected relocation type");
+                *(u64 *)addr += delta;
                 break;
             default:
                 blexit(L"Unsupported relocation type");




[-- Attachment #2: x86-EFI-64bit-reloc-pgtab.patch --]
[-- Type: text/plain, Size: 1324 bytes --]

x86/EFI: don't accept 64-bit base relocations on page tables

Page tables get pre-populated with physical addresses which, due to
living inside the Xen image, will never exceed 32 bits in width. That
in turn results in the tool generating the relocations to produce
32-bit relocations for them instead of the 64-bit ones needed for
relocating virtual addresses. Hence instead of special casing page
tables in the processing of 64-bit relocations, let's be more rigid
and refuse them (as being indicative of something else having gone
wrong in the build process).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -81,12 +81,9 @@ static void __init efi_arch_relocate_ima
                 }
                 break;
             case PE_BASE_RELOC_DIR64:
-                if ( delta )
-                {
-                    *(u64 *)addr += delta;
-                    if ( in_page_tables(addr) )
-                        *(intpte_t *)addr += xen_phys_start;
-                }
+                if ( in_page_tables(addr) )
+                    blexit(L"Unexpected relocation type");
+                *(u64 *)addr += delta;
                 break;
             default:
                 blexit(L"Unsupported relocation type");

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1855 bytes --]

On 19/08/16 08:52, Jan Beulich wrote:
> Page tables get pre-populated with physical addresses which, due to
> living inside the Xen image, will never exceed 32 bits in width. That
> in turn results in the tool generating the relocations to produce
> 32-bit relocations for them instead of the 64-bit ones needed for
> relocating virtual addresses. Hence instead of special casing page
> tables in the processing of 64-bit relocations, let's be more rigid
> and refuse them (as being indicative of something else having gone
> wrong in the build process).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Is it an ABI requirement to use the minimal available relocation?  It is
certainly suboptimal to use a 64bit relocation when a 32bit one would
do, but I wouldn't bet that it is unconditional avoided by all toolchains.

It is currently the case that Xen needs to live below 4GB physical, so
from that point of view a 64bit relocation will not be required in the
pagetables.

~Andrew

>
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -81,12 +81,9 @@ static void __init efi_arch_relocate_ima
>                  }
>                  break;
>              case PE_BASE_RELOC_DIR64:
> -                if ( delta )
> -                {
> -                    *(u64 *)addr += delta;
> -                    if ( in_page_tables(addr) )
> -                        *(intpte_t *)addr += xen_phys_start;
> -                }
> +                if ( in_page_tables(addr) )
> +                    blexit(L"Unexpected relocation type");
> +                *(u64 *)addr += delta;
>                  break;
>              default:
>                  blexit(L"Unsupported relocation type");
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 2699 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Jan Beulich]

>>> On 19.08.16 at 14:39, <andrew.cooper3@citrix.com> wrote:
> On 19/08/16 08:52, Jan Beulich wrote:
>> Page tables get pre-populated with physical addresses which, due to
>> living inside the Xen image, will never exceed 32 bits in width. That
>> in turn results in the tool generating the relocations to produce
>> 32-bit relocations for them instead of the 64-bit ones needed for
>> relocating virtual addresses. Hence instead of special casing page
>> tables in the processing of 64-bit relocations, let's be more rigid
>> and refuse them (as being indicative of something else having gone
>> wrong in the build process).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Is it an ABI requirement to use the minimal available relocation?  It is
> certainly suboptimal to use a 64bit relocation when a 32bit one would
> do, but I wouldn't bet that it is unconditional avoided by all toolchains.

What ABI? The tool in question is one of our own making. And the
way relocations get generated it's hard to tell those that have to
be 32-bit (in early boot code and trampoline code) from those that
may as well be 64-bit ones (in page tables).

> It is currently the case that Xen needs to live below 4GB physical, so
> from that point of view a 64bit relocation will not be required in the
> pagetables.

And even if Xen didn't itself have this requirement, the EFI loader
would always put us below 4Gb.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Andrew Cooper]

On 19/08/16 13:57, Jan Beulich wrote:
>>>> On 19.08.16 at 14:39, <andrew.cooper3@citrix.com> wrote:
>> On 19/08/16 08:52, Jan Beulich wrote:
>>> Page tables get pre-populated with physical addresses which, due to
>>> living inside the Xen image, will never exceed 32 bits in width. That
>>> in turn results in the tool generating the relocations to produce
>>> 32-bit relocations for them instead of the 64-bit ones needed for
>>> relocating virtual addresses. Hence instead of special casing page
>>> tables in the processing of 64-bit relocations, let's be more rigid
>>> and refuse them (as being indicative of something else having gone
>>> wrong in the build process).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Is it an ABI requirement to use the minimal available relocation?  It is
>> certainly suboptimal to use a 64bit relocation when a 32bit one would
>> do, but I wouldn't bet that it is unconditional avoided by all toolchains.
> What ABI? The tool in question is one of our own making. And the
> way relocations get generated it's hard to tell those that have to
> be 32-bit (in early boot code and trampoline code) from those that
> may as well be 64-bit ones (in page tables).
>
>> It is currently the case that Xen needs to live below 4GB physical, so
>> from that point of view a 64bit relocation will not be required in the
>> pagetables.
> And even if Xen didn't itself have this requirement, the EFI loader
> would always put us below 4Gb.

Why is this necessarily true?

xen.efi is built as a 64bit PE, not 32bit.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Jan Beulich]

>>> On 30.08.16 at 18:35, <andrew.cooper3@citrix.com> wrote:
> On 19/08/16 13:57, Jan Beulich wrote:
>>>>> On 19.08.16 at 14:39, <andrew.cooper3@citrix.com> wrote:
>>> On 19/08/16 08:52, Jan Beulich wrote:
>>>> Page tables get pre-populated with physical addresses which, due to
>>>> living inside the Xen image, will never exceed 32 bits in width. That
>>>> in turn results in the tool generating the relocations to produce
>>>> 32-bit relocations for them instead of the 64-bit ones needed for
>>>> relocating virtual addresses. Hence instead of special casing page
>>>> tables in the processing of 64-bit relocations, let's be more rigid
>>>> and refuse them (as being indicative of something else having gone
>>>> wrong in the build process).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Is it an ABI requirement to use the minimal available relocation?  It is
>>> certainly suboptimal to use a 64bit relocation when a 32bit one would
>>> do, but I wouldn't bet that it is unconditional avoided by all toolchains.
>> What ABI? The tool in question is one of our own making. And the
>> way relocations get generated it's hard to tell those that have to
>> be 32-bit (in early boot code and trampoline code) from those that
>> may as well be 64-bit ones (in page tables).
>>
>>> It is currently the case that Xen needs to live below 4GB physical, so
>>> from that point of view a 64bit relocation will not be required in the
>>> pagetables.
>> And even if Xen didn't itself have this requirement, the EFI loader
>> would always put us below 4Gb.
> 
> Why is this necessarily true?
> 
> xen.efi is built as a 64bit PE, not 32bit.

The file format doesn't matter here. And we'd have other problems
to solve if we got loaded above 4Gb.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Andrew Cooper]

On 31/08/16 13:28, Jan Beulich wrote:
>>>> On 30.08.16 at 18:35, <andrew.cooper3@citrix.com> wrote:
>> On 19/08/16 13:57, Jan Beulich wrote:
>>>>>> On 19.08.16 at 14:39, <andrew.cooper3@citrix.com> wrote:
>>>> On 19/08/16 08:52, Jan Beulich wrote:
>>>>> Page tables get pre-populated with physical addresses which, due to
>>>>> living inside the Xen image, will never exceed 32 bits in width. That
>>>>> in turn results in the tool generating the relocations to produce
>>>>> 32-bit relocations for them instead of the 64-bit ones needed for
>>>>> relocating virtual addresses. Hence instead of special casing page
>>>>> tables in the processing of 64-bit relocations, let's be more rigid
>>>>> and refuse them (as being indicative of something else having gone
>>>>> wrong in the build process).
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Is it an ABI requirement to use the minimal available relocation?  It is
>>>> certainly suboptimal to use a 64bit relocation when a 32bit one would
>>>> do, but I wouldn't bet that it is unconditional avoided by all toolchains.
>>> What ABI? The tool in question is one of our own making. And the
>>> way relocations get generated it's hard to tell those that have to
>>> be 32-bit (in early boot code and trampoline code) from those that
>>> may as well be 64-bit ones (in page tables).
>>>
>>>> It is currently the case that Xen needs to live below 4GB physical, so
>>>> from that point of view a 64bit relocation will not be required in the
>>>> pagetables.
>>> And even if Xen didn't itself have this requirement, the EFI loader
>>> would always put us below 4Gb.
>> Why is this necessarily true?
>>
>> xen.efi is built as a 64bit PE, not 32bit.
> The file format doesn't matter here. And we'd have other problems
> to solve if we got loaded above 4Gb.

Right, but that doesn't prohibit the generation of 64bit relocations in
the pagetables.

This patch still seems conceptually incorrect.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Jan Beulich]

>>> On 31.08.16 at 14:32, <andrew.cooper3@citrix.com> wrote:
> On 31/08/16 13:28, Jan Beulich wrote:
>>>>> On 30.08.16 at 18:35, <andrew.cooper3@citrix.com> wrote:
>>> On 19/08/16 13:57, Jan Beulich wrote:
>>>>>>> On 19.08.16 at 14:39, <andrew.cooper3@citrix.com> wrote:
>>>>> On 19/08/16 08:52, Jan Beulich wrote:
>>>>>> Page tables get pre-populated with physical addresses which, due to
>>>>>> living inside the Xen image, will never exceed 32 bits in width. That
>>>>>> in turn results in the tool generating the relocations to produce
>>>>>> 32-bit relocations for them instead of the 64-bit ones needed for
>>>>>> relocating virtual addresses. Hence instead of special casing page
>>>>>> tables in the processing of 64-bit relocations, let's be more rigid
>>>>>> and refuse them (as being indicative of something else having gone
>>>>>> wrong in the build process).
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Is it an ABI requirement to use the minimal available relocation?  It is
>>>>> certainly suboptimal to use a 64bit relocation when a 32bit one would
>>>>> do, but I wouldn't bet that it is unconditional avoided by all toolchains.
>>>> What ABI? The tool in question is one of our own making. And the
>>>> way relocations get generated it's hard to tell those that have to
>>>> be 32-bit (in early boot code and trampoline code) from those that
>>>> may as well be 64-bit ones (in page tables).
>>>>
>>>>> It is currently the case that Xen needs to live below 4GB physical, so
>>>>> from that point of view a 64bit relocation will not be required in the
>>>>> pagetables.
>>>> And even if Xen didn't itself have this requirement, the EFI loader
>>>> would always put us below 4Gb.
>>> Why is this necessarily true?
>>>
>>> xen.efi is built as a 64bit PE, not 32bit.
>> The file format doesn't matter here. And we'd have other problems
>> to solve if we got loaded above 4Gb.
> 
> Right, but that doesn't prohibit the generation of 64bit relocations in
> the pagetables.

How that? As already said, we produce the relocations by a tool of
our own, and that tool together with how the inputs passed to it get
built preclude 64-bit relocations in page tables (or else our image
size would need to come close to or exceed 4Gb).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Ping: [PATCH 3/3] x86/EFI: don't accept 64-bit base relocations on page tables

[Jan Beulich]

>>> On 19.08.16 at 09:52, <JBeulich@suse.com> wrote:
> Page tables get pre-populated with physical addresses which, due to
> living inside the Xen image, will never exceed 32 bits in width. That
> in turn results in the tool generating the relocations to produce
> 32-bit relocations for them instead of the 64-bit ones needed for
> relocating virtual addresses. Hence instead of special casing page
> tables in the processing of 64-bit relocations, let's be more rigid
> and refuse them (as being indicative of something else having gone
> wrong in the build process).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ping?

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -81,12 +81,9 @@ static void __init efi_arch_relocate_ima
>                  }
>                  break;
>              case PE_BASE_RELOC_DIR64:
> -                if ( delta )
> -                {
> -                    *(u64 *)addr += delta;
> -                    if ( in_page_tables(addr) )
> -                        *(intpte_t *)addr += xen_phys_start;
> -                }
> +                if ( in_page_tables(addr) )
> +                    blexit(L"Unexpected relocation type");
> +                *(u64 *)addr += delta;
>                  break;
>              default:
>                  blexit(L"Unsupported relocation type");




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel