Re: [PATCH v7 10/73] target/arm: Expand CPUARMState.exception.syndrome to 64 bits

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi,

On 8/10/25 23:55, Richard Henderson wrote:
> This will be used for storing the ISS2 portion of the
> ESR_ELx registers in aarch64 state.  Re-order the fsr
> member to eliminate two structure holes.
> 
> Drop the comment about "if we implement EL2" since we
> have already done so.
> 
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/arm/cpu.h     |  7 ++-----
>   target/arm/helper.c  |  2 +-
>   target/arm/machine.c | 32 +++++++++++++++++++++++++++++++-
>   3 files changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index c9ea160d03..04b57f1dc5 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -633,13 +633,10 @@ typedef struct CPUArchState {
>        * entry process.
>        */
>       struct {
> -        uint32_t syndrome; /* AArch64 format syndrome register */
> -        uint32_t fsr; /* AArch32 format fault status register info */
> +        uint64_t syndrome; /* AArch64 format syndrome register */
>           uint64_t vaddress; /* virtual addr associated with exception, if any */
> +        uint32_t fsr; /* AArch32 format fault status register info */
>           uint32_t target_el; /* EL the exception should be targeted for */
> -        /* If we implement EL2 we will also need to store information
> -         * about the intermediate physical address for stage 2 faults.
> -         */
>       } exception;


> diff --git a/target/arm/machine.c b/target/arm/machine.c
> index 6666a0c50c..ce20b46f50 100644
> --- a/target/arm/machine.c
> +++ b/target/arm/machine.c
> @@ -848,6 +848,23 @@ static const VMStateInfo vmstate_powered_off = {
>       .put = put_power,
>   };
>   
> +static bool syndrome64_needed(void *opaque)
> +{
> +    ARMCPU *cpu = opaque;
> +    return cpu->env.exception.syndrome > UINT32_MAX;

Hmm...

> +}
> +
> +static const VMStateDescription vmstate_syndrome64 = {
> +    .name = "cpu/syndrome64",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = syndrome64_needed,

Why not simply add a new description for the high bits and
always migrate?

        .info = &vmstate_info_uint32,
        .offset = offsetofhigh32(ARMCPU, env.exception.syndrome),

> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT64(env.exception.syndrome, ARMCPU),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>   static int cpu_pre_save(void *opaque)
>   {
>       ARMCPU *cpu = opaque;
> @@ -1065,7 +1082,19 @@ const VMStateDescription vmstate_arm_cpu = {
>           VMSTATE_UINT64(env.exclusive_val, ARMCPU),
>           VMSTATE_UINT64(env.exclusive_high, ARMCPU),
>           VMSTATE_UNUSED(sizeof(uint64_t)),
> -        VMSTATE_UINT32(env.exception.syndrome, ARMCPU),
> +        /*
> +         * If any bits are set in the upper 32 bits of syndrome,
> +         * then the cpu/syndrome64 subsection will override this
> +         * with the full 64 bit state.
> +         */
> +        {
> +            .name = "env.exception.syndrome",
> +            .version_id = 0,
> +            .size = sizeof(uint32_t),
> +            .info = &vmstate_info_uint32,
> +            .flags = VMS_SINGLE,
> +            .offset = offsetoflow32(ARMCPU, env.exception.syndrome),
> +        },
>           VMSTATE_UINT32(env.exception.fsr, ARMCPU),
>           VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
>           VMSTATE_TIMER_PTR(gt_timer[GTIMER_PHYS], ARMCPU),
> @@ -1098,6 +1127,7 @@ const VMStateDescription vmstate_arm_cpu = {
>           &vmstate_serror,
>           &vmstate_irq_line_state,
>           &vmstate_wfxt_timer,
> +        &vmstate_syndrome64,
>           NULL
>       }
>   };