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

[Juergen Gross <jgross@xxxxxxxx>]

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.

> Then further move PV-only ELF notes inside the XEN_PV
> conditional. Finally have the "supported features" note actually report
> reality: All three of the features are supported and/or applicable only
> in certain cases.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- 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.

>   /*
>    * The IDT and GDT 'descriptors' are a strange 48-bit object
>    * only used by the lidt and lgdt instructions. They are not
> --- 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?

> -#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).


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature