No functional change, but it performs a bit better.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over metadata changes.
---
 xen/arch/x86/x86_64/kexec_reloc.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/x86/x86_64/kexec_reloc.S b/xen/arch/x86/x86_64/kexec_reloc.S
index 9f40c80d7c4b..50ba454abd48 100644
--- a/xen/arch/x86/x86_64/kexec_reloc.S
+++ b/xen/arch/x86/x86_64/kexec_reloc.S
@@ -19,6 +19,7 @@
 #include <xen/kimage.h>
 
 #include <asm/asm_defns.h>
+#include <asm/cache.h>
 #include <asm/msr-index.h>
 #include <asm/page.h>
 #include <asm/machine_kexec.h>
@@ -174,6 +175,9 @@ FUNC_LOCAL(compatibility_mode)
         ud2
 END(compatibility_mode)
 
+        /* Separate code and data into into different cache lines */
+        .balign L1_CACHE_BYTES
+
 DATA_LOCAL(compat_mode_gdt_desc, 4)
         .word .Lcompat_mode_gdt_end - compat_mode_gdt -1
         .quad 0x0000000000000000     /* set in call_32_bit above */
-- 
2.39.5

On 02.10.2024 12:30, Andrew Cooper wrote:
> No functional change, but it performs a bit better.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

A question nevertheless:

> --- a/xen/arch/x86/x86_64/kexec_reloc.S
> +++ b/xen/arch/x86/x86_64/kexec_reloc.S
> @@ -19,6 +19,7 @@
>  #include <xen/kimage.h>
>  
>  #include <asm/asm_defns.h>
> +#include <asm/cache.h>
>  #include <asm/msr-index.h>
>  #include <asm/page.h>
>  #include <asm/machine_kexec.h>
> @@ -174,6 +175,9 @@ FUNC_LOCAL(compatibility_mode)
>          ud2
>  END(compatibility_mode)
>  
> +        /* Separate code and data into into different cache lines */
> +        .balign L1_CACHE_BYTES
> +
>  DATA_LOCAL(compat_mode_gdt_desc, 4)
>          .word .Lcompat_mode_gdt_end - compat_mode_gdt -1
>          .quad 0x0000000000000000     /* set in call_32_bit above */

Because of L1_CACHE_BYTES being 128, you indeed put at least 1 cache line in
between. Is that necessary, though? Just starting data on the next cache line
ought to be enough? IOW if and when we adjust L1_CACHE_BYTES, we won't need
to touch this again, just that the title here then would end up slightly
misleading.

Jan

On 02/10/2024 1:08 pm, Jan Beulich wrote:
> On 02.10.2024 12:30, Andrew Cooper wrote:
>> No functional change, but it performs a bit better.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> A question nevertheless:
>
>> --- a/xen/arch/x86/x86_64/kexec_reloc.S
>> +++ b/xen/arch/x86/x86_64/kexec_reloc.S
>> @@ -19,6 +19,7 @@
>>  #include <xen/kimage.h>
>>  
>>  #include <asm/asm_defns.h>
>> +#include <asm/cache.h>
>>  #include <asm/msr-index.h>
>>  #include <asm/page.h>
>>  #include <asm/machine_kexec.h>
>> @@ -174,6 +175,9 @@ FUNC_LOCAL(compatibility_mode)
>>          ud2
>>  END(compatibility_mode)
>>  
>> +        /* Separate code and data into into different cache lines */
>> +        .balign L1_CACHE_BYTES
>> +
>>  DATA_LOCAL(compat_mode_gdt_desc, 4)
>>          .word .Lcompat_mode_gdt_end - compat_mode_gdt -1
>>          .quad 0x0000000000000000     /* set in call_32_bit above */
> Because of L1_CACHE_BYTES being 128, you indeed put at least 1 cache line in
> between. Is that necessary, though? Just starting data on the next cache line
> ought to be enough?

Correct.  Specifically, we want the write into compat_mode_gdt_desc not
hitting a line in L1i.

> IOW if and when we adjust L1_CACHE_BYTES, we won't need
> to touch this again, just that the title here then would end up slightly
> misleading.

Oh - I'll just copy the comment and say "different".  That is slightly
poor grammar.

~Andrew