From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752251AbcDMRGJ (ORCPT ); Wed, 13 Apr 2016 13:06:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60062 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752033AbcDMRGH (ORCPT ); Wed, 13 Apr 2016 13:06:07 -0400 Date: Wed, 13 Apr 2016 19:06:01 +0200 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: [PART2 RFC v1 5/9] iommu/amd: Introduce amd_iommu_update_ga() Message-ID: <20160413170601.GA18429@potion.brq.redhat.com> References: <1460119770-2896-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1460119770-2896-6-git-send-email-Suravee.Suthikulpanit@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1460119770-2896-6-git-send-email-Suravee.Suthikulpanit@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 2016-04-08 07:49-0500, Suravee Suthikulpanit: > From: Suravee Suthikulpanit > > This patch introduces a new IOMMU interface, amd_iommu_update_ga(), > which allows KVM (SVM) to update existing posted interrupt IOMMU IRTE when > load/unload vcpu. > > Signed-off-by: Suravee Suthikulpanit > --- > diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c > @@ -4330,4 +4330,74 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu) > +int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag, It'd be nicer to generate the tag on SVM side and pass it whole -- IOMMU doesn't have to care how hypervisors use the tag. > + u64 base, bool is_run) > +{ > + unsigned long flags; > + struct amd_iommu *iommu; > + > + if (amd_iommu_guest_ir < AMD_IOMMU_GUEST_IR_GA) > + return 0; > + > + for_each_iommu(iommu) { > + struct amd_ir_data *ir_data; > + > + spin_lock_irqsave(&iommu->ga_hash_lock, flags); > + > + hash_for_each_possible(iommu->ga_hash, ir_data, hnode, > + AMD_IOMMU_GATAG(ga_tag, vcpu_id)) { All tags can map into the same bucket. Code below doesn't check that the ir_data belongs to the tag and will modify unrelated IRTEs. Have you considered a per-VCPU list of IRTEs on the SVM side? > + struct iommu_dev_data *dev_data; > + if (!ir_data) (ir_data can't be NULL.) > + break; > + > + dev_data = search_dev_data(ir_data->irq_2_irte.devid); > + > + if (!dev_data || !dev_data->guest_mode) > + continue; (guest_mode can be also read from the irte.) > + set_irte_ga(iommu, ir_data->irq_2_irte.devid, > + base, cpu, is_run); set_irte_ga() is pretty expensive -- do we need to invalidate the irt when changing cpu and is_run? 2.2.5.2 Interrupt Virtualization Tables with Guest Virtual APIC Enabled, point 9, bullet 5 says that IRTE is read from memory before considering IsRun, GATag and Destination, which makes me think that avoiding races can be faster in the common case.