From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752567AbcCNQko (ORCPT ); Mon, 14 Mar 2016 12:40:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46442 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752022AbcCNQkl (ORCPT ); Mon, 14 Mar 2016 12:40:41 -0400 Date: Mon, 14 Mar 2016 17:40:36 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Suravee Suthikulpanit Cc: pbonzini@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com Subject: Re: [PART1 RFC v2 10/10] svm: Manage vcpu load/unload when enable AVIC Message-ID: <20160314164035.GB4120@potion.brq.redhat.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> <56E6A523.7040701@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <56E6A523.7040701@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2016-03-14 18:48+0700, Suravee Suthikulpanit: > On 03/10/2016 04:46 AM, Radim Krčmář 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, struct vcpu_svm *svm, int id) >>>+static inline int >>>+avic_update_iommu(struct kvm_vcpu *vcpu, int cpu, phys_addr_t pa, bool 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 introduce >> it when merging iommu support.) >> > > I just kept it there to make code merging b/w part1 and 2 that I have been > testing simpler. I didn't think it would cause much confusion. But if you > think that might be the case, I can drop it for now. The iommu part might end up having different requirements for this function, so this husk can only add work when compared to waiting. And avic_update_iommu is logically separable, so it would be nicer as a short separate patch anyway. (It's not a problem if you leave it.) >>>+static int avic_vcpu_load(struct kvm_vcpu *vcpu, int cpu, bool is_load) >> >>This function does a lot and there is only one thing that must be done >>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 > since I noticed the case when the vCPU is busy doing some work for the > guest, then get unloaded and later on get loaded w/o blocking/unblocking. > So, in theory, it should be be set to running during unloaded period, and it > should restore this flag if it is loaded again. (A second mail will be related to this.) >> - kb_pg_ptr when the pointer changes = only on initialization? > > The reason I put this here mainly because it is a convenient place to set > the vAPIC bakcing page address since we already have to set up the host > physical APIC id. I guess I can try setting this separately during vcpu > create. But I don't think it would make much difference. vcpu_load isn't as hot vcpu_run, but it's still called often and the most useful optimization is to avoid unnecessary operations ... (I think the split code is going to be easier to understand as well.) >> - valid when the kb_pg_ptr is valid = always for existing VCPUs? > > According to the programming manual, the valid bit is set when we set the > host physical APIC ID. (Physical APIC ID doesn't affect the valid bit at all.) > However, in theory, the vAPIC backing page address is > required for AVIC hardware to set bits in IRR register, while the host > physical APIC ID is needed for sending doorbell to the target physical core. > So, I would actually interpret the valid bit as it should be set when the > vAPIC backing address is set. Yes, APM (rev 3.23, vol 2, table 15-24): Valid bit. When set, indicates that this entry contains a valid vAPIC backing page pointer. If cleared, this table entry contains no information. > In the current implementation, the valid bit is set during vcpu load, but is > not unset it when unload. This actually reflect the interpretation of the > description above. > > If we decide to move the setting of vAPIC backing page address to the vcpu > create phrase, we would set the valid bit at that point as well. > > Please let me know if you think differently. I agree with your analysis and setting the backing page and valid bit on LAPIC creation seems better to me. Thanks.