From: Dan Carpenter <dan.carpenter@linaro.org>
To: Ethan Zhao <haifeng.zhao@linux.intel.com>
Cc: baolu.lu@linux.intel.com, bhelgaas@google.com,
robin.murphy@arm.com, jgg@ziepe.ca, kevin.tian@intel.com,
dwmw2@infradead.org, will@kernel.org, lukas@wunner.de,
yi.l.liu@intel.com, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid
Date: Fri, 23 Feb 2024 09:08:51 +0300 [thread overview]
Message-ID: <039a19e5-d1ff-47ae-aa35-3347c08acc13@moroto.mountain> (raw)
In-Reply-To: <2d1788da-521c-4531-a159-81d2fb801d6c@linux.intel.com>
On Fri, Feb 23, 2024 at 10:29:28AM +0800, Ethan Zhao wrote:
> > > @@ -1326,6 +1336,21 @@ static int qi_check_fault(struct intel_iommu *iommu, int index, int wait_index)
> > > head = (head - 2 + QI_LENGTH) % QI_LENGTH;
> > > } while (head != tail);
> > > + /*
> > > + * If got ITE, we need to check if the sid of ITE is one of the
> > > + * current valid ATS invalidation target devices, if no, or the
> > > + * target device isn't presnet, don't try this request anymore.
> > > + * 0 value of ite_sid means old VT-d device, no ite_sid value.
> > > + */
> > This comment is kind of confusing.
>
> Really confusing ? this is typo there, resnet-> "present"
>
Reading this comment again, the part about zero ite_sid values is
actually useful, but what does "old" mean in "old VT-d device". How old
is it? One year old?
> >
> > /*
> > * If we have an ITE, then we need to check whether the sid of the ITE
> > * is in the rbtree (meaning it is probed and not released), and that
> > * the PCI device is present.
> > */
> >
> > My comment is slightly shorter but I think it has the necessary
> > information.
> >
> > > + if (ite_sid) {
> > > + dev = device_rbtree_find(iommu, ite_sid);
> > > + if (!dev || !dev_is_pci(dev))
> > > + return -ETIMEDOUT;
> > -ETIMEDOUT is weird. The callers don't care which error code we return.
> > Change this to -ENODEV or something
>
> -ETIMEDOUT means prior ATS invalidation request hit timeout fault, and the
> caller really cares about the returned value.
>
I don't really care about the return value and if you say it should be
-ETIMEDOUT, then you're the expert. However, I don't see anything in
linux-next which cares about the return values except -EAGAIN.
This function is only called from qi_submit_sync() which checks for
-EAGAIN. Then I did a git grep.
$ git grep qi_submit_sync
drivers/iommu/intel/dmar.c:int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/dmar.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/iommu.h:int qi_submit_sync(struct intel_iommu *iommu, struct qi_desc *desc,
drivers/iommu/intel/iommu.h: * Options used in qi_submit_sync:
drivers/iommu/intel/irq_remapping.c: return qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/pasid.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/svm.c: qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN);
drivers/iommu/intel/svm.c: qi_submit_sync(iommu, &desc, 1, 0);
drivers/iommu/intel/svm.c: qi_submit_sync(iommu, &desc, 1, 0);
Only qi_flush_iec() in irq_remapping.c cares about the return. Then I
traced those callers back and nothing cares about -ETIMEOUT.
Are you refering to patches that haven't ben merged yet?
> >
> > > + pdev = to_pci_dev(dev);
> > > + if (!pci_device_is_present(pdev) &&
> > > + ite_sid == pci_dev_id(pci_physfn(pdev)))
> > The && confused me, but then I realized that probably "ite_sid ==
> > pci_dev_id(pci_physfn(pdev))" is always true. Can we delete that part?
>
> Here is the fault handling, just double confirm nothing else goes wrong --
> beyond the assumption.
>
Basically for that to ever be != it would need some kind of memory
corruption? I feel like in that situation, the more conservative thing
is to give up. If the PCI device is not present then just give up.
regards,
dan carpenter
next prev parent reply other threads:[~2024-02-23 6:08 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 9:02 [PATCH v13 0/3] fix vt-d hard lockup when hotplug ATS capable device Ethan Zhao
2024-02-22 9:02 ` [PATCH v13 1/3] PCI: make pci_dev_is_disconnected() helper public for other drivers Ethan Zhao
2024-02-22 12:54 ` Baolu Lu
2024-02-23 6:47 ` Dan Carpenter
2024-02-23 7:35 ` Ethan Zhao
2024-02-26 23:05 ` Bjorn Helgaas
2024-02-29 1:58 ` Ethan Zhao
2024-02-29 22:33 ` Bjorn Helgaas
2024-03-01 2:03 ` Ethan Zhao
2024-02-29 22:26 ` Bjorn Helgaas
2024-02-22 9:02 ` [PATCH v13 2/3] iommu/vt-d: don't issue ATS Invalidation request when device is disconnected Ethan Zhao
2024-02-22 9:02 ` [PATCH v13 3/3] iommu/vt-d: improve ITE fault handling if target device isn't valid Ethan Zhao
2024-02-22 11:24 ` Dan Carpenter
2024-02-23 2:29 ` Ethan Zhao
2024-02-23 6:08 ` Dan Carpenter [this message]
2024-02-23 7:32 ` Ethan Zhao
2024-02-23 8:19 ` Dan Carpenter
2024-02-26 2:48 ` Ethan Zhao
2024-02-26 20:00 ` Ethan Zhao
2024-02-27 4:54 ` Dan Carpenter
2024-02-26 22:52 ` Bjorn Helgaas
2024-02-27 2:30 ` Ethan Zhao
2024-02-27 16:29 ` Bjorn Helgaas
2024-02-28 9:00 ` Ethan Zhao
2024-02-26 11:48 ` kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=039a19e5-d1ff-47ae-aa35-3347c08acc13@moroto.mountain \
--to=dan.carpenter@linaro.org \
--cc=baolu.lu@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=dwmw2@infradead.org \
--cc=haifeng.zhao@linux.intel.com \
--cc=iommu@lists.linux.dev \
--cc=jgg@ziepe.ca \
--cc=kevin.tian@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
--cc=yi.l.liu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).