[PULL,095/107] spapr: clock should count only if vm is running

On 02/02/2017 16:50, Mark Cave-Ayland wrote:
> On 02/02/17 14:20, Laurent Vivier wrote:
> 
>> I think adding the the PPCTimebase field and the VMSTATE_PPC_TIMEBASE_V
>> macro to the PMac machines should fix your issue.
>>
>> Do you have a test case I can try?
>>
>> Laurent
> 
> Hi Laurent,
> 
> Yes I'd say that is required, although I still think you need to migrate
> the decrementer value as per the comments on
> https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00546.html.
> 
> Here's the reproducer from an off-list email I sent last year:
> 
> 1) Download https://www.ilande.co.uk/tmp/darwin_empty.qcow2.xz and
> decompress the image (it's a pre-partitioned empty Apple Partition Map disk)
> 
> 2) Download https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz
> image, gunzip it and rename with .iso extension
> 
> 3) Start QEMU using the attached "empty" disk like this:
> 
> ./qemu-system-ppc -hda darwin_empty.qcow2 -cdrom darwinppc-602.iso -boot d
> 
> 4) Start the installer in the guest and you'll see lots of files with
> ASCII progress bars as the various files are copied to disk
> 
> Then to see the problem with the progress bar, repeat the following:
> 
> 5) Pause the VM
> 
> 6) Issue "savevm foo" in the monitor
> 
> 7) Exit QEMU
> 
> 8) Start QEMU again as below:
> 
> ./qemu-system-ppc -hda darwin_empty.qcow2 -cdrom darwinppc-602.iso -boot
> d -loadvm foo
> 
> If you do this enough times (maybe 10 or so?) you'll see the progress
> bars stop working correctly and get out of sync, i.e. it will freeze for
> long periods of time and then "jump" to catch-up but not all the way.
> 
> With my above patch applied to include the decrementer in the migration,
> the bug was no longer visible in my tests.

Thank you for all these details.

I've been able to reproduce the problem, and I think the proposition you
did in:

   https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01114.html

is the good one:



It's interesting because it doesn't break migration between different
qemu releases as the register is already part of the migration stream.
It was just not updated in the case of TCG (KVM is keeping it alive).
And in case of KVM, calling cpu_ppc_load_decr()/cpu_ppc_store_decr()
will not break anything as:

- cpu_ppc_load_decr() returns "env->spr[SPR_DECR]",
- cpu_ppc_store_decr() does nothing.

Could you re-send this patch with your S-o-b, please?

Thanks,
Laurent

On 07/02/17 15:46, Laurent Vivier wrote:

>> If you do this enough times (maybe 10 or so?) you'll see the progress
>> bars stop working correctly and get out of sync, i.e. it will freeze for
>> long periods of time and then "jump" to catch-up but not all the way.
>>
>> With my above patch applied to include the decrementer in the migration,
>> the bug was no longer visible in my tests.
> 
> Thank you for all these details.
> 
> I've been able to reproduce the problem, and I think the proposition you
> did in:
> 
>    https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01114.html
> 
> is the good one:
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index df9f7a4..1dc95b8 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -172,6 +172,7 @@ static void cpu_pre_save(void *opaque)
>      env->spr[SPR_CFAR] = env->cfar;
>  #endif
>      env->spr[SPR_BOOKE_SPEFSCR] = env->spe_fscr;
> +    env->spr[SPR_DECR] = cpu_ppc_load_decr(env);
> 
>      for (i = 0; (i < 4) && (i < env->nb_BATs); i++) {
>          env->spr[SPR_DBAT0U + 2*i] = env->DBAT[0][i];
> @@ -214,6 +215,7 @@ static int cpu_post_load(void *opaque, int version_id)
>      env->cfar = env->spr[SPR_CFAR];
>  #endif
>      env->spe_fscr = env->spr[SPR_BOOKE_SPEFSCR];
> +    cpu_ppc_store_decr(env, env->spr[SPR_DECR]);
> 
>      for (i = 0; (i < 4) && (i < env->nb_BATs); i++) {
>          env->DBAT[0][i] = env->spr[SPR_DBAT0U + 2*i];
> 
> 
> It's interesting because it doesn't break migration between different
> qemu releases as the register is already part of the migration stream.
> It was just not updated in the case of TCG (KVM is keeping it alive).
> And in case of KVM, calling cpu_ppc_load_decr()/cpu_ppc_store_decr()
> will not break anything as:
> 
> - cpu_ppc_load_decr() returns "env->spr[SPR_DECR]",
> - cpu_ppc_store_decr() does nothing.
> 
> Could you re-send this patch with your S-o-b, please?

Hi Laurent,

No problem, and thanks for the analysis. In fact, I have a couple of
other patches up on github which should fix up the remainder of the
issues and make g3beige migrateable (I would add the mac99 is currently
fairly close, however Ben has several WIP patches that change the mac99
model so I don't think it's worth making that machine officially
migrateable yet).

The one question I would ask is that if cpu_ppc_store_decr() does
nothing on KVM then would this causes issues attempting a migration
between TCG and KVM? In theory I believe I would still need to add
VMSTATE_PPC_TIMEBASE_V to the vmstate and encode the decrementer offset
relative to the timebase for this to work correctly as per the original
thread.

I'm just thinking if we are close to finalising the g3beige vmstate then
it would make sense to get it right so a KVM<>TCG migration can happen
if at all feasible.


ATB,

Mark.

On 09/02/2017 14:11, Mark Cave-Ayland wrote:
> On 07/02/17 15:46, Laurent Vivier wrote:
> 
>>> If you do this enough times (maybe 10 or so?) you'll see the progress
>>> bars stop working correctly and get out of sync, i.e. it will freeze for
>>> long periods of time and then "jump" to catch-up but not all the way.
>>>
>>> With my above patch applied to include the decrementer in the migration,
>>> the bug was no longer visible in my tests.
>>
>> Thank you for all these details.
>>
>> I've been able to reproduce the problem, and I think the proposition you
>> did in:
>>
>>    https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg01114.html
>>
>> is the good one:
>>
>> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
>> index df9f7a4..1dc95b8 100644
>> --- a/target/ppc/machine.c
>> +++ b/target/ppc/machine.c
>> @@ -172,6 +172,7 @@ static void cpu_pre_save(void *opaque)
>>      env->spr[SPR_CFAR] = env->cfar;
>>  #endif
>>      env->spr[SPR_BOOKE_SPEFSCR] = env->spe_fscr;
>> +    env->spr[SPR_DECR] = cpu_ppc_load_decr(env);
>>
>>      for (i = 0; (i < 4) && (i < env->nb_BATs); i++) {
>>          env->spr[SPR_DBAT0U + 2*i] = env->DBAT[0][i];
>> @@ -214,6 +215,7 @@ static int cpu_post_load(void *opaque, int version_id)
>>      env->cfar = env->spr[SPR_CFAR];
>>  #endif
>>      env->spe_fscr = env->spr[SPR_BOOKE_SPEFSCR];
>> +    cpu_ppc_store_decr(env, env->spr[SPR_DECR]);
>>
>>      for (i = 0; (i < 4) && (i < env->nb_BATs); i++) {
>>          env->DBAT[0][i] = env->spr[SPR_DBAT0U + 2*i];
>>
>>
>> It's interesting because it doesn't break migration between different
>> qemu releases as the register is already part of the migration stream.
>> It was just not updated in the case of TCG (KVM is keeping it alive).
>> And in case of KVM, calling cpu_ppc_load_decr()/cpu_ppc_store_decr()
>> will not break anything as:
>>
>> - cpu_ppc_load_decr() returns "env->spr[SPR_DECR]",
>> - cpu_ppc_store_decr() does nothing.
>>
>> Could you re-send this patch with your S-o-b, please?
> 
> Hi Laurent,
> 
> No problem, and thanks for the analysis. In fact, I have a couple of
> other patches up on github which should fix up the remainder of the
> issues and make g3beige migrateable (I would add the mac99 is currently
> fairly close, however Ben has several WIP patches that change the mac99
> model so I don't think it's worth making that machine officially
> migrateable yet).
> 
> The one question I would ask is that if cpu_ppc_store_decr() does
> nothing on KVM then would this causes issues attempting a migration
> between TCG and KVM? In theory I believe I would still need to add

It should work: on the TCG one we read/write the decr_next field of
ppc_tb_t, on the KVM one we write/read the spr[SPR_DECR] field.

> VMSTATE_PPC_TIMEBASE_V to the vmstate and encode the decrementer offset
> relative to the timebase for this to work correctly as per the original

VMSTATE_PPC_TIMEBASE_V is used to update tb_offset.

With TCG, tb_offset is always 0 because TBR is based on a
QEMU_CLOCK_VIRTUAL clock: this clock is started with guest and stopped
when the guest is stopped. With KVM the guest uses the real TBR of the
host, and so when the guest is started it is not 0 and when the guest is
stopped, it continues to count. So we need an offset to adjust the guest
TBR.

So:
- TCG doesn't need VMSTATE_PPC_TIMEBASE_V,
- VMSTATE_PPC_TIMEBASE_V can't be used to migrate between TCG and KVM
guests.

If we want to migrate the TBR between TCG and KVM, I think we should
update spr[SPR_TBL]/spr[SPR_TBU] as we do for spr[SPR_DECR]. In the case
of KVM, it will be overwritten by the computed one from tb_offset when
the guest is restarted.

But of course, if you want to migrate g3beige/mac99 with KVM you need
VMSTATE_PPC_TIMEBASE_V in the machine structures. But only for the TBR,
not for the DECR, as it is a relative time (it's a decrementer :) )not
an absolute time like TBR.

> thread.
> 
> I'm just thinking if we are close to finalising the g3beige vmstate then
> it would make sense to get it right so a KVM<>TCG migration can happen
> if at all feasible.

I'm wondering if it works on the other architectures?

Laurent