[v2] timer/aspeed: fix vmstate version id

[Qemu-devel] [PATCH v2] timer/aspeed: fix vmstate version id

Posted by Cédric Le Goater 6 years ago

commit 1d3e65aa7ac5 ("hw/timer: Add value matching support to
aspeed_timer") increased the vmstate version of aspeed.timer because
the state had changed, but it also bumped the version of the
VMSTATE_STRUCT_ARRAY under the aspeed.timerctrl which did not need to.

Change back this version to fix migration.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 To test on a 'palmetto-bmc' board, download the latest built OpenBMC
 flash image :

   wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/deploy/images/palmetto/flash-palmetto

 start two instances of QEMU with :

   qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults

 and :
 
   qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults -incoming tcp::1234

 then, run migrate from the monitor:
 
   (qemu) migrate tcp:localhost:1234


 Changes since v1:

 - change version of VMSTATE_STRUCT_ARRAY and not vmstate_aspeed_timer_state

 hw/timer/aspeed_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 50acbf530a3a..1e31e22b6f1f 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -504,7 +504,7 @@ static const VMStateDescription vmstate_aspeed_timer_state = {
         VMSTATE_UINT32(ctrl, AspeedTimerCtrlState),
         VMSTATE_UINT32(ctrl2, AspeedTimerCtrlState),
         VMSTATE_STRUCT_ARRAY(timers, AspeedTimerCtrlState,
-                             ASPEED_TIMER_NR_TIMERS, 2, vmstate_aspeed_timer,
+                             ASPEED_TIMER_NR_TIMERS, 1, vmstate_aspeed_timer,
                              AspeedTimer),
         VMSTATE_END_OF_LIST()
     }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2] timer/aspeed: fix vmstate version id

Posted by Peter Maydell 6 years ago

On 23 April 2018 at 11:14, Cédric Le Goater <clg@kaod.org> wrote:
> commit 1d3e65aa7ac5 ("hw/timer: Add value matching support to
> aspeed_timer") increased the vmstate version of aspeed.timer because
> the state had changed, but it also bumped the version of the
> VMSTATE_STRUCT_ARRAY under the aspeed.timerctrl which did not need to.
>
> Change back this version to fix migration.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Thanks, applied to target-arm.next.

>  To test on a 'palmetto-bmc' board, download the latest built OpenBMC
>  flash image :
>
>    wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/deploy/images/palmetto/flash-palmetto
>
>  start two instances of QEMU with :
>
>    qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults

Thanks also for the test image -- I'll add it to the collection I have.

PS: there seems to be something odd up with the DNS for openpower.xyz:

$ host openpower.xyz
openpower.xyz has address 146.20.65.228
;; connection timed out; no servers could be reached

(also manifests as wget pausing for several seconds before starting
the download).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] timer/aspeed: fix vmstate version id

Posted by Cédric Le Goater 6 years ago

On 04/23/2018 12:29 PM, Peter Maydell wrote:
> On 23 April 2018 at 11:14, Cédric Le Goater <clg@kaod.org> wrote:
>> commit 1d3e65aa7ac5 ("hw/timer: Add value matching support to
>> aspeed_timer") increased the vmstate version of aspeed.timer because
>> the state had changed, but it also bumped the version of the
>> VMSTATE_STRUCT_ARRAY under the aspeed.timerctrl which did not need to.
>>
>> Change back this version to fix migration.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks, applied to target-arm.next.
> 
>>  To test on a 'palmetto-bmc' board, download the latest built OpenBMC
>>  flash image :
>>
>>    wget https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=palmetto/lastSuccessfulBuild/artifact/deploy/images/palmetto/flash-palmetto
>>
>>  start two instances of QEMU with :
>>
>>    qemu-system-arm -m 512 -M palmetto-bmc -drive file=./flash-palmetto,format=raw,if=mtd -serial mon:stdio -nographic -nodefaults
> 
> Thanks also for the test image -- I'll add it to the collection I have.

for a romulus-bmc machine, which uses a ARM1176 CPU, you can download this
image :

   https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=romulus/lastSuccessfulBuild/artifact/deploy/images/romulus/flash-romulus

but migration fails :

  qemu-system-arm: error while loading state for instance 0x0 of device 'cpu'

There seems to be an issue while reading the CPACR reg.


> PS: there seems to be something odd up with the DNS for openpower.xyz:
> 
> $ host openpower.xyz
> openpower.xyz has address 146.20.65.228
> ;; connection timed out; no servers could be reached

indeed. hmm, I will report.

Thanks,

C. 

> (also manifests as wget pausing for several seconds before starting
> the download).
> 
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v2] timer/aspeed: fix vmstate version id

Posted by Cédric Le Goater 5 years, 11 months ago

> for a romulus-bmc machine, which uses a ARM1176 CPU, you can download this
> image :
> 
>    https://openpower.xyz/job/openbmc-build/distro=ubuntu,target=romulus/lastSuccessfulBuild/artifact/deploy/images/romulus/flash-romulus
> 
> but migration fails :
> 
>   qemu-system-arm: error while loading state for instance 0x0 of device 'cpu'
> 
> There seems to be an issue while reading the CPACR reg.

With a brutal setting of CPACR in cpu_reset, the romulus-bmc machine 
migrates. What would be a more appropriate approach ? 

Thanks,

C.  


@@ -166,6 +166,23 @@ static void arm_cpu_reset(CPUState *s)
 #if defined(CONFIG_USER_ONLY)
         /* Userspace expects access to cp10 and cp11 for FP/Neon */
         env->cp15.cpacr_el1 = deposit64(env->cp15.cpacr_el1, 20, 4, 0xf);
+#else
+        env->cp15.cpacr_el1 = 0;
+        if (arm_feature(env, ARM_FEATURE_VFP)) {
+            if (!arm_feature(env, ARM_FEATURE_NEON)) {
+                /* ASEDIS [31] bit is RAO/WI */
+                env->cp15.cpacr_el1 |= (1ull << 31);
+            }
+
+            /* VFPv3 and upwards with NEON implement 32 double precision
+             * registers (D0-D31).
+             */
+            if (!arm_feature(env, ARM_FEATURE_NEON) ||
+                    !arm_feature(env, ARM_FEATURE_VFP3)) {
+                /* D32DIS [30] is RAO/WI if D16-31 are not implemented. */
+                env->cp15.cpacr_el1 |= (1ull << 30);
+            }
+        }
 #endif
     }

Re: [Qemu-devel] [PATCH v2] timer/aspeed: fix vmstate version id

Posted by Peter Maydell 5 years, 11 months ago

On 2 May 2018 at 14:56, Cédric Le Goater <clg@kaod.org> wrote:
>> There seems to be an issue while reading the CPACR reg.
>
> With a brutal setting of CPACR in cpu_reset, the romulus-bmc machine
> migrates. What would be a more appropriate approach ?

Ah, yes, the bug here is that we made the cpacr_write() function
insist that certain bits are RAO/WI, but we didn't also change
the reset code to match. So (a) the guest initially reads a
value which doesn't have ones in the RAO bits and (b) when we
migrate we get a mismatch between what we had on the source
end and what we get when we write that to the destination end.

I think we can fix the reset value without having to duplicate
the logic from cpacr_write() using a resetfn. I'll cook up a patch...

thanks
-- PMM