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

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



On 12.07.2018 11:57, Wei Chen wrote:
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 :)


I am fine with this. It also avoids discussions about which features are now actually used by Unikraft. ;-)

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

 


Rackspace

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