target/arm: cp15.dacr migration

target/arm: cp15.dacr migration

[Pavel Dovgalyuk]

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.

I found that kvm_to_cpreg_id is responsible for disabling dacr_s 
migration, because it always selects ns variant.

I used the following changes to solve the problem, but I'm not sure that 
these changes do not break anything in KVM.

Can anyone give me an advice about that?


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;
  }


--
Pavel Dovgalyuk

Re: target/arm: cp15.dacr migration

[Peter Maydell]

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

Re: target/arm: cp15.dacr migration

[Pavel Dovgalyuk]

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

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