[Xen-devel] [PATCH] x86: undo part of "refine link time stub area related assertion"

[Xen-devel] [PATCH] x86: undo part of "refine link time stub area related assertion"

[Jan Beulich]

The original check was not too strict: While we don't use one page of
memory per CPU, we do use ons page of VA space per CPU. It is the
latter which matters here.

Undo that part of the change, but leave everything else in place.

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

--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -2,7 +2,6 @@
 /* Modified for i386/x86-64 Xen by Keir Fraser */
 
 #include <xen/cache.h>
-#include <xen/lib.h>
 #include <asm/page.h>
 #undef ENTRY
 #undef ALIGN
@@ -353,7 +352,7 @@ SECTIONS
 }
 
 ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
-                          DIV_ROUND_UP(NR_CPUS, STUBS_PER_PAGE) * PAGE_SIZE,
+                          NR_CPUS * PAGE_SIZE,
        "Xen image overlaps stubs area")
 
 #ifdef CONFIG_KEXEC

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

Re: [Xen-devel] [PATCH] x86: undo part of "refine link time stub area related assertion"

[Andrew Cooper]

On 29/01/2020 17:03, Jan Beulich wrote:
> The original check was not too strict: While we don't use one page of
> memory per CPU, we do use ons page of VA space per CPU. It is the

one.

> latter which matters here.
>
> Undo that part of the change, but leave everything else in place.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ok, but this begs the question why?  If the stubs are tightly packed
together, but the linear space isn't, we end up having loads of aliases
of the stubs.

There is no security benefit for doing so, but there is a real
performance cost from not compacting the linear space.  Most notably,
two threads unable to share a 4k tlb entry for their own stubs, but also
reduced cache locality of reference for the pagetables requires to map
the overly-large linear space.

~Andrew

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

Re: [Xen-devel] [PATCH] x86: undo part of "refine link time stub area related assertion"

[Jan Beulich]

On 29.01.2020 18:14, Andrew Cooper wrote:
> On 29/01/2020 17:03, Jan Beulich wrote:
>> The original check was not too strict: While we don't use one page of
>> memory per CPU, we do use ons page of VA space per CPU. It is the
> 
> one.
> 
>> latter which matters here.
>>
>> Undo that part of the change, but leave everything else in place.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Ok, but this begs the question why?  If the stubs are tightly packed
> together, but the linear space isn't, we end up having loads of aliases
> of the stubs.
> 
> There is no security benefit for doing so, but there is a real
> performance cost from not compacting the linear space.  Most notably,
> two threads unable to share a 4k tlb entry for their own stubs, but also
> reduced cache locality of reference for the pagetables requires to map
> the overly-large linear space.

The idea, iirc, was to make the addresses even more easily
recognizable this way, in particular in the case a stub was
referenced for a CPU that was taken offline. But yes, this isn't
an overly big win in this regard, so perhaps the arrangement
could be revised (looking over the code there don't look to be
any other dependencies on this layout). Until then though the
assertion should be fixed, as right now it is clearly
insufficient.

Jan

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

Re: [Xen-devel] [PATCH] x86: undo part of "refine link time stub area related assertion"

[Andrew Cooper]

On 30/01/2020 08:09, Jan Beulich wrote:
> On 29.01.2020 18:14, Andrew Cooper wrote:
>> On 29/01/2020 17:03, Jan Beulich wrote:
>>> The original check was not too strict: While we don't use one page of
>>> memory per CPU, we do use ons page of VA space per CPU. It is the
>> one.
>>
>>> latter which matters here.
>>>
>>> Undo that part of the change, but leave everything else in place.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Ok, but this begs the question why?  If the stubs are tightly packed
>> together, but the linear space isn't, we end up having loads of aliases
>> of the stubs.
>>
>> There is no security benefit for doing so, but there is a real
>> performance cost from not compacting the linear space.  Most notably,
>> two threads unable to share a 4k tlb entry for their own stubs, but also
>> reduced cache locality of reference for the pagetables requires to map
>> the overly-large linear space.
> The idea, iirc, was to make the addresses even more easily
> recognizable this way, in particular in the case a stub was
> referenced for a CPU that was taken offline. But yes, this isn't
> an overly big win in this regard, so perhaps the arrangement
> could be revised (looking over the code there don't look to be
> any other dependencies on this layout). Until then though the
> assertion should be fixed, as right now it is clearly
> insufficient.

For the patch, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Improvements can be done at a later point.

~Andrew

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