[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH 0/9] Prepare build scripts to support ARM64



Hi Simon,

> -----Original Message-----
> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Sent: 2018年6月15日 5:17
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Shijie Huang <Shijie.Huang@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; nd
> <nd@xxxxxxx>
> Subject: Re: [UNIKRAFT PATCH 0/9] Prepare build scripts to support ARM64
> 
> Hey Wei,
> 
> On 09.04.2018 08:37, Wei Chen wrote:
> > Hi Simon,
> >
> > Sorry for replying later, I had public holidays last week.
> >
> >> -----Original Message-----
> >> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> >> Sent: 2018年4月5日 5:24
> >> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: Shijie Huang <Shijie.Huang@xxxxxxx>; Kaly Xin <Kaly.Xin@xxxxxxx>; nd
> >> <nd@xxxxxxx>
> >> Subject: Re: [UNIKRAFT PATCH 0/9] Prepare build scripts to support ARM64
> >>
> >> Thanks a lot for this patch series!
> >> I added my comments inline to each patch and look forward for v2. ;-)
> >>
> >> I have some general minor notes:
> >>
> >> Can you confirm that your formatting of source files is correct? (e.g.,
> >> use checkpatch.pl). Patch 3 threw a warning that another blank line was
> >> introduced at EOF.
> >>
> >
> > About the code-style, I also have some concerns. While I was modifying the
> > code, I found some code was using "tab" and some code was using "spaces" for
> > indent. So I wasn't sure which code-style we should use.
> 
> We usually tried to adopt the new code style (Linux) for files we
> ported. In fact, this may reduce traceability of what we changed in the
> code from the original file since you can't do a simple diff anymore. I
> understand your concerns. However, we preferred to unify the code style
> since we expect that we are going to diverge anyways from the original
> (which was MiniOS and Solo5 mainly).
> 

I got it, thanks. I will adopt Linux code style in my patches.

> >
> > I just checked the code again, the "tab" indent is use by those files copied
> > from Xen. I understand we should keep the original code-style for those
> > files. Except such kind of files, should we use the Linux code-style for
> > Unikraft?
> 
> Yes, in general, Linux style.
> 
> >
> >> Please double check the selectors in your short commit messages. Patch
> >> 4, for instance, should use 'include' instead of 'build'.
> >>
> >
> > Oh, that's right. Though this series is for 'build', but patch#4 modified
> > The "include". I should use the 'include:' as subject prefix.
> 
> Great ;-)
> 
> >
> >> Since I saw it in several commit messages: Remove the "have to"s in your
> >> commit messages. They are still fine without ;-). The message of each
> >> patch should just describe what it does - independent of any
> >> circumstances or weightings.
> >
> > Thanks. I would refine the commit messages to address this comment : )
> >
> >>
> >> Thanks,
> >>
> >> Simon
> >>
> >> On 15.03.2018 04:39, Wei Chen wrote:
> >>> Currently, Unikraft only supports arm32 and x86_64. The folder layout
> >>> is not very convenient to add arm64 or x86_32 support to it. In this
> >>> case we will modify the folder layout to support common code for the
> >>> architectures of the same CPU families. We also have to modify the
> >>> build scripts which corresponding to this change at the same time.
> >>>
> >>> Wei Chen (9):
> >>>     build: Adjust sed script to avoid treating arm64 as arm
> >>>     build: Introduce a new variable UK_FAMILY
> >>>     build: Move arm32 libraries to new family/architecture folder
> >>>     build: Move architecture headers to family/architecture folder
> >>>     build: Add a makefile rule to check valid gcc version
> >>>     build: Add arm64 architecture config to menuconfig
> >>>     build: Add a macro to check and add gcc flags for target CPU
> >>>     build: Add compiler and flags for arm64
> >>>     build: Check the minimum GCC version for arm32
> >>>
> >>>    Makefile                              |  36 ++-
> >>>    arch/Arch.uk                          |   2 +
> >>>    arch/Config.uk                        |   7 +-
> >>>    arch/arm/Compiler.uk                  |   4 +
> >>>    arch/arm/Config.uk                    |  67 +++++-
> >>>    arch/arm/Makefile.uk                  |  83 ++++++-
> >>>    arch/arm/arm32/divsi3.S               | 404
> >> ++++++++++++++++++++++++++++++++++
> >>>    arch/arm/arm32/ldivmod.S              |  68 ++++++
> >>>    arch/arm/arm32/ldivmod_helper.c       |  67 ++++++
> >>>    arch/arm/arm32/qdivrem.c              | 324 +++++++++++++++++++++++++++
> >>>    arch/arm/divsi3.S                     | 404 ---------------------------
> ---
> >> ----
> >>>    arch/arm/ldivmod.S                    |  68 ------
> >>>    arch/arm/ldivmod_helper.c             |  67 ------
> >>>    arch/arm/qdivrem.c                    | 324 ---------------------------
> >>>    arch/x86/Compiler.uk                  |   6 +
> >>>    arch/x86/Config.uk                    |  89 ++++++++
> >>>    arch/x86/Makefile.uk                  |  37 ++++
> >>>    arch/x86_64/Compiler.uk               |   6 -
> >>>    arch/x86_64/Config.uk                 |  89 --------
> >>>    arch/x86_64/Makefile.uk               |  37 ----
> >>>    include/uk/arch/arm/arm32/atomic.h    |  64 ++++++
> >>>    include/uk/arch/arm/arm32/intsizes.h  |  45 ++++
> >>>    include/uk/arch/arm/arm32/lcpu.h      |  59 +++++
> >>>    include/uk/arch/arm/arm32/limits.h    |  45 ++++
> >>>    include/uk/arch/arm/arm32/types.h     |  35 +++
> >>>    include/uk/arch/arm/atomic.h          |  64 ------
> >>>    include/uk/arch/arm/intsizes.h        |  45 ----
> >>>    include/uk/arch/arm/lcpu.h            |  59 -----
> >>>    include/uk/arch/arm/limits.h          |  45 ----
> >>>    include/uk/arch/arm/types.h           |  35 ---
> >>>    include/uk/arch/atomic.h              |   8 +-
> >>>    include/uk/arch/lcpu.h                |   8 +-
> >>>    include/uk/arch/limits.h              |  16 +-
> >>>    include/uk/arch/types.h               |  16 +-
> >>>    include/uk/arch/x86/x86_64/atomic.h   |  45 ++++
> >>>    include/uk/arch/x86/x86_64/intsizes.h |  45 ++++
> >>>    include/uk/arch/x86/x86_64/lcpu.h     |  73 ++++++
> >>>    include/uk/arch/x86/x86_64/limits.h   |  46 ++++
> >>>    include/uk/arch/x86/x86_64/types.h    |  38 ++++
> >>>    include/uk/arch/x86_64/atomic.h       |  45 ----
> >>>    include/uk/arch/x86_64/intsizes.h     |  45 ----
> >>>    include/uk/arch/x86_64/lcpu.h         |  73 ------
> >>>    include/uk/arch/x86_64/limits.h       |  46 ----
> >>>    include/uk/arch/x86_64/types.h        |  38 ----
> >>>    support/build/Makefile.rules          |   8 +
> >>>    45 files changed, 1698 insertions(+), 1537 deletions(-)
> >>>    create mode 100644 arch/arm/arm32/divsi3.S
> >>>    create mode 100644 arch/arm/arm32/ldivmod.S
> >>>    create mode 100644 arch/arm/arm32/ldivmod_helper.c
> >>>    create mode 100644 arch/arm/arm32/qdivrem.c
> >>>    delete mode 100644 arch/arm/divsi3.S
> >>>    delete mode 100644 arch/arm/ldivmod.S
> >>>    delete mode 100644 arch/arm/ldivmod_helper.c
> >>>    delete mode 100644 arch/arm/qdivrem.c
> >>>    create mode 100644 arch/x86/Compiler.uk
> >>>    create mode 100644 arch/x86/Config.uk
> >>>    create mode 100644 arch/x86/Makefile.uk
> >>>    delete mode 100644 arch/x86_64/Compiler.uk
> >>>    delete mode 100644 arch/x86_64/Config.uk
> >>>    delete mode 100644 arch/x86_64/Makefile.uk
> >>>    create mode 100644 include/uk/arch/arm/arm32/atomic.h
> >>>    create mode 100644 include/uk/arch/arm/arm32/intsizes.h
> >>>    create mode 100644 include/uk/arch/arm/arm32/lcpu.h
> >>>    create mode 100644 include/uk/arch/arm/arm32/limits.h
> >>>    create mode 100644 include/uk/arch/arm/arm32/types.h
> >>>    delete mode 100644 include/uk/arch/arm/atomic.h
> >>>    delete mode 100644 include/uk/arch/arm/intsizes.h
> >>>    delete mode 100644 include/uk/arch/arm/lcpu.h
> >>>    delete mode 100644 include/uk/arch/arm/limits.h
> >>>    delete mode 100644 include/uk/arch/arm/types.h
> >>>    create mode 100644 include/uk/arch/x86/x86_64/atomic.h
> >>>    create mode 100644 include/uk/arch/x86/x86_64/intsizes.h
> >>>    create mode 100644 include/uk/arch/x86/x86_64/lcpu.h
> >>>    create mode 100644 include/uk/arch/x86/x86_64/limits.h
> >>>    create mode 100644 include/uk/arch/x86/x86_64/types.h
> >>>    delete mode 100644 include/uk/arch/x86_64/atomic.h
> >>>    delete mode 100644 include/uk/arch/x86_64/intsizes.h
> >>>    delete mode 100644 include/uk/arch/x86_64/lcpu.h
> >>>    delete mode 100644 include/uk/arch/x86_64/limits.h
> >>>    delete mode 100644 include/uk/arch/x86_64/types.h
> >>>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.