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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Mar 19, 2019 at 04:45:35AM -0600, Jan Beulich wrote:
> >>> 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.)

I think it would definitely be sensible to fix it there. But we still
have older syslinux to deal with. I don't think they're going away any
time soon.

> 
> > --- 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?
> 

Right. I can make this clearer.

"due to syslinux failing to handle the image size increment induced by
2M alignment".

Also, I think s/binary/image/ in the description would be better.

Looking at syslinux.git I don't immediately see patches to fix the issue
(if there is such issue in the first place).

> > +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?
> 

I thought there would be a case in which users want 4K explicitly? I'm
certainly fine with dropping the 4K alignment option.

> > @@ -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.

Will fix (after we determine what to do with 4K option).

Wei.

> 
> Jan
> 
> 

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