[Qemu-devel] [PATCH] target-arm: Bug fix in filling the cp_regs hashtable

[Qemu-devel] [PATCH] target-arm: Bug fix in filling the cp_regs hashtable

[Abdallah Bouassida]

Check if the CPU supports AARCH64 before adding  the 64bit view of
the coprocessor's register to the cp_regs hashtable.

Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
---
Bug description: if a register has the .state = ARM_CP_STATE_BOTH, its 
64bit view will be added to the hashtable even if the CPU is not 64bit.

  target/arm/helper.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2594faa..7fa2889 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5607,11 +5607,13 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
                              break;
                          }
                      } else {
-                        /* AArch64 registers get mapped to non-secure 
instance
-                         * of AArch32 */
-                        add_cpreg_to_hashtable(cpu, r, opaque, state,
-                                               ARM_CP_SECSTATE_NS,
-                                               crm, opc1, opc2);
+                        if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+                            /* AArch64 registers get mapped to 
non-secure instance
+                             * of AArch32 */
+                            add_cpreg_to_hashtable(cpu, r, opaque, state,
+ ARM_CP_SECSTATE_NS,
+                                                   crm, opc1, opc2);
+                        }
                      }
                  }
              }
-- 
1.9.1

Re: [Qemu-devel] [PATCH] target-arm: Bug fix in filling the cp_regs hashtable

[Peter Maydell]

On 16 June 2017 at 15:42, Abdallah Bouassida
<abdallah.bouassida@lauterbach.com> wrote:
> Check if the CPU supports AARCH64 before adding  the 64bit view of
> the coprocessor's register to the cp_regs hashtable.
>
> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
> ---
> Bug description: if a register has the .state = ARM_CP_STATE_BOTH, its 64bit
> view will be added to the hashtable even if the CPU is not 64bit.

This is deliberate and required. Where the AArch64 and AArch32
states both have a register which shares underlying architectural
state, QEMU chooses to implement migration of that state usually
via the AArch64 version's ARMCPRegInfo struct. If the AArch64
version is not included in the hashtable for an AArch32-only
CPU then the state of the 32-bit register won't be migrated.
The AArch64 register is of course invisible to the guest because
it is only accessible via 64-bit instructions, but it's used
during migration.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Bug fix in filling the cp_regs hashtable

[Abdallah Bouassida]

Oh, I see!
Thanks for the details!

Regards,
Abdallah

Le 6/16/2017 à 3:51 PM, Peter Maydell a écrit :
> On 16 June 2017 at 15:42, Abdallah Bouassida
> <abdallah.bouassida@lauterbach.com> wrote:
>> Check if the CPU supports AARCH64 before adding  the 64bit view of
>> the coprocessor's register to the cp_regs hashtable.
>>
>> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
>> ---
>> Bug description: if a register has the .state = ARM_CP_STATE_BOTH, its 64bit
>> view will be added to the hashtable even if the CPU is not 64bit.
> This is deliberate and required. Where the AArch64 and AArch32
> states both have a register which shares underlying architectural
> state, QEMU chooses to implement migration of that state usually
> via the AArch64 version's ARMCPRegInfo struct. If the AArch64
> version is not included in the hashtable for an AArch32-only
> CPU then the state of the 32-bit register won't be migrated.
> The AArch64 register is of course invisible to the guest because
> it is only accessible via 64-bit instructions, but it's used
> during migration.
>
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] target-arm: Bug fix in filling the cp_regs hashtable

[Philippe Mathieu-Daudé]

On 06/16/2017 11:51 AM, Peter Maydell wrote:
> On 16 June 2017 at 15:42, Abdallah Bouassida
> <abdallah.bouassida@lauterbach.com> wrote:
>> Check if the CPU supports AARCH64 before adding  the 64bit view of
>> the coprocessor's register to the cp_regs hashtable.
>>
>> Signed-off-by: Abdallah Bouassida <abdallah.bouassida@lauterbach.com>
>> ---
>> Bug description: if a register has the .state = ARM_CP_STATE_BOTH, its 64bit
>> view will be added to the hashtable even if the CPU is not 64bit.
>
> This is deliberate and required. Where the AArch64 and AArch32
> states both have a register which shares underlying architectural
> state, QEMU chooses to implement migration of that state usually
> via the AArch64 version's ARMCPRegInfo struct. If the AArch64
> version is not included in the hashtable for an AArch32-only
> CPU then the state of the 32-bit register won't be migrated.
> The AArch64 register is of course invisible to the guest because
> it is only accessible via 64-bit instructions, but it's used
> during migration.

explanation worth to go as comment in the source!