[PATCH 0/6] xen/x86: Misc x86 improvements

[PATCH 0/6] xen/x86: Misc x86 improvements

[Andrew Cooper]

This series is some preparity cleanup to allow the Xen text/data/bss regions
to be mapped with superpages rather than small pages.

There are more areas needing to be tweaked before superpages can be used (pcpu
0's stack being in the bss, .init being between .data and .bss, probably
more), but these improvements are ready to go.

Andrew Cooper (6):
  xen/x86: Discard the alternatives ".discard" sections
  x86/numa: Correct the extern of cpu_to_node
  x86/smp: Clean up use of memflags in cpu_smpboot_alloc()
  x86/link: Introduce and use __bss_end
  x86/smp: Allocate pcpu stacks on their local numa node
  x86/boot: Ensure the BSS is aligned on an 8 byte boundary

 xen/arch/x86/boot/head.S   |    5 +++--
 xen/arch/x86/setup.c       |    4 ++--
 xen/arch/x86/smpboot.c     |   16 +++++++++-------
 xen/arch/x86/tboot.c       |    4 ++--
 xen/arch/x86/xen.lds.S     |    5 +++++
 xen/include/asm-x86/numa.h |    2 +-
 6 files changed, 22 insertions(+), 14 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] x86/link: Discard the alternatives ".discard" sections

[Andrew Cooper]

This appears to have been missed when porting the alternatives framework from
Linux, and saves us a section which is otherwise loaded into memory.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/xen.lds.S |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d4b1f1a..c854181 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -197,6 +197,8 @@ SECTIONS
        *(.exit.text)
        *(.exit.data)
        *(.exitcall.exit)
+       *(.discard)
+       *(.discard.*)
        *(.eh_frame)
 #ifdef EFI
        *(.comment)
-- 
1.7.10.4

Re: [PATCH 1/6] x86/link: Discard the alternatives ".discard" sections

[Tim Deegan]

At 18:26 +0100 on 07 Apr (1428431175), Andrew Cooper wrote:
> This appears to have been missed when porting the alternatives framework from
> Linux, and saves us a section which is otherwise loaded into memory.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Tim Deegan <tim@xen.org>

[PATCH 2/6] x86/numa: Correct the extern of cpu_to_node

[Andrew Cooper]

This was missed by c/s 54ce2db "x86/numa: adjust datatypes for node and pxm"
which changed the array definition in numa.c

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/include/asm-x86/numa.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index 7a489d3..0c5e5b4 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -9,7 +9,7 @@
 
 extern int srat_rev;
 
-extern unsigned char cpu_to_node[];
+extern nodeid_t      cpu_to_node[NR_CPUS];
 extern cpumask_t     node_to_cpumask[];
 
 #define cpu_to_node(cpu)		(cpu_to_node[cpu])
-- 
1.7.10.4

Re: [PATCH 2/6] x86/numa: Correct the extern of cpu_to_node

[Dario Faggioli]

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

On Tue, 2015-04-07 at 18:26 +0100, Andrew Cooper wrote:
> This was missed by c/s 54ce2db "x86/numa: adjust datatypes for node and pxm"
> which changed the array definition in numa.c
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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

Re: [PATCH 2/6] x86/numa: Correct the extern of cpu_to_node

[Tim Deegan]

At 18:26 +0100 on 07 Apr (1428431176), Andrew Cooper wrote:
> --- a/xen/include/asm-x86/numa.h
> +++ b/xen/include/asm-x86/numa.h
> @@ -9,7 +9,7 @@
>  
>  extern int srat_rev;
>  
> -extern unsigned char cpu_to_node[];
> +extern nodeid_t      cpu_to_node[NR_CPUS];

Does the compiler do anything useful with the array size here? In
particular does it check that it matches the size at the definition?
If not, I think it's best to leave it as '[]'.

Cheers,

Tim.

Re: [PATCH 2/6] x86/numa: Correct the extern of cpu_to_node

[Andrew Cooper]

On 09/04/15 16:00, Tim Deegan wrote:
> At 18:26 +0100 on 07 Apr (1428431176), Andrew Cooper wrote:
>> --- a/xen/include/asm-x86/numa.h
>> +++ b/xen/include/asm-x86/numa.h
>> @@ -9,7 +9,7 @@
>>  
>>  extern int srat_rev;
>>  
>> -extern unsigned char cpu_to_node[];
>> +extern nodeid_t      cpu_to_node[NR_CPUS];
> Does the compiler do anything useful with the array size here?

Specifying the size allows ARRAY_SIZE(cpu_to_node) to work in other
translation units.  It also allows static analysers to perform bounds
checks, should they wish.

> In particular does it check that it matches the size at the definition?

It will complain if they are mismatched.

~Andrew

Re: [PATCH 2/6] x86/numa: Correct the extern of cpu_to_node

[Tim Deegan]

At 16:05 +0100 on 09 Apr (1428595536), Andrew Cooper wrote:
> On 09/04/15 16:00, Tim Deegan wrote:
> > At 18:26 +0100 on 07 Apr (1428431176), Andrew Cooper wrote:
> >> --- a/xen/include/asm-x86/numa.h
> >> +++ b/xen/include/asm-x86/numa.h
> >> @@ -9,7 +9,7 @@
> >>  
> >>  extern int srat_rev;
> >>  
> >> -extern unsigned char cpu_to_node[];
> >> +extern nodeid_t      cpu_to_node[NR_CPUS];
> > Does the compiler do anything useful with the array size here?
> 
> Specifying the size allows ARRAY_SIZE(cpu_to_node) to work in other
> translation units.  It also allows static analysers to perform bounds
> checks, should they wish.
> 
> > In particular does it check that it matches the size at the definition?
> 
> It will complain if they are mismatched.

Excellent.  In that case,  Reviewed-by: Tim Deegan <tim@xen.org>

Tim.

[PATCH 3/6] x86/smp: Clean up use of memflags in cpu_smpboot_alloc()

[Andrew Cooper]

Hoist MEMF_node(cpu_to_node(cpu)) to the start of the function, and avoid
passing (potentially bogus) memflags if node information is not available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/smpboot.c |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index d3fe116..a009e91 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -635,32 +635,34 @@ static void cpu_smpboot_free(unsigned int cpu)
 
 static int cpu_smpboot_alloc(unsigned int cpu)
 {
-    unsigned int order;
+    unsigned int order, memflags = 0;
+    nodeid_t node = cpu_to_node(cpu);
     struct desc_struct *gdt;
 
+    if ( node != NUMA_NO_NODE )
+        memflags = MEMF_node(node);
+
     stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, 0);
     if ( stack_base[cpu] == NULL )
         goto oom;
     memguard_guard_stack(stack_base[cpu]);
 
     order = get_order_from_pages(NR_RESERVED_GDT_PAGES);
-    per_cpu(gdt_table, cpu) = gdt =
-        alloc_xenheap_pages(order, MEMF_node(cpu_to_node(cpu)));
+    per_cpu(gdt_table, cpu) = gdt = alloc_xenheap_pages(order, memflags);
     if ( gdt == NULL )
         goto oom;
     memcpy(gdt, boot_cpu_gdt_table, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     BUILD_BUG_ON(NR_CPUS > 0x10000);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
-    per_cpu(compat_gdt_table, cpu) = gdt =
-        alloc_xenheap_pages(order, MEMF_node(cpu_to_node(cpu)));
+    per_cpu(compat_gdt_table, cpu) = gdt = alloc_xenheap_pages(order, memflags);
     if ( gdt == NULL )
         goto oom;
     memcpy(gdt, boot_cpu_compat_gdt_table, NR_RESERVED_GDT_PAGES * PAGE_SIZE);
     gdt[PER_CPU_GDT_ENTRY - FIRST_RESERVED_GDT_ENTRY].a = cpu;
 
     order = get_order_from_bytes(IDT_ENTRIES * sizeof(idt_entry_t));
-    idt_tables[cpu] = alloc_xenheap_pages(order, MEMF_node(cpu_to_node(cpu)));
+    idt_tables[cpu] = alloc_xenheap_pages(order, memflags);
     if ( idt_tables[cpu] == NULL )
         goto oom;
     memcpy(idt_tables[cpu], idt_table, IDT_ENTRIES * sizeof(idt_entry_t));
-- 
1.7.10.4

Re: [PATCH 3/6] x86/smp: Clean up use of memflags in cpu_smpboot_alloc()

[Dario Faggioli]

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

On Tue, 2015-04-07 at 18:26 +0100, Andrew Cooper wrote:
> Hoist MEMF_node(cpu_to_node(cpu)) to the start of the function, and avoid
> passing (potentially bogus) memflags if node information is not available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
>
FWIW,

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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

Re: [PATCH 3/6] x86/smp: Clean up use of memflags in cpu_smpboot_alloc()

[Tim Deegan]

At 18:26 +0100 on 07 Apr (1428431177), Andrew Cooper wrote:
> Hoist MEMF_node(cpu_to_node(cpu)) to the start of the function, and avoid
> passing (potentially bogus) memflags if node information is not available.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

As it happens, MEMF_node(NUMA_NO_NODE) is already == 0.  I'm not sure
if that's by design or not, but this looks robuster. :)

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH 3/6] x86/smp: Clean up use of memflags in cpu_smpboot_alloc()

[Andrew Cooper]

On 09/04/15 16:02, Tim Deegan wrote:
> At 18:26 +0100 on 07 Apr (1428431177), Andrew Cooper wrote:
>> Hoist MEMF_node(cpu_to_node(cpu)) to the start of the function, and avoid
>> passing (potentially bogus) memflags if node information is not available.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> As it happens, MEMF_node(NUMA_NO_NODE) is already == 0.

Only because of a masked overflow.  That is why (potentially) is in
brackets.

> I'm not sure if that's by design or not, but this looks robuster. :)

Indeed.

>
> Reviewed-by: Tim Deegan <tim@xen.org>

~Andrew

[PATCH 4/6] x86/link: Introduce and use __bss_end

[Andrew Cooper]

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/boot/head.S |    2 +-
 xen/arch/x86/setup.c     |    4 ++--
 xen/arch/x86/tboot.c     |    4 ++--
 xen/arch/x86/xen.lds.S   |    1 +
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index c99f739..2d0e56c 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -124,7 +124,7 @@ __start:
 
         /* Initialize BSS (no nasty surprises!) */
         mov     $sym_phys(__bss_start),%edi
-        mov     $sym_phys(_end),%ecx
+        mov     $sym_phys(__bss_end),%ecx
         sub     %edi,%ecx
         xor     %eax,%eax
         rep     stosb
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 89e4827..2b9787a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -185,7 +185,7 @@ static void free_xen_data(char *s, char *e)
     memguard_guard_range(__va(__pa(s)), e-s);
 }
 
-extern char __init_begin[], __init_end[], __bss_start[];
+extern char __init_begin[], __init_end[], __bss_start[], __bss_end[];
 
 static void __init init_idle_domain(void)
 {
@@ -1497,7 +1497,7 @@ int __hwdom_init xen_in_range(unsigned long mfn)
         xen_regions[region_text].e = __pa(&__init_begin);
         /* bss */
         xen_regions[region_bss].s = __pa(&__bss_start);
-        xen_regions[region_bss].e = __pa(&_end);
+        xen_regions[region_bss].e = __pa(&__bss_end);
     }
 
     start = (paddr_t)mfn << PAGE_SHIFT;
diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index 7b95ad3..01b9530 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -48,7 +48,7 @@
 #define TXTCR_HEAP_BASE             0x0300
 #define TXTCR_HEAP_SIZE             0x0308
 
-extern char __init_begin[], __bss_start[];
+extern char __init_begin[], __bss_start[], __bss_end[];
 
 #define SHA1_SIZE      20
 typedef uint8_t   sha1_hash_t[SHA1_SIZE];
@@ -374,7 +374,7 @@ void tboot_shutdown(uint32_t shutdown_type)
                                               __pa(&_stext);
         /* bss */
         g_tboot_shared->mac_regions[2].start = (uint64_t)__pa(&__bss_start);
-        g_tboot_shared->mac_regions[2].size = __pa(&_end) - __pa(&__bss_start);
+        g_tboot_shared->mac_regions[2].size = __pa(&__bss_end) - __pa(&__bss_start);
 
         /*
          * MAC domains and other Xen memory
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index c854181..4699a04 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -175,6 +175,7 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       __bss_end = .;
   } :text
   _end = . ;
 
-- 
1.7.10.4

Re: [PATCH 4/6] x86/link: Introduce and use __bss_end

[Tim Deegan]

At 18:26 +0100 on 07 Apr (1428431178), Andrew Cooper wrote:
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Tim Deegan <tim@xen.org>

[PATCH 5/6] x86/smp: Allocate pcpu stacks on their local numa node

[Andrew Cooper]

Previously, all pcpu stacks tended to be allocated on node 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>

---
I have not done any performance, but this is obviously an improvement.
---
 xen/arch/x86/smpboot.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index a009e91..116c8f8 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -642,7 +642,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
     if ( node != NUMA_NO_NODE )
         memflags = MEMF_node(node);
 
-    stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, 0);
+    stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, memflags);
     if ( stack_base[cpu] == NULL )
         goto oom;
     memguard_guard_stack(stack_base[cpu]);
-- 
1.7.10.4

Re: [PATCH 5/6] x86/smp: Allocate pcpu stacks on their local numa node

[Dario Faggioli]

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

On Tue, 2015-04-07 at 18:26 +0100, Andrew Cooper wrote:
> Previously, all pcpu stacks tended to be allocated on node 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> 
Again, FWIW:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Out of curiosity...

>  xen/arch/x86/smpboot.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index a009e91..116c8f8 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -642,7 +642,7 @@ static int cpu_smpboot_alloc(unsigned int cpu)
>      if ( node != NUMA_NO_NODE )
>          memflags = MEMF_node(node);
>  
> -    stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, 0);
> +    stack_base[cpu] = alloc_xenheap_pages(STACK_ORDER, memflags);
>
... I wonder how/why this was '0', while all the other were already
using MEMF_node(cpu_to_node)...

:-O

Regards,
Dario

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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

Re: [PATCH 5/6] x86/smp: Allocate pcpu stacks on their local numa node

[Tim Deegan]

At 18:26 +0100 on 07 Apr (1428431179), Andrew Cooper wrote:
> Previously, all pcpu stacks tended to be allocated on node 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Tim Deegan <tim@xen.org>

[PATCH 6/6] x86/boot: Ensure the BSS is aligned on an 8 byte boundary

[Andrew Cooper]

... and zero it more efficiently.

A diff of the resulting binaries shows that the size of the BSS doesn't
actually change from the extra ALIGN()s, but they do guarantee that it is safe
to clear in an more efficient manner than 1 byte at a time.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/boot/head.S |    3 ++-
 xen/arch/x86/xen.lds.S   |    2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 2d0e56c..b5a1d45 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -127,7 +127,8 @@ __start:
         mov     $sym_phys(__bss_end),%ecx
         sub     %edi,%ecx
         xor     %eax,%eax
-        rep     stosb
+        shr     $2,%ecx
+        rep     stosl
 
         /* Interrogate CPU extended features via CPUID. */
         mov     $0x80000000,%eax
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 4699a04..b1926e3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -163,6 +163,7 @@ SECTIONS
   __init_end = .;
 
   .bss : {                     /* BSS */
+       . = ALIGN(8);
        __bss_start = .;
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
@@ -175,6 +176,7 @@ SECTIONS
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       . = ALIGN(8);
        __bss_end = .;
   } :text
   _end = . ;
-- 
1.7.10.4

Re: [PATCH 6/6] x86/boot: Ensure the BSS is aligned on an 8 byte boundary

[Tim Deegan]

At 18:26 +0100 on 07 Apr (1428431180), Andrew Cooper wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -127,7 +127,8 @@ __start:
>          mov     $sym_phys(__bss_end),%ecx
>          sub     %edi,%ecx
>          xor     %eax,%eax
> -        rep     stosb
> +        shr     $2,%ecx
> +        rep     stosl

Should this be shr $3 and stosq?  You are aligning to 8 bytes in the
linker runes.

>  
>          /* Interrogate CPU extended features via CPUID. */
>          mov     $0x80000000,%eax
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 4699a04..b1926e3 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -163,6 +163,7 @@ SECTIONS
>    __init_end = .;
>  
>    .bss : {                     /* BSS */
> +       . = ALIGN(8);

Here, we're already aligned to STACK_SIZE, which the
.bss.stack_aligned just below is relying on.  So on the one hand this
new alignment comment is sort-of-harmless, but on the other hand it
distracts from the larger and more important alignment.

Cheers,

Tim.

>         __bss_start = .;
>         *(.bss.stack_aligned)
>         . = ALIGN(PAGE_SIZE);
> @@ -175,6 +176,7 @@ SECTIONS
>         *(.bss.percpu.read_mostly)
>         . = ALIGN(SMP_CACHE_BYTES);
>         __per_cpu_data_end = .;
> +       . = ALIGN(8);
>         __bss_end = .;
>    } :text
>    _end = . ;
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 6/6] x86/boot: Ensure the BSS is aligned on an 8 byte boundary

[Andrew Cooper]

On 09/04/15 16:15, Tim Deegan wrote:
> At 18:26 +0100 on 07 Apr (1428431180), Andrew Cooper wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -127,7 +127,8 @@ __start:
>>          mov     $sym_phys(__bss_end),%ecx
>>          sub     %edi,%ecx
>>          xor     %eax,%eax
>> -        rep     stosb
>> +        shr     $2,%ecx
>> +        rep     stosl
> Should this be shr $3 and stosq?  You are aligning to 8 bytes in the
> linker runes.

It is still 32bit code here, so no stosq available.

I do however happen to know that the impending multiboot2 entry point is
64bit and is able to clear the BSS with stosq.

>
>>  
>>          /* Interrogate CPU extended features via CPUID. */
>>          mov     $0x80000000,%eax
>> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
>> index 4699a04..b1926e3 100644
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -163,6 +163,7 @@ SECTIONS
>>    __init_end = .;
>>  
>>    .bss : {                     /* BSS */
>> +       . = ALIGN(8);
> Here, we're already aligned to STACK_SIZE

So we are - that should be fixed up.

That alignment is not relevant to .init, but is relevant to .bss

> , which the
> .bss.stack_aligned just below is relying on.  So on the one hand this
> new alignment comment is sort-of-harmless, but on the other hand it
> distracts from the larger and more important alignment.

I will see about fixing this up differently, but with the same overall
effect that stosl/stosq can be used.

~Andrew

Re: [PATCH 6/6] x86/boot: Ensure the BSS is aligned on an 8 byte boundary

[Tim Deegan]

At 16:34 +0100 on 09 Apr (1428597298), Andrew Cooper wrote:
> On 09/04/15 16:15, Tim Deegan wrote:
> > At 18:26 +0100 on 07 Apr (1428431180), Andrew Cooper wrote:
> >> --- a/xen/arch/x86/boot/head.S
> >> +++ b/xen/arch/x86/boot/head.S
> >> @@ -127,7 +127,8 @@ __start:
> >>          mov     $sym_phys(__bss_end),%ecx
> >>          sub     %edi,%ecx
> >>          xor     %eax,%eax
> >> -        rep     stosb
> >> +        shr     $2,%ecx
> >> +        rep     stosl
> > Should this be shr $3 and stosq?  You are aligning to 8 bytes in the
> > linker runes.
> 
> It is still 32bit code here, so no stosq available.

Fair enough. :)

> I do however happen to know that the impending multiboot2 entry point is
> 64bit and is able to clear the BSS with stosq.

OK.

> >>          /* Interrogate CPU extended features via CPUID. */
> >>          mov     $0x80000000,%eax
> >> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> >> index 4699a04..b1926e3 100644
> >> --- a/xen/arch/x86/xen.lds.S
> >> +++ b/xen/arch/x86/xen.lds.S
> >> @@ -163,6 +163,7 @@ SECTIONS
> >>    __init_end = .;
> >>  
> >>    .bss : {                     /* BSS */
> >> +       . = ALIGN(8);
> > Here, we're already aligned to STACK_SIZE
> 
> So we are - that should be fixed up.
> 
> That alignment is not relevant to .init, but is relevant to .bss

Yeah, I'm not sure whether it's a problem if __init_end != .bss; if
not the alignment could just be moved down a bit.

Cheers,

Tim.