qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: target/arm: cp15.dacr migration


From: Pavel Dovgalyuk
Subject: Re: target/arm: cp15.dacr migration
Date: Tue, 8 Feb 2022 07:56:37 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]