Re: target/arm: cp15.dacr migration

[Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>]

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