Re: [PATCH 6/7] xen: Enable PV clocksource for HVM

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 03/04/2010 01:36 AM, Sheng Yang wrote:
> And enable it by default in PV extended HVM guest.
>
> Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> ---
>   arch/x86/xen/hvmpv.c |   20 ++++++++++++++++++++
>   arch/x86/xen/time.c  |    4 +++-
>   2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/xen/hvmpv.c b/arch/x86/xen/hvmpv.c
> index 7a741ed..284e021 100644
> --- a/arch/x86/xen/hvmpv.c
> +++ b/arch/x86/xen/hvmpv.c
> @@ -43,6 +43,7 @@ static void __init xen_hvm_pv_banner(void)
>   		pv_info.name);
>   	printk(KERN_INFO "Xen version: %d.%d%s\n",
>   		version>>  16, version&  0xffff, extra.extraversion);
> +	printk(KERN_INFO "PV feature: PV clocksource enabled\n");
>   }
>
>   static int __init xen_para_available(void)
> @@ -117,6 +118,22 @@ static void __init init_shared_info(void)
>   	per_cpu(xen_vcpu, 0) =&HYPERVISOR_shared_info->vcpu_info[0];
>   }
>
> +extern struct clocksource xen_clocksource;
>    

No externs in .c files.  But aside from that, I'd prefer you export a 
function from xen/time.c to do the clocksource registration and call it 
from both places (even if its a one-liner).

> +
> +static void __init init_pv_clocksource(void)
> +{
> +	if (enable_hvm_pv(HVM_PV_CLOCK))
> +		BUG();
>    

BUG is a bit severe.  Will it really never, ever fail?  And if it does, 
the consequence is hardly serious; we just fall back to emulated devices.

> +
> +	pv_time_ops.sched_clock = xen_sched_clock;
> +
> +	x86_platform.calibrate_tsc = xen_tsc_khz;
> +	x86_platform.get_wallclock = xen_get_wallclock;
> +	x86_platform.set_wallclock = xen_set_wallclock;
> +
> +	clocksource_register(&xen_clocksource);
> +}
> +
>   void __init xen_guest_init(void)
>   {
>   	int r;
> @@ -130,4 +147,7 @@ void __init xen_guest_init(void)
>   		return;
>
>   	init_shared_info();
> +
> +	/* PV clocksource would be enabled by default */
> +	init_pv_clocksource();
>   }
> diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> index 0d3f07c..c80eacb 100644
> --- a/arch/x86/xen/time.c
> +++ b/arch/x86/xen/time.c
> @@ -13,6 +13,7 @@
>   #include<linux/clockchips.h>
>   #include<linux/kernel_stat.h>
>   #include<linux/math64.h>
> +#include<linux/module.h>
>
>   #include<asm/pvclock.h>
>   #include<asm/xen/hypervisor.h>
> @@ -243,7 +244,7 @@ int xen_set_wallclock(unsigned long now)
>   	return -1;
>   }
>
> -static struct clocksource xen_clocksource __read_mostly = {
> +struct clocksource xen_clocksource __read_mostly = {
>   	.name = "xen",
>   	.rating = 400,
>   	.read = xen_clocksource_get_cycles,
> @@ -252,6 +253,7 @@ static struct clocksource xen_clocksource __read_mostly = {
>   	.shift = XEN_SHIFT,
>   	.flags = CLOCK_SOURCE_IS_CONTINUOUS,
>   };
> +EXPORT_SYMBOL_GPL(xen_clocksource);
>
>   /*
>      Xen clockevent implementation
>