From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PART1 V5 13/13] svm: Manage vcpu load/unload when enable AVIC Date: Wed, 1 Jun 2016 13:18:10 -0500 Message-ID: <574F26E2.8020803@amd.com> References: <1462388992-25242-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1462388992-25242-14-git-send-email-Suravee.Suthikulpanit@amd.com> <573201AB.2050006@redhat.com> <573B0A08.7040604@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , To: Paolo Bonzini , , , , , Return-path: In-Reply-To: <573B0A08.7040604@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Hi Paolo/Radim, I was a bit behind on catching up with the AVIC stuff the past couple=20 weeks. Thank you for the help cleaning up on this patch and pulling thi= s=20 in to your tree. I can see now that it has been pulled into the 4.7-rc1= =2E=20 I really appreciate your help. Please see a minor comment below. On 5/17/16 07:09, Paolo Bonzini wrote: > > > On 10/05/2016 17:43, Paolo Bonzini wrote: >> >> >> 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 updat= e >>> 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=C4=8Dm=C3=A1=C5=99 >>> --- >> >> I think this is the only patch that needs a little more work, becaus= e >> there are a bunch of unused return values that really should be >> WARN_ON. In addition the load and put cases are different enough th= at >> 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 =3D __default_cpu_present_to_apicid(vcpu->cpu); >> struct vcpu_svm *svm =3D to_svm(vcpu); >> >> if (!kvm_vcpu_apicv_active(vcpu)) >> - return 0; >> + return; >> + >> + svm->avic_is_running =3D is_run; Shouldn't we do this below ---> >> >> /* ID =3D 0xff (broadcast), ID > 0xff (reserved) */ >> - if (h_physical_id >=3D AVIC_MAX_PHYSICAL_ID_COUNT) >> - return -EINVAL; >> + if (WARN_ON(h_physical_id >=3D AVIC_MAX_PHYSICAL_ID_COUNT)) >> + return; <--- HERE >> >> entry =3D READ_ONCE(*(svm->avic_physical_id_cache)); >> WARN_ON(is_run =3D=3D !!(entry & AVIC_PHYSICAL_ID_ENTRY_IS_RUNNIN= G_MASK)); >> @@ -1341,36 +1343,45 @@ static int avic_set_running(struct kvm_vcpu = *vcpu, bool is_run) >> if (is_run) >> entry |=3D AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; >> WRITE_ONCE(*(svm->avic_physical_id_cache), entry); >> - >> - return 0; >> } >> >> [....] >> >> The two functions now have the same signature as their callers, >> svm_vcpu_load and svm_vcpu_put. > > Radim, does this look sane? I plan to include it in my pull request > (I'm running AMD autotest now and it passed the first few tests). > > Thanks, > > Paolo > I have also tested the changes in the 4.7-rc1 on the CZ hardware w/ AVI= C=20 and everything looks good. As for the nested virtualization. Currently, this is not working with=20 AVIC enabled on the host, but it works fine w/ AVIC disabled. I'll look= =20 into enabling nested virtualization w/ AVIC enable next. Thank you for all your help, Suravee