On Tue, 8 Feb 2022 at 04:56, Pavel Dovgalyuk <[email protected]> wrote: > > On 07.02.2022 16:44, Peter Maydell wrote: > > On Mon, 7 Feb 2022 at 12:13, Pavel Dovgalyuk <[email protected]> > > 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
