Received-SPF: pass (google.com: domain of linux-kernel+bounces-10120-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <07285ebd-ae7f-4c4a-a926-8c52a2da8da6@linux.intel.com>
Date: Sat, 23 Dec 2023 07:35:58 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH v5 3/3] iommu/vt-d: abort the devTLB invalidation
 waiting if device is removed
From: Ethan Zhao <haifeng.zhao@linux.intel.com>
To: bhelgaas@google.com, baolu.lu@linux.intel.com, dwmw2@infradead.org,
 will@kernel.org, robin.murphy@arm.com, lukas@wunner.de
Cc: linux-pci@vger.kernel.org, iommu@lists.linux.dev,
 linux-kernel@vger.kernel.org
References: <20231222104108.18499-1-haifeng.zhao@linux.intel.com>
 <20231222104108.18499-4-haifeng.zhao@linux.intel.com>
In-Reply-To: <20231222104108.18499-4-haifeng.zhao@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 12/22/2023 6:41 PM, Ethan Zhao wrote:
> Even the devTLB invalidation request is just submitted and waiting it
> to be done/timeout in qi_submit_sync(), it is possible device is removed
> or powered-off. try to break it out in such rare but possible case.
>
> This patch is sent for more comment. not tested, only passed compiling.
>
> Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
> ---
>   drivers/iommu/intel/dmar.c  |  3 ++-
>   drivers/iommu/intel/iommu.c | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 23cb80d62a9a..d8637ab93387 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1422,7 +1422,8 @@ int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
>   	 */
>   	writel(qi->free_head << shift, iommu->reg + DMAR_IQT_REG);
>   
> -	while (qi->desc_status[wait_index] != QI_DONE) {
> +	while (qi->desc_status[wait_index] != QI_DONE &&
> +			qi->desc_status[wait_index] != QI_ABORT) {

Another way is checking pci_device_is_present() here and bail out,

how about it ?

>   		/*
>   		 * We will leave the interrupts disabled, to prevent interrupt
>   		 * context to queue another cmd while a cmd is already submitted
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 897159dba47d..33075d0688bc 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4472,10 +4472,46 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
>   	return &iommu->iommu;
>   }
>   
> +static void intel_iommu_abort_devtlib_invalidate(struct device *dev)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu = info->iommu;
> +	struct q_inval *qi = iommu->qi;
> +	struct qi_desc *desc, *idesc;
> +	int index, offset, shift;
> +	u16 sid, qdep, pfsid
> +	unsigned long flags;
> +
> +	if (!dev_is_pci(info->dev) || !info->ats_enabled || !qi)
> +		return;
> +	if (!pci_dev_is_disconnected(to_pci_dev(dev)))
> +		return;
> +
> +	sid = info->bus << 8 | info->devfn;
> +	qdep = info->ats_qdep;
> +	pfsid = info->pfsid;
> +
> +	raw_spin_lock_irqsave(&qi->q_lock, flags);
> +	for (index = 1; index < QI_LENGTH; index++) {
> +		offset = index << shift;
> +		desc = qi->desc + offset;
> +		if (desc->qw0 & QI_IWD_TYPE) {
> +			offset = (index-1) << shift;
> +			idesc = qi->desc + offset;
> +			if (idesc->qw0 & QI_DEV_EIOTLB_SID(sid)) {
> +				if (qi->desc_status[index] == QI_IN_USE)
> +					qi->desc_status[index] = QI_ABORT;
> +			}
> +		}
> +	}
> +	raw_spin_unlock_irqrestore(&qi->q_lock, flags);
> +
> +}
>   static void intel_iommu_release_device(struct device *dev)
>   {
>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>   
> +	intel_iommu_abort_devtlib_invalidate(dev);

Wonder if there is lock something prevent pciehp_ist() supprise_removal

interrupt response re-enter to get here when another safe_removal is in 
process,

if so ,  see above

>   	dmar_remove_one_dev_info(dev);
>   	intel_pasid_free_table(dev);
>   	intel_iommu_debugfs_remove_dev(info);