[Xen-devel] [PATCH v3 0/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Xen-devel] [PATCH v3 0/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Andrew Cooper]

Fix data leakage caused by insufficient linker level alignment.

Andrew Cooper (2):
  xen/link: Introduce .bss.percpu.page_aligned
  x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

 xen/arch/arm/xen.lds.S          |  7 +++++--
 xen/arch/x86/setup.c            |  3 ---
 xen/arch/x86/traps.c            |  6 ++++++
 xen/arch/x86/xen.lds.S          |  9 +++++++--
 xen/include/asm-arm/percpu.h    |  6 ++----
 xen/include/asm-x86/percpu.h    |  6 ++----
 xen/include/asm-x86/processor.h |  4 ++--
 xen/include/xen/percpu.h        | 10 ++++++++--
 8 files changed, 32 insertions(+), 19 deletions(-)

-- 
2.11.0


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

[Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

Future changes are going to need to page align some percpu data.

Shuffle the exact link order of items within the BSS to give
.bss.percpu.page_aligned appropriate alignment, even on CPU0, which uses
.bss.percpu itself.

Insert explicit alignment such that the result is safe even with objects
shorter than a page in length.  The POINTER_ALIGN for __bss_end is to cover
the lack of SMP_CACHE_BYTES alignment, as the loops which zero the BSS use
pointer-sized stores on all architectures.

In addition, we need to be able to specify an alignment attribute to
__DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
that it is now possible to grep for .bss.percpu and find all the users.

Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which uses both section and
alignment attributes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>

A sample build including the subsequent patch is now:

ffff82d08092d000 B zero_page
ffff82d08092e000 B per_cpu__init_tss
ffff82d08092e000 B __per_cpu_start
ffff82d08092f000 B per_cpu__cpupool
ffff82d08092f008 b per_cpu__continue_info
ffff82d08092f010 b per_cpu__grant_rwlock

which demonstrates the correct alignment of data in .bss.percpu even when
following a non-page-sized object in .bss.percpu.page_aligned.

v3:
 * Insert explicit alignment.
 * Reduce __bss_end's alignment to just POINTER_ALIGN.

v2:
 * Rework __DEFINE_PER_CPU() to allow for further attributes to be passed.
 * Specify __aligned(PAGE_SIZE) as part of DEFINE_PER_CPU_PAGE_ALIGNED().
---
 xen/arch/arm/xen.lds.S       |  7 +++++--
 xen/arch/x86/xen.lds.S       |  7 +++++--
 xen/include/asm-arm/percpu.h |  6 ++----
 xen/include/asm-x86/percpu.h |  6 ++----
 xen/include/xen/percpu.h     | 10 ++++++++--
 5 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 12c107f45d..cc27131d5e 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -201,14 +201,17 @@ SECTIONS
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
        *(.bss.page_aligned)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
+       . = ALIGN(PAGE_SIZE);
        __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
+       . = ALIGN(PAGE_SIZE);
        *(.bss.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
   _end = . ;
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index a73139cd29..3bf21975a2 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -293,14 +293,17 @@ SECTIONS
        __bss_start = .;
        *(.bss.stack_aligned)
        *(.bss.page_aligned*)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
+       . = ALIGN(PAGE_SIZE);
        __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
+       . = ALIGN(PAGE_SIZE);
        *(.bss.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
   _end = . ;
diff --git a/xen/include/asm-arm/percpu.h b/xen/include/asm-arm/percpu.h
index 9584b830d4..264120b192 100644
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -10,10 +10,8 @@ extern char __per_cpu_start[], __per_cpu_data_end[];
 extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
 
 #define per_cpu(var, cpu)  \
     (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
diff --git a/xen/include/asm-x86/percpu.h b/xen/include/asm-x86/percpu.h
index ff34dc7897..5b6cef04c4 100644
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 #endif
 
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
 
 /* var is in discarded region: offset to particular copy we want */
 #define per_cpu(var, cpu)  \
diff --git a/xen/include/xen/percpu.h b/xen/include/xen/percpu.h
index aeec5c19d6..71a31cc361 100644
--- a/xen/include/xen/percpu.h
+++ b/xen/include/xen/percpu.h
@@ -9,9 +9,15 @@
  * The _##name concatenation is being used here to prevent 'name' from getting
  * macro expanded, while still allowing a per-architecture symbol name prefix.
  */
-#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
+#define DEFINE_PER_CPU(type, name) \
+    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
+
+#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
+    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned") \
+                     __aligned(PAGE_SIZE), type, _ ## name)
+
 #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
-	__DEFINE_PER_CPU(type, _##name, .read_mostly)
+    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)
 
 #define get_per_cpu_var(var)  (per_cpu__##var)
 
-- 
2.11.0


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

[Xen-devel] [PATCH v3 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Andrew Cooper]

The XPTI work restricted the visibility of most of memory, but missed a few
aspects when it came to the TSS.

Given that the TSS is just an object in percpu data, the 4k mapping for it
created in setup_cpu_root_pgt() maps adjacent percpu data, making it all
leakable via Meltdown, even when XPTI is in use.

Furthermore, no care is taken to check that the TSS doesn't cross a page
boundary.  As it turns out, struct tss_struct is aligned on its size which
does prevent it straddling a page boundary.

Move the TSS into the page aligned percpu area, so no adjacent data can be
leaked.  Move the definition from setup.c to traps.c, which is a more
appropriate place for it to live.

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

v3:
 * Drop the remark about CET.  It is no longer accurate in the latest version
   of the CET spec.

v2:
 * Rebase over changes to include __aligned() within
   DEFINE_PER_CPU_PAGE_ALIGNED()
 * Drop now-unused xen/percpu.h from setup.c
---
 xen/arch/x86/setup.c            | 3 ---
 xen/arch/x86/traps.c            | 6 ++++++
 xen/arch/x86/xen.lds.S          | 2 ++
 xen/include/asm-x86/processor.h | 4 ++--
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index d2011910fa..f9d38155d3 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -16,7 +16,6 @@
 #include <xen/domain_page.h>
 #include <xen/version.h>
 #include <xen/gdbstub.h>
-#include <xen/percpu.h>
 #include <xen/hypercall.h>
 #include <xen/keyhandler.h>
 #include <xen/numa.h>
@@ -100,8 +99,6 @@ unsigned long __read_mostly xen_phys_start;
 
 unsigned long __read_mostly xen_virt_end;
 
-DEFINE_PER_CPU(struct tss_struct, init_tss);
-
 char __section(".bss.stack_aligned") __aligned(STACK_SIZE)
     cpu0_stack[STACK_SIZE];
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 38d12013db..de3ac135f5 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -108,6 +108,12 @@ idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
 /* Pointer to the IDT of every CPU. */
 idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
 
+/*
+ * The TSS is smaller than a page, but we give it a full page to avoid
+ * adjacent per-cpu data leaking via Meltdown when XPTI is in use.
+ */
+DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_struct, init_tss);
+
 bool (*ioemul_handle_quirk)(
     u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 3bf21975a2..2732f30be5 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -370,6 +370,8 @@ ASSERT(IS_ALIGNED(__2M_rwdata_end,   SECTION_ALIGN), "__2M_rwdata_end misaligned
 
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
 
+ASSERT(IS_ALIGNED(per_cpu__init_tss, PAGE_SIZE), "per_cpu(init_tss) misaligned")
+
 ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned")
 ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZE), "__init_end misaligned")
 
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 2862321eee..b5bee94931 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
 #define IOBMP_BYTES             8192
 #define IOBMP_INVALID_OFFSET    0x8000
 
-struct __packed __cacheline_aligned tss_struct {
+struct __packed tss_struct {
     uint32_t :32;
     uint64_t rsp0, rsp1, rsp2;
     uint64_t :64;
@@ -425,6 +425,7 @@ struct __packed __cacheline_aligned tss_struct {
     /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
     uint8_t __cacheline_filler[24];
 };
+DECLARE_PER_CPU(struct tss_struct, init_tss);
 
 #define IST_NONE 0UL
 #define IST_DF   1UL
@@ -463,7 +464,6 @@ static inline void disable_each_ist(idt_entry_t *idt)
 extern idt_entry_t idt_table[];
 extern idt_entry_t *idt_tables[];
 
-DECLARE_PER_CPU(struct tss_struct, init_tss);
 DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
 
 extern void write_ptbase(struct vcpu *v);
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich]

On 29.07.2019 19:38, Andrew Cooper wrote:
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -293,14 +293,17 @@ SECTIONS
>         __bss_start = .;
>         *(.bss.stack_aligned)
>         *(.bss.page_aligned*)
> -       *(.bss)
> -       . = ALIGN(SMP_CACHE_BYTES);
> +       . = ALIGN(PAGE_SIZE);
>         __per_cpu_start = .;
> +       *(.bss.percpu.page_aligned)
> +       . = ALIGN(PAGE_SIZE);

But this goes too far: What we want is for the TSS to occupy a full
page, not for whatever random other page-aligned object ends up
last here (should any every appear). If you want to effect this
via linker script (rather than C arrangements), then just like the
CPU0 stack I think we want a dedicated section containing _just_
the TSS. (Having said that I realize that .bss.stack_aligned isn't
really a good name for a dedicated section. It's just that we
obviously want the stack to occupy its entire STACK_SIZE range. Of
course we could put ourselves on the position that
.bss.percpu.page_aligned too _is_ a dedicated section despite its
name, but if you mean it to be like that then it would be nice for
the description to actually say so.)

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

Re: [Xen-devel] [PATCH v3 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Jan Beulich]

On 29.07.2019 19:38, Andrew Cooper wrote:
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -108,6 +108,12 @@ idt_entry_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
>   /* Pointer to the IDT of every CPU. */
>   idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
>   
> +/*
> + * The TSS is smaller than a page, but we give it a full page to avoid
> + * adjacent per-cpu data leaking via Meltdown when XPTI is in use.
> + */
> +DEFINE_PER_CPU_PAGE_ALIGNED(struct tss_struct, init_tss);

I assume there's a reason why you didn't introduce a wrapper
union to pad this to page size - I'd like to understand this
reason (see also my reply to patch 1) before acking both
patches.

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

Re: [Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

On 30/07/2019 09:42, Jan Beulich wrote:
> On 29.07.2019 19:38, Andrew Cooper wrote:
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -293,14 +293,17 @@ SECTIONS
>>         __bss_start = .;
>>         *(.bss.stack_aligned)
>>         *(.bss.page_aligned*)
>> -       *(.bss)
>> -       . = ALIGN(SMP_CACHE_BYTES);
>> +       . = ALIGN(PAGE_SIZE);
>>         __per_cpu_start = .;
>> +       *(.bss.percpu.page_aligned)
>> +       . = ALIGN(PAGE_SIZE);
> But this goes too far: What we want is for the TSS to occupy a full
> page, not for whatever random other page-aligned object ends up
> last here (should any every appear).

Come again?  This is ridiculous.

Objects in a section following foo.page_aligned should never end up in
the tail of the final page of foo.page_aligned, because then they are in
the wrong section.

A short TSS is a pain to deal with, but even you said you didn't like
the xen_tss idea when you suggested it.

The name of the section is very deliberately not TSS specific, because
there is plenty of other cleanup which will end up with objects in this
section.

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich]

On 30.07.2019 11:35, Andrew Cooper wrote:
> On 30/07/2019 09:42, Jan Beulich wrote:
>> On 29.07.2019 19:38, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -293,14 +293,17 @@ SECTIONS
>>>          __bss_start = .;
>>>          *(.bss.stack_aligned)
>>>          *(.bss.page_aligned*)
>>> -       *(.bss)
>>> -       . = ALIGN(SMP_CACHE_BYTES);
>>> +       . = ALIGN(PAGE_SIZE);
>>>          __per_cpu_start = .;
>>> +       *(.bss.percpu.page_aligned)
>>> +       . = ALIGN(PAGE_SIZE);
>> But this goes too far: What we want is for the TSS to occupy a full
>> page, not for whatever random other page-aligned object ends up
>> last here (should any every appear).
> 
> Come again?  This is ridiculous.
> 
> Objects in a section following foo.page_aligned should never end up in
> the tail of the final page of foo.page_aligned, because then they are in
> the wrong section.

How do you derive "wrong section"? Sections have an alignment and a
size. The latter doesn't need to be a multiple of the former. The
section ends wherever its size says it ends. Using this property is
actually desirable in other cases, to limit waste of space.

> A short TSS is a pain to deal with, but even you said you didn't like
> the xen_tss idea when you suggested it.
> 
> The name of the section is very deliberately not TSS specific, because
> there is plenty of other cleanup which will end up with objects in this
> section.

Well, if they're all strictly a multiple of PAGE_SIZE, then writing
down a respective requirement might be acceptable. But even then I
wouldn't be overly happy going that route.

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

Re: [Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

On 30/07/2019 10:49, Jan Beulich wrote:
> On 30.07.2019 11:35, Andrew Cooper wrote:
>> On 30/07/2019 09:42, Jan Beulich wrote:
>>> On 29.07.2019 19:38, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/xen.lds.S
>>>> +++ b/xen/arch/x86/xen.lds.S
>>>> @@ -293,14 +293,17 @@ SECTIONS
>>>>          __bss_start = .;
>>>>          *(.bss.stack_aligned)
>>>>          *(.bss.page_aligned*)
>>>> -       *(.bss)
>>>> -       . = ALIGN(SMP_CACHE_BYTES);
>>>> +       . = ALIGN(PAGE_SIZE);
>>>>          __per_cpu_start = .;
>>>> +       *(.bss.percpu.page_aligned)
>>>> +       . = ALIGN(PAGE_SIZE);
>>> But this goes too far: What we want is for the TSS to occupy a full
>>> page, not for whatever random other page-aligned object ends up
>>> last here (should any every appear).
>> Come again?  This is ridiculous.
>>
>> Objects in a section following foo.page_aligned should never end up in
>> the tail of the final page of foo.page_aligned, because then they are in
>> the wrong section.
> How do you derive "wrong section"? Sections have an alignment and a
> size. The latter doesn't need to be a multiple of the former. The
> section ends wherever its size says it ends. Using this property is
> actually desirable in other cases, to limit waste of space.

The principle of least surprise, for a section with page_aligned in its
name, is that the section starts and ends on a page boundary.  This is
what people expect when they see a name like that.

It really doesn't matter what is technically possible.  What does matter
is peoples reasonable expectations.

>> A short TSS is a pain to deal with, but even you said you didn't like
>> the xen_tss idea when you suggested it.
>>
>> The name of the section is very deliberately not TSS specific, because
>> there is plenty of other cleanup which will end up with objects in this
>> section.
> Well, if they're all strictly a multiple of PAGE_SIZE, then writing
> down a respective requirement might be acceptable. But even then Inot submitted by me.
> wouldn't be overly happy going that route.

This reply, like most others in this thread, is actively unhelpful, and
I give up.

I can't read your mind.  Neither can anyone else, and noone has the time
to divine what you want.

If you don't come up with something more helpful than "I don't like it
this way", then I'm going to commit this series in its current form, and
you can adjusting it to your own taste, in your own time.

This goes for other series as well, including ones submitted by others. 
It is absolutely critical that review feedback identifies, in a clear
manner, how you expect the issue to be resolved.

For any non-trivial piece of feedback, if it can't be phrased in the
form "I would this patch ok if you alter $X to $Y", then it is probably
wants rethinking.  Whatever the feedback actually is, that gives a
concrete $X which is the perceived problem, and a concrete $Y which is
either a clear discussion point, or a clear direction to work towards.

~Andrew

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

Re: [Xen-devel] [PATCH v3 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich]

On 30.07.2019 18:36, Andrew Cooper wrote:
> On 30/07/2019 10:49, Jan Beulich wrote:
>> On 30.07.2019 11:35, Andrew Cooper wrote:
>>> On 30/07/2019 09:42, Jan Beulich wrote:
>>>> On 29.07.2019 19:38, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/xen.lds.S
>>>>> +++ b/xen/arch/x86/xen.lds.S
>>>>> @@ -293,14 +293,17 @@ SECTIONS
>>>>>           __bss_start = .;
>>>>>           *(.bss.stack_aligned)
>>>>>           *(.bss.page_aligned*)
>>>>> -       *(.bss)
>>>>> -       . = ALIGN(SMP_CACHE_BYTES);
>>>>> +       . = ALIGN(PAGE_SIZE);
>>>>>           __per_cpu_start = .;
>>>>> +       *(.bss.percpu.page_aligned)
>>>>> +       . = ALIGN(PAGE_SIZE);
>>>> But this goes too far: What we want is for the TSS to occupy a full
>>>> page, not for whatever random other page-aligned object ends up
>>>> last here (should any every appear).
>>> Come again?  This is ridiculous.
>>>
>>> Objects in a section following foo.page_aligned should never end up in
>>> the tail of the final page of foo.page_aligned, because then they are in
>>> the wrong section.
>> How do you derive "wrong section"? Sections have an alignment and a
>> size. The latter doesn't need to be a multiple of the former. The
>> section ends wherever its size says it ends. Using this property is
>> actually desirable in other cases, to limit waste of space.
> 
> The principle of least surprise, for a section with page_aligned in its
> name, is that the section starts and ends on a page boundary.  This is
> what people expect when they see a name like that.

Hmm, when I see "aligned" I think "aligned", and nothing else. What's
more odd - your response here is inconsistent with your earlier one in
https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg02084.html
There you did appear to agree that .bss.page_aligned may end with an
object the size of which is not a multiple of PAGE_SIZE. Furthermore
you even make provisions for this to happen right in the patch
description here.

(As an aside, you also don't seem to have any problems with .text.kexec
being page aligned but not a multiple of PAGE_SIZE in size. Granted this
section doesn't carry "page_aligned" in its name.)

>>> A short TSS is a pain to deal with, but even you said you didn't like
>>> the xen_tss idea when you suggested it.
>>>
>>> The name of the section is very deliberately not TSS specific, because
>>> there is plenty of other cleanup which will end up with objects in this
>>> section.
>> Well, if they're all strictly a multiple of PAGE_SIZE, then writing
>> down a respective requirement might be acceptable. But even then Inot submitted by me.
>> wouldn't be overly happy going that route.
> 
> This reply, like most others in this thread, is actively unhelpful, and
> I give up.
> 
> I can't read your mind.  Neither can anyone else, and noone has the time
> to divine what you want.
> 
> If you don't come up with something more helpful than "I don't like it
> this way", then I'm going to commit this series in its current form, and
> you can adjusting it to your own taste, in your own time.
> 
> This goes for other series as well, including ones submitted by others.
> It is absolutely critical that review feedback identifies, in a clear
> manner, how you expect the issue to be resolved.
> 
> For any non-trivial piece of feedback, if it can't be phrased in the
> form "I would this patch ok if you alter $X to $Y", then it is probably
> wants rethinking.  Whatever the feedback actually is, that gives a
> concrete $X which is the perceived problem, and a concrete $Y which is
> either a clear discussion point, or a clear direction to work towards.

I have to admit that I'm rather surprised to get _this_ as a reply here,
when we've already sketched out the correct alternative. Despite me not
particularly liking it, I don't see anything wrong with

union _aligned(PAGE_SIZE) tss_union {
     struct __packed tss_struct {
         uint32_t :32;
         uint64_t rsp0, rsp1, rsp2;
         uint64_t :64;
         /*
          * Interrupt Stack Table is 1-based so tss->ist[0] corresponds to an IST
          * value of 1 in an Interrupt Descriptor.
          */
         uint64_t ist[7];
         uint64_t :64;
         uint16_t :16, bitmap;
     };
     char pad[PAGE_SIZE];
};

And since it's a technically correct solution (providing both a type
correctly describing the hardware interface and one correctly describing
our own needs) with no better alternative either of us can see, I think
this (or whatever variation of it) is the way to go.

As to the rest of your rant: I disagree that a reviewer has to always
suggest how things are to be done; that's desirable where possible,
but simply pointing out something is wrong will have to do in certain
cases. In the case here, which is a good example imo, the point of my
response is that from simply looking at the resulting code it is
unclear why _either_ of the two ALIGN(PAGE_SIZE) have been inserted.
This carries the risk of later someone like me coming and deleting
everything that's there without apparent reason (see e.g. 01fe4da624,
albeit istr having done something more extensive at some other time,
but I can't seem to be able to spot it). If anything it should be you
to explain why they need to be there. And while (in a way) you do so
(in the description, the same passage as referenced above), the reason
isn't really as simple as "such that the result is safe even with
objects shorter than a page in length". Instead the reasons actually
differ for both ALIGN()s: In the first case we simply want to avoid
__per_cpu_start living needlessly early. In the latter one you want to
compensate for something that should be done elsewhere (see above).

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