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=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,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 CB074C43461 for ; Thu, 13 May 2021 21:20:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9447F61439 for ; Thu, 13 May 2021 21:20:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233427AbhEMVV6 (ORCPT ); Thu, 13 May 2021 17:21:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50728 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233360AbhEMVV6 (ORCPT ); Thu, 13 May 2021 17:21:58 -0400 Received: from mail-qk1-x731.google.com (mail-qk1-x731.google.com [IPv6:2607:f8b0:4864:20::731]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F0207C061574 for ; Thu, 13 May 2021 14:20:47 -0700 (PDT) Received: by mail-qk1-x731.google.com with SMTP id 197so26920614qkl.12 for ; Thu, 13 May 2021 14:20:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=wZOK6jXFL4VfBbxRN3Gyol/GRGXYnH9ZqWZPQUCK/vU=; b=ERkTalcomV8g7CELNOWW17v3v60GnywD0RuQQ8oBJ6B7gDiopk5H0G7jw0Ttr+7Nr+ q2FV6BSS8CZlnfI6q6/TE1a8E1ODsZyZlr/V2M9wPQNqX3nlwCrXbIWPeQy2FGERWMyO OCZFuG1ljrenAXOfMgETMJt49ucnmzm5g/Tol4E0y9JTvPZlQWkyxq60WbhVwUAO5Eod OZvID5vf4fBVZuGxwFM759PMwRIhL873+8P/lFfq9cbDMbd3LD4apn/dxbNdgvEy5gGW QEk6P5QmmjpEd4XuChtTtG9enk0GiPSZxdv9Q320cVodCmU+cNzncKLw7Gyux7o9O/Do 8WAw== 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=wZOK6jXFL4VfBbxRN3Gyol/GRGXYnH9ZqWZPQUCK/vU=; b=LlXHW2afMghCW1obHIL0PyDHSnyb8yEJ2R51/kl/LeXCDA7o1Yd2Mi0MduSpVSkVIB 4dEz+Vh7aBt57sswxsf0gObIBDJTEPovyGeiCikQgcykX53ecHHv5fa9k+GbPNV9OYEU EW7jNSD7pTEe8e/sOjAy8WiV5Qf34fczNHhoPd7IMCmaHColgCrsBy4qKoo67jx8qjN3 fpXP3/6C23/qv2TZtZTx1KpNzKuEQSTZ79dCi0RxO1hRtfJ/ncBa1WEILK1/NpieMyq4 OWuxcQ2fmetHUcB06NvUIKfQGQy9va90VT7gx4fRBJcYoWRnGPgO9Heq8SU+LMX06P1s UX6w== X-Gm-Message-State: AOAM5313uTMd1n3PCL9xAr0A+D0McaAquhErvTsFgIfP+gK0AvqRCuhG JiIkm7ImUYV4SR3wmMF4AOYwODSl0TUnzoShmlPsTA== X-Google-Smtp-Source: ABdhPJwkbwB6YVFBA0WjvHiJYl4N52PwXiqS9Yp4AlhIHVu3P1oMq6b0rAsfiy2G+Z/T0mvYFOZVEoSlj+ZZ2dnU7rc= X-Received: by 2002:a37:7d41:: with SMTP id y62mr40748866qkc.8.1620940847158; Thu, 13 May 2021 14:20:47 -0700 (PDT) MIME-Version: 1.0 References: <20210419165451.2176200-1-Jonathan.Cameron@huawei.com> <20210419165451.2176200-3-Jonathan.Cameron@huawei.com> <20210506215934.GJ1904484@iweiny-DESK2.sc.intel.com> <20210511175006.00007861@Huawei.com> In-Reply-To: <20210511175006.00007861@Huawei.com> From: Dan Williams Date: Thu, 13 May 2021 14:20:38 -0700 Message-ID: Subject: Re: [RFC PATCH v3 2/4] PCI/doe: Add Data Object Exchange support To: Jonathan Cameron Cc: Ira Weiny , linux-cxl@vger.kernel.org, Linux PCI , Bjorn Helgaas , Lorenzo Pieralisi , Ben Widawsky , Chris Browy , Linux ACPI , "Schofield, Alison" , Vishal L Verma , Linuxarm , Fangjian Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-acpi@vger.kernel.org On Tue, May 11, 2021 at 9:52 AM Jonathan Cameron wrote: > > On Thu, 6 May 2021 14:59:34 -0700 > Ira Weiny wrote: > > > On Tue, Apr 20, 2021 at 12:54:49AM +0800, Jonathan Cameron wrote: > > > + > > > +static int pci_doe_send_req(struct pci_doe *doe, struct pci_doe_exchange *ex) > > > +{ > > > + struct pci_dev *pdev = doe->pdev; > > > + u32 val; > > > + int i; > > > + > > > + /* > > > + * Check the DOE busy bit is not set. If it is set, this could indicate > > > + * someone other than Linux (e.g. firmware) is using the mailbox. Note > > > + * it is expected that firmware and OS will negotiate access rights via > > > + * an, as yet to be defined method. > > > + */ > > > + pci_read_config_dword(pdev, doe->cap + PCI_DOE_STATUS, &val); > > > + if (FIELD_GET(PCI_DOE_STATUS_BUSY, val)) > > > + return -EBUSY; > > > > In discussion with Dan we believe that user space could also be issuing > > commands and would potentially cause us to be locked out. > > > > We agree that firmware should be out of the way here and if it is blocking > > the OS there is not much we can do about it. > > > > However, if user space is using the mailbox we need to synchronize with them > > via pci_cfg_access_[try]lock(). This should avoid this EBUSY condition. > > Hi Ira, thanks for taking a look. > > So the question here is whether we can ever safely work with a > userspace that is accessing the DOE. I think the answer is no we can't. > > We'd have no way of knowing that userspace left the DOE in a clean state > without resetting every time we want to use it (which can take 1 second) > or doing significant sanity checking (can we tell if something is > in flight?). Note that if userspace and kernel were talking different > protocols nothing sensible could be done to prevent them receiving each > other's answers (unless you can rely on userspace holding the lock until > it is done - which you can't as who trusts userspace?) There is no ability for userpsace to lock out the kernel, only kernel locking out userspace. > You could do > something horrible like back off after peeking at the protocol to see > if it might be yours, but even that only works assuming the two are > trying to talk different protocols (talking the same protocol isn't allowed > but no way to enforce that using just pci_cfg_access_lock()). Wait why isn't pci_cfg_access_lock() sufficient? The userspace DOE transfer is halted, the kernel validates the state of DOE, does it's work and releases the lock. > I can't see a way to tell that the DOE might not have responded to an > earlier request. DOE busy indicates the write mailbox register cannot > receive data at the moment. If it's set then there is a message in > flight, but if it is not set there might still be a message in flight. > Busy only indicates if the write mailbox register can sink a request > which doesn't in general tell us anything about the underlying state. > > So if userspace sent a request then quit. Kernel driver would have > no way of knowing if the next response was due to the request it sent > or some earlier one (other than matching IDs) Note you aren't allowed > to have multiple requests for a single protocol in flight at the same > time. With just a lock you would have no way of preventing this. > > So we are back to every request the kernel sent having to be proceeded > by an abort and potentially a 1 second delay whilst some chunk of the > device firmware reboots. > > This came up in dicussion when Dan proposed the patch > [PATCH] PCI: Allow drivers to claim exclusive access to config regions > https://lore.kernel.org/linux-pci/161663543465.1867664.5674061943008380442.stgit@dwillia2-desk3.amr.corp.intel.com/ > Summarizing outcome of that thread. > > 1) Reads of DOE registers are always safe, so we shouldn't stop lspci > and similar accessing config space. > 2) You are on your own if any userspace writes to pci config space. > There are loads of ways it can break the system so it doesn't make much > sense to protect against one more. I'm not quite as enthusiastic about Greg's assertion that "we're already broken why not allow more breakage" as he was also the one supportive of /dev/mem restrictions in the face of obvious collisions. I'll circle back and say as much to Greg. My mistake was not realizing the write dependency in the protocol, so the pushback was warranted that the kernel does not need to block out all access. Given that /dev/mem is optionally disabled for userspace access outside of kernel-lockdown scenarios, I think it is reasonable to have the kernel disable config writes to a register block and the request of a driver. Consider that userspace can certainly trash the system by writing to the BAR registers, for example, but a non-malicious userspace has no reason to do that. Unfortunately DOE has some utility for a non-malicious userspace to access so there is a rationale to figure out a cooperation scheme. > > If there is a reason to provide a userspace interface to a DOE for a > device with a driver attached, then I would agree with Dan's suggestion > to use a proper driver for it. > > Dan briefly mentioned that temporary blocking might be needed. I'm guessing > that was to try and let userspace safely use the DOE. > > The driver would work fine ignoring busy entirely and would perhaps be > less confusing as a result. We reset the DOE at startup anyway and that > would clear existing busy. Any future times busy is set would have no > impact on the flow. If it simplifies the kernel implementation to assume single kernel-initiator then I think that's more than enough reason to block out userspace, and/or provide userspace a method to get into the kernel's queue for service.