From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81DEAC433DB for ; Fri, 22 Jan 2021 00:51:28 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1DF7622527 for ; Fri, 22 Jan 2021 00:51:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1DF7622527 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=skkBdelYLLpCxChsT1AEomw6DcRH4evcAZWAWtUme00=; b=Tp8i/S0cdAR3+9Cs4tZb+mRU6 +DZHmQF/f+M0JXtV3ZGZlCgv47qIUzVHYUHJAYujkhgtQ8tqOlL2kZ0+my8sZAULw2Kd5WjfAPXD8 G24mL6rwLd4dSvKo47HYS/fzwyN8Gm+UokUglojubbGmmkzSsL2IJh+Ka39M1Rzo31L/jn7rgvJkV edEy8AEcCP/1DWre7Qxkf7wjGUp3OYNUw5aS9qdRVkRG4rrTh1aSKbDW1W8aHVcL7D4pDaXLhtpA8 qBS/jOL9UGxWDIo8ymmDyqxHKMlPeyHLww8gJ/e7bE8jJ/NeAxosGO6Vp1AfP6jkpFBc+9b0vClZV ias3X4fdA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2kdp-0007J5-SR; Fri, 22 Jan 2021 00:49:33 +0000 Received: from mail-pj1-f46.google.com ([209.85.216.46]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1l2kdl-0007IT-Sj for linux-arm-kernel@lists.infradead.org; Fri, 22 Jan 2021 00:49:31 +0000 Received: by mail-pj1-f46.google.com with SMTP id b5so2794818pjl.0 for ; Thu, 21 Jan 2021 16:49:29 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=6AIhNuCe/1GTY8tOy/5C+s0wH6veWybr8jaw1m3E9Hg=; b=O9Ng0ZQ0KWu3HGu+j/A8abw89Y6uMK0qDFzACiEYXhKkteeExzYoguk/ECwQzIwy+z gW3jOaCxtpvwTOKGeHJrvhtP22xunr5z05qczJyAEedFOhUNwB6DoUqIr+fqhRxPzMZR bXdG+DhKEKhkvwpWpYGWo+lpMgd1zJXUacwK3RkdCFMr6i7FCkBtZtmEYqD0rCzqAvG5 dz/eVxc+fmE9Denhq2caafrGiBxd5MfDVYLLc067CaMIYUJ5IgVOtaHP6lgLkFyqg8TW BjNWay5MGwdPu6Ku57OOjCePR1PaztDLwpop4R8LHibdQ11BbprK7XSZpkeDCIM/Ci+T 7xwA== X-Gm-Message-State: AOAM530kesEUFvrVRJ1W1U4w4ehupjuHqfnpb46RCxd4kwgF+/r+RBun 0Y8tnJR92t5z02Cu7al0ATo= X-Google-Smtp-Source: ABdhPJyckVCo7YfZkh7bWlgUT2Dd/TuGmMbOaeo7nbGqTHZoBJ4BpFQjb57/oG7YRMBwj4tzAFI4dA== X-Received: by 2002:a17:90b:602:: with SMTP id gb2mr2284162pjb.170.1611276568552; Thu, 21 Jan 2021 16:49:28 -0800 (PST) Received: from localhost ([2601:647:5b00:1162:1ac0:17a6:4cc6:d1ef]) by smtp.gmail.com with ESMTPSA id q22sm5863545pgi.66.2021.01.21.16.49.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 21 Jan 2021 16:49:27 -0800 (PST) Date: Thu, 21 Jan 2021 16:49:25 -0800 From: Moritz Fischer To: Robin Murphy Subject: Re: [PATCH] ACPI/IORT: Do not blindly trust DMA masks from firmware Message-ID: References: <20210121191612.90387-1-mdf@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210121_194929_942739_25CF17F9 X-CRM114-Status: GOOD ( 50.85 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: lorenzo.pieralisi@arm.com, sudeep.holla@arm.com, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, Moritz Fischer , moritzf@google.com, guohanjun@huawei.com, will@kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Robin, On Thu, Jan 21, 2021 at 11:15:05PM +0000, Robin Murphy wrote: > On 2021-01-21 21:17, Moritz Fischer wrote: > > Robin, > > > > On Thu, Jan 21, 2021 at 08:08:42PM +0000, Robin Murphy wrote: > > > On 2021-01-21 19:16, Moritz Fischer wrote: > > > > Address issue observed on real world system with suboptimal IORT table > > > > where DMA masks of PCI devices would get set to 0 as result. > > > > > > > > iort_dma_setup() would query the root complex' IORT entry for a DMA > > > > mask, and use that over the one the device has been configured with > > > > earlier. > > > > > > > > Ideally we want to use the minimum mask of what the IORT contains for > > > > the root complex and what the device was configured with, but never 0. > > > > > > > > Fixes: 5ac65e8c8941 ("ACPI/IORT: Support address size limit for root complexes") > > > > Signed-off-by: Moritz Fischer > > > > --- > > > > Hi all, > > > > > > > > not sure I'm doing this right, but I think the current behavior (while a > > > > corner case) seems to also fail for 32 bit devices if the IORT specifies > > > > 64 bit. It works on my test system now with a 32 bit device. > > > > > > I suppose it could go wrong if it's an old driver that doesn't explicitly > > > set its own masks and assumes they will always be 32-bit. Technically we'd > > > consider that the driver's fault these days, but there's a lot of legacy > > > around still. > > > > Huh, ok :) That's news to me. On my system I had three devices running > > into this, so yeah I think it's quite common. > > Indeed, I'm sure there are plenty of drivers that haven't been touched in > decades because they're complete and working, and back then they were > allowed to make that assumption. > > > If that's the official stance I can send patches for the drivers in > > question :) > > It's certainly good practice, especially for older devices that are still > popular enough to see use on the increasing variety of new systems. Some > people are still using the infamous arm64 platform where all the RAM is > above 40 bits, for instance, and who knows how creative system designers > might continue to be, so better to give the driver a chance to bail out of > probing in the rare event that explicitly setting its 32-bit masks *does* > fail, rather than let it assume DMA should work then get confused when it > doesn't. > > > > > Open to suggestions for better solutions (and maybe the > > > > nc_dma_get_range() should have the same sanity check?) > > > > > > Honestly the more I come back to this, the more I think we should give up > > > trying to be clever and just leave the default masks alone beyond the > > > initial "is anything set up at all?" sanity checks. Setting the bus limit is > > > what really matters these days, and should be sufficient to encode any > > > genuine restriction. There's certainly no real need to widen the default > > > masks above 32 bits just because firmware suggests so, since the driver > > > should definitely be calling dma_set_mask() and friends later if it's > > > > 32-bit capable anyway. > > > > > > > Thanks, > > > > Moritz > > > > > > > > --- > > > > drivers/acpi/arm64/iort.c | 11 ++++++++--- > > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > > > > index d4eac6d7e9fb..c48eabf8c121 100644 > > > > --- a/drivers/acpi/arm64/iort.c > > > > +++ b/drivers/acpi/arm64/iort.c > > > > @@ -1126,6 +1126,11 @@ static int rc_dma_get_range(struct device *dev, u64 *size) > > > > rc = (struct acpi_iort_root_complex *)node->node_data; > > > > + if (!rc->memory_address_limit) { > > > > + dev_warn(dev, "Root complex has broken memory_address_limit\n"); > > > > > > Probably warrants a FW_BUG in there. > > > > > > > + return -EINVAL; > > > > + } > > > > + > > > > *size = rc->memory_address_limit >= 64 ? U64_MAX : > > > > 1ULL<memory_address_limit; > > > > @@ -1172,9 +1177,9 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > > > > */ > > > > end = dmaaddr + size - 1; > > > > mask = DMA_BIT_MASK(ilog2(end) + 1); > > > > - dev->bus_dma_limit = end; > > > > - dev->coherent_dma_mask = mask; > > > > - *dev->dma_mask = mask; > > > > + dev->bus_dma_limit = min_not_zero(dev->bus_dma_limit, end); > > > > > > This doesn't need to change, since the default bus limit is 0 anyway (and > > > that means "no limit"). > > Ok, I'll drop this. > > > > > > > + dev->coherent_dma_mask = min_not_zero(dev->coherent_dma_mask, mask); > > > > + *dev->dma_mask = min_not_zero(*dev->dma_mask, mask); > > > > I'll keep those two? > > Well... > > > > AFAICS the only way an empty mask could get here now is from > > > nc_dma_get_range(), so I'd rather see a consistent warning there than just > > > silently start working around that too. > > > > In my case the empty mask came from the pci dev branch returning a size > > of 1. (1 << 0). > > In fact I think I was too hasty in saying even that - it actually looks like > you can't get a mask of 0 either way. If memory_address_limit is 0, then > size is 1, dmaaddr is 0 (since acpi_dma_get_range() had to fail in the first > place), so end is 0, so mask is DMA_BIT_MASK(0 + 1), which is 1. So > min_not_zero() still does nothing :/ The min_not_zero() is to not go from 32 to > 32 if firmware sets it to say 33? If you prefer we can change it to min() instead? IMHO we should never widen the mask only narrow it, agreed? > > > I'll replace the dev_warn() with a pr_warn(FW_BUG ...) for both > > {nc,rc}_dma_get_range() cases then? > > Yes, I think it's worth being consistent. And then we can't ever get past > the "if (!ret)" condition without a valid size, so we definitely don't need > to touch anything inside it. And by "valid" I mean that if someone goes to > the effort of filling in that field with even a 1, then by 'eck we're > givin'em the 1-bit DMA limit they asked for! > > > > Of course IORT doesn't say these fields are optional (other than the lack of > > > a root complex limit in older table versions), so we're giving bad firmware > > > a pass to never be fixed, ho hum... > > > > I think if we yell loud enough (like FW_BUG) that'll get people's > > attention? > > Ha! I've got a machine where MSIs don't work (let alone SMMU translation...) > because some of the device mapping offsets are pointing into random parts of > the IORT like the middle of other nodes' headers. If it boots to a prompt at > all, someone somewhere will be happy to ship it ;) Whoa :D Thanks for the feedback, Moritz _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel