Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Lu Baolu <baolu.lu@linux.intel.com>,
        Joerg Roedel <joro@8bytes.org>, Steve Wahl <steve.wahl@hpe.com>
CC:     David Woodhouse <dwmw2@infradead.org>,
        Jerry Snitselaar <jsnitsel@redhat.com>,
        Mike Travis <mike.travis@hpe.com>,
        Dimitri Sivanich <sivanich@hpe.com>,
        "Anderson, Russ" <russ.anderson@hpe.com>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each
 domain
Thread-Topic: [PATCH v1 3/6] iommu/vt-d: Refactor iommu information of each
 domain
Thread-Index: AQHYiJMH8ZE5Hw6eJEajUkexzkMKiK1no95w
Date:   Thu, 30 Jun 2022 08:28:07 +0000
Message-ID: <BN9PR11MB5276A78546BD2B38068385E18CBA9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20220625125204.2199437-1-baolu.lu@linux.intel.com>
 <20220625125204.2199437-4-baolu.lu@linux.intel.com>
In-Reply-To: <20220625125204.2199437-4-baolu.lu@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?QLXz5rf83UjuG3Uzloj8hAP8qX84oRbIYu6dV0EGn0iKgl1FXv2v5xZs7QI9?=
 =?us-ascii?Q?kL+OzniIV6IMJnFb+7GFUS0ZalgJSA2Zo70+lBR1QHUXLdAHW3F2N0TesOqq?=
 =?us-ascii?Q?meMs59m69ZEe1FH2EGElGFAHJQbi/MXDSs2hjPmmGehUZlSHiMErLqd4hU9c?=
 =?us-ascii?Q?ijtgXqcjnr3QsmbYg3vp/2Kx/OjyxUCqPTcBMYgKh7AsKEV4UCrL5vF4rXw1?=
 =?us-ascii?Q?U+EppNtDEj79iKJDiRYgjIohoowrNMnxnBwEGZ+BosHI2JOI6rq53h1Jyalj?=
 =?us-ascii?Q?j7KU0rAByHrSqzQD8njEreD4SL3zmtYS1KNFGk94LpsItFeb8XJ9HznNlBen?=
 =?us-ascii?Q?DcULLdDwtP2KSiOuPs3k+Ixok+I56UdfBhHJx5QHubQfzHsq/RRBWoK9erA/?=
 =?us-ascii?Q?tSaGFsz33Q6nQ6cGuMxakOTSIfjyoK4U1puvAZZOgSFMJ4BzrdH1yjBWpnkN?=
 =?us-ascii?Q?goCWVjdjlJJOj8JLLH0KtoYFFXrsf9/8IZbK+Ci20dU3h7QZKhDDamLyEzgS?=
 =?us-ascii?Q?ecW2KEmoJf3oIpPfKv/At+JJfUte+h7TDIwyfRSSFxHNqEJonWLpt3anNcjR?=
 =?us-ascii?Q?Ya6y2siUNme7biyass5vkYr5yzxSHNokdsm0T9Sfu2bgV3M88gU+bGpXukVI?=
 =?us-ascii?Q?PWYiciYgSZZiZtAnF8SNM6bj+lGV2RGm1Bpn8BZGJSc/tn5lxd01T9nzC2ms?=
 =?us-ascii?Q?mKPH8ljb86lF+rRdySYcXb05FuLkkjLWMqTLFVTWP2Aze1I+MqlTZ/fA8tiX?=
 =?us-ascii?Q?6DM9rE1lTm4mn/yDOJrpAEWg2MnXlrYGeLHk+lf6lF295BPJaft4+zCCfiOi?=
 =?us-ascii?Q?kkEfrKts0/RUwedVqpRyXt8So/0PaUgD9uSWlTpN/pbatWWF9iGVopYyCY5n?=
 =?us-ascii?Q?khlEOmNhVRQ7n16I/eFxmp+3JwIHtMU0yytExGSX8i3NrVQ3/aVKQ3KrUzZk?=
 =?us-ascii?Q?t+xJ8D1ajXnRV/BSA7yol453v884yUy1ek+PrrrDsv7FFLqIQknm1YIsLolK?=
 =?us-ascii?Q?kKhZ9lKCN4/7nlv5ZeQDCi8woRrnSzOdziOXTd+eWmQh1c2xmZJ30BFnCFHB?=
 =?us-ascii?Q?o9ftowV7CnGrvJmWQ0OHVmOON6dea0BqKFb6Zm/XDITsoMNp/AirU8OI6xHN?=
 =?us-ascii?Q?9aSsEzT4QCfB9re2GBnPSLccCgXkzK+g5nyAWrkEltBKT6CVG0GP8k8HKnR5?=
 =?us-ascii?Q?KoUjdQHLHqYuqtfRkTA1dRIrScApltBPY2pLAAeo1tlRZwBo4uekhZQR282u?=
 =?us-ascii?Q?nsiXF98wt92y0SclEo1VsNhA1NSmBawYf5SLumWcwX5pK1aKWQhVA1PgI/Xy?=
 =?us-ascii?Q?pKg2adz5r/nRQ0oFAyHhTISnRZes7x+grObYZn3dccDdG27K5z+Wm2QE8LpN?=
 =?us-ascii?Q?OIq6AvSCPb7r7NhMyG8yRx82nWjy6MGILGnnmO3cukJXOb65Kmm/esL8Ou+I?=
 =?us-ascii?Q?nVndQvWtp/n0kUMtuZjXeEcHoenDvR+/lxSdy1VYhLNTM53kAGtwmpYI4ieZ?=
 =?us-ascii?Q?i6GNafWiE4fr75WKCPsVHX8dzoO/LD5WVvLhzft5Z0rew1z86oojE1k3rpzM?=
 =?us-ascii?Q?C0/rpOnBDY+4CJ17d9uLlQY71kYaLgMIVjzofoy5?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 49762fb1-90a4-409e-6b25-08da5a727608
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Jun 2022 08:28:07.8912
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: usoBBwulCT9mO+bCwST9uyj85bsxGMVhNGMPRMrGkODghR57pJAK0EhQCHf3OyS7lZ10qJxydfgaw+0CCopd1Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3413
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Saturday, June 25, 2022 8:52 PM
>=20
> +struct iommu_domain_info {
> +	struct intel_iommu *iommu;
> +	unsigned int refcnt;
> +	u16 did;
> +};
> +
>  struct dmar_domain {
>  	int	nid;			/* node id */
> -
> -	unsigned int iommu_refcnt[DMAR_UNITS_SUPPORTED];
> -					/* Refcount of devices per iommu */
> -
> -
> -	u16		iommu_did[DMAR_UNITS_SUPPORTED];
> -					/* Domain ids per IOMMU. Use u16
> since
> -					 * domain ids are 16 bit wide
> according
> -					 * to VT-d spec, section 9.3 */

It'd make sense to keep the comments when moving above fields.

> +	spin_lock(&iommu->lock);
> +	curr =3D xa_load(&domain->iommu_array, iommu->seq_id);
> +	if (curr) {
> +		curr->refcnt++;
> +		kfree(info);
> +		goto success;

Not a fan of adding a label for success. Just putting two lines (unlock+
return) here is clearer.

> +	ret =3D xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> +			      info, GFP_ATOMIC));

check xa_err in separate line.

>=20
>  static void domain_detach_iommu(struct dmar_domain *domain,
>  				struct intel_iommu *iommu)
>  {
> -	int num;
> +	struct iommu_domain_info *info;
>=20
>  	spin_lock(&iommu->lock);
> -	domain->iommu_refcnt[iommu->seq_id] -=3D 1;
> -	if (domain->iommu_refcnt[iommu->seq_id] =3D=3D 0) {
> -		num =3D domain->iommu_did[iommu->seq_id];
> -		clear_bit(num, iommu->domain_ids);
> +	info =3D xa_load(&domain->iommu_array, iommu->seq_id);
> +	if (--info->refcnt =3D=3D 0) {
> +		clear_bit(info->did, iommu->domain_ids);
> +		xa_erase(&domain->iommu_array, iommu->seq_id);
>  		domain_update_iommu_cap(domain);
> -		domain->iommu_did[iommu->seq_id] =3D 0;
> +		kfree(info);

domain->nid may still point to the node of the removed iommu.