[PATCH v3 0/2] Clear .bss for VP guests

[PATCH v3 0/2] Clear .bss for VP guests

[Boris Ostrovsky]

PV guests need to have their .bss zeroed out since it is not guaranteed
to be cleared by Xen's domain builder

v3:
* Use existing macros for selecting mode-appropriate instructions/registers
* Use 32-bit registers in XOR
* Split removal of mode-selecting ifdefs into a separate patch

Boris Ostrovsky (2):
  xen/x86: Zero out .bss for PV guests
  xen/x86: Drop mode-selecting ifdefs in startup_xen()

 arch/x86/xen/xen-head.S | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

-- 
2.1.0

[PATCH v3 1/2] xen/x86: Zero out .bss for PV guests

[Boris Ostrovsky]

Baremetal kernels clear .bss early in the boot but Xen PV guests don't
execute that code. They have been able to run without problems because
Xen domain builder happens to give out zeroed pages. However, since this
is not really guaranteed, .bss should be explicitly cleared.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: stable@vger.kernel.org
---
 arch/x86/xen/xen-head.S | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index b65f59a..11dbe49 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -38,6 +38,20 @@
 	__INIT
 ENTRY(startup_xen)
 	cld
+
+#ifdef CONFIG_X86_32
+#define WSIZE_SHIFT	2
+#else
+#define WSIZE_SHIFT	3
+#endif
+	/* Clear .bss */
+	xor %eax,%eax
+	mov $__bss_start, %__ASM_REG(di)
+	mov $__bss_stop, %__ASM_REG(cx)
+	sub %__ASM_REG(di), %__ASM_REG(cx)
+	shr $WSIZE_SHIFT, %__ASM_REG(cx)
+	rep __ASM_SIZE(stos)
+
 #ifdef CONFIG_X86_32
 	mov %esi,xen_start_info
 	mov $init_thread_union+THREAD_SIZE,%esp
-- 
2.1.0

Re: [PATCH v3 1/2] xen/x86: Zero out .bss for PV guests

[Brian Gerst]

On Thu, Feb 25, 2016 at 10:16 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> Baremetal kernels clear .bss early in the boot but Xen PV guests don't
> execute that code. They have been able to run without problems because
> Xen domain builder happens to give out zeroed pages. However, since this
> is not really guaranteed, .bss should be explicitly cleared.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/xen/xen-head.S | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
> index b65f59a..11dbe49 100644
> --- a/arch/x86/xen/xen-head.S
> +++ b/arch/x86/xen/xen-head.S
> @@ -38,6 +38,20 @@
>         __INIT
>  ENTRY(startup_xen)
>         cld
> +
> +#ifdef CONFIG_X86_32
> +#define WSIZE_SHIFT    2
> +#else
> +#define WSIZE_SHIFT    3
> +#endif
> +       /* Clear .bss */
> +       xor %eax,%eax
> +       mov $__bss_start, %__ASM_REG(di)
> +       mov $__bss_stop, %__ASM_REG(cx)
> +       sub %__ASM_REG(di), %__ASM_REG(cx)
> +       shr $WSIZE_SHIFT, %__ASM_REG(cx)
> +       rep __ASM_SIZE(stos)
> +
>  #ifdef CONFIG_X86_32
>         mov %esi,xen_start_info
>         mov $init_thread_union+THREAD_SIZE,%esp

Better, but can still be improved.  Replace WSIZE_SHIFT with
__ASM_SEL(2, 3), and use the macros for the registers (ie. __ASM_DI).

--
Brian Gerst

[PATCH v3 2/2] xen/x86: Drop mode-selecting ifdefs in startup_xen()

[Boris Ostrovsky]

Use asm/asm.h macros instead.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/xen/xen-head.S | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 11dbe49..2366895 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -52,13 +52,9 @@ ENTRY(startup_xen)
 	shr $WSIZE_SHIFT, %__ASM_REG(cx)
 	rep __ASM_SIZE(stos)
 
-#ifdef CONFIG_X86_32
-	mov %esi,xen_start_info
-	mov $init_thread_union+THREAD_SIZE,%esp
-#else
-	mov %rsi,xen_start_info
-	mov $init_thread_union+THREAD_SIZE,%rsp
-#endif
+	mov %__ASM_REG(si), xen_start_info
+	mov $init_thread_union+THREAD_SIZE, %__ASM_REG(sp)
+
 	jmp xen_start_kernel
 
 	__FINIT
-- 
2.1.0

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Roger Pau Monné]

El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
> PV guests need to have their .bss zeroed out since it is not guaranteed
> to be cleared by Xen's domain builder

I guess I'm missing something, but elf_load_image (in libelf-loader.c)
seems to be able to clear segments (it will zero the memory between
p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
memory, so if the program headers are correctly setup the .bss should be
zeroed out AFAICT.

Roger.

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Boris Ostrovsky]

On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>> PV guests need to have their .bss zeroed out since it is not guaranteed
>> to be cleared by Xen's domain builder
> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
> seems to be able to clear segments (it will zero the memory between
> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
> memory, so if the program headers are correctly setup the .bss should be
> zeroed out AFAICT.

Right, but I don't think this is guaranteed. It's uninitialized data so 
in principle it can be anything.

The ELF spec says "the system initializes the data with zero when the 
program begins to run" which I read as it's up to runtime and not the 
loader to do so.

And since kernel does it explicitly on baremetal path I think it's a 
good idea for PV to do the same.

-boris

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Brian Gerst]

On Fri, Feb 26, 2016 at 8:51 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
> On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
>>
>> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>>>
>>> PV guests need to have their .bss zeroed out since it is not guaranteed
>>> to be cleared by Xen's domain builder
>>
>> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
>> seems to be able to clear segments (it will zero the memory between
>> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
>> memory, so if the program headers are correctly setup the .bss should be
>> zeroed out AFAICT.
>
>
> Right, but I don't think this is guaranteed. It's uninitialized data so in
> principle it can be anything.
>
> The ELF spec says "the system initializes the data with zero when the
> program begins to run" which I read as it's up to runtime and not the loader
> to do so.
>
> And since kernel does it explicitly on baremetal path I think it's a good
> idea for PV to do the same.

It does it on bare metal because bzImage is a raw binary image, not ELF.

--
Brian Gerst

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Boris Ostrovsky]

On 02/26/2016 09:42 AM, Brian Gerst wrote:
> On Fri, Feb 26, 2016 at 8:51 AM, Boris Ostrovsky
> <boris.ostrovsky@oracle.com> wrote:
>> On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
>>> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>>>> PV guests need to have their .bss zeroed out since it is not guaranteed
>>>> to be cleared by Xen's domain builder
>>> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
>>> seems to be able to clear segments (it will zero the memory between
>>> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
>>> memory, so if the program headers are correctly setup the .bss should be
>>> zeroed out AFAICT.
>>
>> Right, but I don't think this is guaranteed. It's uninitialized data so in
>> principle it can be anything.
>>
>> The ELF spec says "the system initializes the data with zero when the
>> program begins to run" which I read as it's up to runtime and not the loader
>> to do so.
>>
>> And since kernel does it explicitly on baremetal path I think it's a good
>> idea for PV to do the same.
> It does it on bare metal because bzImage is a raw binary image, not ELF.

OK, I didn't think about this.

But nevertheless, is it guaranteed that .bss is cleared by the loader? 
My reading of the spec is that it's not.


-boris

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[David Vrabel]

On 26/02/16 15:10, Boris Ostrovsky wrote:
> On 02/26/2016 09:42 AM, Brian Gerst wrote:
>> On Fri, Feb 26, 2016 at 8:51 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>> On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
>>>> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>>>>> PV guests need to have their .bss zeroed out since it is not
>>>>> guaranteed
>>>>> to be cleared by Xen's domain builder
>>>> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
>>>> seems to be able to clear segments (it will zero the memory between
>>>> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
>>>> memory, so if the program headers are correctly setup the .bss
>>>> should be
>>>> zeroed out AFAICT.
>>>
>>> Right, but I don't think this is guaranteed. It's uninitialized data
>>> so in
>>> principle it can be anything.
>>>
>>> The ELF spec says "the system initializes the data with zero when the
>>> program begins to run" which I read as it's up to runtime and not the
>>> loader
>>> to do so.
>>>
>>> And since kernel does it explicitly on baremetal path I think it's a
>>> good
>>> idea for PV to do the same.
>> It does it on bare metal because bzImage is a raw binary image, not ELF.
> 
> OK, I didn't think about this.
> 
> But nevertheless, is it guaranteed that .bss is cleared by the loader?
> My reading of the spec is that it's not.

I'm going to apply it once you post a final version with the last
suggestion from Brian.

I'll drop the tag for stable though.

David

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Roger Pau Monné]

El 26/2/16 a les 16:10, Boris Ostrovsky ha escrit:
> On 02/26/2016 09:42 AM, Brian Gerst wrote:
>> On Fri, Feb 26, 2016 at 8:51 AM, Boris Ostrovsky
>> <boris.ostrovsky@oracle.com> wrote:
>>> On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
>>>> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>>>>> PV guests need to have their .bss zeroed out since it is not
>>>>> guaranteed
>>>>> to be cleared by Xen's domain builder
>>>> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
>>>> seems to be able to clear segments (it will zero the memory between
>>>> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
>>>> memory, so if the program headers are correctly setup the .bss
>>>> should be
>>>> zeroed out AFAICT.
>>>
>>> Right, but I don't think this is guaranteed. It's uninitialized data
>>> so in
>>> principle it can be anything.
>>>
>>> The ELF spec says "the system initializes the data with zero when the
>>> program begins to run" which I read as it's up to runtime and not the
>>> loader
>>> to do so.
>>>
>>> And since kernel does it explicitly on baremetal path I think it's a
>>> good
>>> idea for PV to do the same.
>> It does it on bare metal because bzImage is a raw binary image, not ELF.
> 
> OK, I didn't think about this.
> 
> But nevertheless, is it guaranteed that .bss is cleared by the loader?
> My reading of the spec is that it's not.

I think this is very blur in general. The copy of the spec I have says:

"the system initializes the data with zeros when the program begins to run"

What is "the system" here, Xen or the guest kernel?

Just to be clear, I'm not opposing to this change in any way, but the
message in patch 1/2 needs to be fixed:

"They have been able to run without problems because Xen domain builder
happens to give out zeroed pages."

This is wrong IMHO, .bss is not cleared because we are using zeroed
pages, but because elf_load_image explicitly zeroes the space between
p_filesz and p_memsz in ELF program headers (which is were .bss resides
on properly arranged ELF binaries) when loading them.

I'm quite sure NetBSD also relies on this, so I would say it's
intrinsically part of the Xen boot ABI now, and this change just adds
seatbelts to Linux.

Roger.

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[David Vrabel]

On 26/02/16 15:22, Roger Pau Monné wrote:
> El 26/2/16 a les 16:10, Boris Ostrovsky ha escrit:
>> On 02/26/2016 09:42 AM, Brian Gerst wrote:
>>> On Fri, Feb 26, 2016 at 8:51 AM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
>>>>> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>>>>>> PV guests need to have their .bss zeroed out since it is not
>>>>>> guaranteed
>>>>>> to be cleared by Xen's domain builder
>>>>> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
>>>>> seems to be able to clear segments (it will zero the memory between
>>>>> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
>>>>> memory, so if the program headers are correctly setup the .bss
>>>>> should be
>>>>> zeroed out AFAICT.
>>>>
>>>> Right, but I don't think this is guaranteed. It's uninitialized data
>>>> so in
>>>> principle it can be anything.
>>>>
>>>> The ELF spec says "the system initializes the data with zero when the
>>>> program begins to run" which I read as it's up to runtime and not the
>>>> loader
>>>> to do so.
>>>>
>>>> And since kernel does it explicitly on baremetal path I think it's a
>>>> good
>>>> idea for PV to do the same.
>>> It does it on bare metal because bzImage is a raw binary image, not ELF.
>>
>> OK, I didn't think about this.
>>
>> But nevertheless, is it guaranteed that .bss is cleared by the loader?
>> My reading of the spec is that it's not.
> 
> I think this is very blur in general. The copy of the spec I have says:
> 
> "the system initializes the data with zeros when the program begins to run"
> 
> What is "the system" here, Xen or the guest kernel?
> 
> Just to be clear, I'm not opposing to this change in any way, but the
> message in patch 1/2 needs to be fixed:
> 
> "They have been able to run without problems because Xen domain builder
> happens to give out zeroed pages."
> 
> This is wrong IMHO, .bss is not cleared because we are using zeroed
> pages, but because elf_load_image explicitly zeroes the space between
> p_filesz and p_memsz in ELF program headers (which is were .bss resides
> on properly arranged ELF binaries) when loading them.
> 
> I'm quite sure NetBSD also relies on this, so I would say it's
> intrinsically part of the Xen boot ABI now, and this change just adds
> seatbelts to Linux.

The tools support loading bzImages, not just ELF images.

David

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Boris Ostrovsky]

On 02/26/2016 10:26 AM, David Vrabel wrote:
>
> The tools support loading bzImages, not just ELF images.

We load them as ELF images though, I believe, after uncompressing. E.g.:

static int xc_dom_load_bzimage_kernel(struct xc_dom_image *dom)
{
     return elf_loader.loader(dom);
}


-boris

Re: [Xen-devel] [PATCH v3 0/2] Clear .bss for VP guests

[Boris Ostrovsky]

On 02/26/2016 10:22 AM, Roger Pau Monné wrote:
> El 26/2/16 a les 16:10, Boris Ostrovsky ha escrit:
>> On 02/26/2016 09:42 AM, Brian Gerst wrote:
>>> On Fri, Feb 26, 2016 at 8:51 AM, Boris Ostrovsky
>>> <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/26/2016 05:53 AM, Roger Pau Monné wrote:
>>>>> El 25/2/16 a les 16:16, Boris Ostrovsky ha escrit:
>>>>>> PV guests need to have their .bss zeroed out since it is not
>>>>>> guaranteed
>>>>>> to be cleared by Xen's domain builder
>>>>> I guess I'm missing something, but elf_load_image (in libelf-loader.c)
>>>>> seems to be able to clear segments (it will zero the memory between
>>>>> p_paddr + p_filesz and p_paddr + p_memsz) while loading the ELF into
>>>>> memory, so if the program headers are correctly setup the .bss
>>>>> should be
>>>>> zeroed out AFAICT.
>>>> Right, but I don't think this is guaranteed. It's uninitialized data
>>>> so in
>>>> principle it can be anything.
>>>>
>>>> The ELF spec says "the system initializes the data with zero when the
>>>> program begins to run" which I read as it's up to runtime and not the
>>>> loader
>>>> to do so.
>>>>
>>>> And since kernel does it explicitly on baremetal path I think it's a
>>>> good
>>>> idea for PV to do the same.
>>> It does it on bare metal because bzImage is a raw binary image, not ELF.
>> OK, I didn't think about this.
>>
>> But nevertheless, is it guaranteed that .bss is cleared by the loader?
>> My reading of the spec is that it's not.
> I think this is very blur in general. The copy of the spec I have says:
>
> "the system initializes the data with zeros when the program begins to run"
>
> What is "the system" here, Xen or the guest kernel?
>
> Just to be clear, I'm not opposing to this change in any way, but the
> message in patch 1/2 needs to be fixed:
>
> "They have been able to run without problems because Xen domain builder
> happens to give out zeroed pages."
>
> This is wrong IMHO, .bss is not cleared because we are using zeroed
> pages, but because elf_load_image explicitly zeroes the space between
> p_filesz and p_memsz in ELF program headers (which is were .bss resides
> on properly arranged ELF binaries) when loading them.

That's what I meant --- that the builder/loader gives out zeroed pages, 
not that Xen's allocator clears them in general. I'll update the commit 
message.

>
> I'm quite sure NetBSD also relies on this, so I would say it's
> intrinsically part of the Xen boot ABI now, and this change just adds
> seatbelts to Linux.

Maybe NetBSD should drive carefully then ;-)

-boris