Re: [PATCH] x86: extend runstate area updates

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 11/21/09 02:57, Ian Campbell wrote:
> On Fri, 2009-11-20 at 18:09 +0000, Ian Campbell wrote:
>   
>>
>> I can't see where the guest runstate pointer is supposed to be either
>> restored or re-setup on resume. I tried adding a setup_runstate_info
>> to xen_timer_resume (to match the call in xen_timer_setup) but that
>> seems like it is already too late -- I still see the warnings trigger.
>> I'm not sure how this is possible since I thought we were in a
>> stop_machine section at this point.
>>     
> The xen_sched_clock calls are as a result of the various printks, e.g.
> in xen_vcpu_setup, in order to add the timestamp to the output.
> Therefore we need to ensure we reset the runstate info before any
> printks.
>   

Thanks for investigating this.  Does this mean there was a
longer-standing bug where the runstate was just completely invalid after
a save/restore?

One comment:
> --- 
>
> Subject: re-register runstate area earlier on resume.
>
> This is necessary to ensure the runstate area is available to
> xen_sched_clock before any calls to printk which will require it in
> order to provide a timestamp.
>
> I chose to pull the xen_setup_runstate_info out of xen_time_init into
> the caller in order to maintain parity with calling
> xen_setup_runstate_info separately from calling xen_time_resume.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index 0a5aa44..fed538a 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -100,7 +102,7 @@ bool xen_vcpu_stolen(int vcpu)
>  	return per_cpu(runstate, vcpu).state == RUNSTATE_runnable;
>  }
>  
> -static void setup_runstate_info(int cpu)
> +void xen_setup_runstate_info(int cpu)
>  {
>  	struct vcpu_register_runstate_memory_area area;
>  
> @@ -442,8 +456,6 @@ void xen_setup_timer(int cpu)
>  
>  	evt->cpumask = cpumask_of(cpu);
>  	evt->irq = irq;
> -
> -	setup_runstate_info(cpu);
>  }
>  
>  void xen_teardown_timer(int cpu)
> @@ -494,6 +507,7 @@ __init void xen_time_init(void)
>  
>  	setup_force_cpu_cap(X86_FEATURE_TSC);
>  
> +	xen_setup_runstate_info(cpu);
>  	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 ca6596b..07ee25c 100644
> --- a/arch/x86/xen/xen-ops.h
> +++ b/arch/x86/xen/xen-ops.h
> @@ -44,6 +44,7 @@ void __init xen_build_dynamic_phys_to_machine(void);
>  
>  void xen_init_irq_ops(void);
>  void xen_setup_timer(int cpu);
> +void xen_setup_runstate_info(int cpu);
>  void xen_teardown_timer(int cpu);
>  cycle_t xen_clocksource_read(void);
>  void xen_setup_cpu_clockevents(void);
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index f09e8c3..219fb3f 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -145,6 +158,8 @@ void xen_vcpu_restore(void)
>  			    HYPERVISOR_vcpu_op(VCPUOP_down, cpu, NULL))
>  				BUG();
>  
> +			xen_setup_runstate_info(cpu);
> +
>  			xen_vcpu_setup(cpu);
>   

This is only run when have_vcpu_info_placement is set.  I checked in a
followup patch to rearrange the loop so that it runs unconditionally,
but then only does xen_vcpu_setup() when have_vcpu_info_placement.

    J