Received: by 2002:a05:6a10:f3d0:0:0:0:0 with SMTP id a16csp4869056pxv; Tue, 6 Jul 2021 11:03:46 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzMiBsHpsC3Ige5eX9SI9IYgLEtrkRaQRCImvvjBBtmgOugHcsyzBJSiRsEKoimBqys8QhJ X-Received: by 2002:a92:d610:: with SMTP id w16mr15340476ilm.252.1625594626443; Tue, 06 Jul 2021 11:03:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1625594626; cv=none; d=google.com; s=arc-20160816; b=QHuUDX2goP7OtrxLuFw/1d98RrTRlzRR0fe796gPFXY88FbeexZpC4SI80mL0WBQN9 H7iUryI3q4Wrq6GJ7L4spsJ4jQw7t9IMk8xDfScwUC8EMFJT/kntPZ9VldRjEnQEoJKx TbEZth4F1gVf89CyKkkiAsxljJ4PIglC7HfU5HjKkHPbaHwlc3C04dQaTwIbcMHuyOeT qP1OkgcN7yVhuPOddpIK9Qwsb8C3nBNRTEmJytQqSIZadvgkvrGo08SbuuPK8C4NwdFA WBMr/WAv2SWJQuMPOLuRJJQCEMuthh5BFdXd15USpDb6HsvxXFpfVeEfYvsEj0nVtryG drSg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=ihbiI4Wev9wicBA9gW/xviwwW9K5DCmkWJzwOiyjrq8=; b=Wfazg653tKA7+BauKBGIU35q8MvaVeoneT4JYtLlUM9J7VFf14G2FdJyURjB0D50aP MyCLZBycoD/azUHbM4OCeH1P8OPshSrGtDa3tMKYbmcmrJiY1Qv9RMqKLXIRrQCu43Eb SWlrQa9tuLc6oWENMRJNU7LtrBJKsnQlPUoYX98ady36ZjIZz+YXARPEljKQlJiE8Xbo t+tB2TgLAE8X5kQr1lwU+YqtvUo5jV18UjqIHOrx3DsL0lMMOT1LCovdU/NOCNyunP1l AG8uMucUNnFDAKsYGuyoWdSxAH+Ua99gcV8R5Gp7Vk0c9OM6R6dRftdtKco6OxvBiIvC amQA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id l22si3235823jaj.38.2021.07.06.11.03.33; Tue, 06 Jul 2021 11:03:46 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230145AbhGFSFo (ORCPT + 99 others); Tue, 6 Jul 2021 14:05:44 -0400 Received: from foss.arm.com ([217.140.110.172]:47540 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229938AbhGFSFo (ORCPT ); Tue, 6 Jul 2021 14:05:44 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 60B191042; Tue, 6 Jul 2021 11:03:03 -0700 (PDT) Received: from [10.57.40.45] (unknown [10.57.40.45]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7E7FE3F5A1; Tue, 6 Jul 2021 11:03:02 -0700 (PDT) Subject: Re: [PATCH] iommu: Fallback to default setting when def_domain_type() callback returns 0 To: Kai-Heng Feng Cc: Joerg Roedel , will@kernel.org, "open list:IOMMU DRIVERS" , open list References: <20210706065106.271765-1-kai.heng.feng@canonical.com> From: Robin Murphy Message-ID: <55a31c97-a3f4-97d7-0663-13c15b68d5c0@arm.com> Date: Tue, 6 Jul 2021 19:02:57 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2021-07-06 17:21, Kai-Heng Feng wrote: > On Tue, Jul 6, 2021 at 5:27 PM Robin Murphy wrote: >> >> On 2021-07-06 07:51, Kai-Heng Feng wrote: >>> Commit 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted >>> device into core") not only moved the check for untrusted device to >>> IOMMU core, it also introduced a behavioral change by returning >>> def_domain_type() directly without checking its return value. That makes >>> many devices no longer use the default IOMMU setting. >>> >>> So revert back to the old behavior which defaults to >>> iommu_def_domain_type when driver callback returns 0. >>> >>> Fixes: 28b41e2c6aeb ("iommu: Move def_domain type check for untrusted device into core") >> >> Are you sure about that? From that same commit: >> >> @@ -1507,7 +1509,7 @@ static int iommu_alloc_default_domain(struct >> iommu_group *group, >> if (group->default_domain) >> return 0; >> >> - type = iommu_get_def_domain_type(dev); >> + type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type; >> >> return iommu_group_alloc_default_domain(dev->bus, group, type); >> } >> >> AFAICS the other two callers should also handle 0 correctly. Have you >> seen a problem in practice? > > Thanks for pointing out how the return value is being handled by the callers. > However, the same check is missing in probe_get_default_domain_type(): > static int probe_get_default_domain_type(struct device *dev, void *data) > { > struct __group_domain_type *gtype = data; > unsigned int type = iommu_get_def_domain_type(dev); > ... > } I'm still not following - the next line right after that is "if (type)", which means it won't touch gtype, and if that happens for every iteration, probe_alloc_default_domain() subsequently hits its "if (!gtype.type)" condition and still ends up with iommu_def_domain_type. This *was* one of the other two callers I was talking about (the second being iommu_change_dev_def_domain()), and in fact on second look I think your proposed change will actually break this logic, since it's necessary to differentiate between a specific type being requested for the given device, and a "don't care" response which only implies to use the global default type if it's still standing after *all* the appropriate devices have been queried. > I personally prefer the old way instead of open coding with ternary > operator, so I'll do that in v2. > > In practice, this causes a kernel panic when probing Realtek WiFi. > Because of the bug, dma_ops isn't set by probe_finalize(), > dma_map_single() falls back to swiotlb which isn't set and caused a > kernel panic. Hmm, but if that's the case, wouldn't it still be a problem anyway if the end result was IOMMU_DOMAIN_IDENTITY? I can't claim to fully understand the x86 swiotlb and no_iommu dance, but nothing really stands out to give me confidence that it handles the passthrough options correctly. Robin. > I didn't attach the panic log because the system simply is frozen at > that point so the message is not logged to the storage. > I'll see if I can find another way to collect the log and attach it in v2. > > Kai-Heng > >> >> Robin. >> >>> Signed-off-by: Kai-Heng Feng >>> --- >>> drivers/iommu/iommu.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 5419c4b9f27a..faac4f795025 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -1507,14 +1507,15 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group); >>> static int iommu_get_def_domain_type(struct device *dev) >>> { >>> const struct iommu_ops *ops = dev->bus->iommu_ops; >>> + unsigned int type = 0; >>> >>> if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) >>> return IOMMU_DOMAIN_DMA; >>> >>> if (ops->def_domain_type) >>> - return ops->def_domain_type(dev); >>> + type = ops->def_domain_type(dev); >>> >>> - return 0; >>> + return (type == 0) ? iommu_def_domain_type : type; >>> } >>> >>> static int iommu_group_alloc_default_domain(struct bus_type *bus, >>>