linux-kernel - Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56FABC1F.2040500@arm.com>
Date:	Tue, 29 Mar 2016 18:32:15 +0100
From:	Marc Zyngier <marc.zyngier@....com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Julien Grall <julien.grall@....com>,
	kvmarm@...ts.cs.columbia.edu
Cc:	christoffer.dall@...aro.org, 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

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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...