RE: [RFC PATCH 3/3] Enable ptp_kvm for arm64

["Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com>]

Hi Marc,

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: Monday, September 9, 2019 7:25 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>
> Cc: netdev@vger.kernel.org; pbonzini@redhat.com;
> sean.j.christopherson@intel.com; richardcochran@gmail.com; Mark Rutland
> <Mark.Rutland@arm.com>; Will Deacon <Will.Deacon@arm.com>; Suzuki
> Poulose <Suzuki.Poulose@arm.com>; linux-kernel@vger.kernel.org; Steve
> Capper <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>
> Subject: Re: [RFC PATCH 3/3] Enable ptp_kvm for arm64
>
> On Mon, 09 Sep 2019 11:17:24 +0100,
> "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@arm.com> wrote:
>
> Hi Jianyoung,
>
> [...]
>
> > > > > I'm definitely not keen on exposing the internals of the
> > > > > arch_timer driver to random subsystems. Furthermore, you seem to
> > > > > expect that the guest kernel will only use the arch timer as a
> > > > > clocksource, and nothing really guarantees that (in which case
> > > get_device_system_crosststamp will fail).
> > > > >
> > > > The code here is really ugly, I need a better solution to offer a
> > > > clock source For the guest.
> > > >
> > > > > It looks to me that we'd be better off exposing a core
> > > > > timekeeping API that populates a struct system_counterval_t
> > > > > based on the
> > > > > *current* timekeeper monotonic clocksource. This would simplify
> > > > > the split between generic and arch-specific code.
> > > > >
> > > > I think it really necessary.
> > > >
> > > > > Whether or not tglx will be happy with the idea is another
> > > > > problem, but I'm certainly not taking any change to the arch
> > > > > timer code based on
> > > this.
> > > > >
> > > > I can have a try, but the detail is not clear for me now.
> > >
> > > Something along those lines:
> > >
> > > From 5f1c061e55c691d64012bc7c1490a1a8c4432c67 Mon Sep 17 00:00:00
> > > 2001
> > > From: Marc Zyngier <maz@kernel.org>
> > > Date: Sat, 7 Sep 2019 10:11:49 +0100
> > > Subject: [PATCH] timekeeping: Expose API allowing retrival of
> > > current clocksource and counter value
> > >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  include/linux/timekeeping.h |  5 +++++
> > >  kernel/time/timekeeping.c   | 12 ++++++++++++
> > >  2 files changed, 17 insertions(+)
> > >
> > > diff --git a/include/linux/timekeeping.h
> > > b/include/linux/timekeeping.h index
> > > b27e2ffa96c1..6df26a913711 100644
> > > --- a/include/linux/timekeeping.h
> > > +++ b/include/linux/timekeeping.h
> > > @@ -275,6 +275,11 @@ extern int get_device_system_crosststamp(
> > >                       struct system_time_snapshot *history,
> > >                       struct system_device_crosststamp *xtstamp);
> > >
> > > +/*
> > > + * Obtain current monotonic clock and its counter value  */ extern
> > > +void get_current_counterval(struct system_counterval_t *sc);
> > > +
> > >  /*
> > >   * Simultaneously snapshot realtime and monotonic raw clocks
> > >   */
> > > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > > index
> > > d911c8470149..de689bbd3808 100644
> > > --- a/kernel/time/timekeeping.c
> > > +++ b/kernel/time/timekeeping.c
> > > @@ -1098,6 +1098,18 @@ static bool cycle_between(u64 before, u64
> > > test,
> > > u64 after)
> > >       return false;
> > >  }
> > >
> > > +/**
> > > + * get_current_counterval - Snapshot the current clocksource and
> > > +counter
> > > value
> > > + * @sc:      Pointer to a struct containing the current clocksource and its
> > > value
> > > + */
> > > +void get_current_counterval(struct system_counterval_t *sc) {
> > > +     struct timekeeper *tk = &tk_core.timekeeper;
> > > +
> > > +     sc->cs = READ_ONCE(tk->tkr_mono.clock);
> > > +     sc->cycles = sc->cs->read(sc->cs); }
> > > +
> > >  /**
> > >   * get_device_system_crosststamp - Synchronously capture
> > > system/device timestamp
> > >   * @get_time_fn:     Callback to get simultaneous device time and
> > >
> > > which should do the right thing.
> > >
> > It is a good news for me. These code is indeed what I need!  So what's
> > your plan about this patch?  Is there any problem with you if I
> > include these code into my patch ?
>
> Just add this patch as part of your series (I'll try to write an actual commit log
> for that).

Very kind of you!
>
> [...]
>
> > > > > Other questions: how does this works with VM migration?
> > > > > Specially when moving from a hypervisor that supports the
> > > > > feature to one that
> > > doesn't?
> > > > >
> > > > I think it won't solve the problem generated by VM migration and
> > > > only for VMs in a single machine.  Ptp_kvm only works for VMs in
> > > > the same machine.  But using ptp (not ptp_kvm) clock, all the
> > > > machines in a low latency network environment can keep time sync
> > > > in high precision, Then VMs move from one machine to another will
> > > > obtain a high precision time sync.
> > >
> > > That's a problem. Migration must be possible from one host to
> > > another, even if that means temporarily loosing some (or a lot of)
> > > precision. The service must be discoverable from userspace on the
> > > host so that the MVV can decie whether a migration is possible or not.
> > >
> > Don't worry, things will be not that bad.  ptp_kvm will not trouble
> > the VM migration. This ptp_kvm is one clocksource of the clock pool
> > for chrony. Chrony will choose the highest precision clock from the
> > pool. If host does not support ptp_kvm, the ptp_kvm will not be chosen
> > as the clocksouce of chrony.  We have roughly the same logic of
> > implementation of ptp_kvm with x86, and ptp_kvm works well in x86.  so
> > I think that will be the case for arm64.
> >
> > Maybe I miss your point, I have no idea of MVV and can't get related
> > info from google.  Also I'm not clear of your last words of how to
> > decide VM migration is possible?
>
> Sorry. s/MVV/VMM/. Basically userspace, such as QEMU.
>
> Here's an example: The guest runs on a PTP aware host, starts using the PTP
> service and uses HVC calls to get its clock. We now migrate the guest to a non
> PTP-aware host. The hypercalls are now going to fail unexpectedly. Is that
> something that is acceptable? I don't think it is. Once you've allowed a guest
> to use a service, this service should be preserved. I'd be more confident if we
> gave to userspace the indication that the hypervisor supports PTP. Userspace
> can then decide whether to perform migration or not.
>

It's really a point we should consider. let me check the behavior of chrony in this scenario first.

Thanks
Jianyong Wu

> Thanks,
>
>       M.
>
> --
> Jazz is not dead, it just smells funny.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.