Re: [PATCH v13 06/18] x86/xen/time: initialize pv xen time in init_hypervisor_platform

[Boris Ostrovsky]

On 07/11/2018 08:04 PM, Pavel Tatashin wrote:
> In every hypervisor except for xen pv time ops are initialized in
> init_hypervisor_platform().
>
> Xen PV domains initialize time ops in x86_init.paging.pagetable_init(),
> by calling xen_setup_shared_info() which is a poor design, as time is
> needed prior to memory allocator.
>
> xen_setup_shared_info() is called from two places: during boot, and
> after suspend. Split the content of xen_setup_shared_info() into
> three places:
>
> 1. add the clock relavent data into new xen pv init_platform vector, and
>    set clock ops in there.
>
> 2. move xen_setup_vcpu_info_placement() to new xen_pv_guest_late_init()
>    call.
>
> 3. Re-initializing parts of shared info copy to xen_pv_post_suspend() to
>    be symmetric to xen_pv_pre_suspend
>
> Also, remove code duplications by calling xen_init_time_ops() from
> xen_hvm_init_time_ops().
>
> Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> ---
>  arch/x86/xen/enlighten_pv.c | 51 +++++++++++++++++--------------------
>  arch/x86/xen/mmu_pv.c       |  6 ++---
>  arch/x86/xen/suspend_pv.c   |  5 ++--
>  arch/x86/xen/time.c         | 14 +++-------
>  arch/x86/xen/xen-ops.h      |  6 ++---
>  5 files changed, 35 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 8d4e2e1ae60b..615ad0f16848 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -119,6 +119,27 @@ static void __init xen_banner(void)
>  	       version >> 16, version & 0xffff, extra.extraversion,
>  	       xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " (preserve-AD)" : "");
>  }
> +
> +static void __init xen_pv_init_platform(void)
> +{
> +	set_fixmap(FIX_PARAVIRT_BOOTMAP, xen_start_info->shared_info);
> +	HYPERVISOR_shared_info = (void *)fix_to_virt(FIX_PARAVIRT_BOOTMAP);
> +
> +	/* xen clock uses per-cpu vcpu_info, need to init it for boot cpu */
> +	xen_vcpu_info_reset(0);


I don't believe this is necessary, it has been done in
xen_start_kernel() for PV guests.


> +
> +	/* pvclock is in shared info area */
> +	xen_init_time_ops();
> +}
> +



>  }
>  
> -void __ref xen_init_time_ops(void)
> +void __init xen_init_time_ops(void)
>  {
>  	pv_time_ops = xen_time_ops;
>  
> @@ -542,17 +542,11 @@ void __init xen_hvm_init_time_ops(void)
>  		return;
>  
>  	if (!xen_feature(XENFEAT_hvm_safe_pvclock)) {
> -		printk(KERN_INFO "Xen doesn't support pvclock on HVM,"
> -				"disable pv timer\n");
> +		pr_info("Xen doesn't support pvclock on HVM, disable pv timer");
>  		return;
>  	}
> -
> -	pv_time_ops = xen_time_ops;
> +	xen_init_time_ops();


As we discussed elsewhere, now that HVM guests call this routine as well
we need to make sure that x86_init.timers.timer_init is not updated
there for HVM since those guests expect a "real" timer to be connected
to IO-APIC.

-boris

>  	x86_init.timers.setup_percpu_clockev = xen_time_init;
>  	x86_cpuinit.setup_percpu_clockev = xen_hvm_setup_cpu_clockevents;
> -
> -	x86_platform.calibrate_tsc = xen_tsc_khz;
> -	x86_platform.get_wallclock = xen_get_wallclock;
> -	x86_platform.set_wallclock = xen_set_wallclock;
>  }
>  #endif
>