Re: target/arm: cp15.dacr migration

[Peter Maydell]

On Tue, 8 Feb 2022 at 04:56, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
>
> 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.

As I say, it doesn't exist any more, so you don't need to.
In any case, this patch isn't sufficient on its own.

thanks
-- PMM