From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PART2 PATCH v5 12/12] svm: Implements update_pi_irte hook to setup posted interrupt Date: Tue, 16 Aug 2016 18:33:29 +0200 Message-ID: <20160816163329.GE12385@potion> References: <1469439131-11308-1-git-send-email-suravee.suthikulpanit@amd.com> <1469439131-11308-13-git-send-email-suravee.suthikulpanit@amd.com> <20160813120325.GH8001@potion> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: joro@8bytes.org, pbonzini@redhat.com, alex.williamson@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, sherry.hurwitz@amd.com To: Suravee Suthikulpanit Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 2016-08-16 22:19+0700, Suravee Suthikulpanit: > On 8/13/16 19:03, Radim Krčmář wrote: >> 2016-07-25 04:32-0500, Suravee Suthikulpanit: >> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> > @@ -1485,9 +1521,16 @@ static void avic_set_running(struct kvm_vcpu *vcpu, bool is_run) >> > +static void svm_pi_list_add(struct vcpu_svm *svm, struct amd_iommu_pi_data *pi) >> > +{ >> > + bool found = false; >> > + unsigned long flags; >> > + struct amd_iommu_pi_data *cur; >> > + >> > + spin_lock_irqsave(&svm->pi_list_lock, flags); >> > + list_for_each_entry(cur, &svm->pi_list, node) { >> > + if (cur->ir_data != pi->ir_data) >> > + continue; >> > + found = true; >> >> This optimization turned out to be ugly ... sorry. > > That's okay. It makes sense to avoid using the hash table if we can. > >> Manipulation with pi_list is hard to understand, IMO, so a comment >> explaining why we couldn't do that without traversing a list and >> comparing pi->ir_data would be nice. > > I'll add more comment here. Thanks. >> Maybe I was a bit confused by reusing amd_iommu_pi_data when all we care >> about is a list of cur->ir_data -- can't we have a list of just ir_data? > > Actually, in SVM, we care about posted-interrupt information, which is > generated from the SVM side, and stored in the amd_iommu_pi_data. This is > also communicated to IOMMU via the irq_set_vcpu_affinity(). > > Here, I only use ir_data to differentiate amd_iommu_pi_data. I'm still confused then. :) struct amd_iommu_pi_data is a throwaway structure for I/O with irq_set_vcpu_affinity(), but we keep it afterwards ... is it more than a wrapper that allows us to put ir_data into a list, because we don't want to add list_head directly to ir_data?