| Message ID | CAH47eN16zb4pJcwV4rYJrXERiPRxhB24_aUut7odxaNMGTBW4A@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 5 March 2014 15:01, alvise rigo <a.rigo@virtualopensystems.com> wrote: > So, if the processor does not start in AArch64 mode, we only add the AArch32 > version of the ARM_CP_STATE_BOTH register to the hashtable, otherwise > nothing changes and we add the two views of the register. Yes, I think if we want TCG<->KVM migration we're going to have to do something like this. It introduces complication for reset handling, though -- at the moment the aarch64 view handles reset and the aarch32 view does not. We'd need to add (back) reset information in the aarch32 views, and also raw read/write functions if needed. > This approach works only > if all the 64bit CPUs will always start in 64 bit mode. You should be checking for whether the CPU has the AARCH64 feature, which would fix that nit. > In my opinion, other solutions would require to revisit the idea of > ARM_CP_STATE_BOTH in favour of distinct definitions of the registers with > multiple views (like VBAR for AArch32 and VBAR_EL1 for AArch64). ARM_CP_STATE_BOTH is purely a shorthand to avoid having to write out two definitions when they'd be virtually the same. I don't think it adds any extra complexity that isn't already present for registers that have split definitions. In fact the split-definition registers are trickier because you end up needing all of (1) aarch64 view (2) aarch32-in-aarch64-cpu view (3) aarch32 view, which is pretty ugly. thanks -- PMM
diff --git a/target-arm/helper.c b/target-arm/helper.c index 90f85f1..7fd8dc8 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -2232,6 +2232,13 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r, /* Private utility function for define_one_arm_cp_reg_with_opaque(): * add a single reginfo struct to the hash table. */ + int is_a64 = (&cpu->env)->aarch64; + if (r->state == ARM_CP_STATE_BOTH && state == ARM_CP_STATE_AA64 + && !is_a64) { + /* no need of the AArch64 cp definition */ + return; + } + uint32_t *key = g_new(uint32_t, 1);
Indeed, this is one of the problem I workaround with the patch. One not much intrusive solution that I see now is to decouple the AArch64 definitions from AArch32. For example we can make add_cpreg_to_hashtable() aware of the processor architecture with something like: ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo)); int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0; @@ -2241,9 +2248,14 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r, * view handles that. AArch64 also handles reset. * We assume it is a cp15 register. */ + if (is_a64) { + /* this is an AArch32 view */ + r2->type |= ARM_CP_NO_MIGRATE; + r2->resetfn = arm_cp_reset_ignore; + } else { + /* this is the AArch32 register itself */ + } r2->cp = 15; - r2->type |= ARM_CP_NO_MIGRATE; - r2->resetfn = arm_cp_reset_ignore; #ifdef HOST_WORDS_BIGENDIAN if (r2->fieldoffset) { r2->fieldoffset += sizeof(uint32_t); So, if the processor does not start in AArch64 mode, we only add the AArch32 version of the ARM_CP_STATE_BOTH register to the hashtable, otherwise nothing changes and we add the two views of the register. This approach works only if all the 64bit CPUs will always start in 64 bit mode. In my opinion, other solutions would require to revisit the idea of ARM_CP_STATE_BOTH in favour of distinct definitions of the registers with multiple views (like VBAR for AArch32 and VBAR_EL1 for AArch64). Regards, alvise On Mon, Mar 3, 2014 at 10:39 PM, Peter Maydell <peter.maydell@linaro.org>wrote: > On 26 February 2014 10:02, alvise rigo <a.rigo@virtualopensystems.com> > wrote: > > I agree that this is a sort of workaround, but it seems to me that a > > proper solution is not possible without changing the ideas contemplated > > now in the migration code. > > Are we willing to accept some major changes in the code to embrace > > this type of migration? > > Incidentally it occurred to me recently that the design we've gone for > for AArch64 support (canonical copy of system register info is the AArch64 > view, AArch32 register encoding is a non-migratable view onto the same > underlying data) clashes rather with the idea of being able to migrate > AArch32 TCG<->KVM. (Migrating AArch32 TCG to/from a KVM which > is configured to run the VM in AArch32 on an AArch64 host doesn't run > into the same problems, because in that case what KVM exposes to > userspace will also be the AArch64 views of registers.) > That was accidental, not an intentional tradeoff, but I'm not entirely > sure how to reconcile things... > > thanks > -- PMM >