Re: [PATCH] x86/Xen: tidy xen-head.S

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.02.2023 13:05, Juergen Gross wrote:
> On 15.02.23 12:33, Jan Beulich wrote:
>> First of all drop 32-bit leftovers, including the inclusion of the file
>> from head_32.S.
> 
> I don't see why we would want to drop 32-bit HVM and PVH support.

HVM doesn't use this, does it? But yes, the PVH aspect as well as ...

>> --- a/arch/x86/kernel/head_32.S
>> +++ b/arch/x86/kernel/head_32.S
>> @@ -524,8 +524,6 @@ __INITRODATA
>>   int_msg:
>>      .asciz "Unknown interrupt or fault at: %p %p %p\n"
>>   
>> -#include "../../x86/xen/xen-head.S"
>> -
> 
> This is wrong for non-PV cases.

... this and ...

>> --- a/arch/x86/xen/xen-head.S
>> +++ b/arch/x86/xen/xen-head.S
>> @@ -83,27 +83,33 @@ SYM_CODE_END(asm_cpu_bringup_and_idle)
>>      ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS,       .asciz "linux")
>>      ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION,  .asciz "2.6")
>>      ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION,    .asciz "xen-3.0")
>> -#ifdef CONFIG_X86_32
>> -    ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR __PAGE_OFFSET)
>> -#else
>>      ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR __START_KERNEL_map)
> 
> This will be wrong for 32-bit guests now. I'm not sure the value is really
> used in Xen for non-PV guests, though.
> 
>>      /* Map the p2m table to a 512GB-aligned user address. */
>>      ELFNOTE(Xen, XEN_ELFNOTE_INIT_P2M,       .quad (PUD_SIZE * 
>> PTRS_PER_PUD))
> 
> Move this under the CONFIG_PV umbrella?

... these occurred to me over lunch (and I was hoping to be able to point
out my mistake before getting feedback). I'll check whether VIRT_BASE can
also be moved into the PV-only section.

>> -#endif
>>   #ifdef CONFIG_XEN_PV
>>      ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
>> +    ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .ascii "!writable_page_tables")
>> +    ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
>> +    ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,
>> +            .quad _PAGE_PRESENT; .quad _PAGE_PRESENT)
>> +# define FEATURES_PV (1 << XENFEAT_writable_page_tables)
>> +#else
>> +# define FEATURES_PV 0
>> +#endif
>> +#ifdef CONFIG_XEN_PVH
>> +# define FEATURES_PVH (1 << XENFEAT_linux_rsdp_unrestricted)
>> +#else
>> +# define FEATURES_PVH 0
>> +#endif
>> +#ifdef CONFIG_XEN_DOM0
>> +# define FEATURES_DOM0 (1 << XENFEAT_dom0)
>> +#else
>> +# define FEATURES_DOM0 0
>>   #endif
>>      ELFNOTE(Xen, XEN_ELFNOTE_HYPERCALL_PAGE, _ASM_PTR hypercall_page)
>> -    ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,
>> -            .ascii "!writable_page_tables|pae_pgdir_above_4gb")
>>      ELFNOTE(Xen, XEN_ELFNOTE_SUPPORTED_FEATURES,
>> -            .long (1 << XENFEAT_writable_page_tables) |       \
>> -                  (1 << XENFEAT_dom0) |                       \
>> -                  (1 << XENFEAT_linux_rsdp_unrestricted))
>> -    ELFNOTE(Xen, XEN_ELFNOTE_PAE_MODE,       .asciz "yes")
>> +            .long FEATURES_PV | FEATURES_PVH | FEATURES_DOM0)
>>      ELFNOTE(Xen, XEN_ELFNOTE_LOADER,         .asciz "generic")
>> -    ELFNOTE(Xen, XEN_ELFNOTE_L1_MFN_VALID,
>> -            .quad _PAGE_PRESENT; .quad _PAGE_PRESENT)
>>      ELFNOTE(Xen, XEN_ELFNOTE_SUSPEND_CANCEL, .long 1)
>>      ELFNOTE(Xen, XEN_ELFNOTE_MOD_START_PFN,  .long 1)
>>      ELFNOTE(Xen, XEN_ELFNOTE_HV_START_LOW,   _ASM_PTR 
>> __HYPERVISOR_VIRT_START)
> 
> Are XEN_ELFNOTE_MOD_START_PFN and XEN_ELFNOTE_HV_START_LOW really relevant
> for the non-PV case? I don't think so (in theory XEN_ELFNOTE_MOD_START_PFN
> could be used, but the main reason for its introduction was PV guests IIRC).

I wasn't sufficiently certain for MOD_START_PFN, so I'd prefer to leave it
untouched for now. HV_START_LOW might be 32-bit PV only really; I'll check
and then maybe drop (or move).

Jan