On 02/03/18 17:04, Jan Beulich wrote:
>>>> On 02.03.18 at 17:53, <andrew.coop...@citrix.com> wrote:
>> On 02/03/18 14:34, Jan Beulich wrote:
>>> Note that the removed BUILD_BUG_ON()s don't get replaced by anything -
>>> there already is a suitable ASSERT() in xen.lds.S.
>> This isn't quite true. You've changed the mechanism by which the stubs
>> get mapped (from entirely common, to per-pcpu), removing the need for
>> the BUILD_BUG_ON().
>>
>> The ASSERT() in xen.lds.S serves a different purpose, checking that the
>> sum total of stubs don't overlap with the compiled code. (On this
>> note... do we perform the same check for livepatches? I can't spot
>> anything.)
> What you say may be true for the one that was in
> setup_cpu_root_pgt(), but surely not the one I'm removing from
> alloc_stub_page(). But I can drop this if you prefer.
I think it might avoid some confusion.
>
>>> What should we do with the TSS? Currently together with it we expose
>>> almost a full page of other per-CPU data. A simple (but slightly
>>> hackish) option would be to use one of the two unused stack slots.
>> In 64bit, the TSS can be mapped read-only, because hardware never has
>> cause to write to it.
>>
>> I believe that Linux now uses a read-only TSS mapping to double as a
>> guard page for the trampoline stack, which is a less hacky way of
>> thinking about it.
>>
>> However, doing that in Xen would mean shattering the directmap
>> superpages in all cases, and we'd inherit the SVM triple fault case into
>> release builds. A different alternative (and perhaps simpler to
>> backport) might be to have .bss.percpu.page_aligned and use that to hide
>> the surrounding data.
> Well, yes, that's obviously an option, but pretty wasteful. I'd then
> be tempted to at least do some sharing of the page similar to how
> the stubs of several CPUs share a single page.
For backport to older releases?
I think the extra almost 4k per pcpu isn't going to concern people (its
the least of their problems right now), and there is a very tangible
benefit of not leaking the other surrounding data.
>
>> Thinking about it, we've got the same problem with the TSS as the BSP
>> IDT, if the link order happens to cause init_tss to cross a page boundary.
> I don't think so, no - the structure is 128 bytes in size and 128
> byte aligned. When I created the original XPTI light patch I did
> specifically check.
This only happens by chance, because sizeof(struct tss_struct) ==
SMP_CACHE_BYTES
If we intend to rely on this behaviour, we want something like this:
diff --git a/xen/include/asm-x86/processor.h
b/xen/include/asm-x86/processor.h
index 9c70a98..fe647dc 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -385,7 +385,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;
@@ -398,7 +398,7 @@ struct __packed __cacheline_aligned tss_struct {
uint16_t :16, bitmap;
/* Pads the TSS to be cacheline-aligned (total size is 0x80). */
uint8_t __cacheline_filler[24];
-};
+} __aligned(sizeof(struct tss_struct));
#define IST_NONE 0UL
#define IST_DF 1UL
except that C can't cope with this expression. I wonder if there is an
alternate way with typedefs.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
