While for CONFIG_USER_ONLY it is policy for the "cpu" to be the most
capable is can be this does cause problems. For example legacy RISU
runs would fail as there are a bunch of implemented instructions which
would have caused failures that now trigger actual calculations.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/cpu64.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 973614dfc6..0dc4debd9c 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -275,6 +275,26 @@ static void aarch64_cpu_set_aarch64(Object *obj, bool value, Error **errp)
     }
 }
 
+#ifdef CONFIG_USER_ONLY
+static bool aarch64_cpu_get_fp16(Object *obj, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    return arm_feature(&cpu->env, ARM_FEATURE_V8_FP16);
+}
+
+static void aarch64_cpu_set_fp16(Object *obj, bool value, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    if (value == false) {
+        unset_feature(&cpu->env, ARM_FEATURE_V8_FP16);
+    } else {
+        set_feature(&cpu->env, ARM_FEATURE_V8_FP16);
+    }
+}
+#endif
+
 static void aarch64_cpu_initfn(Object *obj)
 {
     object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
@@ -283,6 +303,13 @@ static void aarch64_cpu_initfn(Object *obj)
                                     "Set on/off to enable/disable aarch64 "
                                     "execution state ",
                                     NULL);
+#ifdef CONFIG_USER_ONLY
+    object_property_add_bool(obj, "fp16", aarch64_cpu_get_fp16,
+                             aarch64_cpu_set_fp16, NULL);
+    object_property_set_description(obj, "fp16",
+                                    "Set on/off to enable/disable FP16 extensions ",
+                                    NULL);
+#endif
 }
 
 static void aarch64_cpu_finalizefn(Object *obj)
-- 
2.15.1

On 02/08/2018 09:31 AM, Alex Bennée wrote:
> While for CONFIG_USER_ONLY it is policy for the "cpu" to be the most
> capable is can be this does cause problems. For example legacy RISU
> runs would fail as there are a bunch of implemented instructions which
> would have caused failures that now trigger actual calculations.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  target/arm/cpu64.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

On 8 February 2018 at 17:31, Alex Bennée <alex.bennee@linaro.org> wrote:
> While for CONFIG_USER_ONLY it is policy for the "cpu" to be the most
> capable is can be this does cause problems. For example legacy RISU
> runs would fail as there are a bunch of implemented instructions which
> would have caused failures that now trigger actual calculations.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>  static void aarch64_cpu_initfn(Object *obj)
>  {
>      object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
> @@ -283,6 +303,13 @@ static void aarch64_cpu_initfn(Object *obj)
>                                      "Set on/off to enable/disable aarch64 "
>                                      "execution state ",
>                                      NULL);
> +#ifdef CONFIG_USER_ONLY
> +    object_property_add_bool(obj, "fp16", aarch64_cpu_get_fp16,
> +                             aarch64_cpu_set_fp16, NULL);
> +    object_property_set_description(obj, "fp16",
> +                                    "Set on/off to enable/disable FP16 extensions ",
> +                                    NULL);
> +#endif
>  }

Good news everybody -- this is an opportunity for a naming bikeshed
discussion!

The property names we use here are effectively ABI because they'll
be available to the user on the command line, so we want to get the
right names. This is the first of these, but we can reasonably
assume we'll have more feature switches in the future for various
other optional instruction set extensions.

There are two obvious choices here:
 * use the architecture extension names from the Arm ARM A1.7.4
   (in this case that's "ARMv8.2-FP16", which we could reasonably
   abbreviate to fp16)
 * use the hwcaps names that Linux defines and prints in /proc/cpuinfo
   (in this case that would be a combination of "fphp" and "asimdhp",
   since hwcaps reflects the ID register setup that allows a CPU
   to report support for one and not the other)

Whatever we do, we should have a comment describing our naming
conventions, so we can follow it next time we add one of these...

thanks
-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 8 February 2018 at 17:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>> While for CONFIG_USER_ONLY it is policy for the "cpu" to be the most
>> capable is can be this does cause problems. For example legacy RISU
>> runs would fail as there are a bunch of implemented instructions which
>> would have caused failures that now trigger actual calculations.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
>>  static void aarch64_cpu_initfn(Object *obj)
>>  {
>>      object_property_add_bool(obj, "aarch64", aarch64_cpu_get_aarch64,
>> @@ -283,6 +303,13 @@ static void aarch64_cpu_initfn(Object *obj)
>>                                      "Set on/off to enable/disable aarch64 "
>>                                      "execution state ",
>>                                      NULL);
>> +#ifdef CONFIG_USER_ONLY
>> +    object_property_add_bool(obj, "fp16", aarch64_cpu_get_fp16,
>> +                             aarch64_cpu_set_fp16, NULL);
>> +    object_property_set_description(obj, "fp16",
>> +                                    "Set on/off to enable/disable FP16 extensions ",
>> +                                    NULL);
>> +#endif
>>  }
>
> Good news everybody -- this is an opportunity for a naming bikeshed
> discussion!

Everyone's favourite kind of discussion ;-)

> The property names we use here are effectively ABI because they'll
> be available to the user on the command line, so we want to get the
> right names. This is the first of these, but we can reasonably
> assume we'll have more feature switches in the future for various
> other optional instruction set extensions.
>
> There are two obvious choices here:
>  * use the architecture extension names from the Arm ARM A1.7.4
>    (in this case that's "ARMv8.2-FP16", which we could reasonably
>    abbreviate to fp16)

So since I last tested this stuff I noticed upstream broke my RISU
testing with the addition of the crypto instructions. The reason being
the RISU test set does exercise UNDEF's which get used in later revs.

However I realised I could use -cpu cortex-a57 to achieve the same thing
and avoid enabling features for later specs. Maybe it would be simpler
just to add cpu types for the baseline architecture profiles?

  -cpu armv8.0
  -cpu armv8.1
  -cpu armv8.2

Defaulting of course to the most capable CPU type for linux-user.

That said FP16 is an optional feature so it is perfectly legitimate to
have:

  -cpu armv8.2+fp16

In fact the manual goes further in allowing any v8.x+1 feature to be
snarfed into a v8.x confirming CPU.

That said *my* use case is turning features off, maybe that is enough to
expose a plain v8.0 on the command line for now until someone comes up
with a useful for case for building these franken-CPUs?

>  * use the hwcaps names that Linux defines and prints in /proc/cpuinfo
>    (in this case that would be a combination of "fphp" and "asimdhp",
>    since hwcaps reflects the ID register setup that allows a CPU
>    to report support for one and not the other)

In naming I favour the Arm ARM over whatever Linux-ism /proc came up
with.

>
> Whatever we do, we should have a comment describing our naming
> conventions, so we can follow it next time we add one of these...
>
> thanks
> -- PMM


--
Alex Bennée

On 02/21/2018 08:35 AM, Alex Bennée wrote:
>> Good news everybody -- this is an opportunity for a naming bikeshed
>> discussion!
...
>>  * use the hwcaps names that Linux defines and prints in /proc/cpuinfo
>>    (in this case that would be a combination of "fphp" and "asimdhp",
>>    since hwcaps reflects the ID register setup that allows a CPU
>>    to report support for one and not the other)
> 
> In naming I favour the Arm ARM over whatever Linux-ism /proc came up
> with.

Likewise.


r~