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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Simon,

> -----Original Message-----
> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Sent: 2018年7月12日 17:44
> To: Wei Chen <Wei.Chen@xxxxxxx>; Julien Grall <Julien.Grall@xxxxxxx>; minios-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Kaly Xin <Kaly.Xin@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [Minios-devel] [UNIKRAFT PATCHv4 08/43] arch: Add arm64
> architecture config to menuconfig
> 
> Hey Wei, Julien,
> 
> On 10.07.2018 09:16, Wei Chen wrote:
> > 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.
> >>
> 
> Now I learned also something new to me ;-). You are the Arm guys, I
> trust you about the proper typing of Arm.
> 
> >>>
> >>>>
> >>>>> 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.
> >
> 
> For me, both orders are fine.
> 
> >>>
> >>>>>
> >>>>>     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
> >>>
> 
> I agree.
> 
> >>>
> >>>>> +       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.
> >>
> 
> Hum, I retrieved the x86 descriptions from GCC. They are not talking
> about virtualization but if it is properly worded it is fine to mention
> it here (same for TrustZone). It just would need to be clear that we are
> talking about features that a particular CPU/SoC has - and not that
> Unikraft needs it or is going to use it:
> 
> I would do something along the lines. Basically, remove "the compile
> for" to tell which features an Arm v8 SoC/CPU has:
> 
> config MARCH_ARM64_CORTEXA53
> bool "Generic Armv8 Cortex A53"
> help
>       ARMv8 Cortex-A53 CPUs with NEON, advanced SIMD,
>       VFPv4, TrustZone, hardware virtualization, [...] support
> 
> However, it is also fine to do:
> 
> config MARCH_ARM64_CORTEXA53
> bool "Generic Armv8 Cortex A53"
> help
>       Compile for ARMv8 Cortex-A53 (and compatible) CPUs
> 
> Cortex-A53 actually defines a particular feature set, right?
> What do you think?
> 

It seems the second one is more pretty to me :)

> >> 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