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

[David Gibson <david@gibson.dropbear.id.au>]

[-- 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 --]