Re: [Xen-devel] [PATCH 1/2] x86: decouple xen alignment setting from EFI/non-EFI build

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.03.19 at 15:57, <wei.liu2@xxxxxxxxxx> wrote:
> Introduce a new Kconfig option to pick the alignment for xen binary.
> To retain original behaviour, the default pick for EFI build is 2M and
> non-EFI build 4K.

Is this a worthwhile step to take, considering that we mean to
switch to 2M main section boundaries anyway? I realize it's still not
necessarily clear how to get there without re-introducing the boot
regression with syslinux that was observed back then, but perhaps
time would better be spent there than into introducing configurability?
(To be honest I don't recall whether it was determined that there
was no way out of this dilemma, but it seems to me there would
have been a plan.)

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -138,6 +138,32 @@ config TBOOT
>  
>         If unsure, say Y.
>  
> +choice
> +     prompt "Alignment of Xen binary"
> +     depends on X86
> +     default XEN_ALIGN_DEFAULT
> +     ---help---
> +       Specify alignment for Xen binary.
> +
> +       If unsure, choose "default".
> +
> +config XEN_ALIGN_DEFAULT
> +     bool "Default alignment"
> +     ---help---
> +       Pick alignment according to build variants.
> +
> +       For EFI build the default alignment is 2M. For non-EFI build
> +       the default alignment is 4K due to syslinux failing to handle
> +       2M alignment.

Wasn't it the resulting image size rather than the alignment itself
which did cause the problem? Has the issue been fixed in newer
syslinux?

> +config XEN_ALIGN_4K
> +     bool "4K alignment"
> +
> +config XEN_ALIGN_2M
> +     bool "2M alignment"

In patch 2 you only need the latter - is there really much value in
allowing explicitly requesting 4k?

> @@ -20,13 +19,26 @@ ENTRY(efi_start)
>  #else /* !EFI */
>  
>  #define FORMAT "elf64-x86-64"
> -#define SECTION_ALIGN PAGE_SIZE
>  #define DECL_SECTION(x) x : AT(ADDR(x) - __XEN_VIRT_START)
>  
>  ENTRY(start_pa)
>  
>  #endif /* EFI */
>  
> +#if defined CONFIG_XEN_ALIGN_2M
> +#define SECTION_ALIGN MB(2)
> +#elif defined CONFIG_XEN_ALIGN_4K
> +#define SECTION_ALIGN PAGE_SIZE
> +#elif defined CONFIG_XEN_ALIGN_DEFAULT
> +    #ifdef EFI
> +        #define SECTION_ALIGN MB(2)
> +    #else
> +        #define SECTION_ALIGN PAGE_SIZE
> +    #endif
> +#else
> +#error "Section alignment undefined"
> +#endif

How about converting the last #elif to #else and omitting the
#error? Also would you mind using the defined() style (i.e. with
parentheses), as we commonly do elsewhere? And please use
uniform indentation (or not) of directives. I also find the
indentation itself quite unusual - we have almost no examples of
it being like this outside of files we've imported from elsewhere.
Typically we retain the # in column 1 and indent only the actual
directive keyword.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel