Re: target/arm: cp15.dacr migration

[Peter Maydell]

On Mon, 7 Feb 2022 at 12:13, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
>
> I recently encountered a problem with cp15.dacr register.
> It has _s and _ns versions. During the migration only dacr_ns is
> saved/loaded.
> But both of the values are used in get_phys_addr_v5 and get_phys_addr_v6
> functions. Therefore VM behavior becomes incorrect after loading the
> vmstate.

Yes, we don't correctly save and restore the Secure banked
registers. This is a long standing bug (eg it is the
cause of https://gitlab.com/qemu-project/qemu/-/issues/467).
Almost nobody notices this, because almost nobody both runs
Secure-world AArch32 code and also tries migration or save/restore.

> I found that kvm_to_cpreg_id is responsible for disabling dacr_s
> migration, because it always selects ns variant.

> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index c6a4d50e82..d3ffef3640 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2510,11 +2510,6 @@ static inline uint32_t kvm_to_cpreg_id(uint64_t
> kvmid)
>           if ((kvmid & CP_REG_SIZE_MASK) == CP_REG_SIZE_U64) {
>               cpregid |= (1 << 15);
>           }
> -
> -        /* KVM is always non-secure so add the NS flag on AArch32 register
> -         * entries.
> -         */
> -         cpregid |= 1 << CP_REG_NS_SHIFT;
>       }
>       return cpregid;
>   }

This change is wrong, or at least incomplete -- as the comment notes,
a guest running under KVM is always NonSecure, so when KVM says "this is
DACR" (or whatever) it always means "this is the NS banked DACR".
(Though now AArch32 KVM support has been dropped we have some flexibility
to not necessarily use KVM register ID values that exactly match what
the kernel uses, if we need to do that.)
Also, kvm_to_cpreg_id() and cpreg_to_kvm_id() are supposed to be
inverses of each other -- at the moment they are not, hence
this bug, but I think your change has probably resulted in both
the S and the NS banked versions of each register being treated
as the S banked version, which will have a different set of problems.

There is also the question of migration compatibility to consider
in any change in this area.

thanks
-- PMM