Date:   Mon, 09 Sep 2019 12:24:59 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        Mark Rutland <Mark.Rutland@....com>,
        Will Deacon <Will.Deacon@....com>,
        Suzuki Poulose <Suzuki.Poulose@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Steve Capper <Steve.Capper@....com>,
        "Kaly Xin (Arm Technology China)" <Kaly.Xin@....com>,
        "Justin He (Arm Technology China)" <Justin.He@....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@....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@...nel.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@...nel.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).

[...]

> > > > 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.

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.

Powered by blists - more mailing lists