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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

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?

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