[PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
spent up to 50% of the emulation time running it (when using TCG).

Switching to pauth-impdef=on is often given as a solution to speed up execution.
Thus we talked about making it the new default.

The first patch introduce a new property (pauth-qarma5) to allow to select
current default algorithm.
The second one change the default.

Pierrick Bouvier (2):
  target/arm: add new property to select pauth-qarma5
  target/arm: change default pauth algorithm to impdef

 docs/system/arm/cpu-features.rst |  7 +++++--
 docs/system/introduction.rst     |  2 +-
 target/arm/cpu.h                 |  1 +
 target/arm/arm-qmp-cmds.c        |  2 +-
 target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
 tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
 6 files changed, 38 insertions(+), 19 deletions(-)

-- 
2.39.5

[PATCH 1/2] target/arm: add new property to select pauth-qarma5

[Pierrick Bouvier]

Before changing default pauth algorithm, we need to make sure current
default one (QARMA5) can still be selected.

$ qemu-system-aarch64 -cpu max,pauth-qarma5=on ...

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 docs/system/arm/cpu-features.rst |  5 ++++-
 target/arm/cpu.h                 |  1 +
 target/arm/arm-qmp-cmds.c        |  2 +-
 target/arm/cpu64.c               | 20 ++++++++++++++------
 tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
 5 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index a5fb929243c..d69ebc2b852 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -219,7 +219,10 @@ Below is the list of TCG VCPU features and their descriptions.
 ``pauth-qarma3``
   When ``pauth`` is enabled, select the architected QARMA3 algorithm.
 
-Without either ``pauth-impdef`` or ``pauth-qarma3`` enabled,
+``pauth-qarma5``
+  When ``pauth`` is enabled, select the architected QARMA5 algorithm.
+
+Without ``pauth-impdef``, ``pauth-qarma3`` or ``pauth-qarma5`` enabled,
 the architected QARMA5 algorithm is used.  The architected QARMA5
 and QARMA3 algorithms have good cryptographic properties, but can
 be quite slow to emulate.  The impdef algorithm used by QEMU is
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d86e641280d..b7500bebd7f 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1062,6 +1062,7 @@ struct ArchCPU {
     bool prop_pauth;
     bool prop_pauth_impdef;
     bool prop_pauth_qarma3;
+    bool prop_pauth_qarma5;
     bool prop_lpa2;
 
     /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 3cc8cc738bb..33cea080d11 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -94,7 +94,7 @@ static const char *cpu_model_advertised_features[] = {
     "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
     "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
     "kvm-no-adjvtime", "kvm-steal-time",
-    "pauth", "pauth-impdef", "pauth-qarma3",
+    "pauth", "pauth-impdef", "pauth-qarma3", "pauth-qarma5",
     NULL
 };
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 458d1cee012..34ef46d148f 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -520,9 +520,12 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
         }
 
         if (cpu->prop_pauth) {
-            if (cpu->prop_pauth_impdef && cpu->prop_pauth_qarma3) {
+            if ((cpu->prop_pauth_impdef && cpu->prop_pauth_qarma3) ||
+                (cpu->prop_pauth_impdef && cpu->prop_pauth_qarma5) ||
+                (cpu->prop_pauth_qarma3 && cpu->prop_pauth_qarma5)) {
                 error_setg(errp,
-                           "cannot enable both pauth-impdef and pauth-qarma3");
+                           "cannot enable pauth-impdef, pauth-qarma3 and "
+                           "pauth-qarma5 at the same time");
                 return;
             }
 
@@ -532,13 +535,15 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
             } else if (cpu->prop_pauth_qarma3) {
                 isar2 = FIELD_DP64(isar2, ID_AA64ISAR2, APA3, features);
                 isar2 = FIELD_DP64(isar2, ID_AA64ISAR2, GPA3, 1);
-            } else {
+            } else { /* default is pauth-qarma5 */
                 isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, APA, features);
                 isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, GPA, 1);
             }
-        } else if (cpu->prop_pauth_impdef || cpu->prop_pauth_qarma3) {
-            error_setg(errp, "cannot enable pauth-impdef or "
-                       "pauth-qarma3 without pauth");
+        } else if (cpu->prop_pauth_impdef ||
+                   cpu->prop_pauth_qarma3 ||
+                   cpu->prop_pauth_qarma5) {
+            error_setg(errp, "cannot enable pauth-impdef, pauth-qarma3 or "
+                       "pauth-qarma5 without pauth");
             error_append_hint(errp, "Add pauth=on to the CPU property list.\n");
         }
     }
@@ -553,6 +558,8 @@ static Property arm_cpu_pauth_impdef_property =
     DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
 static Property arm_cpu_pauth_qarma3_property =
     DEFINE_PROP_BOOL("pauth-qarma3", ARMCPU, prop_pauth_qarma3, false);
+static Property arm_cpu_pauth_qarma5_property =
+    DEFINE_PROP_BOOL("pauth-qarma5", ARMCPU, prop_pauth_qarma5, false);
 
 void aarch64_add_pauth_properties(Object *obj)
 {
@@ -573,6 +580,7 @@ void aarch64_add_pauth_properties(Object *obj)
     } else {
         qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
         qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_qarma3_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_qarma5_property);
     }
 }
 
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index cfd6f773535..98d6c970ea5 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -419,21 +419,28 @@ static void pauth_tests_default(QTestState *qts, const char *cpu_type)
     assert_has_feature_enabled(qts, cpu_type, "pauth");
     assert_has_feature_disabled(qts, cpu_type, "pauth-impdef");
     assert_has_feature_disabled(qts, cpu_type, "pauth-qarma3");
+    assert_has_feature_disabled(qts, cpu_type, "pauth-qarma5");
     assert_set_feature(qts, cpu_type, "pauth", false);
     assert_set_feature(qts, cpu_type, "pauth", true);
     assert_set_feature(qts, cpu_type, "pauth-impdef", true);
     assert_set_feature(qts, cpu_type, "pauth-impdef", false);
     assert_set_feature(qts, cpu_type, "pauth-qarma3", true);
     assert_set_feature(qts, cpu_type, "pauth-qarma3", false);
+    assert_set_feature(qts, cpu_type, "pauth-qarma5", true);
+    assert_set_feature(qts, cpu_type, "pauth-qarma5", false);
     assert_error(qts, cpu_type,
-                 "cannot enable pauth-impdef or pauth-qarma3 without pauth",
+                 "cannot enable pauth-impdef, pauth-qarma3 or pauth-qarma5 without pauth",
                  "{ 'pauth': false, 'pauth-impdef': true }");
     assert_error(qts, cpu_type,
-                 "cannot enable pauth-impdef or pauth-qarma3 without pauth",
+                 "cannot enable pauth-impdef, pauth-qarma3 or pauth-qarma5 without pauth",
                  "{ 'pauth': false, 'pauth-qarma3': true }");
     assert_error(qts, cpu_type,
-                 "cannot enable both pauth-impdef and pauth-qarma3",
-                 "{ 'pauth': true, 'pauth-impdef': true, 'pauth-qarma3': true }");
+                 "cannot enable pauth-impdef, pauth-qarma3 or pauth-qarma5 without pauth",
+                 "{ 'pauth': false, 'pauth-qarma5': true }");
+    assert_error(qts, cpu_type,
+                 "cannot enable pauth-impdef, pauth-qarma3 and pauth-qarma5 at the same time",
+                 "{ 'pauth': true, 'pauth-impdef': true, 'pauth-qarma3': true,"
+                 "  'pauth-qarma5': true }");
 }
 
 static void test_query_cpu_model_expansion(const void *data)
-- 
2.39.5

[PATCH 2/2] target/arm: change default pauth algorithm to impdef

[Pierrick Bouvier]

Pointer authentication on aarch64 is pretty expensive (up to 50% of
execution time) when running a virtual machine with tcg and -cpu max
(which enables pauth=on).

The advice is always: use pauth-impdef=on.
Our documentation even mentions it "by default" in
docs/system/introduction.rst.

Thus, we change the default to use impdef by default. This does not
affect kvm or hvf acceleration, since pauth algorithm used is the one
from host cpu.

This change is retro compatible, in terms of cli, with previous
versions, as the semantic of using -cpu max,pauth-impdef=on, and -cpu
max,pauth-qarma3=on is preserved.
The new option introduced in previous patch and matching old default is
-cpu max,pauth-qarma5=on.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 docs/system/arm/cpu-features.rst |  2 +-
 docs/system/introduction.rst     |  2 +-
 target/arm/cpu64.c               | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index d69ebc2b852..37d5dfd15b3 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -223,7 +223,7 @@ Below is the list of TCG VCPU features and their descriptions.
   When ``pauth`` is enabled, select the architected QARMA5 algorithm.
 
 Without ``pauth-impdef``, ``pauth-qarma3`` or ``pauth-qarma5`` enabled,
-the architected QARMA5 algorithm is used.  The architected QARMA5
+the QEMU impdef algorithm is used.  The architected QARMA5
 and QARMA3 algorithms have good cryptographic properties, but can
 be quite slow to emulate.  The impdef algorithm used by QEMU is
 non-cryptographic but significantly faster.
diff --git a/docs/system/introduction.rst b/docs/system/introduction.rst
index 746707eb00e..338d3745c3c 100644
--- a/docs/system/introduction.rst
+++ b/docs/system/introduction.rst
@@ -169,7 +169,7 @@ would default to it anyway.
 
 .. code::
 
- -cpu max,pauth-impdef=on \
+ -cpu max \
  -smp 4 \
  -accel tcg \
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 34ef46d148f..8b1f26a9664 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -529,15 +529,15 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
                 return;
             }
 
-            if (cpu->prop_pauth_impdef) {
-                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, API, features);
-                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, GPI, 1);
+            if (cpu->prop_pauth_qarma5) {
+                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, APA, features);
+                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, GPA, 1);
             } else if (cpu->prop_pauth_qarma3) {
                 isar2 = FIELD_DP64(isar2, ID_AA64ISAR2, APA3, features);
                 isar2 = FIELD_DP64(isar2, ID_AA64ISAR2, GPA3, 1);
-            } else { /* default is pauth-qarma5 */
-                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, APA, features);
-                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, GPA, 1);
+            } else { /* default is pauth-impdef */
+                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, API, features);
+                isar1 = FIELD_DP64(isar1, ID_AA64ISAR1, GPI, 1);
             }
         } else if (cpu->prop_pauth_impdef ||
                    cpu->prop_pauth_qarma3 ||
-- 
2.39.5

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

Hi,

On 12/4/24 13:12, Pierrick Bouvier wrote:
> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
> spent up to 50% of the emulation time running it (when using TCG).
> 
> Switching to pauth-impdef=on is often given as a solution to speed up execution.
> Thus we talked about making it the new default.
> 
> The first patch introduce a new property (pauth-qarma5) to allow to select
> current default algorithm.
> The second one change the default.
> 
> Pierrick Bouvier (2):
>    target/arm: add new property to select pauth-qarma5
>    target/arm: change default pauth algorithm to impdef
> 
>   docs/system/arm/cpu-features.rst |  7 +++++--
>   docs/system/introduction.rst     |  2 +-
>   target/arm/cpu.h                 |  1 +
>   target/arm/arm-qmp-cmds.c        |  2 +-
>   target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>   tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>   6 files changed, 38 insertions(+), 19 deletions(-)
> 

gentle ping on this series.

Thanks,
Pierrick

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Richard Henderson]

On 12/4/24 15:12, Pierrick Bouvier wrote:
> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
> spent up to 50% of the emulation time running it (when using TCG).
> 
> Switching to pauth-impdef=on is often given as a solution to speed up execution.
> Thus we talked about making it the new default.
> 
> The first patch introduce a new property (pauth-qarma5) to allow to select
> current default algorithm.
> The second one change the default.
> 
> Pierrick Bouvier (2):
>    target/arm: add new property to select pauth-qarma5
>    target/arm: change default pauth algorithm to impdef
> 
>   docs/system/arm/cpu-features.rst |  7 +++++--
>   docs/system/introduction.rst     |  2 +-
>   target/arm/cpu.h                 |  1 +
>   target/arm/arm-qmp-cmds.c        |  2 +-
>   target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>   tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>   6 files changed, 38 insertions(+), 19 deletions(-)
> 

I understand the motivation, but as-is this will break migration.

I think this will need to be versioned somehow, but the only thing that really gets 
versioned are the boards, and I'm not sure how to link that to the instantiated cpu.


r~

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

On 12/16/24 11:10, Richard Henderson wrote:
> On 12/4/24 15:12, Pierrick Bouvier wrote:
>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>> spent up to 50% of the emulation time running it (when using TCG).
>>
>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>> Thus we talked about making it the new default.
>>
>> The first patch introduce a new property (pauth-qarma5) to allow to select
>> current default algorithm.
>> The second one change the default.
>>
>> Pierrick Bouvier (2):
>>     target/arm: add new property to select pauth-qarma5
>>     target/arm: change default pauth algorithm to impdef
>>
>>    docs/system/arm/cpu-features.rst |  7 +++++--
>>    docs/system/introduction.rst     |  2 +-
>>    target/arm/cpu.h                 |  1 +
>>    target/arm/arm-qmp-cmds.c        |  2 +-
>>    target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>    tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>    6 files changed, 38 insertions(+), 19 deletions(-)
>>
> 
> I understand the motivation, but as-is this will break migration.
> 
> I think this will need to be versioned somehow, but the only thing that really gets
> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>

 From what I understood, and I may be wrong, the use case to migrate 
(tcg) vm with cpu max between QEMU versions is *not* supported, as we 
can't guarantee which features are present or not.

And I'm not sure we want to introduce the notion of "versioned" cpu just 
for this. Maybe we could simply restrict migration between major QEMU 
versions in this scenario.

> 
> r~

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Richard Henderson]

On 12/16/24 13:26, Pierrick Bouvier wrote:
> On 12/16/24 11:10, Richard Henderson wrote:
>> On 12/4/24 15:12, Pierrick Bouvier wrote:
>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>>> spent up to 50% of the emulation time running it (when using TCG).
>>>
>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>>> Thus we talked about making it the new default.
>>>
>>> The first patch introduce a new property (pauth-qarma5) to allow to select
>>> current default algorithm.
>>> The second one change the default.
>>>
>>> Pierrick Bouvier (2):
>>>     target/arm: add new property to select pauth-qarma5
>>>     target/arm: change default pauth algorithm to impdef
>>>
>>>    docs/system/arm/cpu-features.rst |  7 +++++--
>>>    docs/system/introduction.rst     |  2 +-
>>>    target/arm/cpu.h                 |  1 +
>>>    target/arm/arm-qmp-cmds.c        |  2 +-
>>>    target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>>    tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>>    6 files changed, 38 insertions(+), 19 deletions(-)
>>>
>>
>> I understand the motivation, but as-is this will break migration.
>>
>> I think this will need to be versioned somehow, but the only thing that really gets
>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>>
> 
>  From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max 
> between QEMU versions is *not* supported, as we can't guarantee which features are present 
> or not.
This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties(): 
neoverse-n1, neoverse-n2, cortex-a710.


r~

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

On 12/16/24 11:50, Richard Henderson wrote:
> On 12/16/24 13:26, Pierrick Bouvier wrote:
>> On 12/16/24 11:10, Richard Henderson wrote:
>>> On 12/4/24 15:12, Pierrick Bouvier wrote:
>>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>>>> spent up to 50% of the emulation time running it (when using TCG).
>>>>
>>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>>>> Thus we talked about making it the new default.
>>>>
>>>> The first patch introduce a new property (pauth-qarma5) to allow to select
>>>> current default algorithm.
>>>> The second one change the default.
>>>>
>>>> Pierrick Bouvier (2):
>>>>      target/arm: add new property to select pauth-qarma5
>>>>      target/arm: change default pauth algorithm to impdef
>>>>
>>>>     docs/system/arm/cpu-features.rst |  7 +++++--
>>>>     docs/system/introduction.rst     |  2 +-
>>>>     target/arm/cpu.h                 |  1 +
>>>>     target/arm/arm-qmp-cmds.c        |  2 +-
>>>>     target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>>>     tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>>>     6 files changed, 38 insertions(+), 19 deletions(-)
>>>>
>>>
>>> I understand the motivation, but as-is this will break migration.
>>>
>>> I think this will need to be versioned somehow, but the only thing that really gets
>>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>>>
>>
>>   From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max
>> between QEMU versions is *not* supported, as we can't guarantee which features are present
>> or not.
> This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties():
> neoverse-n1, neoverse-n2, cortex-a710.
> 

I think this is still a change worth to do, because people can get a 
100% speedup with this simple change, and it's a better default than the 
previous value.
In more, in case of this migration scenario, QEMU will immediately abort 
upon accessing memory through a pointer.

I'm not sure about what would be the best way to make this change as 
smooth as possible for QEMU users.

Peter, Alex, do you have any suggestion on this topic?

Thanks,
Pierrick

> 
> r~

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 12/16/24 11:50, Richard Henderson wrote:
>> On 12/16/24 13:26, Pierrick Bouvier wrote:
>>> On 12/16/24 11:10, Richard Henderson wrote:
>>>> On 12/4/24 15:12, Pierrick Bouvier wrote:
>>>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>>>>> spent up to 50% of the emulation time running it (when using TCG).
>>>>>
>>>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>>>>> Thus we talked about making it the new default.
>>>>>
>>>>> The first patch introduce a new property (pauth-qarma5) to allow to select
>>>>> current default algorithm.
>>>>> The second one change the default.
>>>>>
>>>>> Pierrick Bouvier (2):
>>>>>      target/arm: add new property to select pauth-qarma5
>>>>>      target/arm: change default pauth algorithm to impdef
>>>>>
>>>>>     docs/system/arm/cpu-features.rst |  7 +++++--
>>>>>     docs/system/introduction.rst     |  2 +-
>>>>>     target/arm/cpu.h                 |  1 +
>>>>>     target/arm/arm-qmp-cmds.c        |  2 +-
>>>>>     target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>>>>     tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>>>>     6 files changed, 38 insertions(+), 19 deletions(-)
>>>>>
>>>>
>>>> I understand the motivation, but as-is this will break migration.
>>>>
>>>> I think this will need to be versioned somehow, but the only thing that really gets
>>>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>>>>
>>>
>>>   From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max
>>> between QEMU versions is *not* supported, as we can't guarantee which features are present
>>> or not.
>> This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties():
>> neoverse-n1, neoverse-n2, cortex-a710.
>> 
>
> I think this is still a change worth to do, because people can get a
> 100% speedup with this simple change, and it's a better default than
> the previous value.
> In more, in case of this migration scenario, QEMU will immediately
> abort upon accessing memory through a pointer.
>
> I'm not sure about what would be the best way to make this change as
> smooth as possible for QEMU users.

Surely we can only honour and apply the new default to -cpu max?

>
> Peter, Alex, do you have any suggestion on this topic?
>
> Thanks,
> Pierrick
>
>> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Peter Maydell]

On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
> > On 12/16/24 11:50, Richard Henderson wrote:
> >> On 12/16/24 13:26, Pierrick Bouvier wrote:
> >>> On 12/16/24 11:10, Richard Henderson wrote:
> >>>> On 12/4/24 15:12, Pierrick Bouvier wrote:
> >>>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
> >>>>> spent up to 50% of the emulation time running it (when using TCG).
> >>>>>
> >>>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
> >>>>> Thus we talked about making it the new default.
> >>>>>
> >>>>> The first patch introduce a new property (pauth-qarma5) to allow to select
> >>>>> current default algorithm.
> >>>>> The second one change the default.
> >>>>>
> >>>>> Pierrick Bouvier (2):
> >>>>>      target/arm: add new property to select pauth-qarma5
> >>>>>      target/arm: change default pauth algorithm to impdef
> >>>>>
> >>>>>     docs/system/arm/cpu-features.rst |  7 +++++--
> >>>>>     docs/system/introduction.rst     |  2 +-
> >>>>>     target/arm/cpu.h                 |  1 +
> >>>>>     target/arm/arm-qmp-cmds.c        |  2 +-
> >>>>>     target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
> >>>>>     tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
> >>>>>     6 files changed, 38 insertions(+), 19 deletions(-)
> >>>>>
> >>>>
> >>>> I understand the motivation, but as-is this will break migration.
> >>>>
> >>>> I think this will need to be versioned somehow, but the only thing that really gets
> >>>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
> >>>>
> >>>
> >>>   From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max
> >>> between QEMU versions is *not* supported, as we can't guarantee which features are present
> >>> or not.
> >> This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties():
> >> neoverse-n1, neoverse-n2, cortex-a710.
> >>
> >
> > I think this is still a change worth to do, because people can get a
> > 100% speedup with this simple change, and it's a better default than
> > the previous value.
> > In more, in case of this migration scenario, QEMU will immediately
> > abort upon accessing memory through a pointer.
> >
> > I'm not sure about what would be the best way to make this change as
> > smooth as possible for QEMU users.
>
> Surely we can only honour and apply the new default to -cpu max?

That was what I thought we were aiming for, yes. We *could* have
a property on the CPU to say "use the old back-compatible default,
not the new one", which we then list in the appropriate hw_compat
array. (Grep for the "backcompat-cntfrq" property for an example of
this.) But I'm not sure if that is worth the effort compared to
just changing 'max'.

(It's not that much extra code to add the property, so I could
easily be persuaded the other way. Possible arguments include
preferring consistency across all CPUs. If we already make the
default be not "what the real CPU of this type uses" then that's
also an argument that we can set it to whatever is convenient;
if we do honour the CPU ID register values for the implementation
default then that's an argument that we should continue to do
so and not change the default to our impdef one.)

-- PMM

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

On 12/17/24 02:38, Peter Maydell wrote:
> On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>
>>> On 12/16/24 11:50, Richard Henderson wrote:
>>>> On 12/16/24 13:26, Pierrick Bouvier wrote:
>>>>> On 12/16/24 11:10, Richard Henderson wrote:
>>>>>> On 12/4/24 15:12, Pierrick Bouvier wrote:
>>>>>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>>>>>>> spent up to 50% of the emulation time running it (when using TCG).
>>>>>>>
>>>>>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>>>>>>> Thus we talked about making it the new default.
>>>>>>>
>>>>>>> The first patch introduce a new property (pauth-qarma5) to allow to select
>>>>>>> current default algorithm.
>>>>>>> The second one change the default.
>>>>>>>
>>>>>>> Pierrick Bouvier (2):
>>>>>>>       target/arm: add new property to select pauth-qarma5
>>>>>>>       target/arm: change default pauth algorithm to impdef
>>>>>>>
>>>>>>>      docs/system/arm/cpu-features.rst |  7 +++++--
>>>>>>>      docs/system/introduction.rst     |  2 +-
>>>>>>>      target/arm/cpu.h                 |  1 +
>>>>>>>      target/arm/arm-qmp-cmds.c        |  2 +-
>>>>>>>      target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>>>>>>      tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>>>>>>      6 files changed, 38 insertions(+), 19 deletions(-)
>>>>>>>
>>>>>>
>>>>>> I understand the motivation, but as-is this will break migration.
>>>>>>
>>>>>> I think this will need to be versioned somehow, but the only thing that really gets
>>>>>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>>>>>>
>>>>>
>>>>>    From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max
>>>>> between QEMU versions is *not* supported, as we can't guarantee which features are present
>>>>> or not.
>>>> This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties():
>>>> neoverse-n1, neoverse-n2, cortex-a710.
>>>>
>>>
>>> I think this is still a change worth to do, because people can get a
>>> 100% speedup with this simple change, and it's a better default than
>>> the previous value.
>>> In more, in case of this migration scenario, QEMU will immediately
>>> abort upon accessing memory through a pointer.
>>>
>>> I'm not sure about what would be the best way to make this change as
>>> smooth as possible for QEMU users.
>>
>> Surely we can only honour and apply the new default to -cpu max?
> 

With all my respect, I think the current default is wrong, and it would 
be sad to keep it when people don't precise cpu max, or for other cpus 
enabling pointer authentication.

In all our conversations, there seems to be a focus on choosing the 
"fastest" emulation solution that satisfies the guest (behaviour wise). 
And, for a reason I ignore, pointer authentication escaped this rule.

I understand the concern regarding retro compatibility, but it would be 
better to ask politely (with an error message) to people to restart 
their virtual machines when they try to migrate, instead of being stuck 
with a slow default forever.
In more, we are talking of a tcg scenario, for which I'm not sure people 
use migration feature (save/restore) heavily, but I may be wrong on this.

Between the risk of breaking migration (with a polite error message), 
and having a default that is 100% faster, I think it would be better to 
favor the second one. If it would be a 5% speedup, I would not argue, 
but slowing down execution with a factor of 2 is really a lot.

> That was what I thought we were aiming for, yes. We *could* have
> a property on the CPU to say "use the old back-compatible default,
> not the new one", which we then list in the appropriate hw_compat
> array. (Grep for the "backcompat-cntfrq" property for an example of
> this.) But I'm not sure if that is worth the effort compared to
> just changing 'max'.

When we'll define hw_compat_10_0, and hw_compat_11_0, do we have to 
carry this on forever? (Same question for "backcompat-cntfrq").

> 
> (It's not that much extra code to add the property, so I could
> easily be persuaded the other way. Possible arguments include
> preferring consistency across all CPUs. If we already make the
> default be not "what the real CPU of this type uses" then that's
> also an argument that we can set it to whatever is convenient;
> if we do honour the CPU ID register values for the implementation
> default then that's an argument that we should continue to do
> so and not change the default to our impdef one.)
>

For the TCG use case, is there any visible side effect for the guest to 
use any specific pointer authentication algorithm?
In other words, is there a scenario where pointer authentication would 
work with impdef, but not with qarma{3,5}?
If no, I don't see any reason for a cpu to favor an expensive emulation.

In the accelerator case, we read the values from the host cpu, so there 
is no problem.

> -- PMM

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 12/17/24 02:38, Peter Maydell wrote:
>> On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> On 12/16/24 11:50, Richard Henderson wrote:
>>>>> On 12/16/24 13:26, Pierrick Bouvier wrote:
>>>>>> On 12/16/24 11:10, Richard Henderson wrote:
>>>>>>> On 12/4/24 15:12, Pierrick Bouvier wrote:
>>>>>>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>>>>>>>> spent up to 50% of the emulation time running it (when using TCG).
>>>>>>>>
>>>>>>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>>>>>>>> Thus we talked about making it the new default.
>>>>>>>>
>>>>>>>> The first patch introduce a new property (pauth-qarma5) to allow to select
>>>>>>>> current default algorithm.
>>>>>>>> The second one change the default.
>>>>>>>>
>>>>>>>> Pierrick Bouvier (2):
>>>>>>>>       target/arm: add new property to select pauth-qarma5
>>>>>>>>       target/arm: change default pauth algorithm to impdef
>>>>>>>>
>>>>>>>>      docs/system/arm/cpu-features.rst |  7 +++++--
>>>>>>>>      docs/system/introduction.rst     |  2 +-
>>>>>>>>      target/arm/cpu.h                 |  1 +
>>>>>>>>      target/arm/arm-qmp-cmds.c        |  2 +-
>>>>>>>>      target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>>>>>>>      tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>>>>>>>      6 files changed, 38 insertions(+), 19 deletions(-)
>>>>>>>>
>>>>>>>
>>>>>>> I understand the motivation, but as-is this will break migration.
>>>>>>>
>>>>>>> I think this will need to be versioned somehow, but the only thing that really gets
>>>>>>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>>>>>>>
>>>>>>
>>>>>>    From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max
>>>>>> between QEMU versions is *not* supported, as we can't guarantee which features are present
>>>>>> or not.
>>>>> This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties():
>>>>> neoverse-n1, neoverse-n2, cortex-a710.
>>>>>
>>>>
>>>> I think this is still a change worth to do, because people can get a
>>>> 100% speedup with this simple change, and it's a better default than
>>>> the previous value.
>>>> In more, in case of this migration scenario, QEMU will immediately
>>>> abort upon accessing memory through a pointer.
>>>>
>>>> I'm not sure about what would be the best way to make this change as
>>>> smooth as possible for QEMU users.
>>>
>>> Surely we can only honour and apply the new default to -cpu max?
>> 
>
> With all my respect, I think the current default is wrong, and it
> would be sad to keep it when people don't precise cpu max, or for
> other cpus enabling pointer authentication.

There is a difference between max and other CPUs. For max as has already
been stated migration is likely to break anyway between QEMU versions -
we should also make that clear in the docs. But for the other CPUs we
need to honour the existing defaults.

> In all our conversations, there seems to be a focus on choosing the
> "fastest" emulation solution that satisfies the guest (behaviour
> wise). And, for a reason I ignore, pointer authentication escaped this
> rule.
>
> I understand the concern regarding retro compatibility, but it would
> be better to ask politely (with an error message) to people to restart
> their virtual machines when they try to migrate, instead of being
> stuck with a slow default forever.

This is why we have compatibility logic so its easy to do the right
thing by specifying the QEMU version in the machine type. 

> In more, we are talking of a tcg scenario, for which I'm not sure
> people use migration feature (save/restore) heavily, but I may be
> wrong on this.

We can't assume its not. We even have explicit tests that check
migration doesn't break between master and $PREVSTABLE.

> Between the risk of breaking migration (with a polite error message),
> and having a default that is 100% faster, I think it would be better
> to favor the second one. If it would be a 5% speedup, I would not
> argue, but slowing down execution with a factor of 2 is really a lot.
>
>> That was what I thought we were aiming for, yes. We *could* have
>> a property on the CPU to say "use the old back-compatible default,
>> not the new one", which we then list in the appropriate hw_compat
>> array. (Grep for the "backcompat-cntfrq" property for an example of
>> this.) But I'm not sure if that is worth the effort compared to
>> just changing 'max'.
>
> When we'll define hw_compat_10_0, and hw_compat_11_0, do we have to
> carry this on forever? (Same question for "backcompat-cntfrq").
>
>> (It's not that much extra code to add the property, so I could
>> easily be persuaded the other way. Possible arguments include
>> preferring consistency across all CPUs. If we already make the
>> default be not "what the real CPU of this type uses" then that's
>> also an argument that we can set it to whatever is convenient;
>> if we do honour the CPU ID register values for the implementation
>> default then that's an argument that we should continue to do
>> so and not change the default to our impdef one.)
>>
>
> For the TCG use case, is there any visible side effect for the guest
> to use any specific pointer authentication algorithm?
> In other words, is there a scenario where pointer authentication would
> work with impdef, but not with qarma{3,5}?
> If no, I don't see any reason for a cpu to favor an expensive
> emulation.

If the user asks for a specific CPU model (not a special case like max)
we should provide the most accurate model that we can as explicitly set
by the user. We don't trade accuracy for speed (c.f. discussions about
floating point and INEXACT detection).

> In the accelerator case, we read the values from the host cpu, so
> there is no problem.
>
>> -- PMM

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

On 12/18/24 01:27, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 12/17/24 02:38, Peter Maydell wrote:
>>> On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>
>>>>> On 12/16/24 11:50, Richard Henderson wrote:
>>>>>> On 12/16/24 13:26, Pierrick Bouvier wrote:
>>>>>>> On 12/16/24 11:10, Richard Henderson wrote:
>>>>>>>> On 12/4/24 15:12, Pierrick Bouvier wrote:
>>>>>>>>> qemu-system-aarch64 default pointer authentication (QARMA5) is expensive, we
>>>>>>>>> spent up to 50% of the emulation time running it (when using TCG).
>>>>>>>>>
>>>>>>>>> Switching to pauth-impdef=on is often given as a solution to speed up execution.
>>>>>>>>> Thus we talked about making it the new default.
>>>>>>>>>
>>>>>>>>> The first patch introduce a new property (pauth-qarma5) to allow to select
>>>>>>>>> current default algorithm.
>>>>>>>>> The second one change the default.
>>>>>>>>>
>>>>>>>>> Pierrick Bouvier (2):
>>>>>>>>>        target/arm: add new property to select pauth-qarma5
>>>>>>>>>        target/arm: change default pauth algorithm to impdef
>>>>>>>>>
>>>>>>>>>       docs/system/arm/cpu-features.rst |  7 +++++--
>>>>>>>>>       docs/system/introduction.rst     |  2 +-
>>>>>>>>>       target/arm/cpu.h                 |  1 +
>>>>>>>>>       target/arm/arm-qmp-cmds.c        |  2 +-
>>>>>>>>>       target/arm/cpu64.c               | 30 +++++++++++++++++++-----------
>>>>>>>>>       tests/qtest/arm-cpu-features.c   | 15 +++++++++++----
>>>>>>>>>       6 files changed, 38 insertions(+), 19 deletions(-)
>>>>>>>>>
>>>>>>>>
>>>>>>>> I understand the motivation, but as-is this will break migration.
>>>>>>>>
>>>>>>>> I think this will need to be versioned somehow, but the only thing that really gets
>>>>>>>> versioned are the boards, and I'm not sure how to link that to the instantiated cpu.
>>>>>>>>
>>>>>>>
>>>>>>>     From what I understood, and I may be wrong, the use case to migrate (tcg) vm with cpu max
>>>>>>> between QEMU versions is *not* supported, as we can't guarantee which features are present
>>>>>>> or not.
>>>>>> This doesn't affect only -cpu max, but anything using aarch64_add_pauth_properties():
>>>>>> neoverse-n1, neoverse-n2, cortex-a710.
>>>>>>
>>>>>
>>>>> I think this is still a change worth to do, because people can get a
>>>>> 100% speedup with this simple change, and it's a better default than
>>>>> the previous value.
>>>>> In more, in case of this migration scenario, QEMU will immediately
>>>>> abort upon accessing memory through a pointer.
>>>>>
>>>>> I'm not sure about what would be the best way to make this change as
>>>>> smooth as possible for QEMU users.
>>>>
>>>> Surely we can only honour and apply the new default to -cpu max?
>>>
>>
>> With all my respect, I think the current default is wrong, and it
>> would be sad to keep it when people don't precise cpu max, or for
>> other cpus enabling pointer authentication.
> 
> There is a difference between max and other CPUs. For max as has already
> been stated migration is likely to break anyway between QEMU versions -
> we should also make that clear in the docs. But for the other CPUs we
> need to honour the existing defaults.
> 

I'll add this in the doc as part of this series.

>> In all our conversations, there seems to be a focus on choosing the
>> "fastest" emulation solution that satisfies the guest (behaviour
>> wise). And, for a reason I ignore, pointer authentication escaped this
>> rule.
>>
>> I understand the concern regarding retro compatibility, but it would
>> be better to ask politely (with an error message) to people to restart
>> their virtual machines when they try to migrate, instead of being
>> stuck with a slow default forever.
> 
> This is why we have compatibility logic so its easy to do the right
> thing by specifying the QEMU version in the machine type.
> 
>> In more, we are talking of a tcg scenario, for which I'm not sure
>> people use migration feature (save/restore) heavily, but I may be
>> wrong on this.
> 
> We can't assume its not. We even have explicit tests that check
> migration doesn't break between master and $PREVSTABLE.
> 
>> Between the risk of breaking migration (with a polite error message),
>> and having a default that is 100% faster, I think it would be better
>> to favor the second one. If it would be a 5% speedup, I would not
>> argue, but slowing down execution with a factor of 2 is really a lot.
>>
>>> That was what I thought we were aiming for, yes. We *could* have
>>> a property on the CPU to say "use the old back-compatible default,
>>> not the new one", which we then list in the appropriate hw_compat
>>> array. (Grep for the "backcompat-cntfrq" property for an example of
>>> this.) But I'm not sure if that is worth the effort compared to
>>> just changing 'max'.
>>
>> When we'll define hw_compat_10_0, and hw_compat_11_0, do we have to
>> carry this on forever? (Same question for "backcompat-cntfrq").
>>
>>> (It's not that much extra code to add the property, so I could
>>> easily be persuaded the other way. Possible arguments include
>>> preferring consistency across all CPUs. If we already make the
>>> default be not "what the real CPU of this type uses" then that's
>>> also an argument that we can set it to whatever is convenient;
>>> if we do honour the CPU ID register values for the implementation
>>> default then that's an argument that we should continue to do
>>> so and not change the default to our impdef one.)
>>>
>>
>> For the TCG use case, is there any visible side effect for the guest
>> to use any specific pointer authentication algorithm?
>> In other words, is there a scenario where pointer authentication would
>> work with impdef, but not with qarma{3,5}?
>> If no, I don't see any reason for a cpu to favor an expensive
>> emulation.
> 
> If the user asks for a specific CPU model (not a special case like max)
> we should provide the most accurate model that we can as explicitly set
> by the user. We don't trade accuracy for speed (c.f. discussions about
> floating point and INEXACT detection).
> 
>> In the accelerator case, we read the values from the host cpu, so
>> there is no problem.
>>
>>> -- PMM
> 

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Peter Maydell]

On Tue, 17 Dec 2024 at 21:08, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> On 12/17/24 02:38, Peter Maydell wrote:
> > On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> >>> I think this is still a change worth to do, because people can get a
> >>> 100% speedup with this simple change, and it's a better default than
> >>> the previous value.
> >>> In more, in case of this migration scenario, QEMU will immediately
> >>> abort upon accessing memory through a pointer.
> >>>
> >>> I'm not sure about what would be the best way to make this change as
> >>> smooth as possible for QEMU users.
> >>
> >> Surely we can only honour and apply the new default to -cpu max?
> >
>
> With all my respect, I think the current default is wrong, and it would
> be sad to keep it when people don't precise cpu max, or for other cpus
> enabling pointer authentication.
>
> In all our conversations, there seems to be a focus on choosing the
> "fastest" emulation solution that satisfies the guest (behaviour wise).
> And, for a reason I ignore, pointer authentication escaped this rule.

I think the reason is just that we didn't understand how much
of a performance hit the architected algorithm for pointer auth
is in emulation. So we took our default approach of "implement
what the architecture says". Then later when we realised how
bad the effect was we added in a faster impdef authentication
algorithm, but we put it in as not-the-default because of our
usual bias towards "don't change existing behaviour".

> I understand the concern regarding retro compatibility, but it would be
> better to ask politely (with an error message) to people to restart
> their virtual machines when they try to migrate, instead of being stuck
> with a slow default forever.
> In more, we are talking of a tcg scenario, for which I'm not sure people
> use migration feature (save/restore) heavily, but I may be wrong on this.
>
> Between the risk of breaking migration (with a polite error message),
> and having a default that is 100% faster, I think it would be better to
> favor the second one. If it would be a 5% speedup, I would not argue,
> but slowing down execution with a factor of 2 is really a lot.

The point here about "breaking migration" is that we have a strong
set of rules:
 * if you say "-machine virt-8.2" you get "exactly the behaviour
   that the 'virt' machine type had in QEMU 8.2, and it is
   migration compatible
 * we can make changes that are not migration compatible only if we
   ensure that they are not applied to older versioned machine types
   (or if they're to devices that are only used in machines which
   do not have versioned machine types at all)
 * TCG '-cpu max' is a special case: it is not a fixed thing, and so
   it may acquire new non-migration-compatible changes between versions
   (and so if you care about VM migration compat you don't use it);
   but this is not true of the named CPU types that match real
   hardware implementations

This patch as it stands will not preserve the migration
guarantees that we make. So we need to fix it by either:
 * only making the default change on -cpu max
 * making the default change be bound to versioned types

As I say, I don't have a strong view on which of these we go for
(and I'm actually kind of leaning to the second, given the discussion).

> > That was what I thought we were aiming for, yes. We *could* have
> > a property on the CPU to say "use the old back-compatible default,
> > not the new one", which we then list in the appropriate hw_compat
> > array. (Grep for the "backcompat-cntfrq" property for an example of
> > this.) But I'm not sure if that is worth the effort compared to
> > just changing 'max'.
>
> When we'll define hw_compat_10_0, and hw_compat_11_0, do we have to
> carry this on forever? (Same question for "backcompat-cntfrq").

The machinery for how this works means that you only need to
put the property in the appropriate hw_compat array for the
machine version before where it was introduced. The 'virt-9.2'
machine type applies the properties listed in hw_compat_9_2
(you can think of the properties listed there as having the
meaning "downgrade the default behaviour back to what it was
in 9.2 and earlier".) The virt-9.1 machine type applies the
properties listed in hw_compat_9_1 and hw_compat_9_2. The
virt-9.0 machine type applies the properties listed in hw_compat_9_0,
_9_1 and _9_2.

This is all implemented by the boilerplate DEFINE_VIRT_MACHINE() and
virt_machine_*_options functions at the bottom of hw/arm/virt.c
(plus the common code that invokes). We have to carry all this
machinery around anyway to handle other migration-breaking changes
in other parts of QEMU, so it's pretty free to add another property
like backcompat-cntfrq here.

The very oldest versioned machine types are deprecated after
3 years and dropped after another 3 years, so eventually the
older hw_compat arrays will go away.

> > (It's not that much extra code to add the property, so I could
> > easily be persuaded the other way. Possible arguments include
> > preferring consistency across all CPUs. If we already make the
> > default be not "what the real CPU of this type uses" then that's
> > also an argument that we can set it to whatever is convenient;
> > if we do honour the CPU ID register values for the implementation
> > default then that's an argument that we should continue to do
> > so and not change the default to our impdef one.)
> >
>
> For the TCG use case, is there any visible side effect for the guest to
> use any specific pointer authentication algorithm?
> In other words, is there a scenario where pointer authentication would
> work with impdef, but not with qarma{3,5}?
> If no, I don't see any reason for a cpu to favor an expensive emulation.

The guest can look at the value that the pointer auth instruction
produces if it likes, so it can certainly tell whether there's
a difference. But the only reason to do that is in test code
that's checking that the pauth instructions do what they're
supposed to do. Architecturally because multiple authentication
options are permitted no well behaved guest is going to depend
on which one exactly is being used.

As I say, I do think it would be good to check whether our
current implementation is "default to qarma5 everywhere", or
whether it is "default to what the real CPU says it has in its
ID registers". If we are already defaulting to something that's
not what the real implementation does it's another piece of
evidence on the side of "we can just default to a different
not-matching-the-hardware choice".

thanks
-- PMM

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

On 12/18/24 05:51, Peter Maydell wrote:
> On Tue, 17 Dec 2024 at 21:08, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> On 12/17/24 02:38, Peter Maydell wrote:
>>> On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>> I think this is still a change worth to do, because people can get a
>>>>> 100% speedup with this simple change, and it's a better default than
>>>>> the previous value.
>>>>> In more, in case of this migration scenario, QEMU will immediately
>>>>> abort upon accessing memory through a pointer.
>>>>>
>>>>> I'm not sure about what would be the best way to make this change as
>>>>> smooth as possible for QEMU users.
>>>>
>>>> Surely we can only honour and apply the new default to -cpu max?
>>>
>>
>> With all my respect, I think the current default is wrong, and it would
>> be sad to keep it when people don't precise cpu max, or for other cpus
>> enabling pointer authentication.
>>
>> In all our conversations, there seems to be a focus on choosing the
>> "fastest" emulation solution that satisfies the guest (behaviour wise).
>> And, for a reason I ignore, pointer authentication escaped this rule.
> 
> I think the reason is just that we didn't understand how much
> of a performance hit the architected algorithm for pointer auth
> is in emulation. So we took our default approach of "implement
> what the architecture says". Then later when we realised how
> bad the effect was we added in a faster impdef authentication
> algorithm, but we put it in as not-the-default because of our
> usual bias towards "don't change existing behaviour".
> 

I understand the reason behind the current choice.
For my personal knowledge, is there a QEMU policy for "breaking changes"?

>> I understand the concern regarding retro compatibility, but it would be
>> better to ask politely (with an error message) to people to restart
>> their virtual machines when they try to migrate, instead of being stuck
>> with a slow default forever.
>> In more, we are talking of a tcg scenario, for which I'm not sure people
>> use migration feature (save/restore) heavily, but I may be wrong on this.
>>
>> Between the risk of breaking migration (with a polite error message),
>> and having a default that is 100% faster, I think it would be better to
>> favor the second one. If it would be a 5% speedup, I would not argue,
>> but slowing down execution with a factor of 2 is really a lot.
> 
> The point here about "breaking migration" is that we have a strong
> set of rules:
>   * if you say "-machine virt-8.2" you get "exactly the behaviour
>     that the 'virt' machine type had in QEMU 8.2, and it is
>     migration compatible
>   * we can make changes that are not migration compatible only if we
>     ensure that they are not applied to older versioned machine types
>     (or if they're to devices that are only used in machines which
>     do not have versioned machine types at all)
>   * TCG '-cpu max' is a special case: it is not a fixed thing, and so
>     it may acquire new non-migration-compatible changes between versions
>     (and so if you care about VM migration compat you don't use it);
>     but this is not true of the named CPU types that match real
>     hardware implementations
> 
> This patch as it stands will not preserve the migration
> guarantees that we make. So we need to fix it by either:
>   * only making the default change on -cpu max
>   * making the default change be bound to versioned types
> 

I'm not sure to follow you on this second approach. The cpu is not 
versioned, and if someone use -machine virt (non versioned), is there a 
guarantee it should stay possible to migrate?

In other words, can we break the migration with "-machine virt -cpu model"?

> As I say, I don't have a strong view on which of these we go for
> (and I'm actually kind of leaning to the second, given the discussion).
> 

After looking more closely, compared to backcompat_cntfreq, the cpu 
registers will be different, and migration fail when calling 
"write_list_to_cpustate" from "cpu_post_load" for register 
ID_AA64ISAR1_EL1, which contains pauth configuration.

If we can break the migration for (non versioned) virt machine, then 
I'll make the change for all cpus using the backcompat strategy, and if 
not possible, I'll only make the change for -cpu max.

>>> That was what I thought we were aiming for, yes. We *could* have
>>> a property on the CPU to say "use the old back-compatible default,
>>> not the new one", which we then list in the appropriate hw_compat
>>> array. (Grep for the "backcompat-cntfrq" property for an example of
>>> this.) But I'm not sure if that is worth the effort compared to
>>> just changing 'max'.
>>
>> When we'll define hw_compat_10_0, and hw_compat_11_0, do we have to
>> carry this on forever? (Same question for "backcompat-cntfrq").
> 
> The machinery for how this works means that you only need to
> put the property in the appropriate hw_compat array for the
> machine version before where it was introduced. The 'virt-9.2'
> machine type applies the properties listed in hw_compat_9_2
> (you can think of the properties listed there as having the
> meaning "downgrade the default behaviour back to what it was
> in 9.2 and earlier".) The virt-9.1 machine type applies the
> properties listed in hw_compat_9_1 and hw_compat_9_2. The
> virt-9.0 machine type applies the properties listed in hw_compat_9_0,
> _9_1 and _9_2.
> 
> This is all implemented by the boilerplate DEFINE_VIRT_MACHINE() and
> virt_machine_*_options functions at the bottom of hw/arm/virt.c
> (plus the common code that invokes). We have to carry all this
> machinery around anyway to handle other migration-breaking changes
> in other parts of QEMU, so it's pretty free to add another property
> like backcompat-cntfrq here.
> 
> The very oldest versioned machine types are deprecated after
> 3 years and dropped after another 3 years, so eventually the
> older hw_compat arrays will go away.
> 
>>> (It's not that much extra code to add the property, so I could
>>> easily be persuaded the other way. Possible arguments include
>>> preferring consistency across all CPUs. If we already make the
>>> default be not "what the real CPU of this type uses" then that's
>>> also an argument that we can set it to whatever is convenient;
>>> if we do honour the CPU ID register values for the implementation
>>> default then that's an argument that we should continue to do
>>> so and not change the default to our impdef one.)
>>>
>>
>> For the TCG use case, is there any visible side effect for the guest to
>> use any specific pointer authentication algorithm?
>> In other words, is there a scenario where pointer authentication would
>> work with impdef, but not with qarma{3,5}?
>> If no, I don't see any reason for a cpu to favor an expensive emulation.
> 
> The guest can look at the value that the pointer auth instruction
> produces if it likes, so it can certainly tell whether there's
> a difference. But the only reason to do that is in test code
> that's checking that the pauth instructions do what they're
> supposed to do. Architecturally because multiple authentication
> options are permitted no well behaved guest is going to depend
> on which one exactly is being used.
> 
> As I say, I do think it would be good to check whether our
> current implementation is "default to qarma5 everywhere", or
> whether it is "default to what the real CPU says it has in its
> ID registers". If we are already defaulting to something that's
> not what the real implementation does it's another piece of
> evidence on the side of "we can just default to a different
> not-matching-the-hardware choice".
> 

We default to qarma5 (for tcg), or for what host cpu configures (for 
other accelerators).

> thanks
> -- PMM

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Richard Henderson]

On 12/18/24 13:08, Pierrick Bouvier wrote:
> On 12/18/24 05:51, Peter Maydell wrote:
>> On Tue, 17 Dec 2024 at 21:08, Pierrick Bouvier
>> <pierrick.bouvier@linaro.org> wrote:
>>>
>>> On 12/17/24 02:38, Peter Maydell wrote:
>>>> On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>
>>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>>> I think this is still a change worth to do, because people can get a
>>>>>> 100% speedup with this simple change, and it's a better default than
>>>>>> the previous value.
>>>>>> In more, in case of this migration scenario, QEMU will immediately
>>>>>> abort upon accessing memory through a pointer.
>>>>>>
>>>>>> I'm not sure about what would be the best way to make this change as
>>>>>> smooth as possible for QEMU users.
>>>>>
>>>>> Surely we can only honour and apply the new default to -cpu max?
>>>>
>>>
>>> With all my respect, I think the current default is wrong, and it would
>>> be sad to keep it when people don't precise cpu max, or for other cpus
>>> enabling pointer authentication.
>>>
>>> In all our conversations, there seems to be a focus on choosing the
>>> "fastest" emulation solution that satisfies the guest (behaviour wise).
>>> And, for a reason I ignore, pointer authentication escaped this rule.
>>
>> I think the reason is just that we didn't understand how much
>> of a performance hit the architected algorithm for pointer auth
>> is in emulation. So we took our default approach of "implement
>> what the architecture says". Then later when we realised how
>> bad the effect was we added in a faster impdef authentication
>> algorithm, but we put it in as not-the-default because of our
>> usual bias towards "don't change existing behaviour".
>>
> 
> I understand the reason behind the current choice.
> For my personal knowledge, is there a QEMU policy for "breaking changes"?
> 
>>> I understand the concern regarding retro compatibility, but it would be
>>> better to ask politely (with an error message) to people to restart
>>> their virtual machines when they try to migrate, instead of being stuck
>>> with a slow default forever.
>>> In more, we are talking of a tcg scenario, for which I'm not sure people
>>> use migration feature (save/restore) heavily, but I may be wrong on this.
>>>
>>> Between the risk of breaking migration (with a polite error message),
>>> and having a default that is 100% faster, I think it would be better to
>>> favor the second one. If it would be a 5% speedup, I would not argue,
>>> but slowing down execution with a factor of 2 is really a lot.
>>
>> The point here about "breaking migration" is that we have a strong
>> set of rules:
>>   * if you say "-machine virt-8.2" you get "exactly the behaviour
>>     that the 'virt' machine type had in QEMU 8.2, and it is
>>     migration compatible
>>   * we can make changes that are not migration compatible only if we
>>     ensure that they are not applied to older versioned machine types
>>     (or if they're to devices that are only used in machines which
>>     do not have versioned machine types at all)
>>   * TCG '-cpu max' is a special case: it is not a fixed thing, and so
>>     it may acquire new non-migration-compatible changes between versions
>>     (and so if you care about VM migration compat you don't use it);
>>     but this is not true of the named CPU types that match real
>>     hardware implementations
>>
>> This patch as it stands will not preserve the migration
>> guarantees that we make. So we need to fix it by either:
>>   * only making the default change on -cpu max
>>   * making the default change be bound to versioned types
>>
> 
> I'm not sure to follow you on this second approach. The cpu is not versioned, and if 
> someone use -machine virt (non versioned), is there a guarantee it should stay possible to 
> migrate?
> 
> In other words, can we break the migration with "-machine virt -cpu model"?

Yes, because "virt" is an alias for "virt-<current>".
Folks who rely on migration compatibility must use "virt-9.2" etc.

Thus having a global property set by version 9.2 compat which says "use qarma5 as default 
pac" will work.  It's a little backward having the version on the board, not the cpu, but 
we've done it before, as Peter mentioned re cpu clock time base.


r~

Re: [PATCH 0/2] Change default pointer authentication algorithm on aarch64 to impdef

[Pierrick Bouvier]

On 12/18/24 11:20, Richard Henderson wrote:
> On 12/18/24 13:08, Pierrick Bouvier wrote:
>> On 12/18/24 05:51, Peter Maydell wrote:
>>> On Tue, 17 Dec 2024 at 21:08, Pierrick Bouvier
>>> <pierrick.bouvier@linaro.org> wrote:
>>>>
>>>> On 12/17/24 02:38, Peter Maydell wrote:
>>>>> On Tue, 17 Dec 2024 at 07:40, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>>
>>>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>>>> I think this is still a change worth to do, because people can get a
>>>>>>> 100% speedup with this simple change, and it's a better default than
>>>>>>> the previous value.
>>>>>>> In more, in case of this migration scenario, QEMU will immediately
>>>>>>> abort upon accessing memory through a pointer.
>>>>>>>
>>>>>>> I'm not sure about what would be the best way to make this change as
>>>>>>> smooth as possible for QEMU users.
>>>>>>
>>>>>> Surely we can only honour and apply the new default to -cpu max?
>>>>>
>>>>
>>>> With all my respect, I think the current default is wrong, and it would
>>>> be sad to keep it when people don't precise cpu max, or for other cpus
>>>> enabling pointer authentication.
>>>>
>>>> In all our conversations, there seems to be a focus on choosing the
>>>> "fastest" emulation solution that satisfies the guest (behaviour wise).
>>>> And, for a reason I ignore, pointer authentication escaped this rule.
>>>
>>> I think the reason is just that we didn't understand how much
>>> of a performance hit the architected algorithm for pointer auth
>>> is in emulation. So we took our default approach of "implement
>>> what the architecture says". Then later when we realised how
>>> bad the effect was we added in a faster impdef authentication
>>> algorithm, but we put it in as not-the-default because of our
>>> usual bias towards "don't change existing behaviour".
>>>
>>
>> I understand the reason behind the current choice.
>> For my personal knowledge, is there a QEMU policy for "breaking changes"?
>>
>>>> I understand the concern regarding retro compatibility, but it would be
>>>> better to ask politely (with an error message) to people to restart
>>>> their virtual machines when they try to migrate, instead of being stuck
>>>> with a slow default forever.
>>>> In more, we are talking of a tcg scenario, for which I'm not sure people
>>>> use migration feature (save/restore) heavily, but I may be wrong on this.
>>>>
>>>> Between the risk of breaking migration (with a polite error message),
>>>> and having a default that is 100% faster, I think it would be better to
>>>> favor the second one. If it would be a 5% speedup, I would not argue,
>>>> but slowing down execution with a factor of 2 is really a lot.
>>>
>>> The point here about "breaking migration" is that we have a strong
>>> set of rules:
>>>    * if you say "-machine virt-8.2" you get "exactly the behaviour
>>>      that the 'virt' machine type had in QEMU 8.2, and it is
>>>      migration compatible
>>>    * we can make changes that are not migration compatible only if we
>>>      ensure that they are not applied to older versioned machine types
>>>      (or if they're to devices that are only used in machines which
>>>      do not have versioned machine types at all)
>>>    * TCG '-cpu max' is a special case: it is not a fixed thing, and so
>>>      it may acquire new non-migration-compatible changes between versions
>>>      (and so if you care about VM migration compat you don't use it);
>>>      but this is not true of the named CPU types that match real
>>>      hardware implementations
>>>
>>> This patch as it stands will not preserve the migration
>>> guarantees that we make. So we need to fix it by either:
>>>    * only making the default change on -cpu max
>>>    * making the default change be bound to versioned types
>>>
>>
>> I'm not sure to follow you on this second approach. The cpu is not versioned, and if
>> someone use -machine virt (non versioned), is there a guarantee it should stay possible to
>> migrate?
>>
>> In other words, can we break the migration with "-machine virt -cpu model"?
> 
> Yes, because "virt" is an alias for "virt-<current>".
> Folks who rely on migration compatibility must use "virt-9.2" etc.
>

Thanks, I didn't realize this, and it makes sense now.
Then, I'll change the behaviour for all cpus using the same pattern as 
cntfreq.

> Thus having a global property set by version 9.2 compat which says "use qarma5 as default
> pac" will work.  It's a little backward having the version on the board, not the cpu, but
> we've done it before, as Peter mentioned re cpu clock time base.
>

We can see it as "the platform configured some devices specifically", 
and it's an acceptable and much simpler solution than having versioned 
cpu for this use case.

> 
> r~