Re: [Minios-devel] [UNIKRAFT PATCH v2 1/6] plat: move common section definitions from linker script to include file

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Costin,


On 5/28/19 11:29 AM, Costin Lupu wrote:
> Hi Florian,
>
> Please see my comments inline.
>
> On 5/24/19 3:11 PM, Florian Schmidt wrote:
> > This only concerns kvm and Xen for now, because linuxu is special and
> > doesn't have a proper full linker script yet.
> >
> > Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> > ---
> >   plat/common/include/common.lds.h | 29 ++++++++++++++++
> >   plat/common/x86/link64.lds       | 59 --------------------------------
> >   plat/kvm/arm/link64.lds.S        | 28 ++-------------
> >   plat/kvm/x86/link64.lds.S        |  4 ++-
> >   plat/linuxu/arm/link.lds         |  1 +
> >   plat/linuxu/x86/link64.lds       | 26 +++++++++++++-
> >   plat/xen/arm/link32.lds.S        | 29 ++--------------
> >   plat/xen/x86/link64.lds.S        |  4 ++-
> >   8 files changed, 65 insertions(+), 115 deletions(-)
> >   delete mode 100644 plat/common/x86/link64.lds
> >
> > diff --git a/plat/common/include/common.lds.h b/plat/common/include/common.lds.h
> > index 8672b6ff..ea030d9e 100644
> > --- a/plat/common/include/common.lds.h
> > +++ b/plat/common/include/common.lds.h
> > @@ -31,6 +31,8 @@
> >   #ifndef __UK_COMMON_LDS_H
> >   #define __UK_COMMON_LDS_H
> >   
> > +#include <uk/arch/limits.h> /* for __PAGE_SIZE */
> > +
> >   /* DWARF debug sections.  Symbols in the DWARF debugging sections are
> >    * relative to the beginning of the section so we begin them at 0.
> >    */
> > @@ -64,4 +66,31 @@
> >         .debug_macro    0 : { *(.debug_macro) }                         \
> >         .gnu.attributes 0 : { KEEP (*(.gnu.attributes)) }
> >   
> > +#define EXCEPTION_SECTIONS						\
> > +       . = ALIGN(__PAGE_SIZE);                                         \
> > +       __eh_frame_start = .;                                           \
> > +       .eh_frame :                                                     \
> > +       {                                                               \
> > +               *(.eh_frame)                                            \
> > +               *(.eh_frame.*)                                          \
> > +       }                                                               \
> > +       __eh_frame_end = .;                                             \
> > +                                                                       \
> > +       __eh_frame_hdr_start = .;                                       \
> > +       .eh_frame_hdr :                                                 \
> > +       {                                                               \
> > +               *(.eh_frame_hdr)                                        \
> > +               *(.eh_frame_hdr.*)                                      \
> > +       }                                                               \
> > +       __eh_frame_hdr_end = .;
> > +
> > +#define CTORTAB_SECTION                                                        
> > \
> > +       . = ALIGN(__PAGE_SIZE);                                         \
> > +       uk_ctortab = .;                                                 \
> > +       .uk_ctortab :                                                   \
> > +       {                                                               \
> > +       KEEP(*(SORT_BY_NAME(.uk_ctortab[0-7])))                         \
> > +       LONG(0)                                                         \
> Nitpicking. If we'll have a v3, please indent these 2 lines above.
Indeed, that should be fixed.


> > +       }
> > +
> >   #endif /* __UK_COMMON_LDS_H */
> > diff --git a/plat/common/x86/link64.lds b/plat/common/x86/link64.lds
> > deleted file mode 100644
> > index 96f353b8..00000000
> > --- a/plat/common/x86/link64.lds
> > +++ /dev/null
> > @@ -1,59 +0,0 @@
> > -/* SPDX-License-Identifier: BSD-3-Clause */
> > -/*
> > - * Authors: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
> > - *
> > - * Copyright (c) 2019, Politehnica University of Bucharest. All rights 
> > reserved.
> Since the definitions from here were moved to common.lds.h, shouldn't we
> also move the copyright and authorship there?
The other file doesn't have author lines, but it does have a copyright 
line, so I can move that over.

> > --- a/plat/linuxu/x86/link64.lds
> > +++ b/plat/linuxu/x86/link64.lds
> > @@ -1,6 +1,30 @@
> >   SECTIONS
> >   {
> > -   INCLUDE plat/common/x86/link64.lds
> > +       /* TODO: include these from common-lds.h */
> Why don't we include these from common.lds.h now?
I'll double-check with Yuri, who did the support for the preprocessable 
linker scripts. I remember him saying there was some issue that made 
support on in for linuxu tricky, but I can't remember for sure.
I'll see what I can do.

Cheers,
Florian

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