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=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no 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 E6625C55186 for ; Tue, 21 Apr 2020 23:44:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C0FCB206D5 for ; Tue, 21 Apr 2020 23:44:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=sifive.com header.i=@sifive.com header.b="S4jVdIKl" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726039AbgDUXoi (ORCPT ); Tue, 21 Apr 2020 19:44:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49986 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725822AbgDUXof (ORCPT ); Tue, 21 Apr 2020 19:44:35 -0400 Received: from mail-lf1-x142.google.com (mail-lf1-x142.google.com [IPv6:2a00:1450:4864:20::142]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id ADA2DC061BD3 for ; Tue, 21 Apr 2020 16:44:34 -0700 (PDT) Received: by mail-lf1-x142.google.com with SMTP id l11so140741lfc.5 for ; Tue, 21 Apr 2020 16:44:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=LQhtlsw1+T6reMAEhFyYFr2m8Qv3jouQUXJ8R8StFM8=; b=S4jVdIKlRsEiRGbtdIe4araEhiFTR6d+6KJqowgAjs2Aw6i7qWq9+rhzeWPW20Lwyv JhvwC5N0zDc010k/ZQ+WQZZcJc/egZZBd/t6Y4mO8M8oWzOIXkF4OQfuGBD1CUnwuMRh WNQD8NykXmqQHrYCtadUapSWEhcTe8FKW9ISCfS9PMxycykPibhwsI+BAq5OCxBcfoA/ BDaLL2qwtILN6XtP3yNpbDfpYwbZAR7KxfkG2tpluOBDpHiHbgo3jtNFD4cQDT8DQ/BP 2hSEMCAkRz9D49XieNfmFuymeONKUhx54J56YEwvhksgchmHLUUYrKXFeCcwmqD/lex6 z63w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=LQhtlsw1+T6reMAEhFyYFr2m8Qv3jouQUXJ8R8StFM8=; b=sNmCoRUPnRUBxJwQUghzjKSBZFTEFeyvBolL5+lkdf5K1pajJa+DC7xRt1t/uve3tO 2Y6ufnxNS96NfX4/KQqTgbZlTC0itC/swGBBcumjI2hGAnqvKU6HYOOO7CLIGstBzomD pdXCnNhWz6cV+QoRAnMvlUYp2wZwDZiMNT1UoLYS9J4YHp30RtE9cDShYQhGJdmvn9oz YJSoBkoYEYg48ZTzC5D3r0hIwgg6nt3c1gtwuSoTz+s6IRMaNZeWHxwg66yxY9lNaAD1 DoKCZaOb69x3V0lCI/6dz9IM3bUr+bUv5NfJsw7VfVo3TUR3y4NEvG9x95IKyZwJnRET b2iQ== X-Gm-Message-State: AGi0PuZoXmIMKqAAamDPhsTz4Bp0goiCOnMP+TIxc9mpNU6EK+x8+bAk Wd8KBOqvpbChLs18S71NCMo2mSrw3J7NVYo12LVQsg== X-Google-Smtp-Source: APiQypKb2BXKLr6nvOOiLiS7ib4bgDZHP5d7Dp+7WeRvGIhSc1y5feKb+/KNTb10Rd1TBtBcLrXe2xWQhq1x5j95PIQ= X-Received: by 2002:ac2:5e26:: with SMTP id o6mr14712374lfg.49.1587512672917; Tue, 21 Apr 2020 16:44:32 -0700 (PDT) MIME-Version: 1.0 References: <1587149322-28104-1-git-send-email-alan.mikhak@sifive.com> <20200418122123.10157ddd@why> <8a03b55223b118c6fc605d7204e01460@kernel.org> <20200421093928.4a600662@why> In-Reply-To: <20200421093928.4a600662@why> From: Alan Mikhak Date: Tue, 21 Apr 2020 16:44:21 -0700 Message-ID: Subject: Re: [PATCH] genirq/msi: Check null pointer before copying struct msi_msg To: Marc Zyngier Cc: Gustavo Pimentel , linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org, linux-pci , tglx@linutronix.de, Kishon Vijay Abraham I , Paul Walmsley Content-Type: text/plain; charset="UTF-8" Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Apr 21, 2020 at 1:39 AM Marc Zyngier wrote: > > On Mon, 20 Apr 2020 09:08:27 -0700 > Alan Mikhak wrote: > > Alan, > > > On Mon, Apr 20, 2020 at 2:14 AM Marc Zyngier wrote: > > > > > > On 2020-04-18 16:19, Gustavo Pimentel wrote: > > > > Hi Marc and Alan, > > > > > > > >> I'm not convinced by this. If you know that, by construction, these > > > >> interrupts are not associated with an underlying MSI, why calling > > > >> get_cached_msi_msg() the first place? > > > >> > > > >> There seem to be some assumptions in the DW EDMA driver that the > > > >> signaling would be MSI based, so maybe someone from Synopsys > > > >> (Gustavo?) > > > >> could clarify that. From my own perspective, running on an endpoint > > > >> device means that it is *generating* interrupts, and I'm not sure what > > > >> the MSIs represent here. > > > > > > > > Giving a little context to this topic. > > > > > > > > The eDMA IP present on the Synopsys DesignWare PCIe Endpoints can be > > > > configured and triggered *remotely* as well *locally*. > > > > For the sake of simplicity let's assume for now the eDMA was > > > > implemented > > > > on the EP and that is the IP that we want to configure and use. > > > > > > > > When I say *remotely* I mean that this IP can be configurable through > > > > the > > > > RC/CPU side, however, for that, it requires the eDMA registers to be > > > > exposed through a PCIe BAR on the EP. This will allow setting the SAR, > > > > DAR and other settings, also need(s) the interrupt(s) address(es) to be > > > > set as well (MSI or MSI-X only) so that it can signal through PCIe (to > > > > the RC and consecutively the associated EP driver) if the data transfer > > > > has been completed, aborted or if the Linked List consumer algorithm > > > > has > > > > passed in some linked element marked with a watermark. > > > > > > > > It was based on this case that the eDMA driver was exclusively > > > > developed. > > > > > > > > However, Alan, wants to expand a little more this, by being able to use > > > > this driver on the EP side (through > > > > pcitest/pci_endpoint_test/pci_epf_test) so that he can configure this > > > > IP > > > > *locally*. > > > > In fact, when doing this, he doesn't need to configure the interrupt > > > > address (MSI or MSI-X), because this IP provides a local interrupt line > > > > so that be connected to other blocks on the EP side. > > > > > > Right, so this confirms my hunch that the driver is being used in > > > a way that doesn't reflect the expected use case. Rather than > > > papering over the problem by hacking the core code, I'd rather see > > > the eDMA driver be updated to support both host and endpoint cases. > > > This probably boils down to a PCI vs non-PCI set of helpers. > > > > > > Alan, could you confirm whether we got it right? > > > > Thanks Marc and Gustavo. I appreciate all your comments and feedback. > > > > You both got it right. As Gustavo mentioned, I am trying to expand dw-edma > > for additional use cases. > > > > First new use case is for integration of dw-edma with pci-epf-test so the latter > > can initiate dma transfers locally from endpoint memory to host memory over the > > PCIe bus in response to a user command issued from the host-side command > > prompt using the pcitest utility. When the locally-initiated dma > > transfer completes > > in this use case on the endpoint side, dw-edma issues an interrupt to the local > > CPU on the endpoint side by way of a legacy interrupt and pci-epf-test issues > > an interrupt toward the remote host CPU across the PCIe bus by way of legacy, > > MSI, or possibly MSI-X interrupt. > > > > Second new use case is for integration of dw-edma with pci_endpoint_test > > running on the host CPU so the latter can initiate dma transfers locally from > > host-side in response to a user command issued from the host-side command > > prompt using the pcitest utility. This use case is for host systems that have > > Synopsys DesignWare PCI eDMA hardware on the host side. When the > > locally-initiated dma transfer completes in this use case on the host-side, > > dw-edma issues a legacy interrupt to its local host CPU and pci-epf-test running > > on the endpoint side issues a legacy, MSI, or possibly MSI-X interrupt > > across the > > PCIe bus toward the host CPU. > > > > When both the host and endpoint sides have the Synopsys DesignWare PCI > > eDMA hardware, more use cases become possible in which eDMA controllers > > from both systems can be engaged to move data. Embedded DMA controllers > > from other PCIe IP vendors may also be supported with additional dmaengine > > drivers under the Linux PCI Endpoint Framework with pci-epf-test, pcitest, and > > pci_endpoint_test suite as well as new PCI endpoint function drivers for such > > applications that require dma, for example nvme or virtio_net endpoint function > > drivers. > > > > I submitted a recent patch [1] and [2] which Gustavo ACk'd to decouple dw-edma > > from struct pci_dev. This enabled me to exercise dw-edma on some riscv host > > and endpoint systems that I work with. > > > > I will submit another patch to decouple dw-edma from struct msi_msg such > > that it would only call get_cached_msi_msg() on the host-side in its > > original use case with remotely initiated dma transfers using the BAR > > access method. > > > > The crash that I reported in __get_cached_msi_msg() is probably worth > > fixing too. It seems to be low impact since get_cached_msi_msg() > > seems to be called infrequently by a few callers. > > It isn't about the frequency of the calls, nor the overhead of this > function. It is about the fundamental difference between a wired > interrupt (in most case a level triggered one) and a MSI (edge by > definition on PCI). By making get_cached_msi_msg() "safe" to be called > for non-MSI IRQs, you hide a variety of design bugs which would > otherwise be obvious, like the one you are currently dealing with. > > Your eDMA driver uses MSI by construction, and is likely to use the MSI > semantics (edge triggering, coalescing, memory barrier). On the other > hand, your use case is likely to have interrupts with very different > semantics (level triggered, no barrier effect). Papering over these > differences is not the way to go, I'm afraid. > > I would recommend that you adapt the driver to have a separate > interrupt management for the non-MSI case, or at least not blindly use > MSI-specific APIs when not using them. Thanks Marc, I understand that the crash I reported here is to be kept to catch such issues. The design of dw-edma was correct for its original use case. Since I am the one trying to expand its use cases, I accept your recommendation and will see if I can offer patches that would be acceptable there. Regards, Alan > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...