From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PART2 PATCH v3 07/11] iommu/amd: Introduce amd_iommu_update_ga() Date: Wed, 13 Jul 2016 15:49:05 +0700 Message-ID: <57860081.8000206@amd.com> References: <1468231899-6987-1-git-send-email-suravee.suthikulpanit@amd.com> <1468231899-6987-8-git-send-email-suravee.suthikulpanit@amd.com> <20160711185943.GA1375@potion> 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: In-Reply-To: <20160711185943.GA1375@potion> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Hi Radim, I have a feeling that there might be some confusion in the use of=20 parameter ga_tag here in various places. My apology. I am in the proces= s=20 of cleaning up this, and will send out the V4. In the meantime, let me try to clarify a couple design detail that migh= t=20 be missed here. On 07/12/2016 01:59 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2016-07-11 05:11-0500, Suravee Suthikulpanit: >> From: Suravee Suthikulpanit >> >> Introduces a new IOMMU API, 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 >> @@ -4481,4 +4481,67 @@ int amd_iommu_create_irq_domain(struct amd_io= mmu *iommu) >> +int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 ga_tag, >> + u64 base, bool is_run) > > Not just in this function does the interface between svm and iommu sp= lit > ga_tag into its two components (vcpu_id and ga_tag), but it seems tha= t > the combined value could always be used instead ... > Is there an advantage to passing two values? Basically, the amd_iommu_update_ga() function is designed to achieve tw= o=20 things: 1. Communicate from SVM to AMD IOMMU driver the interrupt routing=20 information (e.g. the physical CPU to route the guest interrupt to) in=20 case the vcpu is running. 2. In case of vcpu is not running, the IOMMU driver should decode the=20 GATAG field of the GA log entry to find out which VCPU of which VM need= =20 to be scheduled in, and notify KVM/SVM. The GATAG is encode as ((VM_ID=20 << 8) | VCPU_ID). Here, the amd_iommu_update_ga() takes the two separate value for input=20 parameters. Mainly the ga_tag (which is really the vm_id) and vcpu_id.=20 This allow IOMMU driver to decide how to encode the GATAG to be=20 programmed into the IRTE. Currently, the actual GATAG is a 16-bit value= ,=20 . This keeps the interface independent from how we=20 encode the GATAG. >> +{ >> + unsigned long flags; >> + struct amd_iommu *iommu; >> + >> + if (!AMD_IOMMU_GUEST_IR_VAPIC(amd_iommu_guest_ir)) >> + return 0; >> + >> + for_each_iommu(iommu) { >> + struct amd_ir_data *ir_data; >> + >> + spin_lock_irqsave(&iommu->ga_hash_lock, flags); >> + >> + /* Note: Update all possible ir_data for a particular >> + * vcpu in a particular vm. >> + */ >> + hash_for_each_possible(iommu->ga_hash, ir_data, hnode, >> + AMD_IOMMU_GATAG(ga_tag, vcpu_id)) { >> + struct irte_ga *irte =3D (struct irte_ga *) ir_data->entry; > > (The ga_tag check is missing here too.) Here, the intention is to update all interrupt remapping entries in the= =20 bucket w/ the same GATAG (i.e. vm_id + vcpu_id), where GATAG =3D=20 AMD_IOMMU_GATAG(vm_id, vcpu_id). >> + if (!irte->lo.fields_vapic.guest_mode) >> + continue; >> + >> + update_irte_ga((struct irte_ga *)ir_data->ref, >> + ir_data->irq_2_irte.devid, >> + base, cpu, is_run); > > (The lookup leading up to here is avoidable -- svm, the caller, has t= he > ability to map ga_tag into irte/ir_data directly with a pointer. > I'm not sure if the lookup is slow enough to pardon optimization, b= ut > it might make the code simpler as well.) I might have mislead you up to this point. Not sure if the assumption=20 here still hold with my explanation above. Sorry for confusion. Thanks, Suravee