[PATCH v8 08/12] PCI: liveupdate: Adopt ACS controls in incoming preserved devices
Bjorn Helgaas
helgaas at kernel.org
Wed Sep 16 17:23:28 PDT 2026
On Mon, Sep 14, 2026 at 04:45:58PM +0000, David Matlack wrote:
> On 2026-09-11 06:31 PM, David Matlack wrote:
> > On 2026-09-10 06:51 PM, Bjorn Helgaas wrote:
> >
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 77b17b13ee61..22001bdf4c97 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -34,6 +34,8 @@
> > > > #include <linux/aer.h>
> > > > #include <linux/bitfield.h>
> > > > #include <linux/suspend.h>
> > > > +
> > > > +#include "liveupdate.h"
> > > > #include "pci.h"
> > > >
> > > > DEFINE_MUTEX(pci_slot_mutex);
> > > > @@ -1008,6 +1010,9 @@ void pci_enable_acs(struct pci_dev *dev)
> > > > bool enable_acs = false;
> > > > int pos;
> > > >
> > > > + if (!pci_liveupdate_enable_adopted_acs_controls(dev))
> > > > + return;
> > >
> > > Ugh. The asymmetry between pci_save_state(), which does nothing
> > > ACS-related, and pci_restore_state(), which enables it, is sort of
> > > sketchy to begin with.
> > >
> > > The command-line parsing in this path feels like kind of a wart (not
> > > that you're touching it).
> > >
> > > It just seems like this path is already hard to analyze, and
> > > liveupdate is making it harder.
> > >
> > > If we could save/restore the ACS state around the reset, wouldn't that
> > > solve this without any liveupdate specials here?
> >
> > Yeah that would simplify the liveupdate support greatly. Let me work on
> > that for v9.
>
> Here's the patch I have prepped for v9 to save/restore ACS controls
> around reset:
>
> From 0c69424454de0c569aca67bc69f496db915a9bcf Mon Sep 17 00:00:00 2001
> From: David Matlack <dmatlack at google.com>
> Date: Fri, 11 Sep 2026 20:05:04 +0000
> Subject: [PATCH] PCI: Save and restore the ACS Control register
>
> 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.
>
> 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.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: David Matlack <dmatlack at google.com>
I like this a lot, thanks! It would be good to get Alex W's ack, too.
Reviewed-by: Bjorn Helgaas <bhelgaas at google.com>
> ---
> 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);
More information about the kexec
mailing list