Re: target/arm: cp15.dacr migration

[Pavel Dovgalyuk]

On 07.02.2022 16:44, Peter Maydell wrote:
> 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.

We actually did it for reverse debugging of custom firmware.

> > 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.)

Unfortunately, I can't test anything with AArch32 KVM.

> 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.

I checked the flags coming through write_cpustate_to_list. There were 
both dacr_s and dacr_ns flags. Therefore both values were saved.

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


Pavel Dovgalyuk