From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754449AbdIZJ6V (ORCPT ); Tue, 26 Sep 2017 05:58:21 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:24824 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751778AbdIZJ6T (ORCPT ); Tue, 26 Sep 2017 05:58:19 -0400 Subject: Re: [PATCH v2 2/3] x86/xen/time: setup vcpu 0 time info page To: Juergen Gross References: <20170922162530.10165-1-joao.m.martins@oracle.com> <20170922162530.10165-3-joao.m.martins@oracle.com> <3903b031-789d-2681-0d76-3e5164474641@suse.com> Cc: linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Boris Ostrovsky , Andy Lutomirski From: Joao Martins Message-ID: <81c7c004-96af-f7cd-8275-7b110861bbb7@oracle.com> Date: Tue, 26 Sep 2017 10:57:06 +0100 MIME-Version: 1.0 In-Reply-To: <3903b031-789d-2681-0d76-3e5164474641@suse.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/26/2017 10:32 AM, Juergen Gross wrote: > On 22/09/17 18:25, Joao Martins wrote: [snip] >> +static void xen_setup_vsyscall_time_info(void) >> +{ >> + struct vcpu_register_time_memory_area t; >> + struct pvclock_vsyscall_time_info *ti; >> + struct pvclock_vcpu_time_info *pvti; >> + int ret; >> + >> + pvti = &__this_cpu_read(xen_vcpu)->time; >> + >> + /* >> + * We check ahead on the primary time info if this >> + * bit is supported hence speeding up Xen clocksource. >> + */ >> + if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) >> + return; >> + >> + pvclock_set_flags(PVCLOCK_TSC_STABLE_BIT); >> + >> + ti = (struct pvclock_vsyscall_time_info *) get_zeroed_page(GFP_KERNEL); > > Coding style: omit the blank after the cast. > OK. >> + if (!ti) >> + return; >> + >> + t.addr.v = &ti->pvti; >> + >> + ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, 0, &t); >> + if (ret) { >> + pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret); >> + free_page((unsigned long) ti); > > Coding style again, once more below. > OK. >> + return; >> + } >> + >> + /* >> + * If the check above succedded this one should too since it's the >> + * same data on both primary and secondary time infos just different >> + * memory regions. But we still check it in case hypervisor is buggy. >> + */ >> + pvti = &ti->pvti; >> + if (!(pvti->flags & PVCLOCK_TSC_STABLE_BIT)) { >> + t.addr.v = NULL; >> + ret = HYPERVISOR_vcpu_op(VCPUOP_register_vcpu_time_memory_area, >> + 0, &t); >> + if (!ret) >> + free_page((unsigned long) ti); >> + >> + pr_notice("xen: VCLOCK_PVCLOCK not supported (err %d)\n", ret); > > Mind making the message more descriptive? E.g. instead of reporting > "(err 0)" just telling "(tsc unstable)"? > Got it. >> + return; >> + } >> + >> + xen_clock = ti; >> + pvclock_set_pvti_cpu0_va(xen_clock); >> + >> + xen_clocksource.archdata.vclock_mode = VCLOCK_PVCLOCK; >> +} >> + >> static void __init xen_time_init(void) >> { >> int cpu = smp_processor_id(); >> @@ -396,6 +495,7 @@ static void __init xen_time_init(void) >> setup_force_cpu_cap(X86_FEATURE_TSC); >> >> xen_setup_runstate_info(cpu); >> + xen_setup_vsyscall_time_info(); >> xen_setup_timer(cpu); >> xen_setup_cpu_clockevents(); >> >> diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h >> index c8a6d224f7ed..f96dbedb33d4 100644 >> --- a/arch/x86/xen/xen-ops.h >> +++ b/arch/x86/xen/xen-ops.h >> @@ -69,6 +69,8 @@ void xen_setup_runstate_info(int cpu); >> void xen_teardown_timer(int cpu); >> u64 xen_clocksource_read(void); >> void xen_setup_cpu_clockevents(void); >> +void xen_save_time_memory_area(void); >> +void xen_restore_time_memory_area(void); >> void __init xen_init_time_ops(void); >> void __init xen_hvm_init_time_ops(void); >> >> diff --git a/include/xen/interface/vcpu.h b/include/xen/interface/vcpu.h >> index 98188c87f5c1..8da788c5bd4f 100644 >> --- a/include/xen/interface/vcpu.h >> +++ b/include/xen/interface/vcpu.h >> @@ -178,4 +178,32 @@ DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_vcpu_info); >> >> /* Send an NMI to the specified VCPU. @extra_arg == NULL. */ >> #define VCPUOP_send_nmi 11 >> + >> +/* >> + * Register a memory location to get a secondary copy of the vcpu time >> + * parameters. The master copy still exists as part of the vcpu shared >> + * memory area, and this secondary copy is updated whenever the master copy >> + * is updated (and using the same versioning scheme for synchronisation). >> + * >> + * The intent is that this copy may be mapped (RO) into userspace so >> + * that usermode can compute system time using the time info and the >> + * tsc. Usermode will see an array of vcpu_time_info structures, one >> + * for each vcpu, and choose the right one by an existing mechanism >> + * which allows it to get the current vcpu number (such as via a >> + * segment limit). It can then apply the normal algorithm to compute >> + * system time from the tsc. >> + * >> + * @extra_arg == pointer to vcpu_register_time_info_memory_area structure. >> + */ >> +#define VCPUOP_register_vcpu_time_memory_area 13 >> +DEFINE_GUEST_HANDLE_STRUCT(vcpu_time_info_t); >> +struct vcpu_register_time_memory_area { >> + union { >> + GUEST_HANDLE(vcpu_time_info_t) h; >> + struct pvclock_vcpu_time_info *v; >> + uint64_t p; >> + } addr; >> +}; >> +DEFINE_GUEST_HANDLE_STRUCT(vcpu_register_time_memory_area_t); >> + > > Instead of adding only the operation you need, maybe you could sync just > the complete header from Xen? Yeap - I will update it. I suppose this means only adding VCPUOP_get_physid - the rest seems to be up-to-date AFAICT. Joao