From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CFEE8C433EF for ; Wed, 22 Jun 2022 03:27:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1356518AbiFVD1m (ORCPT ); Tue, 21 Jun 2022 23:27:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47674 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233601AbiFVD1j (ORCPT ); Tue, 21 Jun 2022 23:27:39 -0400 Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E083F3139B; Tue, 21 Jun 2022 20:27:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1655868457; x=1687404457; h=message-id:date:mime-version:cc:subject:to:references: from:in-reply-to:content-transfer-encoding; bh=j07rY6WMS0CfEZ9gsbNiNKbfiAjIhPmGB19qMdJh6jw=; b=WkupXrnhlUPoYXbiQLUj2d9daGTE+sL84KtwJU3kCsyrHZMlSy9g2fQF lsxXAhDK1jUZA8ELaFlO9tKYHdKgOpBq5C/45K8fa97g6FIOGcd0czm2N CdaIq3SrYC4GXJPArrYXO3XA6Ow6/NhQl+44CkaGmzAFpaZQ8YTwvAoAQ bbEZ9iXlB5fkbVT9hpcpZYEDbMavcTWyFSbJvX6w9Mtlod9EZgzcHfHFE T5iys060O1faiqbcx1rBn9ckQkLYrRYA52iBzgCYIXRvgGPGxT9lXbnKg KFwvoGGZc/GfVVulWG154BfheT6l9e7yNB6SDiAjV6xUD5+25q2MZ4Bxh Q==; X-IronPort-AV: E=McAfee;i="6400,9594,10385"; a="279069674" X-IronPort-AV: E=Sophos;i="5.92,211,1650956400"; d="scan'208";a="279069674" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jun 2022 20:27:37 -0700 X-IronPort-AV: E=Sophos;i="5.92,211,1650956400"; d="scan'208";a="833882261" Received: from xzhan99-mobl1.ccr.corp.intel.com (HELO [10.249.172.26]) ([10.249.172.26]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jun 2022 20:27:35 -0700 Message-ID: <4316fa3e-3183-beb0-9c4a-d6045c6b5340@linux.intel.com> Date: Wed, 22 Jun 2022 11:27:33 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Cc: baolu.lu@linux.intel.com, "Qiang, Chenyi" , "Liu, Yi L" , "Pan, Jacob jun" , "iommu@lists.linux-foundation.org" , "linux-kernel@vger.kernel.org" , "stable@vger.kernel.org" Subject: Re: [PATCH 1/1] iommu/vt-d: Fix RID2PASID setup failure Content-Language: en-US To: "Tian, Kevin" , Joerg Roedel , "Raj, Ashok" References: <20220620081729.4610-1-baolu.lu@linux.intel.com> <5d13cab5-1f0a-51c7-78a3-fb5d3d793ab1@linux.intel.com> <80457871-a760-69ba-70be-5e95344182ea@linux.intel.com> From: Baolu Lu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2022/6/22 11:06, Tian, Kevin wrote: >> From: Baolu Lu >> Sent: Tuesday, June 21, 2022 5:04 PM >> >> On 2022/6/21 13:48, Tian, Kevin wrote: >>>> From: Baolu Lu >>>> Sent: Tuesday, June 21, 2022 12:28 PM >>>> >>>> On 2022/6/21 11:46, Tian, Kevin wrote: >>>>>> From: Baolu Lu >>>>>> Sent: Tuesday, June 21, 2022 11:39 AM >>>>>> >>>>>> On 2022/6/21 10:54, Tian, Kevin wrote: >>>>>>>> From: Lu Baolu >>>>>>>> Sent: Monday, June 20, 2022 4:17 PM >>>>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct >>>>>>>> dmar_domain *domain, struct device *dev) >>>>>>>> ret = intel_pasid_setup_second_level(iommu, >>>>>>>> domain, >>>>>>>> dev, PASID_RID2PASID); >>>>>>>> spin_unlock_irqrestore(&iommu->lock, flags); >>>>>>>> - if (ret) { >>>>>>>> + if (ret && ret != -EBUSY) { >>>>>>>> dev_err(dev, "Setup RID2PASID failed\n"); >>>>>>>> dmar_remove_one_dev_info(dev); >>>>>>>> return ret; >>>>>>>> -- >>>>>>>> 2.25.1 >>>>>>> It's cleaner to avoid this error at the first place, i.e. only do the >>>>>>> setup when the first device is attached to the pasid table. >>>>>> The logic that identifies the first device might introduce additional >>>>>> unnecessary complexity. Devices that share a pasid table are rare. I >>>>>> even prefer to give up sharing tables so that the code can be >>>>>> simpler.:-) >>>>>> >>>>> It's not that complex if you simply move device_attach_pasid_table() >>>>> out of intel_pasid_alloc_table(). Then do the setup if >>>>> list_empty(&pasid_table->dev) and then attach device to the >>>>> pasid table in domain_add_dev_info(). >>>> The pasid table is part of the device, hence a better place to >>>> allocate/free the pasid table is in the device probe/release paths. >>>> Things will become more complicated if we change relationship between >>>> device and it's pasid table when attaching/detaching a domain. That's >>>> the reason why I thought it was additional complexity. >>>> >>> If you do want to follow current route it’s still cleaner to check >>> whether the pasid entry has pointed to the domain in the individual >>> setup function instead of blindly returning -EBUSY and then ignoring >>> it even if a real busy condition occurs. The setup functions can >>> just return zero for this benign alias case. >> Kevin, how do you like this one? >> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c >> index cb4c1d0cf25c..ecffd0129b2b 100644 >> --- a/drivers/iommu/intel/pasid.c >> +++ b/drivers/iommu/intel/pasid.c >> @@ -575,6 +575,16 @@ static inline int pasid_enable_wpe(struct >> pasid_entry *pte) >> return 0; >> }; >> >> +/* >> + * Return true if @pasid is RID2PASID and the domain @did has already >> + * been setup to the @pte. Otherwise, return false. >> + */ >> +static inline bool >> +rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did) >> +{ >> + return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) == >> did; >> +} > better this is not restricted to RID2PASID only, e.g. pasid_pte_match_domain() > and then read pasid from the pte to compare with the pasid argument. > The pasid value is not encoded in the pasid table entry. This validity check is only for RID2PASID as alias devices share the single RID2PASID entry. For other cases, we should always return -EBUSY as what the code is doing now. Best regards, baolu