Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds

[Jan Beulich <jbeulich@xxxxxxxx>]

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.cooper3@xxxxxxxxxx>
>>> ---
>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>> CC: Wei Liu <wl@xxxxxxx>
>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>
>>> 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?

> I don't think it is wise to suggest that making opt_pv32 runtime
> configurable might work.

I didn't suggest (nor even consider) runtime changing of this
setting. If we wanted, _that_ would be what might require using
code as you have it right now (if we wanted to avoid setting
this up at the time the setting gets flipped from false to true).

Jan