netdev - RE: [PATCH v5 3/6] timekeeping: Add clocksource to system_time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 16 Oct 2019 09:48:43 +0000
From:   "Jianyong Wu (Arm Technology China)" <Jianyong.Wu@....com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Paolo Bonzini <pbonzini@...hat.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "yangbo.lu@....com" <yangbo.lu@....com>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>,
        "sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
        "maz@...nel.org" <maz@...nel.org>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        Mark Rutland <Mark.Rutland@....com>,
        "will@...nel.org" <will@...nel.org>,
        Suzuki Poulose <Suzuki.Poulose@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        Steve Capper <Steve.Capper@....com>,
        "Kaly Xin (Arm Technology China)" <Kaly.Xin@....com>,
        "Justin He (Arm Technology China)" <Justin.He@....com>,
        nd <nd@....com>
Subject: RE: [PATCH v5 3/6] timekeeping: Add clocksource to
 system_time_snapshot

Hi tglx,

> -----Original Message-----
> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Wednesday, October 16, 2019 3:29 PM
> To: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Jianyong Wu (Arm Technology China) <Jianyong.Wu@....com>;
> netdev@...r.kernel.org; yangbo.lu@....com; john.stultz@...aro.org;
> sean.j.christopherson@...el.com; maz@...nel.org;
> richardcochran@...il.com; Mark Rutland <Mark.Rutland@....com>;
> will@...nel.org; Suzuki Poulose <Suzuki.Poulose@....com>; linux-
> kernel@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> kvmarm@...ts.cs.columbia.edu; kvm@...r.kernel.org; Steve Capper
> <Steve.Capper@....com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@....com>; Justin He (Arm Technology China)
> <Justin.He@....com>; nd <nd@....com>
> Subject: Re: [PATCH v5 3/6] timekeeping: Add clocksource to
> system_time_snapshot
> 
> On Wed, 16 Oct 2019, Paolo Bonzini wrote:
> > On 15/10/19 22:13, Thomas Gleixner wrote:
> > > On Tue, 15 Oct 2019, Paolo Bonzini wrote:
> > >> On 15/10/19 12:48, Jianyong Wu wrote:
> > >>>
> > >>>
> > >>
> > >> Reviewed-by: Paolo Bonzini <pbonzini@...hat.com>
> > >
> > > You're sure about having reviewed that in detail?
> >
> > I did review the patch; the void* ugliness is not in this one, and I
> > do have some other qualms on that one.
> >
> > > This changelog is telling absolutely nothing WHY anything outside of
> > > the timekeeping core code needs access to the current clocksource.
> > > Neither does it tell why it is safe to provide the pointer to random callers.
> >
> > Agreed on the changelog, but the pointer to a clocksource is already
> > part of the timekeeping external API via struct system_counterval_t.
> > get_device_system_crosststamp for example expects a clocksource
> > pointer but provides no way to get such a pointer.
> 
> That's a completely different beast, really.
> 
> The clocksource pointer is handed in by the caller and the core code validates
> if the clocksource is the same as the current system clocksource and not the
> other way round.
> 
> So there is no need for getting that pointer from the core code because the
> caller knows already which clocksource needs to be active to make.the whole
> cross device timestamp correlation work. And in that case it's the callers
> responsibility to ensure that the pointer is valid which is the case for the
> current use cases.
> 
I thinks there is something misunderstanding of my patch. See patch 4/6, the reason why I add clocksource is that I want to check if the current clocksouce is
arm_arch_counter in virt/kvm/arm/psci.c and nothing to do with get_device_system_crosststamp.

So I really need a mechanism to do that check.

Thanks
Jianyong

> From your other reply:
> 
> > Why add a global id?  ARM can add it to archdata similar to how x86
> > has vclock_mode.  But I still think the right thing to do is to
> > include the full system_counterval_t in the result of
> > ktime_get_snapshot.  (More in a second, feel free to reply to the other
> email only).
> 
> No, the clocksource pointer is not going to be exposed as there is no
> guarantee that it will be still around after the call returns.
> 
> It's not even guaranteed to be correct when the store happens in Wu's patch
> simply because the store is done outside of the seqcount protected region.

Yeah, all of the elements in system_time_snapshot should be captured in consistency. So
I think the consistency will be guaranteed if the store ops added in the seqcount region.

> 
> Vs. arch data: arch data is an opaque struct, so you'd need to store a pointer
> which has the same issue as the clocksource pointer itself.
> 
> If we want to convey information then it has to be in the generic part of
> struct clocksource.
> 
> In fact we could even simplify the existing get_device_system_crosststamp()
> use case by using the ID field.
> 
> Thanks,
> 
> 	tglx