[PATCH v2 0/3] x86/boot: Some fixes

[PATCH v2 0/3] x86/boot: Some fixes

[Daniel Kiper]

Hi,

As in subject... This is extended version of fix posted earlier as "x86/setup:
do not relocate below the end of current Xen image placement".

Daniel

 xen/arch/x86/setup.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

Daniel Kiper (3):
      x86/crashkernel: avoid Xen image when looking for module/crashkernel position
      x86/setup: do not relocate Xen over current Xen image placement
      x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))


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

[PATCH v2 1/3] x86/crashkernel: avoid Xen image when looking for module/crashkernel position

[Daniel Kiper]

Commit e22e1c4 (x86/EFI: avoid Xen image when looking for module/kexec
position) added relevant check for EFI case. However, since commit
f75a304 (x86: add multiboot2 protocol support for relocatable images)
Multiboot2 compatible bootloaders are able to relocate Xen image too.
So, we have to avoid also Xen image region in such cases.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reported-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 xen/arch/x86/setup.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 32bb02e..55acf63 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -653,7 +653,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     module_t *mod = (module_t *)__va(mbi->mods_addr);
     unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
     int i, j, e820_warn = 0, bytes = 0;
-    bool acpi_boot_table_init_done = false;
+    bool acpi_boot_table_init_done = false, xen_relocated = false;
     struct domain *dom0;
     struct ns16550_defaults ns16550 = {
         .data_bits = 8,
@@ -904,8 +904,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         mod[i].reserved = 0;
     }
 
-    if ( efi_enabled(EFI_LOADER) )
+    if ( xen_phys_start )
     {
+        xen_relocated = true;
+
         /*
          * This needs to remain in sync with xen_in_range() and the
          * respective reserve_e820_ram() invocation below.
@@ -1098,8 +1100,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
             /* Don't overlap with other modules (or Xen itself). */
             end = consider_modules(s, e, size, mod,
-                                   mbi->mods_count + efi_enabled(EFI_LOADER),
-                                   j);
+                                   mbi->mods_count + xen_relocated, j);
 
             if ( highmem_start && end > highmem_start )
                 continue;
@@ -1126,7 +1127,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         {
             /* Don't overlap with modules (or Xen itself). */
             e = consider_modules(s, e, PAGE_ALIGN(kexec_crash_area.size), mod,
-                                 mbi->mods_count + efi_enabled(EFI_LOADER), -1);
+                                 mbi->mods_count + xen_relocated, -1);
             if ( s >= e )
                 break;
             if ( e > kexec_crash_area_limit )
-- 
1.7.10.4


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

[PATCH v2 2/3] x86/setup: do not relocate Xen over current Xen image placement

[Daniel Kiper]

Otherwise, due to Xen code/data changes under CPU feet, Xen may crash
silently at boot.

We were hit by the issue in OVS Xen 4.4 with my earlier version of
EFI/Multiboot2 patches. Initially its implementation allowed relocation
of Xen even if it was relocated by the bootloader. This led to the
crashes on some new Oracle machines because copy destination partially
overlapped with the end of current/initial Xen image placement.

After some discussion on Xen-devel we decided to disable Xen relocation in
my EFI/Multiboot2 upstream patches if the booloader did the work for us.
Though one case is still not covered. If Xen is not relocated by the
booloader then it tries to do that by itself. If all RAM regions above
currently occupied one are unsuitable for relocation then Xen tries to move
itself higher in it. And if (end - reloc_size + XEN_IMG_OFFSET) goes
below _end then copy/relocation destination overlaps, at least partially,
with its source.

I can agree that this should not happen on todays machines very often.
If at all. It is rather unusual to not have usable RAM regions above
~5 MiB nowadays. Though I think that we should at least consider putting
such safety measure here. Otherwise Xen may crash mysteriously without
any stack trace. It is very confusing and impairs further debugging.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v2 - suggestions/fixes:
   - improve comment
     (suggested by Jan Beulich),
   - improve commit message
     (suggested by Jan Beulich).
---
 xen/arch/x86/setup.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 55acf63..5c45496 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -962,7 +962,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
         }
         else
             end = 0;
-        if ( end > s )
+
+        /*
+         * Is the region size greater than zero and does it begins
+         * above or at the end of current Xen image placement?
+         */
+        if ( (end > s) && (end - reloc_size >= _end - _start) )
         {
             l4_pgentry_t *pl4e;
             l3_pgentry_t *pl3e;
-- 
1.7.10.4


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

[PATCH v2 3/3] x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))

[Daniel Kiper]

Current limit, PFN_DOWN(xen_phys_start), introduced by commit b280442
(x86: make Xen early boot code relocatable) is not reliable. Potentially
its value may fall below PFN_DOWN(__pa(_end)) and then part of Xen image
may not be mapped after relocation. This will not happen in current code
thanks to "x86/setup: do not relocate over current Xen image placement"
patch. Though this safety measure may save a lot of debugging time when
somebody decide to relax existing relocation restrictions one day.
Additionally, remapping will execute a bit faster due to this change.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 xen/arch/x86/setup.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 5c45496..c719aed 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -973,6 +973,11 @@ void __init noreturn __start_xen(unsigned long mbi_p)
             l3_pgentry_t *pl3e;
             l2_pgentry_t *pl2e;
             int i, j, k;
+            /*
+             * We have to calculate xen_remap_end_pfn before
+             * xen_phys_start change.
+             */
+            unsigned long xen_remap_end_pfn = PFN_DOWN(__pa(_end));
 
             /* Select relocation address. */
             e = end - reloc_size;
@@ -1002,7 +1007,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                     /* Not present, 1GB mapping, or already relocated? */
                     if ( !(l3e_get_flags(*pl3e) & _PAGE_PRESENT) ||
                          (l3e_get_flags(*pl3e) & _PAGE_PSE) ||
-                         (l3e_get_pfn(*pl3e) > PFN_DOWN(xen_phys_start)) )
+                         (l3e_get_pfn(*pl3e) > xen_remap_end_pfn) )
                         continue;
                     *pl3e = l3e_from_intpte(l3e_get_intpte(*pl3e) +
                                             xen_phys_start);
@@ -1012,7 +1017,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                         /* Not present, PSE, or already relocated? */
                         if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
                              (l2e_get_flags(*pl2e) & _PAGE_PSE) ||
-                             (l2e_get_pfn(*pl2e) > PFN_DOWN(xen_phys_start)) )
+                             (l2e_get_pfn(*pl2e) > xen_remap_end_pfn) )
                             continue;
                         *pl2e = l2e_from_intpte(l2e_get_intpte(*pl2e) +
                                                 xen_phys_start);
@@ -1036,7 +1041,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
                 unsigned int flags;
 
                 if ( !(l2e_get_flags(*pl2e) & _PAGE_PRESENT) ||
-                     (l2e_get_pfn(*pl2e) > PFN_DOWN(xen_phys_start)) )
+                     (l2e_get_pfn(*pl2e) > xen_remap_end_pfn) )
                     continue;
 
                 if ( !using_2M_mapping() )
-- 
1.7.10.4


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

Re: [PATCH v2 1/3] x86/crashkernel: avoid Xen image when looking for module/crashkernel position

[Jan Beulich]

>>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -653,7 +653,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      module_t *mod = (module_t *)__va(mbi->mods_addr);
>      unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
>      int i, j, e820_warn = 0, bytes = 0;
> -    bool acpi_boot_table_init_done = false;
> +    bool acpi_boot_table_init_done = false, xen_relocated = false;

I don't see a need for the xen_ prefix here - with that dropped
(which I guess could be done while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v2 2/3] x86/setup: do not relocate Xen over current Xen image placement

[Jan Beulich]

>>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -962,7 +962,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>          }
>          else
>              end = 0;
> -        if ( end > s )
> +
> +        /*
> +         * Is the region size greater than zero and does it begins

begin

> +         * above or at the end of current Xen image placement?

Without being a native speaker I think this commonly is "at or
above", not the other way around. But I'd be happy to be told
that this other form is equally frequently being used.

> +         */
> +        if ( (end > s) && (end - reloc_size >= _end - _start) )

In your earlier mails following v1 you had __pa(_end) here on the
right side. Why is this _end - _start again now (which is 2Mb too
little imo with the current XEN_IMG_OFFSET value)?

Jan


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

Re: [PATCH v2 3/3] x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))

[Jan Beulich]

>>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> Current limit, PFN_DOWN(xen_phys_start), introduced by commit b280442
> (x86: make Xen early boot code relocatable) is not reliable. Potentially
> its value may fall below PFN_DOWN(__pa(_end))

Under what (perhaps just theoretical) conditions? It seems to imply
to me that we'd be moved Xen downwards if this was to happen, in
which case I'd suggest to simply skip the relocation instead (we
really only ever want to move Xen upwards).

Jan


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

Re: [PATCH v2 1/3] x86/crashkernel: avoid Xen image when looking for module/crashkernel position

[Daniel Kiper]

On Thu, Dec 07, 2017 at 04:08:31AM -0700, Jan Beulich wrote:
> >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -653,7 +653,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >      module_t *mod = (module_t *)__va(mbi->mods_addr);
> >      unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
> >      int i, j, e820_warn = 0, bytes = 0;
> > -    bool acpi_boot_table_init_done = false;
> > +    bool acpi_boot_table_init_done = false, xen_relocated = false;
>
> I don't see a need for the xen_ prefix here - with that dropped
> (which I guess could be done while committing)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

I am OK with that change. Go ahead...

Daniel

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

Re: [PATCH v2 2/3] x86/setup: do not relocate Xen over current Xen image placement

[Daniel Kiper]

On Thu, Dec 07, 2017 at 04:51:32AM -0700, Jan Beulich wrote:
> >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -962,7 +962,12 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >          }
> >          else
> >              end = 0;
> > -        if ( end > s )
> > +
> > +        /*
> > +         * Is the region size greater than zero and does it begins
>
> begin

Yep.

> > +         * above or at the end of current Xen image placement?
>
> Without being a native speaker I think this commonly is "at or
> above", not the other way around. But I'd be happy to be told
> that this other form is equally frequently being used.

OK.

> > +         */
> > +        if ( (end > s) && (end - reloc_size >= _end - _start) )
>
> In your earlier mails following v1 you had __pa(_end) here on the
> right side. Why is this _end - _start again now (which is 2Mb too
> little imo with the current XEN_IMG_OFFSET value)?

(end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) and
(end - reloc_size >= _end - _start) are equal.

You should remember that there is nothing to copy between 0 and XEN_IMG_OFFSET.

Daniel

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

Re: [PATCH v2 3/3] x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))

[Daniel Kiper]

On Thu, Dec 07, 2017 at 05:02:02AM -0700, Jan Beulich wrote:
> >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> > Current limit, PFN_DOWN(xen_phys_start), introduced by commit b280442
> > (x86: make Xen early boot code relocatable) is not reliable. Potentially
> > its value may fall below PFN_DOWN(__pa(_end))
>
> Under what (perhaps just theoretical) conditions? It seems to imply
> to me that we'd be moved Xen downwards if this was to happen, in
> which case I'd suggest to simply skip the relocation instead (we
> really only ever want to move Xen upwards).

Not always. If __pa(__image_base__) > xen_phys_start and even
if xen_phys_start < __pa(_end) then we are still moving upwards.
That is why we should change the condition. And we have to reference
to something constant not to the moving one.

Daniel

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

Re: [PATCH v2 2/3] x86/setup: do not relocate Xen over current Xen image placement

[Jan Beulich]

>>> On 11.12.17 at 15:59, <daniel.kiper@oracle.com> wrote:
> On Thu, Dec 07, 2017 at 04:51:32AM -0700, Jan Beulich wrote:
>> >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
>> > +        if ( (end > s) && (end - reloc_size >= _end - _start) )
>>
>> In your earlier mails following v1 you had __pa(_end) here on the
>> right side. Why is this _end - _start again now (which is 2Mb too
>> little imo with the current XEN_IMG_OFFSET value)?
> 
> (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) and

If you used this expression, I think I could live without it having a
comment.

> (end - reloc_size >= _end - _start) are equal.
> 
> You should remember that there is nothing to copy between 0 and 
> XEN_IMG_OFFSET.

If, however, you want to stick to the expression you currently use,
I think this very much needs a comment.

Jan


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

Re: [PATCH v2 3/3] x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))

[Jan Beulich]

>>> On 11.12.17 at 16:12, <daniel.kiper@oracle.com> wrote:
> On Thu, Dec 07, 2017 at 05:02:02AM -0700, Jan Beulich wrote:
>> >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
>> > Current limit, PFN_DOWN(xen_phys_start), introduced by commit b280442
>> > (x86: make Xen early boot code relocatable) is not reliable. Potentially
>> > its value may fall below PFN_DOWN(__pa(_end))
>>
>> Under what (perhaps just theoretical) conditions? It seems to imply
>> to me that we'd be moved Xen downwards if this was to happen, in
>> which case I'd suggest to simply skip the relocation instead (we
>> really only ever want to move Xen upwards).
> 
> Not always. If __pa(__image_base__) > xen_phys_start and even

I'm being increasingly confused: Isn't xen_phys_start identical to
__pa(__image_base__)?

> if xen_phys_start < __pa(_end) then we are still moving upwards.

And xen_phys_start always below __pa(_end)?

> That is why we should change the condition. And we have to reference
> to something constant not to the moving one.

I also don't understand what would be "the moving one" here:
xen_phys_start is being updated just once, before any of the
relocation code executes.

Jan


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

Re: [PATCH v2 3/3] x86/setup: remap Xen image up to PFN_DOWN(__pa(_end))

[Daniel Kiper]

On Tue, Dec 12, 2017 at 02:42:50AM -0700, Jan Beulich wrote:
> >>> On 11.12.17 at 16:12, <daniel.kiper@oracle.com> wrote:
> > On Thu, Dec 07, 2017 at 05:02:02AM -0700, Jan Beulich wrote:
> >> >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> >> > Current limit, PFN_DOWN(xen_phys_start), introduced by commit b280442
> >> > (x86: make Xen early boot code relocatable) is not reliable. Potentially
> >> > its value may fall below PFN_DOWN(__pa(_end))
> >>
> >> Under what (perhaps just theoretical) conditions? It seems to imply
> >> to me that we'd be moved Xen downwards if this was to happen, in
> >> which case I'd suggest to simply skip the relocation instead (we
> >> really only ever want to move Xen upwards).
> >
> > Not always. If __pa(__image_base__) > xen_phys_start and even
>
> I'm being increasingly confused: Isn't xen_phys_start identical to
> __pa(__image_base__)?

Right, it is confusing and condition is incorrect. I should say:
  NEW_xen_phys_start > __pa_BEFORE_xen_phys_start_CHANGE(__image_base__)...

> > if xen_phys_start < __pa(_end) then we are still moving upwards.
>
> And xen_phys_start always below __pa(_end)?

...and NEW_xen_phys_start < __pa_BEFORE_xen_phys_start_CHANGE(_end).

> > That is why we should change the condition. And we have to reference
> > to something constant not to the moving one.
>
> I also don't understand what would be "the moving one" here:
> xen_phys_start is being updated just once, before any of the

I am referring to this change. And it means that everything between NEW_xen_phys_start
and __pa_BEFORE_xen_phys_start_CHANGE(_end) may not be mapped because ramping
condition/limit refers to xen_phys_start which may fall below
__pa_BEFORE_xen_phys_start_CHANGE(_end) (right now in theory due to lack
of relocation if boot loader did it for us).

Daniel

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

Re: [PATCH v2 1/3] x86/crashkernel: avoid Xen image when looking for module/crashkernel position

[Daniel Kiper]

On Thu, Dec 07, 2017 at 04:39:37PM +0100, Daniel Kiper wrote:
> On Thu, Dec 07, 2017 at 04:08:31AM -0700, Jan Beulich wrote:
> > >>> On 04.12.17 at 11:24, <daniel.kiper@oracle.com> wrote:
> > > --- a/xen/arch/x86/setup.c
> > > +++ b/xen/arch/x86/setup.c
> > > @@ -653,7 +653,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> > >      module_t *mod = (module_t *)__va(mbi->mods_addr);
> > >      unsigned long nr_pages, raw_max_page, modules_headroom, *module_map;
> > >      int i, j, e820_warn = 0, bytes = 0;
> > > -    bool acpi_boot_table_init_done = false;
> > > +    bool acpi_boot_table_init_done = false, xen_relocated = false;
> >
> > I don't see a need for the xen_ prefix here - with that dropped
> > (which I guess could be done while committing)
> > Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I am OK with that change. Go ahead...

I have seen that you have applied this. Great! I have just realized
that this patch also fixed another issue which we discovered a few
days ago. Machines with less than 4 GiB rebooted mysteriously if Xen
was loaded with Multiboot2 and relocated by the bootloader. This
happened because relocator chosen to relocate e.g. dom0 kernel over
the Xen image. The problem does not appear if Xen is loaded with
Multiboot protocol. This happens because end of RAM region (e) occupied
by Xen is updated by Xen relocation code. So, one shot and two bugs
killed at once! Nice!

Daniel

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