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

[Julien Grall <julien.grall@xxxxxxx>]

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.

> > >    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).
Cheers,

--
Julien Grall

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