From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Thu, 24 Dec 2015 12:37:10 +0100 (CET) Received: from mx2.parallels.com ([199.115.105.18]:53288 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27006604AbbLXLhIuscus (ORCPT ); Thu, 24 Dec 2015 12:37:08 +0100 Received: from [199.115.105.250] (helo=mail.odin.com) by mx2.parallels.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) (Exim 4.86) (envelope-from ) id 1aC4Cf-0003P7-8D; Thu, 24 Dec 2015 03:37:05 -0800 Received: from [10.30.16.30] (10.30.16.30) by US-EXCH2.sw.swsoft.com (10.255.249.46) with Microsoft SMTP Server (TLS) id 15.0.1130.7; Thu, 24 Dec 2015 03:36:55 -0800 Reply-To: Subject: Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap References: <1450949426-25545-1-git-send-email-asmetanin@virtuozzo.com> <20151224111450.GA19296@rkaganb.sw.ru> To: Roman Kagan , , Paolo Bonzini , Gleb Natapov , James Hogan , Paul Burton , Ralf Baechle , Alexander Graf , Christian Borntraeger , Cornelia Huck , , , , "Denis V. Lunev" , From: Andrey Smetanin Message-ID: <567BD8D0.4070004@virtuozzo.com> Date: Thu, 24 Dec 2015 14:36:48 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: <20151224111450.GA19296@rkaganb.sw.ru> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: US-EXCH.sw.swsoft.com (10.255.249.47) To US-EXCH2.sw.swsoft.com (10.255.249.46) Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 50747 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: asmetanin@virtuozzo.com Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips On 12/24/2015 02:14 PM, Roman Kagan wrote: > On Thu, Dec 24, 2015 at 12:30:26PM +0300, Andrey Smetanin wrote: >> Currently on x86 arch we has already 32 requests defined >> so the newer request bits can't be placed inside >> vcpu->requests(unsigned long) inside x86 32 bit system. >> But we are going to add a new request in x86 arch >> for Hyper-V tsc page support. >> >> To solve the problem the patch replaces vcpu->requests by >> bitmap with 64 bit length and uses bitmap API. >> >> The patch consists of: >> * announce kvm_vcpu_has_requests() to check whether vcpu has >> requests >> * announce kvm_vcpu_requests() to get vcpu requests pointer > > I think that if abstracting out the implementation of the request > container is what you're after, you'd better not define this function; > accesses to the request map should be through your accessor functions. Good idea! I'll rework this patch and will send V2. > >> * announce kvm_clear_request() to clear particular vcpu request >> * replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu)) >> * replace clear_bit(req, vcpu->requests) by >> kvm_clear_request(req, vcpu) > > Apparently one accessor is missing: test the presence of a request > without clearing it from the bitmap (because kvm_check_request() clears > it). agree > >> diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h >> index 2e0e67e..4015b88 100644 >> --- a/arch/powerpc/kvm/trace.h >> +++ b/arch/powerpc/kvm/trace.h >> @@ -109,7 +109,7 @@ TRACE_EVENT(kvm_check_requests, >> >> TP_fast_assign( >> __entry->cpu_nr = vcpu->vcpu_id; >> - __entry->requests = vcpu->requests; >> + __entry->requests = (__u32)kvm_vcpu_requests(vcpu)[0]; >> ), > > This doesn't make sense, to expose only subset of the requests. BTW I > don't see this event in Linus tree, nor in linux-next, I found it here: arch/powerpc/kvm/powerpc.c:104: trace_kvm_check_requests(vcpu); >so I'm not quite > sure why it's formed this way; I guess the interesting part is the > request number and the return value (i.e. whether it's set), not the > whole bitmap. No, the code actually dumps part of first 32 bit of bitmap: TP_printk("vcpu=%x requests=%x", __entry->cpu_nr, __entry->requests) So, for this corner case we can make __entry->requests as unsigned long variable and make the API helper to get first unsigned long from vcpu->requests bitmap. Next print part of bitmap this way: TP_printk("vcpu=0x%x requests=0x%lx", __entry->cpu_nr, __entry->requests) (as unsigned long). POWERPC guys what do you think about such approach? Or could we even drop trace_kvm_check_requests() at all. Does trace_kvm_check_requests() still useful for you? > >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -5957,7 +5957,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) >> if (intr_window_requested && vmx_interrupt_allowed(vcpu)) >> return handle_interrupt_window(&vmx->vcpu); >> >> - if (test_bit(KVM_REQ_EVENT, &vcpu->requests)) >> + if (test_bit(KVM_REQ_EVENT, kvm_vcpu_requests(vcpu))) > > Here you'd rather want that function to test for the request without > clearing it. agree, i'll provide such function. > >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -2117,7 +2117,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> >> if (ka->boot_vcpu_runs_old_kvmclock != tmp) >> set_bit(KVM_REQ_MASTERCLOCK_UPDATE, >> - &vcpu->requests); >> + kvm_vcpu_requests(vcpu)); > > This should have been kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); > agree >> @@ -8048,7 +8048,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) >> if (atomic_read(&vcpu->arch.nmi_queued)) >> return true; >> >> - if (test_bit(KVM_REQ_SMI, &vcpu->requests)) >> + if (test_bit(KVM_REQ_SMI, kvm_vcpu_requests(vcpu))) > > Again the test-only accessor is due here. agree > >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -146,6 +146,8 @@ static inline bool is_error_page(struct page *page) >> #define KVM_REQ_HV_EXIT 30 >> #define KVM_REQ_HV_STIMER 31 >> >> +#define KVM_REQ_MAX 64 >> + >> #define KVM_USERSPACE_IRQ_SOURCE_ID 0 >> #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 >> >> @@ -233,7 +235,7 @@ struct kvm_vcpu { >> int vcpu_id; >> int srcu_idx; >> int mode; >> - unsigned long requests; >> + DECLARE_BITMAP(requests, KVM_REQ_MAX); >> unsigned long guest_debug; >> >> int pre_pcpu; >> @@ -286,6 +288,16 @@ struct kvm_vcpu { >> struct kvm_vcpu_arch arch; >> }; >> >> +static inline bool kvm_vcpu_has_requests(struct kvm_vcpu *vcpu) >> +{ >> + return !bitmap_empty(vcpu->requests, KVM_REQ_MAX); >> +} >> + >> +static inline ulong *kvm_vcpu_requests(struct kvm_vcpu *vcpu) >> +{ >> + return (ulong *)vcpu->requests; >> +} > > As I wrote above, I believe this function doesn't belong in the API. agree, i'll drop it. > >> + >> static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu) >> { >> return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE); >> @@ -1002,7 +1014,7 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa) >> >> static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu) >> { >> - set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests); >> + set_bit(KVM_REQ_MIGRATE_TIMER, kvm_vcpu_requests(vcpu)); > > kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu); agree > >> @@ -1118,19 +1130,24 @@ static inline bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) { return true; } >> >> static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu) >> { >> - set_bit(req, &vcpu->requests); >> + set_bit(req, kvm_vcpu_requests(vcpu)); >> } >> >> static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu) >> { >> - if (test_bit(req, &vcpu->requests)) { >> - clear_bit(req, &vcpu->requests); >> + if (test_bit(req, kvm_vcpu_requests(vcpu))) { >> + clear_bit(req, kvm_vcpu_requests(vcpu)); >> return true; >> } else { >> return false; >> } > > I wonder why this doesn't use test_and_clear_bit instead. agree, i'll replace it by test_and_clear_bit > > Roman. > From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx2.parallels.com ([199.115.105.18]:53288 "EHLO mx2.parallels.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S27006604AbbLXLhIuscus (ORCPT ); Thu, 24 Dec 2015 12:37:08 +0100 Reply-To: Subject: Re: [PATCH v1] kvm: Make vcpu->requests as 64 bit bitmap References: <1450949426-25545-1-git-send-email-asmetanin@virtuozzo.com> <20151224111450.GA19296@rkaganb.sw.ru> From: Andrey Smetanin Message-ID: <567BD8D0.4070004@virtuozzo.com> Date: Thu, 24 Dec 2015 14:36:48 +0300 MIME-Version: 1.0 In-Reply-To: <20151224111450.GA19296@rkaganb.sw.ru> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: To: Roman Kagan , kvm@vger.kernel.org, Paolo Bonzini , Gleb Natapov , James Hogan , Paul Burton , Ralf Baechle , Alexander Graf , Christian Borntraeger , Cornelia Huck , linux-mips@linux-mips.org, kvm-ppc@vger.kernel.org, linux-s390@vger.kernel.org, "Denis V. Lunev" , qemu-devel@nongnu.org Message-ID: <20151224113648.NGfSvJ1BJm6XTWDd5-h8QCfHc5mTXsXkISm-TNjoHeg@z> On 12/24/2015 02:14 PM, Roman Kagan wrote: > On Thu, Dec 24, 2015 at 12:30:26PM +0300, Andrey Smetanin wrote: >> Currently on x86 arch we has already 32 requests defined >> so the newer request bits can't be placed inside >> vcpu->requests(unsigned long) inside x86 32 bit system. >> But we are going to add a new request in x86 arch >> for Hyper-V tsc page support. >> >> To solve the problem the patch replaces vcpu->requests by >> bitmap with 64 bit length and uses bitmap API. >> >> The patch consists of: >> * announce kvm_vcpu_has_requests() to check whether vcpu has >> requests >> * announce kvm_vcpu_requests() to get vcpu requests pointer > > I think that if abstracting out the implementation of the request > container is what you're after, you'd better not define this function; > accesses to the request map should be through your accessor functions. Good idea! I'll rework this patch and will send V2. > >> * announce kvm_clear_request() to clear particular vcpu request >> * replace if (vcpu->requests) by if (kvm_vcpu_has_requests(vcpu)) >> * replace clear_bit(req, vcpu->requests) by >> kvm_clear_request(req, vcpu) > > Apparently one accessor is missing: test the presence of a request > without clearing it from the bitmap (because kvm_check_request() clears > it). agree > >> diff --git a/arch/powerpc/kvm/trace.h b/arch/powerpc/kvm/trace.h >> index 2e0e67e..4015b88 100644 >> --- a/arch/powerpc/kvm/trace.h >> +++ b/arch/powerpc/kvm/trace.h >> @@ -109,7 +109,7 @@ TRACE_EVENT(kvm_check_requests, >> >> TP_fast_assign( >> __entry->cpu_nr = vcpu->vcpu_id; >> - __entry->requests = vcpu->requests; >> + __entry->requests = (__u32)kvm_vcpu_requests(vcpu)[0]; >> ), > > This doesn't make sense, to expose only subset of the requests. BTW I > don't see this event in Linus tree, nor in linux-next, I found it here: arch/powerpc/kvm/powerpc.c:104: trace_kvm_check_requests(vcpu); >so I'm not quite > sure why it's formed this way; I guess the interesting part is the > request number and the return value (i.e. whether it's set), not the > whole bitmap. No, the code actually dumps part of first 32 bit of bitmap: TP_printk("vcpu=%x requests=%x", __entry->cpu_nr, __entry->requests) So, for this corner case we can make __entry->requests as unsigned long variable and make the API helper to get first unsigned long from vcpu->requests bitmap. Next print part of bitmap this way: TP_printk("vcpu=0x%x requests=0x%lx", __entry->cpu_nr, __entry->requests) (as unsigned long). POWERPC guys what do you think about such approach? Or could we even drop trace_kvm_check_requests() at all. Does trace_kvm_check_requests() still useful for you? > >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -5957,7 +5957,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) >> if (intr_window_requested && vmx_interrupt_allowed(vcpu)) >> return handle_interrupt_window(&vmx->vcpu); >> >> - if (test_bit(KVM_REQ_EVENT, &vcpu->requests)) >> + if (test_bit(KVM_REQ_EVENT, kvm_vcpu_requests(vcpu))) > > Here you'd rather want that function to test for the request without > clearing it. agree, i'll provide such function. > >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -2117,7 +2117,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >> >> if (ka->boot_vcpu_runs_old_kvmclock != tmp) >> set_bit(KVM_REQ_MASTERCLOCK_UPDATE, >> - &vcpu->requests); >> + kvm_vcpu_requests(vcpu)); > > This should have been kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu); > agree >> @@ -8048,7 +8048,7 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu) >> if (atomic_read(&vcpu->arch.nmi_queued)) >> return true; >> >> - if (test_bit(KVM_REQ_SMI, &vcpu->requests)) >> + if (test_bit(KVM_REQ_SMI, kvm_vcpu_requests(vcpu))) > > Again the test-only accessor is due here. agree > >> --- a/include/linux/kvm_host.h >> +++ b/include/linux/kvm_host.h >> @@ -146,6 +146,8 @@ static inline bool is_error_page(struct page *page) >> #define KVM_REQ_HV_EXIT 30 >> #define KVM_REQ_HV_STIMER 31 >> >> +#define KVM_REQ_MAX 64 >> + >> #define KVM_USERSPACE_IRQ_SOURCE_ID 0 >> #define KVM_IRQFD_RESAMPLE_IRQ_SOURCE_ID 1 >> >> @@ -233,7 +235,7 @@ struct kvm_vcpu { >> int vcpu_id; >> int srcu_idx; >> int mode; >> - unsigned long requests; >> + DECLARE_BITMAP(requests, KVM_REQ_MAX); >> unsigned long guest_debug; >> >> int pre_pcpu; >> @@ -286,6 +288,16 @@ struct kvm_vcpu { >> struct kvm_vcpu_arch arch; >> }; >> >> +static inline bool kvm_vcpu_has_requests(struct kvm_vcpu *vcpu) >> +{ >> + return !bitmap_empty(vcpu->requests, KVM_REQ_MAX); >> +} >> + >> +static inline ulong *kvm_vcpu_requests(struct kvm_vcpu *vcpu) >> +{ >> + return (ulong *)vcpu->requests; >> +} > > As I wrote above, I believe this function doesn't belong in the API. agree, i'll drop it. > >> + >> static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu) >> { >> return cmpxchg(&vcpu->mode, IN_GUEST_MODE, EXITING_GUEST_MODE); >> @@ -1002,7 +1014,7 @@ static inline bool kvm_is_error_gpa(struct kvm *kvm, gpa_t gpa) >> >> static inline void kvm_migrate_timers(struct kvm_vcpu *vcpu) >> { >> - set_bit(KVM_REQ_MIGRATE_TIMER, &vcpu->requests); >> + set_bit(KVM_REQ_MIGRATE_TIMER, kvm_vcpu_requests(vcpu)); > > kvm_make_request(KVM_REQ_MIGRATE_TIMER, vcpu); agree > >> @@ -1118,19 +1130,24 @@ static inline bool kvm_vcpu_compatible(struct kvm_vcpu *vcpu) { return true; } >> >> static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu) >> { >> - set_bit(req, &vcpu->requests); >> + set_bit(req, kvm_vcpu_requests(vcpu)); >> } >> >> static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu) >> { >> - if (test_bit(req, &vcpu->requests)) { >> - clear_bit(req, &vcpu->requests); >> + if (test_bit(req, kvm_vcpu_requests(vcpu))) { >> + clear_bit(req, kvm_vcpu_requests(vcpu)); >> return true; >> } else { >> return false; >> } > > I wonder why this doesn't use test_and_clear_bit instead. agree, i'll replace it by test_and_clear_bit > > Roman. >