Message-ID: <56FBA1C0.7030108@linaro.org>
Date:	Wed, 30 Mar 2016 11:52:00 +0200
From:	Daniel Lezcano <daniel.lezcano@...aro.org>
To:	Marc Zyngier <marc.zyngier@....com>,
	Christoffer Dall <christoffer.dall@...aro.org>
Cc:	Julien Grall <julien.grall@....com>, kvmarm@...ts.cs.columbia.edu,
	fu.wei@...aro.org, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	wei@...hat.com, al.stone@...aro.org, gg@...mlogic.co.uk,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific
 information in a structure

On 03/30/2016 11:12 AM, Marc Zyngier wrote:
> On 30/03/16 10:06, Christoffer Dall wrote:
>> On Tue, Mar 29, 2016 at 06:32:15PM +0100, Marc Zyngier wrote:
>>> Daniel,
>>>
>>> On 29/03/16 18:13, Daniel Lezcano wrote:
>>>> On 03/24/2016 06:53 PM, Julien Grall wrote:
>>>>> Introduce a structure which are filled up by the arch timer driver and
>>>>> used by the virtual timer in KVM.
>>>>>
>>>>> The first member of this structure will be the timecounter. More members
>>>>> will be added later.
>>>>>
>>>>> A stub for the new helper isn't introduced because KVM requires the arch
>>>>> timer for both ARM64 and ARM32.
>>>>>
>>>>> The function arch_timer_get_timecounter is kept for the time being and
>>>>> will be dropped in a subsequent patch.
>>>>>
>>>>> Signed-off-by: Julien Grall <julien.grall@....com>
>>>>
>>>>> Cc: Daniel Lezcano <daniel.lezcano@...aro.org>
>>>>> Cc: Thomas Gleixner <tglx@...utronix.de>
>>>>> Cc: Marc Zyngier <marc.zyngier@....com>
>>>>>
>>>>>       Changes in v3:
>>>>>           - Rename the patch
>>>>>           - Move the KVM changes and removal of arch_timer_get_timecounter
>>>>>           in separate patches.
>>>>> ---
>>>>>    drivers/clocksource/arm_arch_timer.c | 12 +++++++++---
>>>>>    include/clocksource/arm_arch_timer.h |  5 +++++
>>>>>    2 files changed, 14 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>>>>> index 5152b38..62bdfe7 100644
>>>>> --- a/drivers/clocksource/arm_arch_timer.c
>>>>> +++ b/drivers/clocksource/arm_arch_timer.c
>>>>> @@ -468,11 +468,16 @@ static struct cyclecounter cyclecounter = {
>>>>>    	.mask	= CLOCKSOURCE_MASK(56),
>>>>>    };
>>>>>
>>>>> -static struct timecounter timecounter;
>>>>> +static struct arch_timer_kvm_info arch_timer_kvm_info;
>>>>
>>>> This structure is statically defined in this subsystem but not used in
>>>> this file and a couple of a accessors is added to let another subsystem
>>>> to access it.
>>>>
>>>> That sounds there is something wrong here with the design of the current
>>>> code, virt/phys are mixed.
>>>>
>>>> It isn't possible to split the virt/phys timer code respectively in
>>>> virt/kvm/arm/arch_timer.c and drivers/clocksource/arm_arch_timer.c ?
>>>
>>> No, that'd be the wrong thing to do. The kernel uses *either* the virt
>>> or phys timer depending on how it has been booted, and both counters are
>>> in use.
>>>
>>> What KVM (or any other hypervisor) needs from the timer subsystem is:
>>> - an interrupt (so that it can force a guest exit when the timer fires),
>>> - a way to convert the values programmed into the HW into a timer event
>>> (which is what the time counter structure is for).
>>>
>>> That allows the hypervisor to *emulate* a timer for the guest, and
>>> that's what virt/kvm/arm/arch_timer.c is all about. We have a clear
>>> separation of what is driving the HW vs what is emulating it, and I'm
>>> quite eager to preserve that.
>>>
>>>> At least, 'struct arch_timer_kvm_info' should belong to
>>>> virt/kvm/arm/arch_timer.c.
>>>
>>> At the cost of mandating separate storage in the arm_arch_timer driver.
>>> I do not find that much nicer, but if you prefer that, fine by me.
>>>
>> If arch_timer_kvm_info is declared in virt/kvm/arm/arch_timer.c, then
>> do you want to make it globally accessible and populated by this code or
>> make it static to the KVM code and populate it with accessor functions?
>
> That'd be the latter, as I'm really not fond of global data.
>
>> To me the natural thing is that the arch timer driver maintains data
>> about the device it drives, and consumers of that data can ask the arch
>> timer driver for the details.
>
> That was my approach too, and that's the way the code proposed by Julien
> works. Daniel seems to have a different take on it though.

Well, I'm not against Julien's changes. The arm_arch_timer is complex 
and I don't have all the knowledge for the virt side. So I am just 
asking if everything is clearly separated which seems to be the case 
regarding your previous email.

What sounds strange to me is we have a static global function which is 
not used (except at init time) by the timer and then we add accessors 
function to retrieve it. I would have expected arch_timer to pass a 
structure at init time to the timer driver and this one fills it. Then 
the arch timer can directly use its own structure.

Anyway, perhaps I am splitting hairs. So up to you if you want to keep 
the current approach.

   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists