Message ID | 20231224050657.182022-5-haifeng.zhao@linux.intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-10641-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:2483:b0:fb:cd0c:d3e with SMTP id q3csp1988046dyi; Sat, 23 Dec 2023 21:08:19 -0800 (PST) X-Google-Smtp-Source: AGHT+IG39+5J/QgC8GRZi6tdqiEvvTOU/l4HH8Gn5k7HtWrDVqU1P81byEb1pLzjqqfTixDjAEeL X-Received: by 2002:a05:6a20:1610:b0:195:82b3:4c8 with SMTP id l16-20020a056a20161000b0019582b304c8mr1446453pzj.88.1703394499668; Sat, 23 Dec 2023 21:08:19 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1703394499; cv=none; d=google.com; s=arc-20160816; b=XeCdCpISXOkn5j2r/ns5Za2YvytSEIspjMdXLHdY1dbopWwgCnxeEUciWhXqGe4hAd HMlUN152PpU6pbItzbPwmp0OVvIwszz97X0Z6ou5nFow6jWDPvktKIs02rkQZ0K/NHzF dQgicp2KTLaFgo+BFMWl7CWVDsjruyxOXsfs22hWKF3C5jr5KBeQsLZiF/z02ywpWF8U CMMO45AkUXpnQu0fF7iNkFOe7yZBm01UFoLf7yYCHUrZLgPtzEOECG+LFdU9l/uAJ8V1 kk72zikE0CX+WjvPsFlI3yoOwqwh+JCJHr00KtKrIFQopf+F1v5wcfvoXOI9QYWaWh2c iLag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=idDi2H4fWtXWNkBsckogz0Mv8lKMudGxK2qONFSA1vg=; fh=cQTxynQ7rGExgmbyXq8tNmd/VmE1Fzgt9QW51hABj2c=; b=nCuuE0TR7bacsqndKWtlvDBZucCJEmNDevqMkuiyQG0X3hqYKhu5kufIiKm+nFbeTr C8bDllaHIvAse1OpkglTnUkxDoAOn+QCaBpaicI8mpZqfDQf3Mjs5OOzwNged1/8qYcI 20lnoEM12MJxkGpa6nEpRhaakRGmk/pNTWwdYJq83ZJJXrN00mNf+CE4rhXgtabOSicM Dm6kQS7jlFJlLITgRj/82ZK01CFZrRHc56XdS15NXFgbhzH77VbsrkHEn9hb6+eRfKi5 R7k3e2ePHGxHU61W2n/UtDxAI06BWgdTQ2hTS98tBQ+63OttMy/Gw0BnuS1Fr4rGAeLO meCw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="faJ/Md4k"; spf=pass (google.com: domain of linux-kernel+bounces-10641-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-10641-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [139.178.88.99]) by mx.google.com with ESMTPS id m11-20020a63580b000000b005ce0474353csi1760105pgb.238.2023.12.23.21.08.19 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 23 Dec 2023 21:08:19 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-10641-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b="faJ/Md4k"; spf=pass (google.com: domain of linux-kernel+bounces-10641-ouuuleilei=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) smtp.mailfrom="linux-kernel+bounces-10641-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 71D83282B3B for <ouuuleilei@gmail.com>; Sun, 24 Dec 2023 05:08:19 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 704206AA1; Sun, 24 Dec 2023 05:07:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="faJ/Md4k" X-Original-To: linux-kernel@vger.kernel.org Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B35DE46B1; Sun, 24 Dec 2023 05:07:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1703394433; x=1734930433; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=sULywt8zsfoKfsRdUyCgR8KmLm7/3YpeQ7pKzjInUqY=; b=faJ/Md4kMjMI7WdaQCpptBBy4ncj3fq1jgV9gJlv9g7aI/9Lb4JvBNra E4aPZP94Xw899iDju72tpNt/DSD3g4eiXm/A5UzBbK+bNBqmHX47/2N1R 7JCeoJGqDlZdFCc6ik4Kl3TJtCaroR8LyP86mxKCrSuc2zVvJxIuoTjp0 lUBafoPOe07LaUTpYjWSx+tn3xogOM13nYFH0YM5CqVCYxXeOY7GovBu5 +dba+o4GQha0pcoXhPHJ+8PpnYSu1mh2whvTCuOPHpdG/PKMGcMNmDUK9 sjq5odcy8FKewLdrWPRx1K49LAGsiTMk7vbrotsm6A9Z8iYpPsnIC1oEY g==; X-IronPort-AV: E=McAfee;i="6600,9927,10933"; a="399005142" X-IronPort-AV: E=Sophos;i="6.04,300,1695711600"; d="scan'208";a="399005142" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Dec 2023 21:07:13 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10933"; a="780982582" X-IronPort-AV: E=Sophos;i="6.04,300,1695711600"; d="scan'208";a="780982582" Received: from ply01-vm-store.bj.intel.com ([10.238.153.201]) by fmsmga007.fm.intel.com with ESMTP; 23 Dec 2023 21:07:11 -0800 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 Subject: [RFC PATCH v6 4/4] iommu/vt-d: break out devTLB invalidation if target device is gone Date: Sun, 24 Dec 2023 00:06:57 -0500 Message-Id: <20231224050657.182022-5-haifeng.zhao@linux.intel.com> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20231224050657.182022-1-haifeng.zhao@linux.intel.com> References: <20231224050657.182022-1-haifeng.zhao@linux.intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1786138591007864474 X-GMAIL-MSGID: 1786138591007864474 |
Series |
fix vt-d hard lockup when hotplug ATS capable device
|
|
Commit Message
Ethan Zhao
Dec. 24, 2023, 5:06 a.m. UTC
to fix the rare case, the in-process safe_removal unpluged device could
be supprise_removed anytime, thus check the target device state if it
is gone, don't wait for the completion/timeout anymore. it might cause
hard lockup or system hang
Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
---
drivers/iommu/intel/dmar.c | 7 +++++++
1 file changed, 7 insertions(+)
Comments
On Sun, Dec 24, 2023 at 12:06:57AM -0500, Ethan Zhao wrote: > --- a/drivers/iommu/intel/dmar.c > +++ b/drivers/iommu/intel/dmar.c > @@ -1423,6 +1423,13 @@ 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) { > + /* > + * if the devTLB invalidation target device is gone, don't wait > + * anymore, it might take up to 1min+50%, causes system hang. > + */ > + if (type == QI_DIOTLB_TYPE && iommu->flush_target_dev) > + if (!pci_device_is_present(to_pci_dev(iommu->flush_target_dev))) > + break; As a general approach, this is much better now. Please combine the nested if-clauses into one. Please amend the code comment with a spec reference, i.e. "(see Implementation Note in PCIe r6.1 sec 10.3.1)" so that readers of the code know where the magic number "1min+50%" is coming from. Is flush_target_dev guaranteed to always be a pci_dev? I'll let iommu maintainers comment on whether storing a flush_target_dev pointer is the right approach. (May store a back pointer from struct intel_iommu to struct device_domain_info?) Maybe move the "to_pci_dev(iommu->flush_target_dev)" lookup outside the loop to avoid doing this over and over again? I think we still have a problem here if the device is not removed but simply takes a long time to respond to Invalidate Requests (as it is permitted to do per the Implementation Note). We'll busy-wait for the completion and potentially run into the watchdog's time limit again. So I think you or someone else in your org should add OKRs to refactor the code so that it sleeps in-between polling for Invalidate Completions (instead of busy-waiting with interrupts disabled). Thanks, Lukas
On 12/24/2023 6:47 PM, Lukas Wunner wrote: > On Sun, Dec 24, 2023 at 12:06:57AM -0500, Ethan Zhao wrote: >> --- a/drivers/iommu/intel/dmar.c >> +++ b/drivers/iommu/intel/dmar.c >> @@ -1423,6 +1423,13 @@ 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) { >> + /* >> + * if the devTLB invalidation target device is gone, don't wait >> + * anymore, it might take up to 1min+50%, causes system hang. >> + */ >> + if (type == QI_DIOTLB_TYPE && iommu->flush_target_dev) >> + if (!pci_device_is_present(to_pci_dev(iommu->flush_target_dev))) >> + break; > As a general approach, this is much better now. > > Please combine the nested if-clauses into one. That would be harder to read ? > Please amend the code comment with a spec reference, i.e. > "(see Implementation Note in PCIe r6.1 sec 10.3.1)" > so that readers of the code know where the magic number "1min+50%" > is coming from. Yup. > > Is flush_target_dev guaranteed to always be a pci_dev? yes, as Baolu said, only PCI and ATS capable device supports devTLB invalidation operation, this is checked by its caller path. > > I'll let iommu maintainers comment on whether storing a flush_target_dev > pointer is the right approach. (May store a back pointer from > struct intel_iommu to struct device_domain_info?) One of them, wonder which one is better, but device_domain_info is still per device...seems no good to back it there. > > Maybe move the "to_pci_dev(iommu->flush_target_dev)" lookup outside the > loop to avoid doing this over and over again? hmm. that is a macro renam of container_of(), exactly, doesn't matter. right ? > > I think we still have a problem here if the device is not removed > but simply takes a long time to respond to Invalidate Requests > (as it is permitted to do per the Implementation Note). We'll > busy-wait for the completion and potentially run into the watchdog's > time limit again. So I think you or someone else in your org should > add OKRs to refactor the code so that it sleeps in-between polling refactor code would be long story, so far still a quick fix for the issue. and I think developers have other justifiction or conern about the non-sync version, once again, thanks for your comment. regards, Ethan > for Invalidate Completions (instead of busy-waiting with interrupts > disabled). > > Thanks, > > Lukas
On 12/24/2023 6:47 PM, Lukas Wunner wrote: > On Sun, Dec 24, 2023 at 12:06:57AM -0500, Ethan Zhao wrote: >> --- a/drivers/iommu/intel/dmar.c >> +++ b/drivers/iommu/intel/dmar.c >> @@ -1423,6 +1423,13 @@ 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) { >> + /* >> + * if the devTLB invalidation target device is gone, don't wait >> + * anymore, it might take up to 1min+50%, causes system hang. >> + */ >> + if (type == QI_DIOTLB_TYPE && iommu->flush_target_dev) >> + if (!pci_device_is_present(to_pci_dev(iommu->flush_target_dev))) >> + break; > As a general approach, this is much better now. > > Please combine the nested if-clauses into one. > > Please amend the code comment with a spec reference, i.e. > "(see Implementation Note in PCIe r6.1 sec 10.3.1)" > so that readers of the code know where the magic number "1min+50%" > is coming from. > > Is flush_target_dev guaranteed to always be a pci_dev? > > I'll let iommu maintainers comment on whether storing a flush_target_dev > pointer is the right approach. (May store a back pointer from > struct intel_iommu to struct device_domain_info?) > > Maybe move the "to_pci_dev(iommu->flush_target_dev)" lookup outside the > loop to avoid doing this over and over again? > > I think we still have a problem here if the device is not removed > but simply takes a long time to respond to Invalidate Requests > (as it is permitted to do per the Implementation Note). We'll If the hardware implenmentation didn't extend the PCIe spec, that is possible and horrible case for current synchromous queue model for ATS invalidation. but to wipe the concern and quote info not public here, perhaps not proper for me. Thanks, Ethan > busy-wait for the completion and potentially run into the watchdog's > time limit again. So I think you or someone else in your org should > add OKRs to refactor the code so that it sleeps in-between polling > for Invalidate Completions (instead of busy-waiting with interrupts > disabled). > > Thanks, > > Lukas >
diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c index 23cb80d62a9a..7a273ee80c49 100644 --- a/drivers/iommu/intel/dmar.c +++ b/drivers/iommu/intel/dmar.c @@ -1423,6 +1423,13 @@ 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) { + /* + * if the devTLB invalidation target device is gone, don't wait + * anymore, it might take up to 1min+50%, causes system hang. + */ + if (type == QI_DIOTLB_TYPE && iommu->flush_target_dev) + if (!pci_device_is_present(to_pci_dev(iommu->flush_target_dev))) + break; /* * We will leave the interrupts disabled, to prevent interrupt * context to queue another cmd while a cmd is already submitted