From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Subject: Re: [PATCH RFC 1/3] x86/pvclock: add setter for pvclock_pvti_cpu0_va Date: Tue, 29 Dec 2015 05:03:09 -0800 Message-ID: References: <1451339557-24473-1-git-send-email-joao.m.martins@oracle.com> <1451339557-24473-2-git-send-email-joao.m.martins@oracle.com> <568281AC.9010900@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <568281AC.9010900@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Joao Martins Cc: Marcelo Tosatti , kvm list , Gleb Natapov , X86 ML , "linux-kernel@vger.kernel.org" , xen-devel , Ingo Molnar , Andy Lutomirski , "H. Peter Anvin" , Paolo Bonzini , Thomas Gleixner , Borislav Petkov List-Id: xen-devel@lists.xenproject.org On Tue, Dec 29, 2015 at 4:50 AM, Joao Martins wrote: > On 12/28/2015 11:45 PM, Andy Lutomirski wrote: >> On Mon, Dec 28, 2015 at 1:52 PM, Joao Martins wrote: >>> Right now there is only a pvclock_pvti_cpu0_va() which is defined on >>> kvmclock since: >>> >>> commit dac16fba6fc5 >>> ("x86/vdso: Get pvclock data from the vvar VMA instead of the fixmap") >>> >>> The only user of this interface so far is kvm. This commit adds a setter >>> function for the pvti page and moves pvclock_pvti_cpu0_va to pvclock, which >>> is a more generic place to have it; and would allow other PV clocksources >>> to use it, such as Xen. >>> >> >>> + >>> +void pvclock_set_pvti_cpu0_va(struct pvclock_vsyscall_time_info *pvti) >>> +{ >>> + pvti_cpu0_va = pvti; >>> +} >> >> IMO this either wants to be __init or wants a >> WARN_ON(vclock_was_used(VCLOCK_PVCLOCK)). The latter hasn't landed in >> -tip yet, but I think it'll land next week unless the merge window >> opens early. > OK, I will add those two once it lands in -tip. > > I had a silly mistake in this patch as I bindly ommited the parameter name to > keep checkpatch happy, but didn't compile check when built without PARAVIRT. > Apologies for that and will fix that also on the next version. > >> >> It may pay to actually separate out the kvm-clock clocksource and >> rename it rather than partially duplicating it, assuming the result >> wouldn't be messy. >> > Not sure if I follow but I moved out pvclock_pvti_cpu0_va from kvm-clock or do > you mean to separate out kvm-clock in it's enterity, or something else within > kvm-clock is that is common to both (such as kvm_setup_vsyscall_timeinfo) ? I meant literally using the same clocksource. I don't know whether the Xen and KVM variants are similar enough for that to make sense. > >> Can you CC me on the rest of the series for new versions? >> > Sure! Thanks for the prompt reply. > >> BTW, since this seems to require hypervisor changes to be useful, it >> might make sense to rethink the interface a bit. Are you actually >> planning to support per-cpu pvti for this in any useful way? If not, >> I think that this would work a whole lot better and be considerably >> less code if you had a single global pvti that lived in >> hypervisor-allocated memory instead of an array that lives in guest >> memory. I'd be happy to discuss next week in more detail (currently >> on vacation). > Initially I had this series using per-cpu pvti's based on Linux 4.4 but since > that was removed in favor of vdso using solely cpu0 pvti, then I ended up just > registering the cpu 0 page. I don't intend to add per-cpu pvti's since it would > only be used for this case: (unless the reviewers think it should be done) > meaning I would register pvti's for the other CPUs without having them used. > Having a global pvti as you suggest it would get a lot simpler for the guest, > but I guess this would only work assuming PVCLOCK_TSC_STABLE_BIT is there? > Looking forward to discuss it next week. Sounds good. --Andy