From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PART1 RFC v2 10/10] svm: Manage vcpu load/unload when enable AVIC Date: Mon, 14 Mar 2016 18:48:51 +0700 Message-ID: <56E6A523.7040701@amd.com> References: <1457124368-2025-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1457124368-2025-11-git-send-email-Suravee.Suthikulpanit@amd.com> <20160309214629.GE19459@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , , , , , , To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: Received: from mail-bn1bon0066.outbound.protection.outlook.com ([157.56.111.66]:17056 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751754AbcCNLtO (ORCPT ); Mon, 14 Mar 2016 07:49:14 -0400 In-Reply-To: <20160309214629.GE19459@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Hi, On 03/10/2016 04:46 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2016-03-04 14:46-0600, Suravee Suthikulpanit: >> From: Suravee Suthikulpanit >> >> When a vcpu is loaded/unloaded to a physical core, we need to update >> information in the Physical APIC-ID table accordingly. >> >> Signed-off-by: Suravee Suthikulpanit >> --- >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> @@ -1508,6 +1510,146 @@ static int avic_vcpu_init(struct kvm *kvm, s= truct vcpu_svm *svm, int id) >> +static inline int >> +avic_update_iommu(struct kvm_vcpu *vcpu, int cpu, phys_addr_t pa, b= ool r) >> +{ >> + if (!kvm_arch_has_assigned_device(vcpu->kvm)) >> + return 0; >> + >> + /* TODO: We will hook up with IOMMU API at later time */ > > (It'd be best to drop avic_update_iommu from this series and introduc= e > it when merging iommu support.) > I just kept it there to make code merging b/w part1 and 2 that I have=20 been testing simpler. I didn't think it would cause much confusion. But= =20 if you think that might be the case, I can drop it for now. >> + return 0; >> +} >> + >> +static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_l= oad) > > This function does a lot and there is only one thing that must be don= e > in svm_vcpu_load: change host physical APIC ID if the CPU changed. > The rest can be done elsewhere: > - is_running when blocking. I added the logic here to track if the is_running is set when unloading= =20 since I noticed the case when the vCPU is busy doing some work for the=20 guest, then get unloaded and later on get loaded w/o=20 blocking/unblocking. So, in theory, it should be be set to running=20 during unloaded period, and it should restore this flag if it is loaded= =20 again. > - kb_pg_ptr when the pointer changes =3D only on initialization? The reason I put this here mainly because it is a convenient place to=20 set the vAPIC bakcing page address since we already have to set up the=20 host physical APIC id. I guess I can try setting this separately during= =20 vcpu create. But I don't think it would make much difference. > - valid when the kb_pg_ptr is valid =3D always for existing VCPUs? According to the programming manual, the valid bit is set when we set=20 the host physical APIC ID. However, in theory, the vAPIC backing page=20 address is required for AVIC hardware to set bits in IRR register, whil= e=20 the host physical APIC ID is needed for sending doorbell to the target=20 physical core. So, I would actually interpret the valid bit as it shoul= d=20 be set when the vAPIC backing address is set. In the current implementation, the valid bit is set during vcpu load,=20 but is not unset it when unload. This actually reflect the=20 interpretation of the description above. If we decide to move the setting of vAPIC backing page address to the=20 vcpu create phrase, we would set the valid bit at that point as well. Please let me know if you think differently. >> +static int avic_set_running(struct kvm_vcpu *vcpu, bool is_run) > > avic_set_running should get address of the entry and write is_run to = it. > (No need to touch avic_bk_page, h_phy_apic_id or do bound checks.) > > I think you can cache the physical apic id table entry in *vcpu, so b= oth > functions are going to be very simple. > I was actually thinking about doing this. Lemme try to come up with the= =20 logic to cache this. Thanks, Suravee