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@xxxxxxxxxx>
>
> 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home