Re: [win-pv-devel] [PATCH] Wallclock Time Calculation Checks Update Versions For Consistency

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel [mailto:win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On
> Behalf Of Eric Mackay
> Sent: 19 August 2017 01:23
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Eric Mackay <mackayem@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH] Wallclock Time Calculation Checks Update
> Versions For Consistency
> 
> Checking the shared_info update versions is necessary to get a
> consistent set of values. The version is incremented once when the
> update starts, and then incremented again after the update has
> completed. To verify that a set of values obtained from shared_info
> is consistent, the version must not only look at equality of
> versions, but the version must also be even. Data can only be safely
> be captured within the version check loop.
> 
> There is no need to use a hypercall to get the system time, since
> this is alredy captured in the shared_info struct. A cached version
> of the time since boot is stored in structures for each vcpu, but
> this has to be combined with the timestamp counter and some scaling
> factors to get the actual current time since boot.
> 
> Clock synchronization can also occur, and the dom0 will ensure that
> the values in the shared_info and vcpu_time_info structs are kept
> current to reflect this.
> 
> Signed-off-by: Eric Mackay <mackayem@xxxxxxxxxx>

Eric,

The change looks good to me. What sort of testing has been done on this?

  Cheers,

    Paul

> ---
>  src/xenbus/shared_info.c | 65
> ++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/src/xenbus/shared_info.c b/src/xenbus/shared_info.c
> index d6babcf..2666f76 100644
> --- a/src/xenbus/shared_info.c
> +++ b/src/xenbus/shared_info.c
> @@ -329,6 +329,10 @@ SharedInfoEvtchnUnmask(
>      return SharedInfoTestBit(&Shared->evtchn_pending[SelectorBit],
> PortBit);
>  }
> 
> +#ifndef BooleanFlagOn
> +#define BooleanFlagOn(F,SF)   ((BOOLEAN)(((F) & (SF)) != 0))
> +#endif
> +
>  static LARGE_INTEGER
>  SharedInfoGetTime(
>      IN  PINTERFACE              Interface
> @@ -336,51 +340,80 @@ SharedInfoGetTime(
>  {
>      PXENBUS_SHARED_INFO_CONTEXT Context = Interface->Context;
>      shared_info_t               *Shared;
> -    ULONG                       Version;
> +    ULONG                       WcVersion;
> +    ULONG                       TimeVersion;
>      ULONGLONG                   Seconds;
>      ULONGLONG                   NanoSeconds;
> +    ULONGLONG                   Timestamp;
> +    ULONGLONG                   Tsc;
> +    ULONGLONG                   SystemTime;
> +    ULONG                       TscSystemMul;
> +    CHAR                        TscShift;
>      LARGE_INTEGER               Now;
>      TIME_FIELDS                 Time;
>      KIRQL                       Irql;
> -    NTSTATUS                    status;
> 
>      // Make sure we don't suspend
> -    KeRaiseIrql(DISPATCH_LEVEL, &Irql);
> +    KeRaiseIrql(DISPATCH_LEVEL, &Irql);
> 
>      Shared = Context->Shared;
> 
> +    // Loop until we can read a consistent set of values from the same update
>      do {
> -        Version = Shared->wc_version;
> +        WcVersion = Shared->wc_version;
> +        TimeVersion = Shared->vcpu_info[0].time.version;
>          KeMemoryBarrier();
> 
> +        // Wallclock time at system time zero (guest boot or resume)
>          Seconds = Shared->wc_sec;
>          NanoSeconds = Shared->wc_nsec;
> +
> +        // Cached time in nanoseconds since guest boot
> +        SystemTime = Shared->vcpu_info[0].time.system_time;
> +
> +        // Timestamp counter value when these time values were last updated
> +        Timestamp = Shared->vcpu_info[0].time.tsc_timestamp;
> +
> +        // Timestamp modifiers
> +        TscShift = Shared->vcpu_info[0].time.tsc_shift;
> +        TscSystemMul = Shared->vcpu_info[0].time.tsc_to_system_mul;
>          KeMemoryBarrier();
> -    } while (Shared->wc_version != Version);
> 
> -    // Get the number of nanoseconds since boot
> -    status = HvmGetTime(&Now);
> -    if (!NT_SUCCESS(status))
> -        Now.QuadPart = Shared->vcpu_info[0].time.system_time;
> +    // Version is incremented to indicate update in progress
> +    // LSB of version is set if update in progress
> +    // Version is incremented again once update has completed
> +    } while (Shared->wc_version != WcVersion ||
> +             Shared->vcpu_info[0].time.version != TimeVersion ||
> +             BooleanFlagOn(WcVersion, 0x1) ||
> +             BooleanFlagOn(TimeVersion, 0x1));
> +
> +    // Read counter ticks
> +    Tsc = ReadTimeStampCounter();
> 
>      KeLowerIrql(Irql);
> 
> -    Trace("WALLCLOCK: Seconds = %llu NanoSeconds = %llu\n",
> +    // Number of elapsed ticks since timestamp was captured
> +    Tsc -= Timestamp;
> +
> +    // Time in nanoseconds since boot
> +    SystemTime += ((Tsc << TscShift) * TscSystemMul) >> 32;
> +
> +    Trace("WALLCLOCK TIME AT BOOT: Seconds = %llu NanoSeconds =
> %llu\n",
>            Seconds,
>            NanoSeconds);
> 
> -    Trace("BOOT: Seconds = %llu NanoSeconds = %llu\n",
> -          Now.QuadPart / 1000000000ull,
> -          Now.QuadPart % 1000000000ull);
> +    Trace("TIME SINCE BOOT: Seconds = %llu NanoSeconds = %llu\n",
> +          SystemTime / 1000000000ull,
> +          SystemTime % 1000000000ull);
> 
>      // Convert wallclock from Unix epoch (1970) to Windows epoch (1601)
>      Seconds += 11644473600ull;
> 
>      // Add in time since host boot
> -    Seconds += Now.QuadPart / 1000000000ull;
> -    NanoSeconds += Now.QuadPart % 1000000000ull;
> +    Seconds += SystemTime / 1000000000ull;
> +    NanoSeconds += SystemTime % 1000000000ull;
> 
> -    // Converto to system time format
> +    // Convert to system time format
>      Now.QuadPart = (Seconds * 10000000ull) + (NanoSeconds / 100ull);
> 
>      RtlTimeToTimeFields(&Now, &Time);
> --
> 2.10.1.windows.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel