From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751296AbcGNJNk (ORCPT ); Thu, 14 Jul 2016 05:13:40 -0400 Received: from mail-cys01nam02on0069.outbound.protection.outlook.com ([104.47.37.69]:14471 "EHLO NAM02-CY1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751081AbcGNJNe (ORCPT ); Thu, 14 Jul 2016 05:13:34 -0400 X-Greylist: delayed 58648 seconds by postgrey-1.27 at vger.kernel.org; Thu, 14 Jul 2016 05:13:34 EDT Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Suravee.Suthikulpanit@amd.com; Subject: Re: [PART2 PATCH v4 07/11] iommu/amd: Introduce amd_iommu_update_ga() To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= References: <1468416032-7692-1-git-send-email-suravee.suthikulpanit@amd.com> <1468416032-7692-8-git-send-email-suravee.suthikulpanit@amd.com> <20160713141457.GF21976@potion> CC: , , , , , From: Suravee Suthikulpanit Message-ID: <578757A8.3000200@amd.com> Date: Thu, 14 Jul 2016 16:13:12 +0700 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <20160713141457.GF21976@potion> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [223.24.91.92] X-ClientProxiedBy: SG2PR06CA0054.apcprd06.prod.outlook.com (10.167.73.150) To BY1PR12MB0438.namprd12.prod.outlook.com (10.162.147.14) X-MS-Office365-Filtering-Correlation-Id: 7ee0bd0b-d29a-4998-8747-08d3abc72044 X-Microsoft-Exchange-Diagnostics: 1;BY1PR12MB0438;2:pw75if8wLQ4ccIJF0YeZMR0Kcu+YhfpT9u1E+jqFvuyi4OmajtLHTq8VXQ4Y3TmUYMElD18dZ9u9Z7+T7vKgaTysFmLNnB7vg8PWad+v2aw7CorWrAiq+3KJHpGUbtcTjz75YNuoME58wIm0Ckwe5oznL8zD6Vnu7AhdPKyh09EtnwPhCoGv7k4QiWXwhKvC;3:YSv2Ax/rnxV/f/C1xuu2vHIsEWcGBGUAPKYfqKAjcEIPZW4DmL7gmt9egPWSZIVUtJPGmjvGuUlaC3gZ6SKZBa/llwcoLFyWGsbS6ZtcKGoetoullFoKnGQmB/NtnSus X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY1PR12MB0438; X-Microsoft-Exchange-Diagnostics: 1;BY1PR12MB0438;25:Ug6p8cszENZsoCkwNL4e/6mrPSIav/7SDuonM1857Omt1DisW0T1DQGyLstBt6bcI9RxHUL4GwOzUGY/+9KAbUL9lCCvFjSM3MqI00KKz9FILbBD1982rs5V7uiHhPfZI5+1DaHezWdIp3nF0QfT/iPAJFPXQuCZt3tQPWQtuUCb6a4f4TfZjZ8qb57HJvMogYyDGxqIf25wHzp4HmV/cHbsqud2iANZlGCoUufDe1uIDJoNZ/QjM/RlwXYHzQOi3wdp2fFXTuDh3D5RxO/JFbDTd4RyR4dXA35JiQnZ9hsi3mOQdcCqZB8xRSs32ZwzqWR0euSXvqOEPPf1jmjuvRyaDqMhQbjGF/ix0bOahBQKH5uEnkCr8Q8g6YIsb/ClXoLgzZbIGK2haQUNQ77/THLHTj8nBZi0f6KVk0w/VlmQ3H0qqfqdz2iDVslcQNv1gbMPq08mRc2UauotQ/V+WcenkNyA8Exe5JC+c8RJb+nWYZndx+jNM3rcGb3iG64dJkxzPSZcRHkI87FsFqSNgeU2PioMvZ+inKiqSTA4PuFZqzqpNMn6JzDHolMORFgjPG56mzlxSYanLUeizylZ6NG7KygcoWtnFa9KVA/5sMdSPmA2ojNCLhjA4AamAd8AwaOln2E8w7ChDqLRAnz4uDm+vhNHqsT8eEsCgM48jJl8EaIiAVLPsmZCPVeF6fDaS/d9HJeAGgyfr+plvlWxEg==;31:W9INSYsuntTr0maGD4MsIufyxqcDF1O1x/I8TgFvGT3fMTvdXDzcWu+XyiXLbcmWd02Ld3imZDQCMo52SFQqLCBV/OWpdwlSZTHXIT75V7keFM7P4GX9fC+bWjA45/VLRRB18QhwOWRY8oCS5G0uSYsa4DLIc6LFXVw0Y19+IBapTCrW7BmFhC3q1Yvd7JmCzRiboCRvhkLtI9X36byfTw== X-Microsoft-Exchange-Diagnostics: 1;BY1PR12MB0438;20:u7pYuugGDDew2Xop7f4rP7AIcGsZfS9vKt3lr6A1UKHjXJknH/I5e9J1OJ4KpVvo7ZVEFMUWBqjrf1MfBh7z0Rsk2g1lZgy7rF5BZP6xO8rkTl6CKE83VQOxBBcs6E7CQ9JGDR+WX1yjExJ7UO98/higgws9eT5qaqStgThdJD1Ce3BxjYYJTVClfzzwL4/mwbRNs59OpGE5WYG6SytLtyvjSb/u2XILC2tfRhLVd7EzlJveKUc9p+ck3De6xTkmVfSZ2WStkQ3Qrhe0AZ4Hf7NXzr8jM1m2z0puOtTRKoW4WoFnQeB4JQDQOWIxH2vxmp7m1TG4Yj9AeRiQP3DkETAtP4iHb1eFgISOPk1Qqe+Ndc883HFlD4HagFGd7MemHe5PAfZF18HFSq1feyCE4H5ZoTSDfcKb/kY/dpKQ6J1DV7sl9yy/JcKaAkow3ohSmbRYLPFveLev4NFYdP2uql62DEOQOjibNBPjwfdo+V99oIySE1smf+d34EXFU833;4:/u0VU42r//hAUH7dLTIIktZPhKD6Y4tc/Pm4eVuHF552Pn3v3ngE2Y0B2i6eMiN4nCqwVzIiIBCJtsml69ZJoDPUEW9zG9T1wQvkHDionn4k9H/ZhDhuXTKVHu6MPxSBos4O8F0P42SQNDRSthGdttXJCRs89nSxnQR/NWRnftGVMyxB3Vx+3pc3UaqcW7p5yNW3Xc93h7ygVm59iZRJR2zME7PY9ocFDUmR2ARN4/vejn4OnoB+erpxlZvAe4aHo/R+xkceTfHkfD8ZnUjQ0CXKHHBzQUvaQjW3gkJFBkiVttxW2XWqMtG8rjkIulM/pSwkbsCPiwc5RnlDrwbz4+u1vbW19ZL8s332On8GUOwgC9c96yWLBCaPUJRdYkR70Lt6O2V60fr6v4dyCA0s3AodP+ExxUMQb9VoBWrxuog= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(767451399110); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026);SRVR:BY1PR12MB0438;BCL:0;PCL:0;RULEID:;SRVR:BY1PR12MB0438; X-Forefront-PRVS: 00032065B2 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4630300001)(6009001)(6049001)(7916002)(377424004)(189002)(24454002)(377454003)(199003)(86362001)(8676002)(23676002)(2950100001)(106356001)(7846002)(305945005)(7736002)(81166006)(81156014)(2870700001)(36756003)(47776003)(42186005)(59896002)(83506001)(110136002)(117156001)(189998001)(97736004)(19580395003)(33656002)(65806001)(66066001)(105586002)(65816999)(101416001)(64126003)(4326007)(2906002)(586003)(99136001)(50466002)(54356999)(87266999)(92566002)(6116002)(3846002)(50986999)(31430400001)(68736007)(77096005)(19580405001)(80316001)(4001350100001)(65956001)(76176999);DIR:OUT;SFP:1101;SCL:1;SRVR:BY1PR12MB0438;H:[192.168.43.18];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtCWTFQUjEyTUIwNDM4OzIzOnBVVEhmbTZ3TndTU1VSVWhhY1VZaFd5dTlP?= =?utf-8?B?a2hoSlpUVUF1cTZJUXVYbDBGNmFQSi83c0pJVmpORnpLZWFicmtmK1VIeHhZ?= =?utf-8?B?cnR3VkJBb0k5d21tUysxZDJSZUJTcjlkQkpWNUROa1l4NnNHZ0xyL0hDcU5D?= =?utf-8?B?Si93TjM0aHpVa0wrbWVFRW1PWWxpWFVxTm0wNVExZjJvR0VYNmRQWHhuT2tv?= =?utf-8?B?TWNZTVlsaVdocG5QaWhSaE1OcDZETFAwQXNZVzlja0ZqSnM3NUQvZHl3bnJI?= =?utf-8?B?YTJqaC9PMGNKYlN5VHQySTJrWXlSTlhudnA3U2RmeFRyUXJ2cE9iaFlMdDlQ?= =?utf-8?B?dlpVWG4wZjM3ZFZYTEcxbWlYMi9Pa2VKb2hucDFkb01FZUk1WUM3dzJjMDds?= =?utf-8?B?SXFDVG9jcERKbGVudDd5RWhHMUE0Y0E4MjlHdTJvbUQrUzJVdGVaNFQrYysw?= =?utf-8?B?VFhFSEFkN2dybTlDd3pWRXpjM0xCeXZpU1RLTWtINkQ5ZEd5WUczSzZBa2N5?= =?utf-8?B?MDBoM0VjOXVmZVpBcWU4bUx4Wm9tZ0d4eitYWmJ2NTN3WVIrTDQ1VWtlQklD?= =?utf-8?B?ZExZcW1leGNzL2tzNmJiaWdsamVpNzBXMmJSMlVrS1N0Ukt0QlFvdEZBZElw?= =?utf-8?B?b2syWlIvUUh0Znl4amhIbytmalJ1Qmh0UkY1UXczSkJ6bWxsK3VtN2c3aFlI?= =?utf-8?B?Vk1KSDdUQ2hIblE0Mmh4MDBMNHhUUTJ5empGd0dTeHBnamppK1JOQ01HQ2RG?= =?utf-8?B?TThYMnpmczNpQk0zM1A2UGNqNTJKbEV5Vks5a29nNzl1cUt4eGZyTXJ6c21o?= =?utf-8?B?cFJ4OHB1VE9ZTGRaNVlUNXAxRlZHbm0xVmR5Z0o2VVNVMVJuRXZaMzdmYUhl?= =?utf-8?B?eHZMckF1YVk3NlZsK0tLckFPL0NLTldhWHRmVlR2UGJobk1wMEF3UjgvamVq?= =?utf-8?B?cDJBKzNwVmphbGgwQS9OZ0ljTTd5Zis5TXFmZnpML1VPUGkyY3NsNnpXQzda?= =?utf-8?B?Y2taUEdqZnZnOVBQOUFRcGtwTDNENHZ4L2gyS25XZzVvdTRET0J2aTdJNS84?= =?utf-8?B?ek4xZzM2UkJnVG5kckpiazg1NnhkUW5McklpYVAxKzdaS0xGSC9DMTRCSlNn?= =?utf-8?B?bkRPS3lMMVRuMzVVT0ZvZVU5UlFQQVM5TkFPWENWN0pQcmVlSXl2ckM3S2I5?= =?utf-8?B?dEZqNEhQZmttQmlXd2dJRHdza01tVDR0MjFQelBzclpYamU3K09jSHlIQ1FQ?= =?utf-8?B?bFNrUTJNWmpINHJBd1VBVUk4VWQ3RVZJUDVsMEF1QVQ3L25YQSswanJBVVMz?= =?utf-8?B?Zk1DRjNqdERBVWRJUDYyRE9mbmNzcm5hbytYQ0tIRUIxRGN4bkpsQll4TFZi?= =?utf-8?B?NE5aSHRjL1FONDdacXpoemhFT2psSFR5ZlFnMy9iaHphQi9id0RicXZKZ1l2?= =?utf-8?B?dFFaRGlCZVhDNHlwWDc2TDJtVVpkOWNmclA1aU5VTENQNkZacVBxMUhyZ0w2?= =?utf-8?B?SFVQU2Z0RmI3QWRrNmYydVRZTDd3YW44TnZ5bVZZUmRzUW8rK2FiQ0lFQU03?= =?utf-8?B?a1Z5K2lYQWhJcnpheEhyeW1iV20xbmgrS3FyM0JyeXlIZ0FlSmFUdlV1NE9y?= =?utf-8?B?dEFvazlTQTdpbzJUcms3RWFWUkJob3FuMVVteWVWQWI1d05IRHVoeWJ0Yk1k?= =?utf-8?B?dGNmOFBqWEFleGFnTkl5WlZ2UmRmelpMaDdTN2JJUTN1YVRPL2dtNHBFSCtQ?= =?utf-8?B?OGo3VFhKMVpwalp4dk5XUmdLRXFxQlpaSjk5cFZ4Tjg2WlhQcXRMSEQ2d3lU?= =?utf-8?B?ZWxTdi9yNjhSc2Y0RmFRcG0vK2FwbHFkYUk2emRodS9XMlZBUC9oTDJNaFdo?= =?utf-8?Q?GVu9ojUoaYY=3D?= X-Microsoft-Exchange-Diagnostics: 1;BY1PR12MB0438;6:ISfb7K6PFah97W6dhnDUOrQ9vD69nOkSKzXnhwhtXmu/8VaqkmCwo0CRa0qAJ/Sqxyoy/4awxP/ip8KdYyEAzR1/kEmwkf9EY4EUrlU1xQ7ColpLmYMMa1AiACry5e0+Ea+dZf4XjyzqW3xXXhd/VC3QQEZfFqQxSRoRpLvUW3GMQf8u9uRkMpK4itX4V/KHjD46E3eds/g51eLgtARH8m8PvkzNtu4ERfiq+KUEvTYx8FfaRWfRENcbm/V1WjgDZt8jExRvto1LInXftg2BBEhO7lIr3RGpUVYZypN+RCnOhV7Bkr5g7ma4qpzwEEiE8rA4fiaQofXX/2SZF1y2SQ==;5:e47+RM2EtOQx21t5tCT5BgttvExZS98TJEHlSlJ+HZx8520s/NleFCW9+VZsHaKI0ipHRQ91TokZlvul18aQ0AYx/a8XTfQ432hIYhvzZKXbzTbU11VoPJCbg5d5Hx7C0uSSW5of7ohgg0Nh/GkHHA==;24:mhYMvBY0paYVKAT8/7cSEc7xc0e8IS1lIVsDz+fVK9NSOnLKWlXDFl71mmUP4AxknwD2ZEpuei/e1I9vQlODjQHhwRf2fbtwNOWszl/a+I0=;7:kZ1Oulx/Z4gomY0sjiChSgbowPTn/VUCqq1G+XYgmpqhjETV/d8LvrHTm10aSGR/gRyKHITKAEDy7eDqAhlqP8q/UqfMOGgO3p7V6/4ZPgHIiRVHfANp/VoEngdgNtI4SuenLoBtCTsx5vFg1Mo2Ndp3rf/7J5xFwhFt4RvXEDcktGpWoilhtPDDs7ZHKzEcTS3fqXX6t+wMr+cE1G2jAauoHzU5sBwClpeZfIVZb6w6QtaizU30HxN87Sk7pQTv SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;BY1PR12MB0438;20:vAvd0uZ9/gf4xQTt42gjVFEzMOe2oh1qf9ss0Qyst7Qgha/vDKFoR/hmKbRgIdKlcqujKTPBjkPrBLjCjGWwmP9BOMIG7sLRPrbSzv4ovnqHWsMPRvunHjobROBSwFkYflhyMfMNXjOQCEKsVLPUiCYbYIcfPNwbT8G4zRp+XTWftyIdGtrwnawXIBfLbQTnOf3FXjyHNccwhAirQjjNw9Xut74GC1ZB5uufZmDkXQBAR0EWHcBvEjFd+HCAeWWB X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jul 2016 09:13:29.0993 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR12MB0438 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Radim, On 7/13/16 21:14, Radim Krčmář wrote: > [I pasted v3 reviews prefixed with a pipe where I think they still apply.] > > 2016-07-13 08:20-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 >> @@ -4461,4 +4461,69 @@ int amd_iommu_create_irq_domain(struct amd_iommu *iommu) >> +int amd_iommu_update_ga(u32 vcpu_id, u32 cpu, u32 vm_id, >> + u64 base, bool is_run) > > |2016-07-13 15:49+0700, Suravee Suthikulpanit: > |> On 07/12/2016 01:59 AM, Radim Krčmář wrote: > |>> Not just in this function does the interface between svm and iommu split > |>> ga_tag into its two components (vcpu_id and ga_tag), but it seems that > |>> the combined value could always be used instead ... > |>> Is there an advantage to passing two values? > |> > |> Here, the amd_iommu_update_ga() takes the two separate value for input > |> parameters. Mainly the ga_tag (which is really the vm_id) and vcpu_id. This > |> allow IOMMU driver to decide how to encode the GATAG to be programmed into > |> the IRTE. Currently, the actual GATAG is a 16-bit value, . > |> This keeps the interface independent from how we encode the GATAG. > > I was thinking about making the IOMMU unaware how SVM or other Linux > hypervisors use the ga_tag, i.e. passing the final u32 ga_tag. > For example 32 bit hypervisor doesn't need to use lookup, because any > pointer can used as the ga_tag directly. Ahh....... (w/ a big light bulb) I get your point now. Let's just have SVM (or other hypervisor) define what the tag should be and just pass-on the value to IOMMU. IOMMU can just simply use this w/o knowing what it is. Sorry, I'm slow :) > And there are other viable algoritms for assigning the ga_tag -- > why isn't the vm_id 24 bits? Good point! Actually, I am somehow limited to 30-bit hash value. So, the VM_ID can be 22 bits, I'll make that change. > >> + 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->gatag_ir_hash_lock, flags); >> + >> + /* Note: >> + * We need to update all interrupt remapping table entries >> + * for targeting the specified vcpu. Here, we use gatag >> + * as a hash key and iterate through all entries in the bucket. >> + */ >> + hash_for_each_possible(iommu->gatag_ir_hash, ir_data, hnode, >> + AMD_IOMMU_GATAG(vm_id, vcpu_id)) { >> + struct irte_ga *irte = (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 > |> bucket w/ the same GATAG (i.e. vm_id + vcpu_id), where GATAG = > |> AMD_IOMMU_GATAG(vm_id, vcpu_id). > > Which is why you need to check that > AMD_IOMMU_GATAG(vm_id, vcpu_id) == entry->fields_vapic.ga_tag > > The hashing function can map two different vm_id + vcpu_id to the same > bucket and hash_for_each_possible() would return both of them, but only > one belongs to the VCPU that we want to update. > > (And shouldn't there be only one match?) Actually, with your suggestion above, the hask key would be (vm_id & 0x3FFFFF << 8)| (vcpu_id & 0xFF). So, it should be unique for each vcpu of each vm, or am I still missing something? Also, since we will not be passing the vmid and vcpuid as separate value, and just passing the (u32)ga_tag, we would not be able to do the check you suggested here. > >> + >> + 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 the > |>> ability to map ga_tag into irte/ir_data directly with a pointer. I'm not sure about this optimization to avoid look up. The struct amd_ir_data is part of the IOMMU driver, and the SVM knows nothing about it. I don't think it would be able to find out the pointer to amd_ir_data/irte. Also, with the current design, each ga_tag can be mapped to different irte since there could be multiple interrupts targeting a particular cpu. Here, we would want to update all of the IRTEs with the same ga_tag. > |>> I'm not sure if the lookup is slow enough to pardon optimization, but > |>> it might make the code simpler as well.) > |> > |> I might have mislead you up to this point. Not sure if the assumption here > |> still hold with my explanation above. Sorry for confusion. > > SVM configures IOMMU with ga_tag, so IOMMU could return the pointer to > ir_data/irte that was just configured. Also, IIUC, you want to use the pointer to ir_data/irte as the ga_tag value. The issue would be ga_tag is a 32-bit value, and this would not work with 64-bit address. > SVM would couple it with a VCPU > (and hence a ga_tag) and when amd_iommu_update_ga() was needed, SVM > would pass the ir_data/irte pointer directly, instead of looking it up > though a ga_tag. Please let me know if I am still missing any points. Thanks, Suravee