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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew