On 20/04/2020 16:47, Jan Beulich wrote: > On 20.04.2020 16:39, Andrew Cooper wrote: >> On 20/04/2020 15:12, Jan Beulich wrote: >>> On 17.04.2020 17:50, Andrew Cooper wrote: >>>> There is no need for the Compat GDT if there are no 32bit PV guests. This >>>> saves 4k per online CPU >>>> >>>> Bloat-o-meter reports the following savings in Xen itself: >>>> >>>> add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605) >>>> Function old new delta >>>> cpu_smpboot_free 1249 1256 +7 >>>> per_cpu__compat_gdt_l1e 8 - -8 >>>> per_cpu__compat_gdt 8 - -8 >>>> init_idt_traps 442 420 -22 >>>> load_system_tables 414 364 -50 >>>> trap_init 444 280 -164 >>>> cpu_smpboot_callback 1255 991 -264 >>>> boot_compat_gdt 4096 - -4096 >>>> Total: Before=3062726, After=3058121, chg -0.15% >>>> >>>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >>>> --- >>>> CC: Jan Beulich <jbeul...@suse.com> >>>> CC: Wei Liu <w...@xen.org> >>>> CC: Roger Pau Monné <roger....@citrix.com> >>>> >>>> The increase in cpu_smpboot_free() appears to be a consequence of a totally >>>> different layout of basic blocks. >>>> --- >>>> xen/arch/x86/cpu/common.c | 5 +++-- >>>> xen/arch/x86/desc.c | 2 ++ >>>> xen/arch/x86/smpboot.c | 5 ++++- >>>> xen/arch/x86/traps.c | 10 +++++++--- >>>> 4 files changed, 16 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c >>>> index 1b33f1ed71..7b093cb421 100644 >>>> --- a/xen/arch/x86/cpu/common.c >>>> +++ b/xen/arch/x86/cpu/common.c >>>> @@ -752,8 +752,9 @@ void load_system_tables(void) >>>> >>>> _set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss, >>>> sizeof(*tss) - 1, SYS_DESC_tss_avail); >>>> - _set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss, >>>> - sizeof(*tss) - 1, SYS_DESC_tss_busy); >>>> + if ( IS_ENABLED(CONFIG_PV32) ) >>>> + _set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss, >>>> + sizeof(*tss) - 1, SYS_DESC_tss_busy); >>> Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then. >> Doing it like this specifically ensures that there is never a case where >> things are half configured. > But this way you set up something in the GDT that's never going > to be used when "pv=no-32". Why leave a TSS accessible that we > don't need?
Defence in depth. Having it only partially set up is more likely to fail in a security relevant way, than having it fully set up. This particular example is poor. There is no need to have the TSS in either GDT after the `ltr` instruction at boot for 64bit, because we don't task switch, but ISTR you requesting that this stayed as-was for consistency. (For 32bit Xen, it was strictly necessary for the #DF task switch to work.) However, the other logic, particularly the cached l1e pointing at the percpu compat_gdt is more liable to go wrong in interesting ways. ~Andrew
