Messages in this thread | | | Subject | Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure | From | Daniel Lezcano <> | Date | Wed, 30 Mar 2016 11:52:00 +0200 |
| |
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@arm.com> >>>> >>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>> Cc: Thomas Gleixner <tglx@linutronix.de> >>>>> Cc: Marc Zyngier <marc.zyngier@arm.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
| |