| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <85a3b2c4bd9b49d9a9e7bf88ee670119@HXTBJIDCEMVIW02.hxtcorp.net>
Date: Wed, 11 Apr 2018 03:18:56 +0000
From: "Wang, Dongsheng" <dongsheng.wang@...-semitech.com>
To: Robin Murphy <robin.murphy@....com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>
CC: "rjw@...ysocki.net" <rjw@...ysocki.net>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"hanjun.guo@...aro.org" <hanjun.guo@...aro.org>,
"sudeep.holla@....com" <sudeep.holla@....com>,
"Zheng, Joey" <yu.zheng@...-semitech.com>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: Re: [RFC PATCH 2/2] ACPI/IORT: use swiotlb_dma_ops when smmu
probe failed
> -----Original Message-----
> From: linux-acpi-owner@...r.kernel.org
> [mailto:linux-acpi-owner@...r.kernel.org] On Behalf Of Robin Murphy
> Sent: Monday, April 09, 2018 8:11 PM
> To: Wang, Dongsheng <dongsheng.wang@...-semitech.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@....com>
> Cc: rjw@...ysocki.net; gregkh@...uxfoundation.org; hanjun.guo@...aro.org;
> sudeep.holla@....com; Zheng, Joey <yu.zheng@...-semitech.com>;
> linux-acpi@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: [此邮件可能存在风险] Re: [RFC PATCH 2/2] ACPI/IORT: use
> swiotlb_dma_ops when smmu probe failed
>
> On 08/04/18 09:10, Wang, Dongsheng wrote:
> >
> >> -----Original Message-----
> >> From: Robin Murphy [mailto:robin.murphy@....com]
> >> Sent: Thursday, April 05, 2018 2:57 AM
> >> To: Lorenzo Pieralisi <lorenzo.pieralisi@....com>; Wang, Dongsheng
> >> <dongsheng.wang@...-semitech.com>
> >> Cc: rjw@...ysocki.net; gregkh@...uxfoundation.org;
> hanjun.guo@...aro.org;
> >> sudeep.holla@....com; Zheng, Joey <yu.zheng@...-semitech.com>;
> >> linux-acpi@...r.kernel.org; linux-kernel@...r.kernel.org
> >> Subject: [此邮件可能存在风险] Re: [RFC PATCH 2/2] ACPI/IORT: use
> >> swiotlb_dma_ops when smmu probe failed
> >>
> >> On 04/04/18 17:01, Lorenzo Pieralisi wrote:
> >>> [+cc Robin]
> >>>
> >>> On Thu, Mar 29, 2018 at 03:01:00AM -0700, Wang Dongsheng wrote:
> >>>> If SMMU probe failed, master should use swiotlb as dma ops.
> >>>> SMMU may probe failed with specified environment, so there
> >>>> are not any iommu resources in iommu_device_list.
> >>>>
> >>>> The master will always get EPROBE_DEFER from really_probe
> >>>> (dma_configure) but in fact SMMU has probe failed. The issue
> >>>> causes all of masters failed to be driven.
> >>
> >> Let's just take a step back - why is SMMU probe failing? That seems to
> >> be the primary issue here, because it implies that either your hardware,
> >> firmware or kernel is broken, any of which would make boot failure
> >> somewhat unsurprising anyway.
> >>
> > It's actually not a hardware issue. This is my test case, just return
> > -EINVAL in arm_smmu_device_probe. The HW
> probe(arm_smmu_device_hw_probe)
> > is just part of SMMU driver probe and the failure may be caused by SW. So
> > I design this case, just make sure even if SMMU probe failed that cause by SW,
> > the MASTER also can work. _Because of our SMMU default mode is bypass._
>
> I don't think it's particularly justifiable to make core API changes for
> the sake of contrived testcases. On real systems, the SMMU is a
> fundamental system component which is no more expected to fail probe
> than, say, the GIC, and as such if it *does* fail then further progress
> is on a best-effort basis at most.
Yes. Agree with you.
> Just because *your* system happens to work fine in this state doesn't make it true for every SMMU
> implementation and integration that Linux may ever run on.
:(, yes, this is my mistake.
>
> If you want the kernel to ignore an SMMU, either configure out the
> driver or don't describe that SMMU in firmware in the first place.
>
> >>> I added Robin to pick his brain. An alternative would consist
> >>> in using a bus notifier to prevent deferred probing once the SMMU
> >>> driver probing failed but that seems backwards given that a major
> >>> reason to move to deferred probing was to remove the bus notifiers
> >>> dependency in the first place.
> >>>
> >>> It seems to me this is both an OF/ACPI issue - it is not an IORT
> >>> only problem.
> >>
> >> Yes, this is just an instance of the general probe-deferral problem,
> >> e.g. once you have multiple dependencies it's possible to end up in a
> >> stalemate where everything including the IOMMU ends up on the deferred
> >> probe list with nothing to kick it and make progress again.
> >>
> >> Furthermore it seems to me that the whole premise in this patch is
> >> flawed,
> > Ditto. :)
> >
> >
> >> since even genuine probe failure may well be transient - just
> >> because one attempt failed doesn't mean a later attempt can't succeed.
> >> Thus "the most recent probe attempt failed" cannot be considered a
> >> fundamentally different state from "no driver is currently bound".
> >>
> > Agree, the genuine probe failure may well be transient. But there is
> > depend on SMMU probe(IOMMU instance) status. There are two situations:
> >
> > 1. MASTER probing, SMMU doesn't probe yet.
> > This case will match "the transient failure".
> > really_probe get an EPROBE_DEFER from IORT and the MASTER probe will
> be
> > delayed until SMMU probe successful.
> > 2. MASTER probing, SMMU probe has failed.
> > really_probe will always get an EPROBE_DEFER from IORT, because kernel
> > has build in SMMU driver.(iort_iommu_driver_enabled) And the master
> > never cannot do probe.
> >
> > The case 2 is I want to handle.
>
> Handle it by not deliberately breaking the SMMU driver. In all other
> cases, either re-triggering SMMU probe might make it succeed (i.e. the
> DL_DEV_PROBE_FAILED state is meaningless), or things are so broken that
> you're probably dead in the water anyway.
>
Drop this patch.
Thanks for your review.
Cheers,
-Dongsheng
> Robin.
>
> >
> > Cheers,
> > -Dongsheng
> >
> >> Robin.
> >>
> >>>
> >>> Lorenzo
> >>>
> >>>> Signed-off-by: Wang Dongsheng <dongsheng.wang@...-semitech.com>
> >>>> ---
> >>>> drivers/acpi/arm64/iort.c | 39
> >> +++++++++++++++++++++++++++++++++------
> >>>> 1 file changed, 33 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>>> index e2f7bdd..a6f4c27 100644
> >>>> --- a/drivers/acpi/arm64/iort.c
> >>>> +++ b/drivers/acpi/arm64/iort.c
> >>>> @@ -774,17 +774,45 @@ static int arm_smmu_iort_xlate(struct device
> >> *dev, u32 streamid,
> >>>> return ret;
> >>>> }
> >>>>
> >>>> -static inline bool iort_iommu_driver_enabled(u8 type)
> >>>> +static int iort_check_dev_dl_status(struct device *dev, void *data)
> >>>> {
> >>>> + struct fwnode_handle *fwnode = data;
> >>>> +
> >>>> + if (dev->fwnode != fwnode)
> >>>> + return 0;
> >>>> +
> >>>> + if (dev->links.status == DL_DEV_PROBE_FAILED)
> >>>> + return -ENODEV;
> >>>> +
> >>>> + return -EPROBE_DEFER;
> >>>> +}
> >>>> +
> >>>> +static int iort_iommu_driver_enabled(u8 type, struct fwnode_handle
> >> *fwnode)
> >>>> +{
> >>>> + bool buildin;
> >>>> + int ret;
> >>>> +
> >>>> switch (type) {
> >>>> case ACPI_IORT_NODE_SMMU_V3:
> >>>> - return IS_BUILTIN(CONFIG_ARM_SMMU_V3);
> >>>> + buildin = IS_BUILTIN(CONFIG_ARM_SMMU_V3);
> >>>> + break;
> >>>> case ACPI_IORT_NODE_SMMU:
> >>>> - return IS_BUILTIN(CONFIG_ARM_SMMU);
> >>>> + buildin = IS_BUILTIN(CONFIG_ARM_SMMU);
> >>>> + break;
> >>>> default:
> >>>> pr_warn("IORT node type %u does not describe an
> SMMU\n",
> >> type);
> >>>> - return false;
> >>>> + buildin = false;
> >>>> }
> >>>> +
> >>>> + if (!buildin)
> >>>> + return -ENODEV;
> >>>> +
> >>>> + ret = bus_for_each_dev(&platform_bus_type, NULL, fwnode,
> >>>> + iort_check_dev_dl_status);
> >>>> + if (!ret)
> >>>> + return -EPROBE_DEFER;
> >>>> +
> >>>> + return ret;
> >>>> }
> >>>>
> >>>> #ifdef CONFIG_IOMMU_API
> >>>> @@ -919,8 +947,7 @@ static int iort_iommu_xlate(struct device *dev,
> >> struct acpi_iort_node *node,
> >>>> */
> >>>> ops = iommu_ops_from_fwnode(iort_fwnode);
> >>>> if (!ops)
> >>>> - return iort_iommu_driver_enabled(node->type) ?
> >>>> - -EPROBE_DEFER : -ENODEV;
> >>>> + return iort_iommu_driver_enabled(node->type, iort_fwnode);
> >>>>
> >>>> return arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
> >>>> }
> >>>> --
> >>>> 2.7.4
> >>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists