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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1FD73C982DE for ; Sat, 19 Sep 2026 01:19:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc: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=f07OJapwP7n4iN5o+VTBr7NTU5yudYQ1EeJavoRTsAU=; b=DtWVJeb6kHBNKsAnTCqPfE8ijv TrO72VbXkYJlgCBHAroVQSrkf2G5r+UVVf4lndai/VxOIakeEPy6Hwzj5hz70XJBEMjhB7omrRIet WWxhqXfZvQ6UKr3e46ndsJrY793GjR6L7FlHyhlb0q4kjLnMIXGFvALiMc+Uy1g6lybI7J0AWqrFf ZA74efo2h363f2XyjBUx1fffLxd+TiFzqU0Jxwr+VZin/O2b/pH51y5XN6QGf/Q+tR4rHUqQynyoE MXq5xqcIBqa85j+v1JnzmdKNEoZ3DBxITjuJK0aeYH9v3krHNrycXofROY1dr7VpRjaTxxxvwAL2Z x7WWqmZw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1x7jjL-0000000Fohj-1l0x; Sat, 19 Sep 2026 01:19:03 +0000 Received: from fhigh-b4-smtp.messagingengine.com ([202.12.124.155]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1x7jjH-0000000Fogi-3Jhd for kexec@lists.infradead.org; Sat, 19 Sep 2026 01:19:01 +0000 Received: from phl-compute-05.internal (phl-compute-05.internal [10.202.2.45]) by mailfhigh.stl.internal (Postfix) with ESMTP id A06117A00F7; Fri, 18 Sep 2026 21:18:54 -0400 (EDT) Received: from phl-frontend-03 ([10.202.2.162]) by phl-compute-05.internal (MEProxy); Fri, 18 Sep 2026 21:18:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=shazbot.org; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to; s=fm3; t=1789780734; x=1789867134; bh=f07OJapwP7n4iN5o+VTBr7NTU5yudYQ1EeJavoRTsAU=; b= GfU3B8bzgvN3baH4hN7kVi3lK+nwsYLBoo8cuedw+XfuLRi7C2LDOr2zGEqpyItk eQoRHmiP7gdOYs4G+8gpNILRD1NXFmda7hFAMawTqLZlswHVgxBzxcrjACt0t1mW tgO9e4sqsVcqDI7Uw0UTvZ4BtfzKqUbSP7IPivwb7zwTOCGwHGaX3v7NqsU2WNMo omFYls3/du51cPE6bB8koXlKIW2lNpV7owGIhw1oTppNYkBPxiEtBttdBwAWRz7m 7efK2Z7EnC+/QZYvSIBHjfWU/Fau93vviQy345fFfOQMwK3BZuTz+Ov0bDw3uj6c DluMmuq+++tmqRxCLPnIow== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:subject:subject:to:to:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1789780734; x= 1789867134; bh=f07OJapwP7n4iN5o+VTBr7NTU5yudYQ1EeJavoRTsAU=; b=I ThjHthLB53HlQOXiE/B8C3BzG82k34YmTXWtypVrrj1sjAl98hJPFZ/PA3L3KWGO zMZCQjnbil2KmYaPnS/La+VdZhooz7N38ZnWV/G8dAla26+qRY3PLTlOjvlFBBJ1 +mx2dQybAiByY9eqhXx2HduZXZsj0yzqh71wvT0igfSONIw0bX5t6aBcbIaDHpIe LU0HR6/dtgJhImLLqevRoUrkgh2jX6IqChVBAZ228wP8225XjZvUHi0L0XgCug8V a+S8pOhSrolB8dq+eSA1w75N5gjmio1xONFAojmsmVDmsDxusNdJBl4tLAEnpq+4 7BZb90CwahB0U5AxiriZg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: dmFkZTFsi21agycuHwmyYJyjldjG/6AIpBHOLfkjZs+7QWdmfXu3roB/D7PU/BJ5GavaBQ AU4Zx86E3Ar9fkk1i41sLL79DnqebEpC8pDQ6c5ESWbp577Kk3af/AT9f5eoXlX0Lw/Mwn FcwH1rl6rXf0ysQZescsD7HR4aD9cokOy6JHc/odoFo5LW74f6xcHPp7/3IXor/7lPRlg4 yVxI+ykZmTAbWVw9hIWIcQv2dY+r8MD8Wy3QjrkvIeBpxUqwnvjaOFAxf3/hHEtPNnLxjs ds98IeB4eQbJsmnMJLcg38qvtl3mGH/yUl4cVyev259XESUEPVRdDW+b9fBfBPjDcrgVI+ BVIlyr12V9nYqdU5Zo1weZP/CyuOjwUSS3xmHA+sFvyIYwVdXKsVOOrcJLgBflm22kdNyO OefOMArnTas4zRxUhJgbvX1OAkCf+nqcf4j6frGTX205opZyUxnl5xlsaBa3ibVeWM9erA ePj0w2q/rKlqCJNRlmxVdXJwfsxkckiDWN/E3g0t1fXf8kv9cv+RYjAWIatFvyc6rL8rxQ u3X3RlPxpeu3mB7lNH5rrGoxRaqoxgs4aW/La6pfZxaymhcUewyZeQ84Z/YefVcxKQhX9/ ib/pnrw0CtPk1XE16IBpOJ7VT/SFvHGcnz/4zucdY+TDS4kvL8ePHyTyeVYg X-ME-Proxy: Feedback-ID: i03f14258:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 18 Sep 2026 21:18:48 -0400 (EDT) Date: Fri, 18 Sep 2026 19:18:46 -0600 From: Alex Williamson To: David Matlack Cc: kexec@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linux-pci@vger.kernel.org, Adithya Jayachandran , Alexander Graf , Bjorn Helgaas , Chris Li , David Rientjes , Jacob Pan , Jason Gunthorpe , Jonathan Corbet , Josh Hilke , Leon Romanovsky , Lukas Wunner , Mike Rapoport , Parav Pandit , Pasha Tatashin , Pranjal Shrivastava , Pratyush Yadav , Randy Dunlap , Saeed Mahameed , Samiullah Khawaja , Shuah Khan , Vipin Sharma , William Tu , Yi Liu , alex@shazbot.org Subject: Re: [PATCH v9 08/13] PCI: Save and restore the ACS Control register Message-ID: <20260918191846.2f68b23b@shazbot.org> In-Reply-To: <20260918200640.887030-9-dmatlack@google.com> References: <20260918200640.887030-1-dmatlack@google.com> <20260918200640.887030-9-dmatlack@google.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260918_181900_436580_64128B47 X-CRM114-Status: GOOD ( 31.72 ) X-BeenThere: kexec@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "kexec" Errors-To: kexec-bounces+kexec=archiver.kernel.org@lists.infradead.org On Fri, 18 Sep 2026 20:06:34 +0000 David Matlack wrote: > Save the ACS Control register in pci_save_state() and write it back > in pci_restore_state(), instead of recomputing the ACS controls from > scratch with pci_enable_acs(). > > This makes ACS symmetric with the rest of a device's saved state. Today > pci_save_state() ignores ACS entirely and pci_restore_state() re-enables > the ACS controls from the kernel's current ACS policy. As a result, a > device can come out of a reset with different ACS controls than it went > in with, e.g. any controls programmed outside of pci_enable_acs() are > silently dropped. Controls, yes, but if we're restoring controls that might have EC set now, should the Egress Control Vector also be part of the save state? Currently EC always gets cleared on reset and won't be restored by pci_enable_acs(), so we can lose both EC and the EC vector. With this, I think we restore EC but still lose the EC vector. Thanks, Alex > pci_enable_acs() runs when a driver binds to a device > (pci_dma_configure()), i.e. after pci_bus_add_device() has already saved > the device's state. Refresh the saved ACS Control register there as > well, otherwise a subsequent reset would revert ACS back to the > configuration left behind by firmware. > > Devices that rely on device-specific quirks to enable an ACS equivalent > keep that configuration outside of the ACS Control register, so keep > configuring ACS from scratch for them. Do the same for devices that have > no saved ACS state at all. > > Reviewed-by: Bjorn Helgaas > Assisted-by: Claude:claude-opus-5 > Signed-off-by: David Matlack > --- > drivers/pci/pci.c | 66 +++++++++++++++++++++++++++++++++++++++++++- > drivers/pci/pci.h | 5 ++++ > drivers/pci/quirks.c | 7 +++++ > 3 files changed, 77 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index b2879a6be5f8..dd25c01736b4 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1021,6 +1021,55 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps) > caps->ctrl |= (dev->acs_capabilities & PCI_ACS_TB); > } > > +/** > + * pci_save_acs_state - save the ACS Control register > + * @dev: the PCI device > + * > + * Record the ACS controls currently programmed in hardware so that > + * pci_restore_acs_state() can reapply them after a reset. > + */ > +static void pci_save_acs_state(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state; > + > + if (!dev->acs_cap) > + return; > + > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ACS); > + if (!save_state) > + return; > + > + pci_read_config_word(dev, dev->acs_cap + PCI_ACS_CTRL, > + (u16 *)&save_state->cap.data[0]); > +} > + > +/** > + * pci_restore_acs_state - restore the ACS Control register > + * @dev: the PCI device > + */ > +static void pci_restore_acs_state(struct pci_dev *dev) > +{ > + struct pci_cap_saved_state *save_state = NULL; > + > + if (dev->acs_cap && !pci_need_dev_specific_enable_acs(dev)) > + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ACS); > + > + /* > + * Devices that rely on device-specific quirks to enable an ACS > + * equivalent keep that configuration outside of the ACS Control > + * register, so there is nothing useful to restore for them. Configure > + * ACS from scratch instead, which also covers devices that have no > + * saved ACS state at all. > + */ > + if (!save_state) { > + pci_enable_acs(dev); > + return; > + } > + > + pci_write_config_word(dev, dev->acs_cap + PCI_ACS_CTRL, > + *(u16 *)&save_state->cap.data[0]); > +} > + > /** > * pci_enable_acs - enable ACS if hardware support it > * @dev: the PCI device > @@ -1057,6 +1106,15 @@ void pci_enable_acs(struct pci_dev *dev) > __pci_config_acs(dev, &caps, config_acs_param, 0, 0); > > pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl); > + > + /* > + * pci_enable_acs() runs when a driver binds to the device, i.e. after > + * pci_bus_add_device() has already saved the device's state. Refresh > + * the saved ACS Control register so that a subsequent reset restores > + * the controls programmed here rather than the ones left behind by > + * firmware. > + */ > + pci_save_acs_state(dev); > } > > /** > @@ -1800,6 +1858,7 @@ int pci_save_state(struct pci_dev *dev) > pci_save_aer_state(dev); > pci_save_ptm_state(dev); > pci_save_tph_state(dev); > + pci_save_acs_state(dev); > return pci_save_vc_state(dev); > } > EXPORT_SYMBOL(pci_save_state); > @@ -1877,7 +1936,7 @@ void pci_restore_state(struct pci_dev *dev) > pci_restore_msi_state(dev); > > /* Restore ACS and IOV configuration state */ > - pci_enable_acs(dev); > + pci_restore_acs_state(dev); > pci_restore_iov_state(dev); > > dev->state_saved = false; > @@ -3532,6 +3591,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev) > if (error) > pci_err(dev, "unable to allocate suspend buffer for LTR\n"); > > + error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ACS, > + sizeof(u16)); > + if (error) > + pci_err(dev, "unable to allocate suspend buffer for ACS\n"); > + > pci_allocate_vc_save_buffers(dev); > } > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index ba3c3fddddc2..037c1674f164 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -1095,6 +1095,7 @@ void pci_acs_init(struct pci_dev *dev); > void pci_enable_acs(struct pci_dev *dev); > #ifdef CONFIG_PCI_QUIRKS > int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags); > +bool pci_need_dev_specific_enable_acs(struct pci_dev *dev); > int pci_dev_specific_enable_acs(struct pci_dev *dev); > int pci_dev_specific_disable_acs_redir(struct pci_dev *dev); > void pci_disable_broken_acs_cap(struct pci_dev *pdev); > @@ -1105,6 +1106,10 @@ static inline int pci_dev_specific_acs_enabled(struct pci_dev *dev, > { > return -ENOTTY; > } > +static inline bool pci_need_dev_specific_enable_acs(struct pci_dev *dev) > +{ > + return false; > +} > static inline int pci_dev_specific_enable_acs(struct pci_dev *dev) > { > return -ENOTTY; > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 7aee30734303..e500c202d2ec 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5476,6 +5476,13 @@ static const struct pci_dev_acs_ops *pci_dev_acs_ops_get(struct pci_dev *dev) > return NULL; > } > > +bool pci_need_dev_specific_enable_acs(struct pci_dev *dev) > +{ > + const struct pci_dev_acs_ops *p = pci_dev_acs_ops_get(dev); > + > + return p && p->enable_acs; > +} > + > int pci_dev_specific_enable_acs(struct pci_dev *dev) > { > const struct pci_dev_acs_ops *p = pci_dev_acs_ops_get(dev);