[Qemu-devel] [PATCH 0/3] ppc: add timebase migration support to Mac machines

[Qemu-devel] [PATCH 0/3] ppc: add timebase migration support to Mac machines

[Mark Cave-Ayland]

This patchset allows migration of the PPC timebase for g3beige/mac99
machines under TCG on non-PPC hosts.

The majority of the work is in patch 2: here the existing migration code is
split into PPC and non-PPC host codepaths (where the previous behaviour is
preserved). In effect, non-PPC hosts use QEMU's emulated timebase routines
which are based upon the guest virtual clock, but it is still possible to
migrate guests in the same manner.

Finally patch 3 enables the inclusion of the timebase in the migration stream
for both Old World and New World Macs.

Unfortunately I have no ability to test this on KVM-enabled hardware, however
it should preserve the existing behaviour, barring the bugfix in patch 1.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Mark Cave-Ayland (3):
  ppc: fix timebase adjustment during migration
  ppc: add support for timebase migration on non-PPC hosts
  ppc: include timebase in migration stream for g3beige/mac99 machines

 hw/ppc/mac_newworld.c |    4 ++++
 hw/ppc/mac_oldworld.c |    4 ++++
 hw/ppc/ppc.c          |   35 ++++++++++++++++++++++++++++-------
 3 files changed, 36 insertions(+), 7 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/3] ppc: fix timebase adjustment during migration

[Mark Cave-Ayland]

ns_diff is already clamped to a minimum of 0 to prevent the timebase going
backwards during migration due to misaligned clocks. Following on from this
migration_duration_tb is also subject to the same constraint; hence the
expression MIN(0, migration_duration_tb) always evaluates to 0 and so no
timebase adjustment ever takes place.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/ppc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index ce90b09..19f4570 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -877,7 +877,7 @@ static int timebase_post_load(void *opaque, int version_id)
     migration_duration_ns = MIN(NANOSECONDS_PER_SECOND, ns_diff);
     migration_duration_tb = muldiv64(migration_duration_ns, freq,
                                      NANOSECONDS_PER_SECOND);
-    guest_tb = tb_remote->guest_timebase + MIN(0, migration_duration_tb);
+    guest_tb = tb_remote->guest_timebase + migration_duration_tb;
 
     tb_off_adj = guest_tb - cpu_get_host_ticks();
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/3] ppc: fix timebase adjustment during migration

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

On Sun, Jan 31, 2016 at 07:19:34PM +0000, Mark Cave-Ayland wrote:
> ns_diff is already clamped to a minimum of 0 to prevent the timebase going
> backwards during migration due to misaligned clocks. Following on from this
> migration_duration_tb is also subject to the same constraint; hence the
> expression MIN(0, migration_duration_tb) always evaluates to 0 and so no
> timebase adjustment ever takes place.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

So, there are actually two problems here, which could be expressed a
bit more clearly in the commit message.

First, this clamping is redundant, because of the earlier clamp on
ns_diff.  Well.. probably.. I do wonder if we could get an overflow
anywhere giving us a negative number again.

More importantly, though, this is supposed to be a clamp below, which
needs a MAX.  MIN is Just Plain Wrong.

> ---
>  hw/ppc/ppc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index ce90b09..19f4570 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -877,7 +877,7 @@ static int timebase_post_load(void *opaque, int version_id)
>      migration_duration_ns = MIN(NANOSECONDS_PER_SECOND, ns_diff);
>      migration_duration_tb = muldiv64(migration_duration_ns, freq,
>                                       NANOSECONDS_PER_SECOND);
> -    guest_tb = tb_remote->guest_timebase + MIN(0, migration_duration_tb);
> +    guest_tb = tb_remote->guest_timebase + migration_duration_tb;
>  
>      tb_off_adj = guest_tb - cpu_get_host_ticks();
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/3] ppc: add support for timebase migration on non-PPC hosts

[Mark Cave-Ayland]

This patch provides support for migration of the PPC guest timebase on non-PPC
host architectures (i.e those using QEMU's virtual emulated timebase).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/ppc.c |   33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 19f4570..9b80c1d 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -832,6 +832,15 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
     cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
 }
 
+static int host_cpu_is_ppc(void)
+{
+#if defined(_ARCH_PPC)
+    return -1;
+#else
+    return 0;
+#endif
+}
+
 static void timebase_pre_save(void *opaque)
 {
     PPCTimebase *tb = opaque;
@@ -844,11 +853,16 @@ static void timebase_pre_save(void *opaque)
     }
 
     tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
-    /*
-     * tb_offset is only expected to be changed by migration so
-     * there is no need to update it from KVM here
-     */
-    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
+
+    if (host_cpu_is_ppc()) {
+        /*
+         * tb_offset is only expected to be changed by migration so
+         * there is no need to update it from KVM here
+         */
+        tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
+    } else {
+        tb->guest_timebase = cpu_ppc_load_tbl(&first_ppc_cpu->env);
+    }
 }
 
 static int timebase_post_load(void *opaque, int version_id)
@@ -879,7 +893,14 @@ static int timebase_post_load(void *opaque, int version_id)
                                      NANOSECONDS_PER_SECOND);
     guest_tb = tb_remote->guest_timebase + migration_duration_tb;
 
-    tb_off_adj = guest_tb - cpu_get_host_ticks();
+    if (host_cpu_is_ppc()) {
+        /* Hardware timebase */
+        tb_off_adj = guest_tb - cpu_get_host_ticks();
+    } else {
+        /* Software timebase */
+        tb_off_adj = guest_tb - muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                                         freq, get_ticks_per_sec());
+    }
 
     tb_off = first_ppc_cpu->env.tb_env->tb_offset;
     trace_ppc_tb_adjust(tb_off, tb_off_adj, tb_off_adj - tb_off,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/3] ppc: add support for timebase migration on non-PPC hosts

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]

On Sun, Jan 31, 2016 at 07:19:35PM +0000, Mark Cave-Ayland wrote:
> This patch provides support for migration of the PPC guest timebase on non-PPC
> host architectures (i.e those using QEMU's virtual emulated timebase).
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

We shouldn't need an explicit test for a ppc host.  Instead we should
never be touching any host-dependent ticks values, only using host
side interfaces which work in realtime units like ns.

Worse, the ppc host variants here will still be wrong if the host has
a different timebase frequency to the guest, which will always be true
for a g3beige (16MHz) on a modern ppc host (512 MHz).


> ---
>  hw/ppc/ppc.c |   33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 19f4570..9b80c1d 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -832,6 +832,15 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
>      cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
>  }
>  
> +static int host_cpu_is_ppc(void)
> +{
> +#if defined(_ARCH_PPC)
> +    return -1;
> +#else
> +    return 0;
> +#endif
> +}
> +
>  static void timebase_pre_save(void *opaque)
>  {
>      PPCTimebase *tb = opaque;
> @@ -844,11 +853,16 @@ static void timebase_pre_save(void *opaque)
>      }
>  
>      tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> -    /*
> -     * tb_offset is only expected to be changed by migration so
> -     * there is no need to update it from KVM here
> -     */
> -    tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> +
> +    if (host_cpu_is_ppc()) {
> +        /*
> +         * tb_offset is only expected to be changed by migration so
> +         * there is no need to update it from KVM here
> +         */
> +        tb->guest_timebase = ticks + first_ppc_cpu->env.tb_env->tb_offset;
> +    } else {
> +        tb->guest_timebase = cpu_ppc_load_tbl(&first_ppc_cpu->env);
> +    }
>  }
>  
>  static int timebase_post_load(void *opaque, int version_id)
> @@ -879,7 +893,14 @@ static int timebase_post_load(void *opaque, int version_id)
>                                       NANOSECONDS_PER_SECOND);
>      guest_tb = tb_remote->guest_timebase + migration_duration_tb;
>  
> -    tb_off_adj = guest_tb - cpu_get_host_ticks();
> +    if (host_cpu_is_ppc()) {
> +        /* Hardware timebase */
> +        tb_off_adj = guest_tb - cpu_get_host_ticks();
> +    } else {
> +        /* Software timebase */
> +        tb_off_adj = guest_tb - muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                                         freq, get_ticks_per_sec());
> +    }
>  
>      tb_off = first_ppc_cpu->env.tb_env->tb_offset;
>      trace_ppc_tb_adjust(tb_off, tb_off_adj, tb_off_adj - tb_off,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 3/3] ppc: include timebase in migration stream for g3beige/mac99 machines

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/mac_newworld.c |    4 ++++
 hw/ppc/mac_oldworld.c |    4 ++++
 2 files changed, 8 insertions(+)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index f95086b..3283f1d 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -179,6 +179,7 @@ static void ppc_core99_init(MachineState *machine)
     int *token = g_new(int, 1);
     hwaddr nvram_addr = 0xFFF04000;
     uint64_t tbfreq;
+    PPCTimebase *tb;
 
     linux_boot = (kernel_filename != NULL);
 
@@ -201,6 +202,9 @@ static void ppc_core99_init(MachineState *machine)
         /* Set time-base frequency to 100 Mhz */
         cpu_ppc_tb_init(env, TBFREQ);
         qemu_register_reset(ppc_core99_reset, cpu);
+
+        tb = g_malloc0(sizeof(PPCTimebase));
+        vmstate_register(NULL, -1, &vmstate_ppc_timebase, tb);
     }
 
     /* allocate RAM */
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 8984398..45e410b 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -104,6 +104,7 @@ static void ppc_heathrow_init(MachineState *machine)
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     void *fw_cfg;
     uint64_t tbfreq;
+    PPCTimebase *tb;
 
     linux_boot = (kernel_filename != NULL);
 
@@ -121,6 +122,9 @@ static void ppc_heathrow_init(MachineState *machine)
         /* Set time-base frequency to 16.6 Mhz */
         cpu_ppc_tb_init(env,  TBFREQ);
         qemu_register_reset(ppc_heathrow_reset, cpu);
+
+        tb = g_malloc0(sizeof(PPCTimebase));
+        vmstate_register(NULL, -1, &vmstate_ppc_timebase, tb);
     }
 
     /* allocate RAM */
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/3] ppc: include timebase in migration stream for g3beige/mac99 machines

[Peter Maydell]

On 31 January 2016 at 19:19, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/ppc/mac_newworld.c |    4 ++++
>  hw/ppc/mac_oldworld.c |    4 ++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index f95086b..3283f1d 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -179,6 +179,7 @@ static void ppc_core99_init(MachineState *machine)
>      int *token = g_new(int, 1);
>      hwaddr nvram_addr = 0xFFF04000;
>      uint64_t tbfreq;
> +    PPCTimebase *tb;
>
>      linux_boot = (kernel_filename != NULL);
>
> @@ -201,6 +202,9 @@ static void ppc_core99_init(MachineState *machine)
>          /* Set time-base frequency to 100 Mhz */
>          cpu_ppc_tb_init(env, TBFREQ);
>          qemu_register_reset(ppc_core99_reset, cpu);
> +
> +        tb = g_malloc0(sizeof(PPCTimebase));
> +        vmstate_register(NULL, -1, &vmstate_ppc_timebase, tb);

Is there no way to avoid the vmstate_register here (ie to
tie the migration data to an actual device or CPU object) ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/3] ppc: include timebase in migration stream for g3beige/mac99 machines

[Mark Cave-Ayland]

On 31/01/16 19:58, Peter Maydell wrote:

> On 31 January 2016 at 19:19, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/ppc/mac_newworld.c |    4 ++++
>>  hw/ppc/mac_oldworld.c |    4 ++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index f95086b..3283f1d 100644
>> --- a/hw/ppc/mac_newworld.c
>> +++ b/hw/ppc/mac_newworld.c
>> @@ -179,6 +179,7 @@ static void ppc_core99_init(MachineState *machine)
>>      int *token = g_new(int, 1);
>>      hwaddr nvram_addr = 0xFFF04000;
>>      uint64_t tbfreq;
>> +    PPCTimebase *tb;
>>
>>      linux_boot = (kernel_filename != NULL);
>>
>> @@ -201,6 +202,9 @@ static void ppc_core99_init(MachineState *machine)
>>          /* Set time-base frequency to 100 Mhz */
>>          cpu_ppc_tb_init(env, TBFREQ);
>>          qemu_register_reset(ppc_core99_reset, cpu);
>> +
>> +        tb = g_malloc0(sizeof(PPCTimebase));
>> +        vmstate_register(NULL, -1, &vmstate_ppc_timebase, tb);
> 
> Is there no way to avoid the vmstate_register here (ie to
> tie the migration data to an actual device or CPU object) ?

Not exactly that I know of - although I shamelessly borrowed this part
from similar code in spapr which has this comment:

    /* FIXME: Should register things through the MachineState's qdev
     * interface, this is a legacy from the sPAPREnvironment structure
     * which predated MachineState but had a similar function */

Is this something that is now possible?


ATB,

Mark.

Re: [Qemu-devel] [PATCH 3/3] ppc: include timebase in migration stream for g3beige/mac99 machines

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]

On Sun, Jan 31, 2016 at 08:10:08PM +0000, Mark Cave-Ayland wrote:
> On 31/01/16 19:58, Peter Maydell wrote:
> 
> > On 31 January 2016 at 19:19, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>  hw/ppc/mac_newworld.c |    4 ++++
> >>  hw/ppc/mac_oldworld.c |    4 ++++
> >>  2 files changed, 8 insertions(+)
> >>
> >> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> >> index f95086b..3283f1d 100644
> >> --- a/hw/ppc/mac_newworld.c
> >> +++ b/hw/ppc/mac_newworld.c
> >> @@ -179,6 +179,7 @@ static void ppc_core99_init(MachineState *machine)
> >>      int *token = g_new(int, 1);
> >>      hwaddr nvram_addr = 0xFFF04000;
> >>      uint64_t tbfreq;
> >> +    PPCTimebase *tb;
> >>
> >>      linux_boot = (kernel_filename != NULL);
> >>
> >> @@ -201,6 +202,9 @@ static void ppc_core99_init(MachineState *machine)
> >>          /* Set time-base frequency to 100 Mhz */
> >>          cpu_ppc_tb_init(env, TBFREQ);
> >>          qemu_register_reset(ppc_core99_reset, cpu);
> >> +
> >> +        tb = g_malloc0(sizeof(PPCTimebase));
> >> +        vmstate_register(NULL, -1, &vmstate_ppc_timebase, tb);
> > 
> > Is there no way to avoid the vmstate_register here (ie to
> > tie the migration data to an actual device or CPU object) ?
> 
> Not exactly that I know of - although I shamelessly borrowed this part
> from similar code in spapr which has this comment:
> 
>     /* FIXME: Should register things through the MachineState's qdev
>      * interface, this is a legacy from the sPAPREnvironment structure
>      * which predated MachineState but had a similar function */
> 
> Is this something that is now possible?

Well, it's certainly possible to do better than this.  You want to
make a vmstate_g3beige and vmstate_mac99 which contain all the machine
level things to migrate for these machines, similar to vmstate_spapr.
They will be attached to the MachineState object.

That will at least mean that if more things need to get added to
migration for these machines, then additional vmstate_register() calls
won't be needed.

I'm not sure if there's a better way to register a vmstate for a
machine type.  I thought there was, but I couldn't spot it in a quick
lock.


Peter,

I believe this does need to be attached to the machine, not to the
cpu, even though the cpu would seem to make more sense on a first
look.  The reason is that attaching it to the cpu means it will be
transferred separately for each cpu, and unless we're super-careful
about timing the destination cpus could end up with slightly different
values.  That would be bad, because ppc has a pretty strong
requirement that the timebases be synchronized across all cpus in an
smp system.  The means of initially accomplishing that vary by
platform - usually there's some board level register to freeze /
resume all the timebases - but however it's been done, we don't want
to mess it up on migration.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]