Re: [Minios-devel] [UNIKRAFT PATCHv4 08/43] arch: Add arm64 architecture config to menuconfig

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien.grall@xxxxxxx>
> Sent: 2018年7月9日 18:24
> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> simon.kuenzer@xxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 08/43] arch: Add arm64
> architecture config to menuconfig
> 
> 
> 
> On 09/07/18 10:03, Wei Chen wrote:
> > Hi Julien,
> 
> Hi Wei,
> 
> >> -----Original Message-----
> >> From: Julien Grall <julien.grall@xxxxxxx>
> >> Sent: 2018年7月8日 5:56
> >> To: Wei Chen <Wei.Chen@xxxxxxx>; minios-devel@xxxxxxxxxxxxxxxxxxxx;
> >> simon.kuenzer@xxxxxxxxx
> >> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> >> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 08/43] arch: Add arm64
> >> architecture config to menuconfig
> >>
> >> Hi,
> >>
> >> On 07/06/2018 10:03 AM, Wei Chen wrote:
> >>> Add the arm64 entry for menuconfig. As different silicon vendors may
> >>> have different 64-bit ARMv8 SoCs. If we want to add them to Config,
> >>
> >> I know some people will find me very picky :). Based on the new
> >
> > Sometimes ; )
> >
> >> branding, this should be Armv8 (i.e no upper-case for r, m). I am not
> >> too fuss for the commit message, but I would like to be at list fixed in
> >> the Kconfig description.
> >
> > Honestly, Arm looks very very awkward to me. But I think you're right, it's
> > the new branding, I would change them to Arm, although I still think arm or
> > ARM looks better. . Maybe I am a little Obsessive compulsive : )
> 
> Sadly, 'Arm' or 'arm' is the way to go nowadays. The latter is preferred
> in sentence to avoid confusion with another close word ;).
> 
> 'ARM' should not be used anymore.
> 
> >
> >>
> >>> it will be a large list. So we only provide ARM's cortex A53~A75 CPUs
> >>
> >> Sam here.
> >
> > Ok.
> >
> >>
> >>> for "Processor Optimization"
> >>>
> >>> If we use MARCH_ as the prefix for ARM64 CPUs as x86, when we select
> >>> "generic", the MARCH_GENERIC will conflict with x86's MARCH_GENERIC.
> >>> So, we use MARCH_ARM64_ for ARM64 as the prefix.
> >>>
> >>> Current supported arm64 CPU models:
> >>> native, generic, cortex-a53, cortex-a57, cortex-a72, cortex-a73,
> >>> cortex-a55 and cortex-a75.
> >>>
> >>> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> >>> ---
> >>>    Config.uk                |  2 +-
> >>>    arch/Arch.uk             |  2 ++
> >>>    arch/Config.uk           |  6 ++++
> >>>    arch/arm/arm64/Config.uk | 60 ++++++++++++++++++++++++++++++++++++++++
> >>>    4 files changed, 69 insertions(+), 1 deletion(-)
> >>>    create mode 100644 arch/arm/arm64/Config.uk
> >>>
> >>> diff --git a/Config.uk b/Config.uk
> >>> index 21cec9b..e7a26b2 100644
> >>> --- a/Config.uk
> >>> +++ b/Config.uk
> >>> @@ -65,7 +65,7 @@ config OPTIMIZE_SIZE
> >>>    endchoice
> >>>
> >>>    comment "Hint: Specify a CPU type to get most benefits from performance
> >> optimization"
> >>> - depends on OPTIMIZE_PERF && MARCH_GENERIC
> >>> + depends on OPTIMIZE_PERF && (MARCH_GENERIC || MARCH_ARM64_GENERIC)
> >>
> >> Not even looking at the code, the naming looks wrong here. When I read
> >> MARCH_GENERIC, I would expect to be selected by anyone.
> >>
> >> It feels like to me we want to introduce yet another Kconfig
> >> HAS_OPTIMIZE_PERF that will be selected by MARCH_GENERIC (x86) and
> >> MARCH_ARM64_GENERIC (Arm64).
> >>
> >
> > MARCH_GENERIC here should be MARCH_X86_64_GENERIC. When Simon released this
> > code, Unikraft only support x86_64. So he didn't add X86_64 to this
> > CONFIG_OPTION. I have renamed MARCH_GENERIC to MARCH_X86_64_GENERIC
> > in next patch.
> 
> Can you move this next patch before? This would make clearer this patch.
> 

Ok, I will adjust the order.

> >
> >>>
> >>>    config OPTIMIZE_DEADELIM
> >>>           bool "Drop unused functions and data"
> >>> diff --git a/arch/Arch.uk b/arch/Arch.uk
> >>> index f11308b..a8b3ca2 100644
> >>> --- a/arch/Arch.uk
> >>> +++ b/arch/Arch.uk
> >>> @@ -1,6 +1,8 @@
> >>>    # Selects architecture according to .config
> >>>    ifeq ($(CONFIG_ARCH_X86_64),y)
> >>>    CONFIG_UK_ARCH := x86_64
> >>> +else ifeq ($(CONFIG_ARCH_ARM_64),y)
> >>> +CONFIG_UK_ARCH := arm64
> >>>    else ifeq ($(CONFIG_ARCH_ARM_32),y)
> >>>    CONFIG_UK_ARCH := arm
> >>>    endif
> >>> diff --git a/arch/Config.uk b/arch/Config.uk
> >>> index 9236273..f08274d 100644
> >>> --- a/arch/Config.uk
> >>> +++ b/arch/Config.uk
> >>> @@ -1,12 +1,15 @@
> >>>    choice
> >>>           prompt "Architecture"
> >>>           default ARCH_ARM_32 if (UK_ARCH = "arm")
> >>> + default ARCH_ARM_64 if (UK_ARCH = "arm64")
> >>>           default ARCH_X86_64
> >>>           help
> >>>             Select the target CPU architecture.
> >>>
> >>>    config ARCH_X86_64
> >>>           bool "x86 compatible (64 bits)"
> >>> +config ARCH_ARM_64
> >>> +       bool "ARMv8 compatible (64 bits)"
> >>>    config ARCH_ARM_32
> >>>           bool "ARMv7 compatible (32 bits)"
> >>>
> >>> @@ -18,3 +21,6 @@ endif
> >>>    if (ARCH_ARM_32)
> >>>           source "arch/arm/arm/Config.uk"
> >>>    endif
> >>> +if (ARCH_ARM_64)
> >>> + source "arch/arm/arm64/Config.uk"
> >>> +endif
> >>> diff --git a/arch/arm/arm64/Config.uk b/arch/arm/arm64/Config.uk
> >>> new file mode 100644
> >>> index 0000000..634ec50
> >>> --- /dev/null
> >>> +++ b/arch/arm/arm64/Config.uk
> >>> @@ -0,0 +1,60 @@
> >>> +choice
> >>> + prompt "Processor Optimization"
> >>> + default MARCH_ARM64_GENERIC
> >>
> >> Do we really need to have ARM64 in the name?
> >>
> >
> > Yes, we have MARCH_X86_64_GENERIC, MARCH_ARM64_GENERIC now. And in
> > The future we may have MARCH_ARM_GENERIC, MARCH_PPC64_GENERIC and
> > etc. Without them, in some cases, we have to use following similar
> > combination: CONFIG_ARM64 && CONFIG_MARCH_GENERIC
> >
> >
> >>> + help
> >>> +         Optimize the code for selected target processor
> >>> +
> >>> +config MARCH_ARM64_NATIVE
> >>> + bool "Auto-detect host CPU"
> >>> + help
> >>> +         Optimize compilation to host CPU. Please note that this
> >>> +         option will fail in case of cross-compilation
> >>> +
> >>> +config MARCH_ARM64_GENERIC
> >>> + bool "Generic ARMv8 CPU"
> >>
> >> s/ARM/Arm/
> >
> > Ok.
> >
> >>
> >>> + help
> >>> +         Compile for Generic ARMv8 compatible CPUs
> >>> +
> >>> +config MARCH_ARM64_CORTEXA53
> >>> + bool "Generic ARMv8 Cortex A53"
> >>> + help
> >>> +         Compile for ARMv8 Cortex-A53 CPUs. Support TrustZone, NEON
> >>
> >> Ditto.
> >
> > Ok
> >
> >>
> >>> +         advanced SIMD, VFPv4, hardware virtualization, dual issue,
> >>
> >> How virtualization matters for Unikraft? Shouldn't this just describe
> >> what will be the benefits for Unikraft?
> >
> > This is just a description for the Cortex-A53. I copy them from wiki.
> 
> Which wiki? In general, the description of a config should explain why a
> user should select the option. It does not need to know that the
> Cortex-A53 supports virtualization (or even allow 32-bit).
> 

I forget, maybe from Wikipedia.

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel