Re: [Xen-devel] [PATCH v3 5/6] x86/time: implement PVCLOCK_TSC_STABLE_BIT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.08.16 at 14:43, <joao.m.martins@xxxxxxxxxx> wrote:
> This patch proposes relying on host TSC synchronization and
> passthrough to the guest, when running on a TSC-safe platform. On
> time_calibration we retrieve the platform time in ns and the counter
> read by the clocksource that was used to compute system time. We
> introduce a new rendezous function which doesn't require
> synchronization between master and slave CPUS and just reads
> calibration_rendezvous struct and writes it down the stime and stamp
> to the cpu_calibration struct to be used later on. We can guarantee that
> on a platform with a constant and reliable TSC, that the time read on
> vcpu B right after A is bigger independently of the VCPU calibration
> error. Since pvclock time infos are monotonic as seen by any vCPU set
> PVCLOCK_TSC_STABLE_BIT, which then enables usage of VDSO on Linux.
> IIUC, this is similar to how it's implemented on KVM.

Without any tools side change, how is it guaranteed that a guest
which observed the stable bit won't get migrated to a host not
providing that guarantee?

> Changes since v2:
>  - Add XEN_ prefix to pvclock flags.
>  - Adapter time_calibration_rendezvous_tail to have the case of setting master
>  tsc/stime and use it for the nop_rendezvous.
>  - Removed hotplug CPU option that was added in v1
>  - Prevent online of CPUs when clocksource is tsc.

Some of the above listed changes don't seem to belong here, but
rather in one of the earlier patches.

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -631,7 +631,8 @@ ret_t 
> do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>          if ( ret )
>              break;
>  
> -        if ( cpu >= nr_cpu_ids || !cpu_present(cpu) )
> +        if ( cpu >= nr_cpu_ids || !cpu_present(cpu) ||
> +             host_tsc_is_clocksource() )

I have to admit that I consider the "host" here confusing: What other
TSCs could one think of on x86? Maybe clocksource_is_tsc()?

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -480,6 +480,13 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>  
>  static s64 __init init_tsctimer(struct platform_timesource *pts)
>  {
> +    if ( nr_cpu_ids != num_present_cpus() )
> +    {
> +        printk(XENLOG_INFO "TSC: CPU Hotplug intended,"
> +                           "not using TSC as clocksource\n");

Please don't split log messages across lines. Keep the XENLOG_INFO
on one line, and then the whole literal on the next.

Also you/we will need to take some measures to avoid this triggering
on systems where extra MADT entries exist just as dummies (perhaps
to ease firmware implementation, as was the case prior to commit
0875433389 ["hvmloader: limit CPUs exposed to guests"] with our
own ACPI tables we present to HVM guests).

> @@ -1328,12 +1337,22 @@ struct calibration_rendezvous {
>  };
>  
>  static void
> -time_calibration_rendezvous_tail(const struct calibration_rendezvous *r)
> +time_calibration_rendezvous_tail(const struct calibration_rendezvous *r,
> +                                 bool_t master_tsc)

Please use plain"bool" in new code. And then I'm not convinced this
refactoring is better than simply having the new rendezvous function
not call time_calibration_rendezvous_tail().

>  {
>      struct cpu_time_stamp *c = &this_cpu(cpu_calibration);
>  
> -    c->local_tsc    = rdtsc_ordered();
> -    c->local_stime  = get_s_time_fixed(c->local_tsc);
> +    if ( master_tsc )
> +    {
> +        c->local_tsc    = r->master_tsc_stamp;

Doesn't this require the TSCs to run in perfect sync (not even off
wrt each other by a single cycle)? Is such even possible on multi
socket systems? I.e. how would multiple chips get into such a
mode in the first place, considering their TSCs can't possibly start
ticking at exactly the same (perhaps even sub-)nanosecond?

> +static void time_calibration_nop_rendezvous(void *rv)
> +{
> +    const struct calibration_rendezvous *r = rv;
> +
> +    time_calibration_rendezvous_tail(r, true);
>  }

I don't see what you need the local variable for, unless you
stopped calling time_calibration_rendezvous_tail() as suggested
above.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel