From mboxrd@z Thu Jan  1 00:00:00 1970
From: Glauber Costa <glommer@parallels.com>
Subject: Re: [patch 10/16] x86: vdso: pvclock gettime support
Date: Fri, 2 Nov 2012 14:25:37 +0400
Message-ID: <50939FA1.7060809@parallels.com>
References: <20121031224656.417434866@redhat.com> <20121031224824.293748067@redhat.com> <50928A2A.70104@parallels.com> <20121101214243.GC19712@amt.cnet> <20121102003327.GA5141@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset="ISO-8859-1"
Content-Transfer-Encoding: 7bit
Cc: <kvm@vger.kernel.org>, <johnstul@us.ibm.com>, <jeremy@goop.org>,
	<zamsden@gmail.com>, <gleb@redhat.com>, <avi@redhat.com>,
	<pbonzini@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.parallels.com ([64.131.90.16]:35721 "EHLO mx2.parallels.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750927Ab2KBKZo (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 2 Nov 2012 06:25:44 -0400
In-Reply-To: <20121102003327.GA5141@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 11/02/2012 04:33 AM, Marcelo Tosatti wrote:
> On Thu, Nov 01, 2012 at 07:42:43PM -0200, Marcelo Tosatti wrote:
>> On Thu, Nov 01, 2012 at 06:41:46PM +0400, Glauber Costa wrote:
>>> On 11/01/2012 02:47 AM, Marcelo Tosatti wrote:
>>>> +#ifdef CONFIG_PARAVIRT_CLOCK
>>>> +
>>>> +static notrace const struct pvclock_vsyscall_time_info *get_pvti(int cpu)
>>>> +{
>>>> +	const aligned_pvti_t *pvti_base;
>>>> +	int idx = cpu / (PAGE_SIZE/PVTI_SIZE);
>>>> +	int offset = cpu % (PAGE_SIZE/PVTI_SIZE);
>>>> +
>>>> +	BUG_ON(PVCLOCK_FIXMAP_BEGIN + idx > PVCLOCK_FIXMAP_END);
>>>> +
>>>> +	pvti_base = (aligned_pvti_t *)__fix_to_virt(PVCLOCK_FIXMAP_BEGIN+idx);
>>>> +
>>>> +	return &pvti_base[offset].info;
>>>> +}
>>>> +
>>> Does BUG_ON() really do what you believe it does while in userspace
>>> context? We're not running with the kernel descriptors, so this will
>>> probably just kill the process without any explanation
>>
>> A coredump is generated which can be used to trace back to ud2a instruction
>> at the vdso code.
> 
> All comments have been addressed. Let me know if there is anything else
> on v3 that you'd like to see done differently.
> 
Mainly:

1) stick a "v3" string in the subject. You didn't do it for v2, and I
got confused at some points while looking for the correct patches

2) The changelogs are, in general, a bit poor. I've pointed to the ones
specifically that pops out, but I would appreciate if you would go over
them again, making them more informative.

3) Please make sure Peter is okay with the proposed notifier change.

4) Please consider allocating memory with __alloc_bootmem_node instead.