Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/hvf/hvf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index b043eac8c6..99d8672b9b 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -925,6 +925,9 @@ int hvf_arch_init_vcpu(CPUState *cpu)
     arm_cpu->cpreg_array_len = sregs_cnt;
     arm_cpu->cpreg_vmstate_array_len = sregs_cnt;
 
+    /* cpreg tuples must be in strictly ascending order */
+    qsort(arm_cpu->cpreg_indexes, sregs_cnt, sizeof(uint64_t), compare_u64);
+
     assert(write_cpustate_to_list(arm_cpu, false));
 
     /* Set CP_NO_RAW system registers on init */
-- 
2.43.0

On 16/9/25 16:22, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/hvf/hvf.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index b043eac8c6..99d8672b9b 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -925,6 +925,9 @@ int hvf_arch_init_vcpu(CPUState *cpu)
>       arm_cpu->cpreg_array_len = sregs_cnt;
>       arm_cpu->cpreg_vmstate_array_len = sregs_cnt;
>   
> +    /* cpreg tuples must be in strictly ascending order */

I don't get the "why". If this is related to a previous change,
maybe better to squash there?

Anyhow,
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +    qsort(arm_cpu->cpreg_indexes, sregs_cnt, sizeof(uint64_t), compare_u64);
> +
>       assert(write_cpustate_to_list(arm_cpu, false));
>   
>       /* Set CP_NO_RAW system registers on init */

On Thu, 25 Sept 2025 at 11:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 16/9/25 16:22, Richard Henderson wrote:
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   target/arm/hvf/hvf.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> > index b043eac8c6..99d8672b9b 100644
> > --- a/target/arm/hvf/hvf.c
> > +++ b/target/arm/hvf/hvf.c
> > @@ -925,6 +925,9 @@ int hvf_arch_init_vcpu(CPUState *cpu)
> >       arm_cpu->cpreg_array_len = sregs_cnt;
> >       arm_cpu->cpreg_vmstate_array_len = sregs_cnt;
> >
> > +    /* cpreg tuples must be in strictly ascending order */
>
> I don't get the "why". If this is related to a previous change,
> maybe better to squash there?

It's a requirement on the cpreg list data structure;
compare kvm_arm_init_cpreg_list()'s comment
    /* Sort the list we get back from the kernel, since cpreg_tuples
     * must be in strictly ascending order.
     */
and the one in init_cpreg_list():
    /*
     * Initialise the cpreg_tuples[] array based on the cp_regs hash.
     * Note that we require cpreg_tuples[] to be sorted by key ID.
     */

The underlying reason for this is the algorithm we use
for incoming migration in cpu_post_load(), where we
iterate through the incoming-data list and the one
we have on the destination comparing indexes. This
only works if the list is in order so that we can
easily identify "register in our list but not theirs"
and "register in their list but not ours" by comparing
the next value in the index lists.

-- PMM

On 25/9/25 15:23, Peter Maydell wrote:
> On Thu, 25 Sept 2025 at 11:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 16/9/25 16:22, Richard Henderson wrote:
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>    target/arm/hvf/hvf.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
>>> index b043eac8c6..99d8672b9b 100644
>>> --- a/target/arm/hvf/hvf.c
>>> +++ b/target/arm/hvf/hvf.c
>>> @@ -925,6 +925,9 @@ int hvf_arch_init_vcpu(CPUState *cpu)
>>>        arm_cpu->cpreg_array_len = sregs_cnt;
>>>        arm_cpu->cpreg_vmstate_array_len = sregs_cnt;
>>>
>>> +    /* cpreg tuples must be in strictly ascending order */
>>
>> I don't get the "why". If this is related to a previous change,
>> maybe better to squash there?
> 
> It's a requirement on the cpreg list data structure;
> compare kvm_arm_init_cpreg_list()'s comment
>      /* Sort the list we get back from the kernel, since cpreg_tuples
>       * must be in strictly ascending order.
>       */
> and the one in init_cpreg_list():
>      /*
>       * Initialise the cpreg_tuples[] array based on the cp_regs hash.
>       * Note that we require cpreg_tuples[] to be sorted by key ID.
>       */
> 
> The underlying reason for this is the algorithm we use
> for incoming migration in cpu_post_load(), where we
> iterate through the incoming-data list and the one
> we have on the destination comparing indexes. This
> only works if the list is in order so that we can
> easily identify "register in our list but not theirs"
> and "register in their list but not ours" by comparing
> the next value in the index lists.

Thank you, this is very clear now. Could we centralize this knowledge
as a docstring comment in ARMCPU::cpreg_indexes? (This is where I
naturally looked, trying to understand).

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>