Message ID | 662aca02-da99-524f-d9df-cd61427ca520@ispras.ru |
---|---|
State | New |
Headers | show |
Series | target/arm: cp15.dacr migration | expand |
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
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
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
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;