[PATCH] i386: always clear bss

[PATCH] i386: always clear bss

[Jeremy Fitzhardinge]

When the paravirt dispatcher gets run immediately on entry to
startup_32, the bss isn't cleared.  This happens to work if the
hypervisor's domain builder loaded the complete kernel image and
cleared the bss for us, but this may not always be true (for example,
if we're running out of a decompressed bzImage).

Change head.S so that it unconditionally clears the bss before doing
the paravirt dispatch or continuing on to normal native boot.

There are a couple of points to note:
 - We can't, in general, load the segment registers before paravirt
   dispatch, because we could be running with a non-standard gdt and
   segment selectors.  In practice though, all code which ends up
   jumping into startup_32 will have already set the segment registers
   up to sane values, so we don't need to do it again.
 - Paging may or may not be enabled, and if enabled we may or may not
   be mapped to the proper kernel virtual address.  To deal with this,
   we compare the kernel's linked address with where we're actually
   running, and use that to offset the bss pointer.

Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>

---
 arch/i386/kernel/head.S |   48 ++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

===================================================================
--- a/arch/i386/kernel/head.S
+++ b/arch/i386/kernel/head.S
@@ -70,6 +70,33 @@ INIT_MAP_BEYOND_END = BOOTBITMAP_SIZE + 
  */
 .section .text.head,"ax",@progbits
 ENTRY(startup_32)
+/*
+ * Clear BSS first so that there are no surprises...
+ * This relies on the the segment registers to be set
+ * to something sensible, which will have already happened.
+ */
+	cld
+	xorl %eax,%eax
+	movl $__bss_start,%edi
+	movl $__bss_stop,%ecx
+	subl %edi,%ecx
+	shrl $2,%ecx
+	/*
+	 * Work out whether we're running mapped or not:
+	 * - call a local label
+	 * - pop the return address to get the actual eip
+	 * - subtract local label from %edi (= bss pointer)
+	 * - add in actual eip
+	 *
+	 * This will result in %edi being a virtual pointer if
+	 * we're currently mapped, or a physical pointer if we're
+	 * not (either no paging or 1:1 mapping).
+	 */
+	call 1f
+1:	popl %ebx
+	subl $1b, %edi
+	addl %ebx, %edi
+	rep ; stosl
 
 #ifdef CONFIG_PARAVIRT
         movl %cs, %eax
@@ -77,27 +104,6 @@ ENTRY(startup_32)
         jnz startup_paravirt
 #endif
 
-/*
- * Set segments to known values.
- */
-	cld
-	lgdt boot_gdt_descr - __PAGE_OFFSET
-	movl $(__BOOT_DS),%eax
-	movl %eax,%ds
-	movl %eax,%es
-	movl %eax,%fs
-	movl %eax,%gs
-
-/*
- * Clear BSS first so that there are no surprises...
- * No need to cld as DF is already clear from cld above...
- */
-	xorl %eax,%eax
-	movl $__bss_start - __PAGE_OFFSET,%edi
-	movl $__bss_stop - __PAGE_OFFSET,%ecx
-	subl %edi,%ecx
-	shrl $2,%ecx
-	rep ; stosl
 /*
  * Copy bootup parameters out of the way.
  * Note: %esi still has the pointer to the real-mode data.

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> When the paravirt dispatcher gets run immediately on entry to
> startup_32, the bss isn't cleared.  This happens to work if the
> hypervisor's domain builder loaded the complete kernel image and
> cleared the bss for us, but this may not always be true (for example,
> if we're running out of a decompressed bzImage).
>
> Change head.S so that it unconditionally clears the bss before doing
> the paravirt dispatch or continuing on to normal native boot.
>
> There are a couple of points to note:
>  - We can't, in general, load the segment registers before paravirt
>    dispatch, because we could be running with a non-standard gdt and
>    segment selectors.  In practice though, all code which ends up
>    jumping into startup_32 will have already set the segment registers
>    up to sane values, so we don't need to do it again.
>  - Paging may or may not be enabled, and if enabled we may or may not
>    be mapped to the proper kernel virtual address.  To deal with this,
>    we compare the kernel's linked address with where we're actually
>    running, and use that to offset the bss pointer.

NAK.  

Skipping the segment register load is likely fine.
Supporting V!=P at startup_32 is not.
Assuming that we have a stack at startup_32 is not.

If you want to figure out where the kernel is loaded you can do
(from arch/i386/boot/head.S)
> 
> /* Calculate the delta between where we were compiled to run
>  * at and where we were actually loaded at.  This can only be done
>  * with a short local call on x86.  Nothing  else will tell us what
>  * address we are running at.  The reserved chunk of the real-mode
>  * data at 0x34-0x3f are used as the stack for this calculation.
>  * Only 4 bytes are needed.
>  */
> 	leal 0x40(%esi), %esp
> 	call 1f
> 1:	popl %ebp
> 	subl $1b, %ebp
> 

Eric

Re: [PATCH] i386: always clear bss

[Jeremy Fitzhardinge]

Eric W. Biederman wrote:
> Jeremy Fitzhardinge <jeremy@goop.org> writes:
>
>   
>> When the paravirt dispatcher gets run immediately on entry to
>> startup_32, the bss isn't cleared.  This happens to work if the
>> hypervisor's domain builder loaded the complete kernel image and
>> cleared the bss for us, but this may not always be true (for example,
>> if we're running out of a decompressed bzImage).
>>
>> Change head.S so that it unconditionally clears the bss before doing
>> the paravirt dispatch or continuing on to normal native boot.
>>
>> There are a couple of points to note:
>>  - We can't, in general, load the segment registers before paravirt
>>    dispatch, because we could be running with a non-standard gdt and
>>    segment selectors.  In practice though, all code which ends up
>>    jumping into startup_32 will have already set the segment registers
>>    up to sane values, so we don't need to do it again.
>>  - Paging may or may not be enabled, and if enabled we may or may not
>>    be mapped to the proper kernel virtual address.  To deal with this,
>>    we compare the kernel's linked address with where we're actually
>>    running, and use that to offset the bss pointer.
>>     

BTW, I should have marked this as an RFC comment, rather than an actual
submission.  We don't need it for .22.

> NAK.  
>
> Skipping the segment register load is likely fine.
> Supporting V!=P at startup_32 is not.
>   

Why?

> Assuming that we have a stack at startup_32 is not.
>
> If you want to figure out where the kernel is loaded you can do
> (from arch/i386/boot/head.S)
>   

Yes, that's more or less the same code, aside from using 0x40(%esi) as a
stack.  Would that be OK here?

    J

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> BTW, I should have marked this as an RFC comment, rather than an actual
> submission.  We don't need it for .22.
>
>> NAK.  
>>
>> Skipping the segment register load is likely fine.
>> Supporting V!=P at startup_32 is not.
>>   
>
> Why?
>
>> Assuming that we have a stack at startup_32 is not.
>>
>> If you want to figure out where the kernel is loaded you can do
>> (from arch/i386/boot/head.S)
>>   
>
> Yes, that's more or less the same code, aside from using 0x40(%esi) as a
> stack.  Would that be OK here?

Using 0x40 as a stack would be ok.

There are issues with CONFIG_RELOCATABLE and V!=P that I'm not
comfortable with yet, because we can't tell the difference.

Eric

Re: [PATCH] i386: always clear bss

[Jeremy Fitzhardinge]

Eric W. Biederman wrote:
> Using 0x40 as a stack would be ok.
>   

OK.

> There are issues with CONFIG_RELOCATABLE and V!=P that I'm not
> comfortable with yet, because we can't tell the difference.

But it doesn't matter in this case, does it?  It just needs to find the
current address, whether it be virtual or physical, of the bss.  It
doesn't assume any particular offset.

When does the relocation happen?   Does the bzImage loader do it as part
of decompression?  Or does the kernel do it to itself?  (Not that it
makes any difference here.)

    J

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

Jeremy Fitzhardinge <jeremy@goop.org> writes:

> Eric W. Biederman wrote:
>> Using 0x40 as a stack would be ok.
>>   
>
> OK.
>
>> There are issues with CONFIG_RELOCATABLE and V!=P that I'm not
>> comfortable with yet, because we can't tell the difference.
>
> But it doesn't matter in this case, does it?  It just needs to find the
> current address, whether it be virtual or physical, of the bss.  It
> doesn't assume any particular offset.

For the bss that sounds correct.

> When does the relocation happen?   Does the bzImage loader do it as part
> of decompression?  Or does the kernel do it to itself?  (Not that it
> makes any difference here.)

Currently right after compression just before we jump to startup_32.
But if the usage of vmlinux continues to increase we should really
move it to just after startup_32.  Which is where we run into problems
with supporting virtual addresses at our normal kernel entry point.

The relocation doesn't live just after startup_32 now because it is
hard to put there.

The practical challenge is that we need to compute the delta between
where we are at and where we were compiled for, and with possibility of
virtual address and physical address I don't know how we would compute
where we are at, in a way we could compare to our compile time physical
or virtual addresses.

Xen when it comes it at a completely isolated entry point is fine
because there is no pretence of code reuse, and we don't have to auto-detect
how we were started.

Eric

Re: [PATCH] i386: always clear bss

[H. Peter Anvin]

Jeremy Fitzhardinge wrote:
> 
> Yes, that's more or less the same code, aside from using 0x40(%esi) as a
> stack.  Would that be OK here?
> 

I saw the 0x40(%esi) stack stuff, and I'm utterly puzzled by it.  There
is no reason one can't set up %esp to point to a hunk in ordinary memory
and use it?

	-hpa

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Jeremy Fitzhardinge wrote:
>> 
>> Yes, that's more or less the same code, aside from using 0x40(%esi) as a
>> stack.  Would that be OK here?
>> 
>
> I saw the 0x40(%esi) stack stuff, and I'm utterly puzzled by it.  There
> is no reason one can't set up %esp to point to a hunk in ordinary memory
> and use it?

That is what we are doing, remind me to make certain we have this
field of the boot protocol documented as permanently reserved for
this.

This comes from the relocatable kernel patches where we run the
kernel where the bootloader chooses to put it assuming we are >= 1M.

The problem is that we don't have any IP relative data access
instructions, we don't have a stack, and so the only valid address
that we know is valid is %esi.  Once we compute where we are running
we can setup a base address register and a stack and everything is
easy, but the bootstrap to figure out where we are is just a little
tricky.

Eric

Re: [PATCH] i386: always clear bss

[H. Peter Anvin]

Eric W. Biederman wrote:
>>>
>> I saw the 0x40(%esi) stack stuff, and I'm utterly puzzled by it.  There
>> is no reason one can't set up %esp to point to a hunk in ordinary memory
>> and use it?
> 
> That is what we are doing, remind me to make certain we have this
> field of the boot protocol documented as permanently reserved for
> this.
> 
> This comes from the relocatable kernel patches where we run the
> kernel where the bootloader chooses to put it assuming we are >= 1M.
> 
> The problem is that we don't have any IP relative data access
> instructions, we don't have a stack, and so the only valid address
> that we know is valid is %esi.  Once we compute where we are running
> we can setup a base address register and a stack and everything is
> easy, but the bootstrap to figure out where we are is just a little
> tricky.

Oh, right.  And this runs with interrupts off, so you only need one
dword.  That's fine, of course, although the location is a bit awkward.

	-hpa

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Eric W. Biederman wrote:
>>>>
>>> I saw the 0x40(%esi) stack stuff, and I'm utterly puzzled by it.  There
>>> is no reason one can't set up %esp to point to a hunk in ordinary memory
>>> and use it?
>> 
>> That is what we are doing, remind me to make certain we have this
>> field of the boot protocol documented as permanently reserved for
>> this.
>> 
>> This comes from the relocatable kernel patches where we run the
>> kernel where the bootloader chooses to put it assuming we are >= 1M.
>> 
>> The problem is that we don't have any IP relative data access
>> instructions, we don't have a stack, and so the only valid address
>> that we know is valid is %esi.  Once we compute where we are running
>> we can setup a base address register and a stack and everything is
>> easy, but the bootstrap to figure out where we are is just a little
>> tricky.
>
> Oh, right.  And this runs with interrupts off, so you only need one
> dword.  That's fine, of course, although the location is a bit awkward.

Yep.  That is what I found when surveyed the available locations.

Eric

Re: [PATCH] i386: always clear bss

[H. Peter Anvin]

Eric W. Biederman wrote:
>> Oh, right.  And this runs with interrupts off, so you only need one
>> dword.  That's fine, of course, although the location is a bit awkward.
> 
> Yep.  That is what I found when surveyed the available locations.

Look at the structure definition I just posted; I think you'll find that
using something not in the immediate extension area for the video area
would have been preferrable, I would suggest 0x5c or 0x1e4.

	-hpa

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Eric W. Biederman wrote:
>>> Oh, right.  And this runs with interrupts off, so you only need one
>>> dword.  That's fine, of course, although the location is a bit awkward.
>> 
>> Yep.  That is what I found when surveyed the available locations.
>
> Look at the structure definition I just posted; I think you'll find that
> using something not in the immediate extension area for the video area
> would have been preferrable, I would suggest 0x5c or 0x1e4.

My notes show 0x5c reserved for additional apm_bios_info, although
of the top of my head I don't know how realistic that is.

0x1e4 does look available.

It has been a long time since I made that choice, and I do see that
looking at struct screen_info I did remember to document that I was
using 0x3c, even in your structure.

It is all internal to our boot process and external code isn't going
to use it so we can change it if we feel like.

Eric

Re: [PATCH] i386: always clear bss

[H. Peter Anvin]

Eric W. Biederman wrote:
> 
> My notes show 0x5c reserved for additional apm_bios_info, although
> of the top of my head I don't know how realistic that is.
> 
> 0x1e4 does look available.
> 
> It has been a long time since I made that choice, and I do see that
> looking at struct screen_info I did remember to document that I was
> using 0x3c, even in your structure.
> 
> It is all internal to our boot process and external code isn't going
> to use it so we can change it if we feel like.
> 

True.

	-hpa

Re: [PATCH] i386: always clear bss

[H. Peter Anvin]

Eric W. Biederman wrote:
> 
> My notes show 0x5c reserved for additional apm_bios_info, although
> of the top of my head I don't know how realistic that is.
> 
> 0x1e4 does look available.
> 
> It has been a long time since I made that choice, and I do see that
> looking at struct screen_info I did remember to document that I was
> using 0x3c, even in your structure.
> 
> It is all internal to our boot process and external code isn't going
> to use it so we can change it if we feel like.
> 

I don't see the actual instruction that does that anywhere in my tree,
which was branched from Andi's "for-linus" git tree, but I have reserved
0x1e4 for that purpose as "scratch".

	-hpa

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Eric W. Biederman wrote:
>> 
>> My notes show 0x5c reserved for additional apm_bios_info, although
>> of the top of my head I don't know how realistic that is.
>> 
>> 0x1e4 does look available.
>> 
>> It has been a long time since I made that choice, and I do see that
>> looking at struct screen_info I did remember to document that I was
>> using 0x3c, even in your structure.
>> 
>> It is all internal to our boot process and external code isn't going
>> to use it so we can change it if we feel like.
>> 
>
> I don't see the actual instruction that does that anywhere in my tree,
> which was branched from Andi's "for-linus" git tree, but I have reserved
> 0x1e4 for that purpose as "scratch".

I have this vague memory of liking 0x3c because if we do happen to use
more room then we intended the consequences are pretty benign.

But that is a pretty minor consequence.

Eric

Re: [PATCH] i386: always clear bss

[H. Peter Anvin]

Eric W. Biederman wrote:
> 
> I have this vague memory of liking 0x3c because if we do happen to use
> more room then we intended the consequences are pretty benign.
> 
> But that is a pretty minor consequence.
> 

That's a dangerous assumption (besides, it's likely wrong, since there
are only two unused bytes below it.)

	-hpa

Re: [PATCH] i386: always clear bss

[Eric W. Biederman]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Eric W. Biederman wrote:
>> 
>> I have this vague memory of liking 0x3c because if we do happen to use
>> more room then we intended the consequences are pretty benign.
>> 
>> But that is a pretty minor consequence.
>> 

I meant to say it is a pretty minor consideration.

> That's a dangerous assumption (besides, it's likely wrong, since there
> are only two unused bytes below it.)

Worse case on a modern machine is that we get a messed up screen.
The odds of it happening are sufficiently remote that it isn't worth
worrying about.

Eric