Re: [PATCH v2 2/3] x86/xen/time: setup vcpu 0 time info page

[Joao Martins <joao.m.martins@oracle.com>]

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