Re: [PATCH 5/5] x86: don't build unused entry code when !PV32

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2021 16:01, Roger Pau Monné wrote:
> On Wed, Nov 25, 2020 at 09:51:33AM +0100, Jan Beulich wrote:
>> Except for the initial part of cstar_enter compat/entry.S is all dead
>> code in this case. Further, along the lines of the PV conditionals we
>> already have in entry.S, make code PV32-conditional there too (to a
>> fair part because this code actually references compat/entry.S).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> TBD: I'm on the fence of whether (in a separate patch) to also make
>>      conditional struct pv_domain's is_32bit field.
>>
>> --- a/xen/arch/x86/x86_64/asm-offsets.c
>> +++ b/xen/arch/x86/x86_64/asm-offsets.c
>> @@ -9,7 +9,7 @@
>>  #include <xen/perfc.h>
>>  #endif
>>  #include <xen/sched.h>
>> -#ifdef CONFIG_PV
>> +#ifdef CONFIG_PV32
>>  #include <compat/xen.h>
>>  #endif
>>  #include <asm/hardirq.h>
>> @@ -102,19 +102,21 @@ void __dummy__(void)
>>      BLANK();
>>  #endif
>>  
>> -#ifdef CONFIG_PV
>> +#ifdef CONFIG_PV32
>>      OFFSET(DOMAIN_is_32bit_pv, struct domain, arch.pv.is_32bit);
>>      BLANK();
>>  
>> -    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, 
>> evtchn_upcall_pending);
>> -    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
>> -    BLANK();
>> -
>>      OFFSET(COMPAT_VCPUINFO_upcall_pending, struct compat_vcpu_info, 
>> evtchn_upcall_pending);
>>      OFFSET(COMPAT_VCPUINFO_upcall_mask, struct compat_vcpu_info, 
>> evtchn_upcall_mask);
>>      BLANK();
>>  #endif
>>  
>> +#ifdef CONFIG_PV
>> +    OFFSET(VCPUINFO_upcall_pending, struct vcpu_info, 
>> evtchn_upcall_pending);
>> +    OFFSET(VCPUINFO_upcall_mask, struct vcpu_info, evtchn_upcall_mask);
>> +    BLANK();
>> +#endif
>> +
>>      OFFSET(CPUINFO_guest_cpu_user_regs, struct cpu_info, 
>> guest_cpu_user_regs);
>>      OFFSET(CPUINFO_verw_sel, struct cpu_info, verw_sel);
>>      OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -29,8 +29,6 @@ ENTRY(entry_int82)
>>          mov   %rsp, %rdi
>>          call  do_entry_int82
>>  
>> -#endif /* CONFIG_PV32 */
>> -
>>  /* %rbx: struct vcpu */
>>  ENTRY(compat_test_all_events)
>>          ASSERT_NOT_IN_ATOMIC
>> @@ -197,6 +195,8 @@ ENTRY(cr4_pv32_restore)
>>          xor   %eax, %eax
>>          ret
>>  
>> +#endif /* CONFIG_PV32 */
>> +
>>          .section .text.entry, "ax", @progbits
>>  
>>  /* See lstar_enter for entry register state. */
>> @@ -230,6 +230,13 @@ ENTRY(cstar_enter)
>>          sti
>>  
>>          movq  STACK_CPUINFO_FIELD(current_vcpu)(%rbx), %rbx
>> +
>> +#ifndef CONFIG_PV32
>> +
>> +        jmp   switch_to_kernel
>> +
>> +#else
>> +
>>          movq  VCPU_domain(%rbx),%rcx
>>          cmpb  $0,DOMAIN_is_32bit_pv(%rcx)
>>          je    switch_to_kernel
>> @@ -393,3 +400,5 @@ compat_crash_page_fault:
>>          jmp   .Lft14
>>  .previous
>>          _ASM_EXTABLE(.Lft14, .Lfx14)
>> +
>> +#endif /* CONFIG_PV32 */
> 
> Seeing this chunk, would it make sense to move the cstar_enter part
> relevant to !is_32bit_pv into the common entry.S and leave the rest
> here as compat_cstar_enter or some such?
> 
> AFAICT we only need a tiny part of the compat stuff when !CONFIG_PV32,
> so I think we could make the hole compat/entry.S depend on
> CONFIG_PV32.

While it grows the patch, doing things this way looks to work out
nicely. v2 in the works (making in fact compat/ as a whole build
only when PV32, as it's really only the one object file that gets
built there) ...

Jan