From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PART1 V5 13/13] svm: Manage vcpu load/unload when enable AVIC Date: Tue, 17 May 2016 14:09:44 +0200 Message-ID: <573B0A08.7040604@redhat.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com To: Suravee Suthikulpanit , rkrcmar@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com Return-path: In-Reply-To: <573201AB.2050006@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 10/05/2016 17:43, Paolo Bonzini wrote: >=20 >=20 > 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 t= he >> physical APIC-ID table. >> >> Signed-off-by: Suravee Suthikulpanit >> Reviewed-by: Radim Kr=C4=8Dm=C3=A1=C5=99 >> --- >=20 > I think this is the only patch that needs a little more work, because= =20 > there are a bunch of unused return values that really should be=20 > WARN_ON. In addition the load and put cases are different enough tha= t=20 > they should be separate functions. >=20 > Can you please test this? >=20 > 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; > }; > =20 > #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); > =20 > if (!kvm_vcpu_apicv_active(vcpu)) > - return 0; > + return; > + > + svm->avic_is_running =3D is_run; > =20 > /* 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; > =20 > entry =3D READ_ONCE(*(svm->avic_physical_id_cache)); > WARN_ON(is_run =3D=3D !!(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 |=3D AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; > WRITE_ONCE(*(svm->avic_physical_id_cache), entry); > - > - return 0; > } > =20 > -static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_lo= ad) > +static void avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > { > u64 entry; > + /* ID =3D 0xff (broadcast), ID > 0xff (reserved) */ > int h_physical_id =3D __default_cpu_present_to_apicid(cpu); > struct vcpu_svm *svm =3D to_svm(vcpu); > =20 > if (!kvm_vcpu_apicv_active(vcpu)) > - return 0; > + return; > =20 > - /* 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; > =20 > entry =3D 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 &=3D ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK; > + entry |=3D (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID= _MASK); > =20 > entry &=3D ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; > - if (is_load) { > - entry &=3D ~AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_ID_MASK; > - entry |=3D (h_physical_id & AVIC_PHYSICAL_ID_ENTRY_HOST_PHYSICAL_I= D_MASK); > - if (!svm->avic_is_blocking) > - entry |=3D AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; > - } > + if (svm->avic_is_running) > + entry |=3D AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; > + > WRITE_ONCE(*(svm->avic_physical_id_cache), entry); > +} > =20 > - return 0; > +static void avic_vcpu_put(struct kvm_vcpu *vcpu) > +{ > + u64 entry; > + struct vcpu_svm *svm =3D to_svm(vcpu); > + > + if (!kvm_vcpu_apicv_active(vcpu)) > + return; > + > + entry =3D READ_ONCE(*(svm->avic_physical_id_cache)); > + entry &=3D ~AVIC_PHYSICAL_ID_ENTRY_IS_RUNNING_MASK; > + WRITE_ONCE(*(svm->avic_physical_id_cache), entry); > } > =20 > static void svm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) > @@ -1436,10 +1447,10 @@ static struct kvm_vcpu *svm_create_vcpu(struc= t kvm *kvm, unsigned int id) > goto free_page4; > } > =20 > - /* 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 =3D false; > + svm->avic_is_running =3D true; > =20 > svm->nested.hsave =3D page_address(hsave_page); > =20 > @@ -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); > =20 > - avic_vcpu_load(vcpu, cpu, true); > + avic_vcpu_load(vcpu, cpu); > } > =20 > 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 =3D to_svm(vcpu); > int i; > =20 > - avic_vcpu_load(vcpu, 0, false); > + avic_vcpu_put(vcpu); > =20 > ++vcpu->stat.host_state_reload; > kvm_load_ldt(svm->host.ldt); > @@ -1545,13 +1556,11 @@ static void svm_vcpu_put(struct kvm_vcpu *vcp= u) > =20 > static void svm_vcpu_blocking(struct kvm_vcpu *vcpu) > { > - to_svm(vcpu)->avic_is_blocking =3D true; > avic_set_running(vcpu, false); > } > =20 > static void svm_vcpu_unblocking(struct kvm_vcpu *vcpu) > { > - to_svm(vcpu)->avic_is_blocking =3D false; > avic_set_running(vcpu, true); > } > =20 >=20 > 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