Re: [Xen-devel] [PATCH] x86: partially revert use of 2M mappings for hypervisor image

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/03/16 15:12, Jan Beulich wrote:
> As explained by Andrew in
> http://lists.xenproject.org/archives/html/xen-devel/2016-03/msg01380.html 
> that change makes the uncompressed xen.gz image too large for certain
> boot environments. As a result this change makes some of the effects of
> commits cf393624ee ("x86: use 2M superpages for text/data/bss
> mappings") and 53aa3dde17 ("x86: unilaterally remove .init mappings")
> conditional, restoring alternative previous code where necessary. This
> is so that xen.efi can still benefit from the new mechanisms, as it is
> unaffected by said limitations.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> The first, neater attempt (making the __2M_* symbols weak) failed:
> - older gcc doesn't access the weak symbols through .got
> - GOTPCREL relocations get treated just like PCREL ones by ld when
>   linking xen.efi
>
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -497,6 +497,17 @@ static void __init kexec_reserve_area(st
>  #endif
>  }
>  
> +static inline bool_t using_2M_mapping(void)
> +{
> +    return !l1_table_offset((unsigned long)__2M_text_end) &&
> +           !l1_table_offset((unsigned long)__2M_rodata_start) &&
> +           !l1_table_offset((unsigned long)__2M_rodata_end) &&
> +           !l1_table_offset((unsigned long)__2M_init_start) &&
> +           !l1_table_offset((unsigned long)__2M_init_end) &&
> +           !l1_table_offset((unsigned long)__2M_rwdata_start) &&
> +           !l1_table_offset((unsigned long)__2M_rwdata_end);

I would recommend

#ifdef EFI
return 1;
#else
return 0;
#endif

The compiler is unable to collapse that expression into a constant,
because it can only be evaluated at link time.

> +}
> +
>  static void noinline init_done(void)
>  {
>      void *va;
> @@ -509,10 +520,19 @@ static void noinline init_done(void)
>      for ( va = __init_begin; va < _p(__init_end); va += PAGE_SIZE )
>          clear_page(va);
>  
> -    /* Destroy Xen's mappings, and reuse the pages. */
> -    destroy_xen_mappings((unsigned long)&__2M_init_start,
> -                         (unsigned long)&__2M_init_end);
> -    init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
> +    if ( using_2M_mapping() )
> +    {
> +        /* Destroy Xen's mappings, and reuse the pages. */

I would be tempted to leave this comment back outside using_2M_mapping()
which reduces the size of this hunk.

> +        destroy_xen_mappings((unsigned long)&__2M_init_start,
> +                             (unsigned long)&__2M_init_end);
> +        init_xenheap_pages(__pa(__2M_init_start), __pa(__2M_init_end));
> +    }
> +    else
> +    {
> +        destroy_xen_mappings((unsigned long)&__init_begin,
> +                             (unsigned long)&__init_end);
> +        init_xenheap_pages(__pa(__init_begin), __pa(__init_end));
> +    }
>  
>      printk("Freed %ldkB init memory.\n", 
> (long)(__init_end-__init_begin)>>10);
>  
> @@ -922,6 +942,8 @@ void __init noreturn __start_xen(unsigne
>               * Undo the temporary-hooking of the l1_identmap.  
> __2M_text_start
>               * is contained in this PTE.
>               */
> +            BUG_ON(l2_table_offset((unsigned long)_erodata) ==
> +                   l2_table_offset((unsigned long)_stext));

Is this intentional to stay, or the remnants of debugging?

Irrespective of some of these minor tweaks, Reviewed-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel