From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752446AbcEJPns (ORCPT ); Tue, 10 May 2016 11:43:48 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:33817 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751561AbcEJPnq (ORCPT ); Tue, 10 May 2016 11:43:46 -0400 Subject: Re: [PART1 V5 13/13] svm: Manage vcpu load/unload when enable AVIC To: Suravee Suthikulpanit , rkrcmar@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com References: <1462388992-25242-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1462388992-25242-14-git-send-email-Suravee.Suthikulpanit@amd.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com From: Paolo Bonzini Message-ID: <573201AB.2050006@redhat.com> Date: Tue, 10 May 2016 17:43:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <1462388992-25242-14-git-send-email-Suravee.Suthikulpanit@amd.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/05/2016 21:09, Suravee Suthikulpanit wrote: > From: Suravee Suthikulpanit > > When a vcpu is loaded/unloaded to a physical core, we need to update > host physical APIC ID information in the Physical APIC-ID table > accordingly. > > Also, when vCPU is blocking/un-blocking (due to halt instruction), > we need to make sure that the is-running bit in set accordingly in the > physical APIC-ID table. > > Signed-off-by: Suravee Suthikulpanit > Reviewed-by: Radim Krčmář > --- I think this is the only patch that needs a little more work, because there are a bunch of unused return values that really should be WARN_ON. In addition the load and put cases are different enough that they should be separate functions. Can you please test this? diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index f3dbf1d33a61..3168d6c8d24f 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -184,7 +184,7 @@ struct vcpu_svm { u32 ldr_reg; struct page *avic_backing_page; u64 *avic_physical_id_cache; - bool avic_is_blocking; + bool avic_is_running; }; #define AVIC_LOGICAL_ID_ENTRY_GUEST_PHYSICAL_ID_MASK (0xFF) @@ -1321,18 +1321,20 @@ free_avic: /** * This function is called during VCPU halt/unhalt. */ -static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run) +static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run) { u64 entry; int h_physical_id = __default_cpu_present_to_apicid(vcpu->cpu); struct vcpu_svm *svm = to_svm(vcpu); if (!kvm_vcpu_apicv_active(vcpu)) - return 0; + return; + + svm->avic_is_running = is_run; /* ID = 0xff (broadcast), ID > 0xff (reserved) */ - if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT) - return -EINVAL; + if (WARN_ON(h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)) + return; entry = READ_ONCE(*(svm->avic_physical_id_cache)); WARN_ON(is_run == !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK)); @@ -1341,36 +1343,45 @@ static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run) if (is_run) entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; WRITE_ONCE(*(svm->avic_physical_id_cache), entry); - - return 0; } -static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_load) +static void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu) { u64 entry; + /* ID = 0xff (broadcast), ID > 0xff (reserved) */ int h_physical_id = __default_cpu_present_to_apicid(cpu); struct vcpu_svm *svm = to_svm(vcpu); if (!kvm_vcpu_apicv_active(vcpu)) - return 0; + return; - /* ID = 0xff (broadcast), ID > 0xff (reserved) */ - if (h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT) - return -EINVAL; + if (WARN_ON(h_physical_id >= AVIC_MAX_PHYSICAL_ID_COUNT)) + return; entry = READ_ONCE(*(svm->avic_physical_id_cache)); - WARN_ON(is_load && (entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK)); + WARN_ON(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK); + + entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK; + entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK); entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; - if (is_load) { - entry &= ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK; - entry |= (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK); - if (!svm->avic_is_blocking) - entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; - } + if (svm->avic_is_running) + entry |= AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; + WRITE_ONCE(*(svm->avic_physical_id_cache), entry); +} - return 0; +static void avic_vcpu_put(struct kvm_vcpu *vcpu) +{ + u64 entry; + struct vcpu_svm *svm = to_svm(vcpu); + + if (!kvm_vcpu_apicv_active(vcpu)) + return; + + entry = READ_ONCE(*(svm->avic_physical_id_cache)); + entry &= ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; + WRITE_ONCE(*(svm->avic_physical_id_cache), entry); } static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) @@ -1436,10 +1447,10 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) goto free_page4; } - /* We initialize this flag to one to make sure that the is_running + /* We initialize this flag to true to make sure that the is_running * bit would be set the first time the vcpu is loaded. */ - svm->avic_is_blocking = false; + svm->avic_is_running = true; svm->nested.hsave = page_address(hsave_page); @@ -1518,7 +1529,7 @@ static void svm_vcpu_load(struct kvm_vcpu *vcpu, int cpu) if (static_cpu_has(X86_FEATURE_RDTSCP)) wrmsrl(MSR_TSC_AUX, svm->tsc_aux); - avic_vcpu_load(vcpu, cpu, true); + avic_vcpu_load(vcpu, cpu); } static void svm_vcpu_put(struct kvm_vcpu *vcpu) @@ -1526,7 +1537,7 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu) struct vcpu_svm *svm = to_svm(vcpu); int i; - avic_vcpu_load(vcpu, 0, false); + avic_vcpu_put(vcpu); ++vcpu->stat.host_state_reload; kvm_load_ldt(svm->host.ldt); @@ -1545,13 +1556,11 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu) static void svm_vcpu_blocking(struct kvm_vcpu *vcpu) { - to_svm(vcpu)->avic_is_blocking = true; avic_set_running(vcpu, false); } static void svm_vcpu_unblocking(struct kvm_vcpu *vcpu) { - to_svm(vcpu)->avic_is_blocking = false; avic_set_running(vcpu, true); } The two functions now have the same signature as their callers, svm_vcpu_load and svm_vcpu_put. Thanks, Paolo