[Qemu-devel] [PATCH] target/arm/helper: document potential CNTV register bear trap

[Qemu-devel] [PATCH] target/arm/helper: document potential CNTV register bear trap

[Alex Bennée]

The ARM KVM encodings have been inadvertently switched for
CNTV_CVAL_EL0/CNTVCT_EL0 in the register API since its introduction.
Fortunately this doesn't currently mater as the reset values for both
are the same. However if this ever changes things will break in
interesting ways.

Migration is currently unaffected as we just use
cpu->cpreg_vmstate_indexes/cpreg_vmstate_values verbatim. However this
would break if we ever supported migration between KVM and TCG models.

For now we just warn future generations who may touch this code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Christoffer Dall <cdall@linaro.org>
---
 target/arm/helper.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2594faa9b8..e7a0e39583 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -211,6 +211,11 @@ static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
     return true;
 }
 
+/*
+ * FIXME: the KVM API has switched encodings for
+ * CNTV_CVAL_EL0/CNTVCT_EL0 which need to be fixed if we want to
+ * change the reset values or support KVM<->TCG migration.
+ */
 bool write_cpustate_to_list(ARMCPU *cpu)
 {
     /* Write the coprocessor state from cpu->env to the (index,value) list. */
@@ -234,6 +239,7 @@ bool write_cpustate_to_list(ARMCPU *cpu)
     return ok;
 }
 
+/* FIXME: see above re:CNTV_CVAL_EL0/CNTVCT_EL0 encodings */
 bool write_list_to_cpustate(ARMCPU *cpu)
 {
     int i;
@@ -1961,6 +1967,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
                                    cp15.c14_timer[GTIMER_VIRT].ctl),
       .writefn = gt_virt_ctl_write, .raw_writefn = raw_write,
     },
+    /* WARNING! For *KVM only* the switched API encoding means this
+     * actually gets loaded in fresh VMs as CNTV_CVAL_EL0. This will
+     * need to be fixed in write_[cpustate_to_list|list_to_cpustate]
+     * if you want to change the reset value.
+     */
     { .name = "CNTV_CTL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 1,
       .type = ARM_CP_IO, .access = PL1_RW | PL0_R,
@@ -2053,6 +2064,11 @@ static const ARMCPRegInfo generic_timer_cp_reginfo[] = {
       .accessfn = gt_vtimer_access,
       .writefn = gt_virt_cval_write, .raw_writefn = raw_write,
     },
+    /* WARNING! For *KVM only* the switched API encoding means this
+     * actually gets loaded in fresh VMs as CNTVCT_EL0. This will need
+     * to be fixed in write_[cpustate_to_list|list_to_cpustate] if you
+     * want to change the reset value.
+     */
     { .name = "CNTV_CVAL_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 3, .opc2 = 2,
       .access = PL1_RW | PL0_R,
-- 
2.13.0

Re: [Qemu-devel] [PATCH] target/arm/helper: document potential CNTV register bear trap

[Peter Maydell]

On 22 June 2017 at 12:18, Alex Bennée <alex.bennee@linaro.org> wrote:
> The ARM KVM encodings have been inadvertently switched for
> CNTV_CVAL_EL0/CNTVCT_EL0 in the register API since its introduction.
> Fortunately this doesn't currently mater as the reset values for both
> are the same. However if this ever changes things will break in
> interesting ways.

Augh.

> Migration is currently unaffected as we just use
> cpu->cpreg_vmstate_indexes/cpreg_vmstate_values verbatim. However this
> would break if we ever supported migration between KVM and TCG models.
>
> For now we just warn future generations who may touch this code.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Christoffer Dall <cdall@linaro.org>
> ---
>  target/arm/helper.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2594faa9b8..e7a0e39583 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -211,6 +211,11 @@ static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
>      return true;
>  }
>
> +/*
> + * FIXME: the KVM API has switched encodings for
> + * CNTV_CVAL_EL0/CNTVCT_EL0 which need to be fixed if we want to
> + * change the reset values or support KVM<->TCG migration.
> + */

I don't really like just peppering the code with FIXMEs like this.
How much work would it be to determine what the actual required fix
is (whether kernel side or QEMU side) and implement it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/arm/helper: document potential CNTV register bear trap

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 22 June 2017 at 12:18, Alex Bennée <alex.bennee@linaro.org> wrote:
>> The ARM KVM encodings have been inadvertently switched for
>> CNTV_CVAL_EL0/CNTVCT_EL0 in the register API since its introduction.
>> Fortunately this doesn't currently mater as the reset values for both
>> are the same. However if this ever changes things will break in
>> interesting ways.
>
> Augh.
>
>> Migration is currently unaffected as we just use
>> cpu->cpreg_vmstate_indexes/cpreg_vmstate_values verbatim. However this
>> would break if we ever supported migration between KVM and TCG models.
>>
>> For now we just warn future generations who may touch this code.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Christoffer Dall <cdall@linaro.org>
>> ---
>>  target/arm/helper.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index 2594faa9b8..e7a0e39583 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -211,6 +211,11 @@ static bool raw_accessors_invalid(const ARMCPRegInfo *ri)
>>      return true;
>>  }
>>
>> +/*
>> + * FIXME: the KVM API has switched encodings for
>> + * CNTV_CVAL_EL0/CNTVCT_EL0 which need to be fixed if we want to
>> + * change the reset values or support KVM<->TCG migration.
>> + */
>
> I don't really like just peppering the code with FIXMEs like this.
> How much work would it be to determine what the actual required fix
> is (whether kernel side or QEMU side) and implement it?

We can't fix it kernel side as it is now a feature of the ABI (arguably
the indexes are just unique numbers rather than the register encoding).

While discussing the impact with Christoffer we came to the conclusion
that we are safe at the moment but it may well bite in the future -
hence the FIXME patch. I guess we could fix it up now, I guess one
approach would be:

  Add an ARM_CP_QUIRK flag bit to the affected registers
  Check for ARM_CP_QUIRK in write_list_to_cpustate/write_cpustate_to_list
    if hit lookup the fixup

>
> thanks
> -- PMM


--
Alex Bennée