Re: [PATCH v4 1/3] x86: Introduce a new constant KERNEL_MAPPING_SIZE

[Baoquan He]

Hi Boris,

Thanks a lot for your comments, sorry so late to reply!

On 02/14/17 at 06:32pm, Borislav Petkov wrote:
> > diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
> > index 9215e05..24c9098 100644
> > --- a/arch/x86/include/asm/page_64_types.h
> > +++ b/arch/x86/include/asm/page_64_types.h
> > @@ -50,16 +50,22 @@
> >  #define __VIRTUAL_MASK_SHIFT	47
> >  
> >  /*
> > - * Kernel image size is limited to 1GiB due to the fixmap living in the
> > + * Kernel image size is limited to 512 MB. The kernel code+data+bss
> 
> This is not what it said there before. With your change you have:
> 
> - 0
> .
> .
> .
> - 512 - KERNEL_IMAGE_SIZE
> .
> .
> .
> - 1024 - KERNEL_MAPPING_SIZE
> 
> and KERNEL_IMAGE_SIZE is not limited to 512Mb but it is "Use 512Mib by
> default". And we do enforce that in various places like in the linker
> script assertions but there's some headroom open in the upper 512Mib if
> needed.

Before below commit merged, it said:

 * Kernel image size is limited to 512 MB (see level2_kernel_pgt in
 * arch/x86/kernel/head_64.S), and it is mapped here:
 */
#define KERNEL_IMAGE_SIZE      (512 * 1024 * 1024)

commit 6145cfe3 ("x86, kaslr: Raise the maximum virtual address to -1
GiB on x86_64")

So you can see KERNEL_IMAGE_SIZE is a constant value and not optional.
Then Kees posted above commit, The default mentioned in "Use 512Mib by
default" should be KERNEL_IMAGE_SIZE in the case that kaslr code not
compiled in, 512M, which is relative to the case that kaslr code
compiled in, 1G. In fact, it's meaning the kernel mapping size defaults
to 512M, and will change to 1G if CONFIG_RANDOMIZE_BASE is chosen.

Seems in kernel only linker script checks kernel image size as below:

. = ASSERT((_end - _text <= KERNEL_IMAGE_SIZE),                                                                                                  
           "kernel image bigger than KERNEL_IMAGE_SIZE");

In arch/x86/kernel/head64.c, it's more likely checking the kernel
mapping size, just because they share the same constant,
KERNEL_IMAGE_SIZE.

About headroom, in boot/compressed/kaslr.c kernel iamge size is
considered, including head room. Here I am not quite sure. Do you mean:
KERNEL_IMAGE_SIZE use 512Mib by default, only kerel image size is
limited to 512M which is (_end - _text) since linker script will check
it. While with headroom, it could be bigger than 512M if needed.

Am I right on understanding it?

> 
> KERNEL_MAPPING_SIZE OTOH is the one limited to 1G due to the fixmap L2
> PGT...
> 
> > + * must not be bigger than that.
> > + */
> > +#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
> > +
> > +/*
> > + * Kernel mapping size is limited to 1GiB due to the fixmap living in the
> >   * next 1GiB (see level2_kernel_pgt in arch/x86/kernel/head_64.S). Use
> >   * 512MiB by default, leaving 1.5GiB for modules once the page tables
> >   * are fully set up. If kernel ASLR is configured, it can extend the
> >   * kernel page table mapping, reducing the size of the modules area.
> >   */
> >  #if defined(CONFIG_RANDOMIZE_BASE)
> > -#define KERNEL_IMAGE_SIZE	(1024 * 1024 * 1024)
> > +#define KERNEL_MAPPING_SIZE	(1024 * 1024 * 1024)
> >  #else
> > -#define KERNEL_IMAGE_SIZE	(512 * 1024 * 1024)
> > +#define KERNEL_MAPPING_SIZE	(512 * 1024 * 1024)
> >  #endif
> 
> ... and since you're adding that define now, fixup the comments in this
> patch too, to explain what they mean.

Yes, agree, this makes it clearer. Will do.

> 
> Also, I'd like for the text to say that both defines are dependent in
> the sense that IMAGE_SIZE <= MAPPING_SIZE so that people know what's
> going on and which is which.

It makes sense, will do. 

Thanks for great suggestion!