From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Subject: Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure Date: Tue, 29 Mar 2016 19:13:54 +0200 Message-ID: <56FAB7D2.1000508@linaro.org> References: <1458842023-31853-1-git-send-email-julien.grall@arm.com> <1458842023-31853-2-git-send-email-julien.grall@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 9B9D1410EB for ; Tue, 29 Mar 2016 13:13:06 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qH6dTx7MVG-f for ; Tue, 29 Mar 2016 13:13:04 -0400 (EDT) Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) by mm01.cs.columbia.edu (Postfix) with ESMTPS id 816EC40C9D for ; Tue, 29 Mar 2016 13:13:04 -0400 (EDT) Received: by mail-wm0-f43.google.com with SMTP id p65so36640573wmp.0 for ; Tue, 29 Mar 2016 10:14:04 -0700 (PDT) In-Reply-To: <1458842023-31853-2-git-send-email-julien.grall@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: Julien Grall , kvmarm@lists.cs.columbia.edu Cc: al.stone@linaro.org, kvm@vger.kernel.org, marc.zyngier@arm.com, linux-kernel@vger.kernel.org, fu.wei@linaro.org, Thomas Gleixner , linux-arm-kernel@lists.infradead.org, gg@slimlogic.co.uk List-Id: kvmarm@lists.cs.columbia.edu T24gMDMvMjQvMjAxNiAwNjo1MyBQTSwgSnVsaWVuIEdyYWxsIHdyb3RlOgo+IEludHJvZHVjZSBh IHN0cnVjdHVyZSB3aGljaCBhcmUgZmlsbGVkIHVwIGJ5IHRoZSBhcmNoIHRpbWVyIGRyaXZlciBh bmQKPiB1c2VkIGJ5IHRoZSB2aXJ0dWFsIHRpbWVyIGluIEtWTS4KPgo+IFRoZSBmaXJzdCBtZW1i ZXIgb2YgdGhpcyBzdHJ1Y3R1cmUgd2lsbCBiZSB0aGUgdGltZWNvdW50ZXIuIE1vcmUgbWVtYmVy cwo+IHdpbGwgYmUgYWRkZWQgbGF0ZXIuCj4KPiBBIHN0dWIgZm9yIHRoZSBuZXcgaGVscGVyIGlz bid0IGludHJvZHVjZWQgYmVjYXVzZSBLVk0gcmVxdWlyZXMgdGhlIGFyY2gKPiB0aW1lciBmb3Ig Ym90aCBBUk02NCBhbmQgQVJNMzIuCj4KPiBUaGUgZnVuY3Rpb24gYXJjaF90aW1lcl9nZXRfdGlt ZWNvdW50ZXIgaXMga2VwdCBmb3IgdGhlIHRpbWUgYmVpbmcgYW5kCj4gd2lsbCBiZSBkcm9wcGVk IGluIGEgc3Vic2VxdWVudCBwYXRjaC4KPgo+IFNpZ25lZC1vZmYtYnk6IEp1bGllbiBHcmFsbCA8 anVsaWVuLmdyYWxsQGFybS5jb20+Cgo+IENjOiBEYW5pZWwgTGV6Y2FubyA8ZGFuaWVsLmxlemNh bm9AbGluYXJvLm9yZz4KPiBDYzogVGhvbWFzIEdsZWl4bmVyIDx0Z2x4QGxpbnV0cm9uaXguZGU+ Cj4gQ2M6IE1hcmMgWnluZ2llciA8bWFyYy56eW5naWVyQGFybS5jb20+Cj4KPiAgICAgIENoYW5n ZXMgaW4gdjM6Cj4gICAgICAgICAgLSBSZW5hbWUgdGhlIHBhdGNoCj4gICAgICAgICAgLSBNb3Zl IHRoZSBLVk0gY2hhbmdlcyBhbmQgcmVtb3ZhbCBvZiBhcmNoX3RpbWVyX2dldF90aW1lY291bnRl cgo+ICAgICAgICAgIGluIHNlcGFyYXRlIHBhdGNoZXMuCj4gLS0tCj4gICBkcml2ZXJzL2Nsb2Nr c291cmNlL2FybV9hcmNoX3RpbWVyLmMgfCAxMiArKysrKysrKystLS0KPiAgIGluY2x1ZGUvY2xv Y2tzb3VyY2UvYXJtX2FyY2hfdGltZXIuaCB8ICA1ICsrKysrCj4gICAyIGZpbGVzIGNoYW5nZWQs IDE0IGluc2VydGlvbnMoKyksIDMgZGVsZXRpb25zKC0pCj4KPiBkaWZmIC0tZ2l0IGEvZHJpdmVy cy9jbG9ja3NvdXJjZS9hcm1fYXJjaF90aW1lci5jIGIvZHJpdmVycy9jbG9ja3NvdXJjZS9hcm1f YXJjaF90aW1lci5jCj4gaW5kZXggNTE1MmIzOC4uNjJiZGZlNyAxMDA2NDQKPiAtLS0gYS9kcml2 ZXJzL2Nsb2Nrc291cmNlL2FybV9hcmNoX3RpbWVyLmMKPiArKysgYi9kcml2ZXJzL2Nsb2Nrc291 cmNlL2FybV9hcmNoX3RpbWVyLmMKPiBAQCAtNDY4LDExICs0NjgsMTYgQEAgc3RhdGljIHN0cnVj dCBjeWNsZWNvdW50ZXIgY3ljbGVjb3VudGVyID0gewo+ICAgCS5tYXNrCT0gQ0xPQ0tTT1VSQ0Vf TUFTSyg1NiksCj4gICB9Owo+Cj4gLXN0YXRpYyBzdHJ1Y3QgdGltZWNvdW50ZXIgdGltZWNvdW50 ZXI7Cj4gK3N0YXRpYyBzdHJ1Y3QgYXJjaF90aW1lcl9rdm1faW5mbyBhcmNoX3RpbWVyX2t2bV9p bmZvOwoKVGhpcyBzdHJ1Y3R1cmUgaXMgc3RhdGljYWxseSBkZWZpbmVkIGluIHRoaXMgc3Vic3lz dGVtIGJ1dCBub3QgdXNlZCBpbiAKdGhpcyBmaWxlIGFuZCBhIGNvdXBsZSBvZiBhIGFjY2Vzc29y cyBpcyBhZGRlZCB0byBsZXQgYW5vdGhlciBzdWJzeXN0ZW0gCnRvIGFjY2VzcyBpdC4KClRoYXQg c291bmRzIHRoZXJlIGlzIHNvbWV0aGluZyB3cm9uZyBoZXJlIHdpdGggdGhlIGRlc2lnbiBvZiB0 aGUgY3VycmVudCAKY29kZSwgdmlydC9waHlzIGFyZSBtaXhlZC4KCkl0IGlzbid0IHBvc3NpYmxl IHRvIHNwbGl0IHRoZSB2aXJ0L3BoeXMgdGltZXIgY29kZSByZXNwZWN0aXZlbHkgaW4gCnZpcnQv a3ZtL2FybS9hcmNoX3RpbWVyLmMgYW5kIGRyaXZlcnMvY2xvY2tzb3VyY2UvYXJtX2FyY2hfdGlt ZXIuYyA/CgpBdCBsZWFzdCwgJ3N0cnVjdCBhcmNoX3RpbWVyX2t2bV9pbmZvJyBzaG91bGQgYmVs b25nIHRvIAp2aXJ0L2t2bS9hcm0vYXJjaF90aW1lci5jLgoKPiArc3RydWN0IGFyY2hfdGltZXJf a3ZtX2luZm8gKmFyY2hfdGltZXJfZ2V0X2t2bV9pbmZvKHZvaWQpCj4gK3sKPiArCXJldHVybiAm YXJjaF90aW1lcl9rdm1faW5mbzsKPiArfQo+Cj4gICBzdHJ1Y3QgdGltZWNvdW50ZXIgKmFyY2hf dGltZXJfZ2V0X3RpbWVjb3VudGVyKHZvaWQpCj4gICB7Cj4gLQlyZXR1cm4gJnRpbWVjb3VudGVy Owo+ICsJcmV0dXJuICZhcmNoX3RpbWVyX2t2bV9pbmZvLnRpbWVjb3VudGVyOwo+ICAgfQo+Cj4g ICBzdGF0aWMgdm9pZCBfX2luaXQgYXJjaF9jb3VudGVyX3JlZ2lzdGVyKHVuc2lnbmVkIHR5cGUp Cj4gQEAgLTUwMCw3ICs1MDUsOCBAQCBzdGF0aWMgdm9pZCBfX2luaXQgYXJjaF9jb3VudGVyX3Jl Z2lzdGVyKHVuc2lnbmVkIHR5cGUpCj4gICAJY2xvY2tzb3VyY2VfcmVnaXN0ZXJfaHooJmNsb2Nr c291cmNlX2NvdW50ZXIsIGFyY2hfdGltZXJfcmF0ZSk7Cj4gICAJY3ljbGVjb3VudGVyLm11bHQg PSBjbG9ja3NvdXJjZV9jb3VudGVyLm11bHQ7Cj4gICAJY3ljbGVjb3VudGVyLnNoaWZ0ID0gY2xv Y2tzb3VyY2VfY291bnRlci5zaGlmdDsKPiAtCXRpbWVjb3VudGVyX2luaXQoJnRpbWVjb3VudGVy LCAmY3ljbGVjb3VudGVyLCBzdGFydF9jb3VudCk7Cj4gKwl0aW1lY291bnRlcl9pbml0KCZhcmNo X3RpbWVyX2t2bV9pbmZvLnRpbWVjb3VudGVyLAo+ICsJCQkgJmN5Y2xlY291bnRlciwgc3RhcnRf Y291bnQpOwo+Cj4gICAJLyogNTYgYml0cyBtaW5pbXVtLCBzbyB3ZSBhc3N1bWUgd29yc3QgY2Fz ZSByb2xsb3ZlciAqLwo+ICAgCXNjaGVkX2Nsb2NrX3JlZ2lzdGVyKGFyY2hfdGltZXJfcmVhZF9j b3VudGVyLCA1NiwgYXJjaF90aW1lcl9yYXRlKTsKPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9jbG9j a3NvdXJjZS9hcm1fYXJjaF90aW1lci5oIGIvaW5jbHVkZS9jbG9ja3NvdXJjZS9hcm1fYXJjaF90 aW1lci5oCj4gaW5kZXggMjVkMDkxNC4uOTEwMWVkNmIgMTAwNjQ0Cj4gLS0tIGEvaW5jbHVkZS9j bG9ja3NvdXJjZS9hcm1fYXJjaF90aW1lci5oCj4gKysrIGIvaW5jbHVkZS9jbG9ja3NvdXJjZS9h cm1fYXJjaF90aW1lci5oCj4gQEAgLTQ5LDExICs0OSwxNiBAQCBlbnVtIGFyY2hfdGltZXJfcmVn IHsKPgo+ICAgI2RlZmluZSBBUkNIX1RJTUVSX0VWVF9TVFJFQU1fRlJFUQkxMDAwMAkvKiAxMDB1 cyAqLwo+Cj4gK3N0cnVjdCBhcmNoX3RpbWVyX2t2bV9pbmZvIHsKPiArCXN0cnVjdCB0aW1lY291 bnRlciB0aW1lY291bnRlcjsKPiArfTsKPiArCj4gICAjaWZkZWYgQ09ORklHX0FSTV9BUkNIX1RJ TUVSCj4KPiAgIGV4dGVybiB1MzIgYXJjaF90aW1lcl9nZXRfcmF0ZSh2b2lkKTsKPiAgIGV4dGVy biB1NjQgKCphcmNoX3RpbWVyX3JlYWRfY291bnRlcikodm9pZCk7Cj4gICBleHRlcm4gc3RydWN0 IHRpbWVjb3VudGVyICphcmNoX3RpbWVyX2dldF90aW1lY291bnRlcih2b2lkKTsKPiArZXh0ZXJu IHN0cnVjdCBhcmNoX3RpbWVyX2t2bV9pbmZvICphcmNoX3RpbWVyX2dldF9rdm1faW5mbyh2b2lk KTsKPgo+ICAgI2Vsc2UKPgo+CgoKLS0gCiAgPGh0dHA6Ly93d3cubGluYXJvLm9yZy8+IExpbmFy by5vcmcg4pSCIE9wZW4gc291cmNlIHNvZnR3YXJlIGZvciBBUk0gU29DcwoKRm9sbG93IExpbmFy bzogIDxodHRwOi8vd3d3LmZhY2Vib29rLmNvbS9wYWdlcy9MaW5hcm8+IEZhY2Vib29rIHwKPGh0 dHA6Ly90d2l0dGVyLmNvbS8jIS9saW5hcm9vcmc+IFR3aXR0ZXIgfAo8aHR0cDovL3d3dy5saW5h cm8ub3JnL2xpbmFyby1ibG9nLz4gQmxvZwoKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX18Ka3ZtYXJtIG1haWxpbmcgbGlzdAprdm1hcm1AbGlzdHMuY3MuY29s dW1iaWEuZWR1Cmh0dHBzOi8vbGlzdHMuY3MuY29sdW1iaWEuZWR1L21haWxtYW4vbGlzdGluZm8v a3ZtYXJtCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel.lezcano@linaro.org (Daniel Lezcano) Date: Tue, 29 Mar 2016 19:13:54 +0200 Subject: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure In-Reply-To: <1458842023-31853-2-git-send-email-julien.grall@arm.com> References: <1458842023-31853-1-git-send-email-julien.grall@arm.com> <1458842023-31853-2-git-send-email-julien.grall@arm.com> Message-ID: <56FAB7D2.1000508@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > Cc: Daniel Lezcano > Cc: Thomas Gleixner > Cc: Marc Zyngier > > 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 ? At least, 'struct arch_timer_kvm_info' should belong to virt/kvm/arm/arch_timer.c. > +struct arch_timer_kvm_info *arch_timer_get_kvm_info(void) > +{ > + return &arch_timer_kvm_info; > +} > > struct timecounter *arch_timer_get_timecounter(void) > { > - return &timecounter; > + return &arch_timer_kvm_info.timecounter; > } > > static void __init arch_counter_register(unsigned type) > @@ -500,7 +505,8 @@ static void __init arch_counter_register(unsigned type) > clocksource_register_hz(&clocksource_counter, arch_timer_rate); > cyclecounter.mult = clocksource_counter.mult; > cyclecounter.shift = clocksource_counter.shift; > - timecounter_init(&timecounter, &cyclecounter, start_count); > + timecounter_init(&arch_timer_kvm_info.timecounter, > + &cyclecounter, start_count); > > /* 56 bits minimum, so we assume worst case rollover */ > sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate); > diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h > index 25d0914..9101ed6b 100644 > --- a/include/clocksource/arm_arch_timer.h > +++ b/include/clocksource/arm_arch_timer.h > @@ -49,11 +49,16 @@ enum arch_timer_reg { > > #define ARCH_TIMER_EVT_STREAM_FREQ 10000 /* 100us */ > > +struct arch_timer_kvm_info { > + struct timecounter timecounter; > +}; > + > #ifdef CONFIG_ARM_ARCH_TIMER > > extern u32 arch_timer_get_rate(void); > extern u64 (*arch_timer_read_counter)(void); > extern struct timecounter *arch_timer_get_timecounter(void); > +extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void); > > #else > > -- Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757847AbcC2ROQ (ORCPT ); Tue, 29 Mar 2016 13:14:16 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:33957 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757825AbcC2ROO (ORCPT ); Tue, 29 Mar 2016 13:14:14 -0400 Subject: Re: [PATCH v4 1/9] clocksource: arm_arch_timer: Gather KVM specific information in a structure To: Julien Grall , kvmarm@lists.cs.columbia.edu References: <1458842023-31853-1-git-send-email-julien.grall@arm.com> <1458842023-31853-2-git-send-email-julien.grall@arm.com> Cc: christoffer.dall@linaro.org, marc.zyngier@arm.com, fu.wei@linaro.org, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, wei@redhat.com, al.stone@linaro.org, gg@slimlogic.co.uk, Thomas Gleixner From: Daniel Lezcano Message-ID: <56FAB7D2.1000508@linaro.org> Date: Tue, 29 Mar 2016 19:13:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <1458842023-31853-2-git-send-email-julien.grall@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Daniel Lezcano > Cc: Thomas Gleixner > Cc: Marc Zyngier > > 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 ? At least, 'struct arch_timer_kvm_info' should belong to virt/kvm/arm/arch_timer.c. > +struct arch_timer_kvm_info *arch_timer_get_kvm_info(void) > +{ > + return &arch_timer_kvm_info; > +} > > struct timecounter *arch_timer_get_timecounter(void) > { > - return &timecounter; > + return &arch_timer_kvm_info.timecounter; > } > > static void __init arch_counter_register(unsigned type) > @@ -500,7 +505,8 @@ static void __init arch_counter_register(unsigned type) > clocksource_register_hz(&clocksource_counter, arch_timer_rate); > cyclecounter.mult = clocksource_counter.mult; > cyclecounter.shift = clocksource_counter.shift; > - timecounter_init(&timecounter, &cyclecounter, start_count); > + timecounter_init(&arch_timer_kvm_info.timecounter, > + &cyclecounter, start_count); > > /* 56 bits minimum, so we assume worst case rollover */ > sched_clock_register(arch_timer_read_counter, 56, arch_timer_rate); > diff --git a/include/clocksource/arm_arch_timer.h b/include/clocksource/arm_arch_timer.h > index 25d0914..9101ed6b 100644 > --- a/include/clocksource/arm_arch_timer.h > +++ b/include/clocksource/arm_arch_timer.h > @@ -49,11 +49,16 @@ enum arch_timer_reg { > > #define ARCH_TIMER_EVT_STREAM_FREQ 10000 /* 100us */ > > +struct arch_timer_kvm_info { > + struct timecounter timecounter; > +}; > + > #ifdef CONFIG_ARM_ARCH_TIMER > > extern u32 arch_timer_get_rate(void); > extern u64 (*arch_timer_read_counter)(void); > extern struct timecounter *arch_timer_get_timecounter(void); > +extern struct arch_timer_kvm_info *arch_timer_get_kvm_info(void); > > #else > > -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog