[PATCH 0/1] target/arm: Fix SCR_EL3 migration issue

[PATCH 0/1] target/arm: Fix SCR_EL3 migration issue

[michael.nawrocki--- via]

The SCR_EL3 register reset value (0)  and the value produced when
writing 0 via the scr_write function (set as writefn in the register
struct) differ. This causes migration to fail.

I believe the solution is to specify a raw_writefn for that register.

Failing invocation:
$ qemu-system-arm -machine vexpress-a9 -cpu cortex-a9 -nographic
QEMU 5.2.0 monitor - type 'help' for more information
(qemu) migrate "exec:cat > img"
(qemu) q
$ qemu-system-arm -machine vexpress-a9 -cpu cortex-a9 -nographic -incoming "exec:cat img"
qemu-system-arm: error while loading state for instance 0x0 of device 'cpu'
qemu-system-arm: load of migration failed: Operation not permitted


Mike Nawrocki (1):
  target/arm: Add raw_writefn to SCR_EL3 register

 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/1] target/arm: Add raw_writefn to SCR_EL3 register

[michael.nawrocki--- via]

Fixes an issue in migration where the reset value of SCR and the value
produced by scr_write via the writefn for SCR_EL3 mismatch.

Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index d2ead3fcbd..e3c4fe76cb 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5785,7 +5785,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
     { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
       .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
-      .resetvalue = 0, .writefn = scr_write },
+      .resetvalue = 0, .writefn = scr_write, .raw_writefn = raw_write },
     { .name = "SCR",  .type = ARM_CP_ALIAS | ARM_CP_NEWEL,
       .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
       .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
-- 
2.20.1

Re: [PATCH 0/1] target/arm: Fix SCR_EL3 migration issue

[Peter Maydell]

On Thu, 28 Jan 2021 at 14:31, Mike Nawrocki
<michael.nawrocki@gtri.gatech.edu> wrote:
>
> The SCR_EL3 register reset value (0)  and the value produced when
> writing 0 via the scr_write function (set as writefn in the register
> struct) differ. This causes migration to fail.
>
> I believe the solution is to specify a raw_writefn for that register.
>
> Failing invocation:
> $ qemu-system-arm -machine vexpress-a9 -cpu cortex-a9 -nographic
> QEMU 5.2.0 monitor - type 'help' for more information
> (qemu) migrate "exec:cat > img"
> (qemu) q
> $ qemu-system-arm -machine vexpress-a9 -cpu cortex-a9 -nographic -incoming "exec:cat img"
> qemu-system-arm: error while loading state for instance 0x0 of device 'cpu'
> qemu-system-arm: load of migration failed: Operation not permitted

I'll review the patch later, but for the moment just a note that
I'm pretty sure this is not the only issue you'll run into with
trying to migrate an AArch32 TrustZone-enabled CPU.
https://bugs.launchpad.net/qemu/+bug/1839807 has the details
but in summary we aren't migrating the Secure banked contents
of cp15 registers which are banked Secure/Non-Secure. The
symptom will be that migration succeeds but the guest doesn't
behave correctly on the destination/after state restore.

thanks
-- PMM

Re: [PATCH 1/1] target/arm: Add raw_writefn to SCR_EL3 register

[Peter Maydell]

On Thu, 28 Jan 2021 at 14:31, Mike Nawrocki
<michael.nawrocki@gtri.gatech.edu> wrote:
>
> Fixes an issue in migration where the reset value of SCR and the value
> produced by scr_write via the writefn for SCR_EL3 mismatch.
>
> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
> ---
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d2ead3fcbd..e3c4fe76cb 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -5785,7 +5785,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>      { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
> -      .resetvalue = 0, .writefn = scr_write },
> +      .resetvalue = 0, .writefn = scr_write, .raw_writefn = raw_write },
>      { .name = "SCR",  .type = ARM_CP_ALIAS | ARM_CP_NEWEL,
>        .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>        .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
> --

I think the problem here is not the lack of a raw_writefn,
but that we're not handling the RES1 bits [5:4] in SCR_EL3 correctly.
Specifically:

 * if the CPU is AArch64-only then the resetvalue for SCR_EL3 should
   not be 0, but 0x30 (but if the CPU has AArch32 at all then it should
   still be 0)
 * scr_write() should not be saying "force the FW and AW bits to 1 if
   written as an AArch64 register". It can, but is not obliged to,
   do this if the CPU is AArch64-only; it must not if the CPU has
   AArch32, even if the register is being accessed via its AArch64 form.

This is because RES1 has some complicated semantics for bits like
this which are "RES1 only in some architectural contexts" (this is all
defined in the Glossary entry in the v8A Arm ARM), which basically
means that if AArch32 is supported then the bit has to be reads-as-written
from the AArch64 side.

thanks
-- PMM

Re: [PATCH 1/1] target/arm: Add raw_writefn to SCR_EL3 register

[michael.nawrocki--- via]

On 2/2/21 6:29 AM, Peter Maydell wrote:
> On Thu, 28 Jan 2021 at 14:31, Mike Nawrocki
> <michael.nawrocki@gtri.gatech.edu> wrote:
>>
>> Fixes an issue in migration where the reset value of SCR and the value
>> produced by scr_write via the writefn for SCR_EL3 mismatch.
>>
>> Signed-off-by: Mike Nawrocki <michael.nawrocki@gtri.gatech.edu>
>> ---
>>   target/arm/helper.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index d2ead3fcbd..e3c4fe76cb 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -5785,7 +5785,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
>>       { .name = "SCR_EL3", .state = ARM_CP_STATE_AA64,
>>         .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 0,
>>         .access = PL3_RW, .fieldoffset = offsetof(CPUARMState, cp15.scr_el3),
>> -      .resetvalue = 0, .writefn = scr_write },
>> +      .resetvalue = 0, .writefn = scr_write, .raw_writefn = raw_write },
>>       { .name = "SCR",  .type = ARM_CP_ALIAS | ARM_CP_NEWEL,
>>         .cp = 15, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 0,
>>         .access = PL1_RW, .accessfn = access_trap_aa32s_el1,
>> --
> 
> I think the problem here is not the lack of a raw_writefn,
> but that we're not handling the RES1 bits [5:4] in SCR_EL3 correctly.
> Specifically:
> 
>   * if the CPU is AArch64-only then the resetvalue for SCR_EL3 should
>     not be 0, but 0x30 (but if the CPU has AArch32 at all then it should
>     still be 0)
>   * scr_write() should not be saying "force the FW and AW bits to 1 if
>     written as an AArch64 register". It can, but is not obliged to,
>     do this if the CPU is AArch64-only; it must not if the CPU has
>     AArch32, even if the register is being accessed via its AArch64 form.
> 
> This is because RES1 has some complicated semantics for bits like
> this which are "RES1 only in some architectural contexts" (this is all
> defined in the Glossary entry in the v8A Arm ARM), which basically
> means that if AArch32 is supported then the bit has to be reads-as-written
> from the AArch64 side.
> 
> thanks
> -- PMM
> 

I see what you mean. Does QEMU support AArch64-only CPU models, and if 
so, is there a way to determine if the CPU has AArch32?

Thanks,
Mike

Re: [PATCH 1/1] target/arm: Add raw_writefn to SCR_EL3 register

[Peter Maydell]

On Wed, 3 Feb 2021 at 14:50, Michael Nawrocki
<michael.nawrocki@gtri.gatech.edu> wrote:
>
> On 2/2/21 6:29 AM, Peter Maydell wrote:
> I see what you mean. Does QEMU support AArch64-only CPU models, and if
> so, is there a way to determine if the CPU has AArch32?

We don't have any currently, but in theory the support is there
and we'll likely end up adding some in future.

More specifically, in this case what you want to know is "can
the guest ever see the AArch32 view of SCR_EL3", which is
"is there support for AArch32 at EL1 or above"?", which you can
check for in the EL1 field of ID_AA64PFR0.

So you need a function similar to the existing isar_feature_aa64_aa32(),
but which cecks the EL1 field instead of the EL0 field (you could
call it isar_feature_aa64_aa32_el1()). Then you can test it with
 cpu_isar_feature(aa64_aa32_el1, cpu).

NB that you must only call it when you know that the CPU has
AArch64, ie when arm_feature(env, ARM_FEATURE_AARCH64) is true.

thanks
-- PMM

Re: [PATCH 0/1] target/arm: Fix SCR_EL3 migration issue

[Philippe Mathieu-Daudé]

Cc'ing avocado-devel for test idea.

On 1/28/21 3:31 PM, michael.nawrocki--- via wrote:
> The SCR_EL3 register reset value (0)  and the value produced when
> writing 0 via the scr_write function (set as writefn in the register
> struct) differ. This causes migration to fail.
> 
> I believe the solution is to specify a raw_writefn for that register.
> 
> Failing invocation:
> $ qemu-system-arm -machine vexpress-a9 -cpu cortex-a9 -nographic
> QEMU 5.2.0 monitor - type 'help' for more information
> (qemu) migrate "exec:cat > img"
> (qemu) q
> $ qemu-system-arm -machine vexpress-a9 -cpu cortex-a9 -nographic -incoming "exec:cat img"
> qemu-system-arm: error while loading state for instance 0x0 of device 'cpu'
> qemu-system-arm: load of migration failed: Operation not permitted
> 
> 
> Mike Nawrocki (1):
>   target/arm: Add raw_writefn to SCR_EL3 register
> 
>  target/arm/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Re: [PATCH 1/1] target/arm: Add raw_writefn to SCR_EL3 register

[michael.nawrocki--- via]

On 2/3/21 10:04 AM, Peter Maydell wrote:
> On Wed, 3 Feb 2021 at 14:50, Michael Nawrocki
> <michael.nawrocki@gtri.gatech.edu> wrote:
>>
>> On 2/2/21 6:29 AM, Peter Maydell wrote:
>> I see what you mean. Does QEMU support AArch64-only CPU models, and if
>> so, is there a way to determine if the CPU has AArch32?
> 
> We don't have any currently, but in theory the support is there
> and we'll likely end up adding some in future.
> 
> More specifically, in this case what you want to know is "can
> the guest ever see the AArch32 view of SCR_EL3", which is
> "is there support for AArch32 at EL1 or above"?", which you can
> check for in the EL1 field of ID_AA64PFR0.
> 
> So you need a function similar to the existing isar_feature_aa64_aa32(),
> but which cecks the EL1 field instead of the EL0 field (you could
> call it isar_feature_aa64_aa32_el1()). Then you can test it with
>   cpu_isar_feature(aa64_aa32_el1, cpu).
> 
> NB that you must only call it when you know that the CPU has
> AArch64, ie when arm_feature(env, ARM_FEATURE_AARCH64) is true.
> 
> thanks
> -- PMM
> 

Thanks, that's very helpful. I submitted a patch v2 that I believe 
encapsulates this discussion.

Thanks,
Mike