[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20251002091613.GD408411@p1gen4-pw042f0m>
Date: Thu, 2 Oct 2025 11:16:13 +0200
From: Benjamin Block <bblock@...ux.ibm.com>
To: Farhan Ali <alifm@...ux.ibm.com>
Cc: linux-s390@...r.kernel.org, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
alex.williamson@...hat.com, helgaas@...nel.org, clg@...hat.com,
schnelle@...ux.ibm.com, mjrosato@...ux.ibm.com
Subject: Re: [PATCH v4 01/10] PCI: Avoid saving error values for config space
On Wed, Oct 01, 2025 at 10:12:03AM -0700, Farhan Ali wrote:
>
> On 10/1/2025 8:15 AM, Benjamin Block wrote:
> > On Wed, Sep 24, 2025 at 10:16:19AM -0700, Farhan Ali wrote:
> >> @@ -1792,6 +1798,14 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
> >> int pci_save_state(struct pci_dev *dev)
> >> {
> >> int i;
> >> + u32 val;
> >> +
> >> + pci_read_config_dword(dev, PCI_COMMAND, &val);
> >> + if (PCI_POSSIBLE_ERROR(val)) {
> >> + pci_warn(dev, "Device config space inaccessible, will only be partially restored\n");
> >> + return -EIO;
> >
> > Should it set `dev->state_saved` to `false`, to be on the save side?
> > Not sure whether we run a risk of restoring an old, outdated state otherwise.
>
> AFAIU if the state_saved flag was set to true then any state that we
> have saved should be valid and should be okay to be restored from. We
> just want to avoid saving any invalid data.
Hmm, so I dug a bit more, and I see
void pci_restore_state(struct pci_dev *dev) {}
has `dev->state_saved = false` at the end, so I guess if a device is put into
suspend, and then later woken again, the flag gets reset every time.
And then there is also code like this:
static pci_ers_result_t e1000_io_slot_reset(struct pci_dev *pdev)
{
...
err = pci_enable_device_mem(pdev);
if (err) {
...
} else {
pdev->state_saved = true;
pci_restore_state(pdev);
I don't know..
But I see Alex suggested this before.
> >> + }
> >> +
> >> /* XXX: 100% dword access ok here? */
> >> for (i = 0; i < 16; i++) {
> >> pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
> >> @@ -1854,6 +1868,14 @@ static void pci_restore_config_space_range(struct pci_dev *pdev,
> >>
> >> static void pci_restore_config_space(struct pci_dev *pdev)
> >> {
> >> + if (!pdev->state_saved) {
> >> + pci_warn(pdev, "No saved config space, restoring BARs\n");
> >> + pci_restore_bars(pdev);
> >> + pci_write_config_word(pdev, PCI_COMMAND,
> >> + PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
> >
> > Is this really something that ought to be universally enabled? I thought this
> > depends on whether attached resources are IO and/or MEM?
> >
> > int pci_enable_resources(struct pci_dev *dev, int mask)
> > {
> > ...
> > pci_dev_for_each_resource(dev, r, i) {
> > ...
> > if (r->flags & IORESOURCE_IO)
> > cmd |= PCI_COMMAND_IO;
> > if (r->flags & IORESOURCE_MEM)
> > cmd |= PCI_COMMAND_MEMORY;
> > }
> > ...
> > }
> >
> > Also IIRC, especially on s390, we never have IO resources?
> >
> > int zpci_setup_bus_resources(struct zpci_dev *zdev)
> > {
> > ...
> > for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > ...
> > /* only MMIO is supported */
> > flags = IORESOURCE_MEM;
> > if (zdev->bars[i].val & 8)
> > flags |= IORESOURCE_PREFETCH;
> > if (zdev->bars[i].val & 4)
> > flags |= IORESOURCE_MEM_64;
> > ...
> > }
> > ...
> > }
> >
> > So I guess this would have to have some form of the same logic as in
> > `pci_enable_resources()`, after restoring the BARs.
> >
> > Or am I missing something?
>
> As per my understanding of the spec, setting both I/O Space and Memory
> Space should be safe. The spec also mentions if a function doesn't
> support IO/Memory space access it could hardwire the bit to zero. We
> could add the logic to iterate through all the resources and set the
> bits accordingly, but in this case trying a best effort restoration it
> should be fine?
>
> Also I didn't see any issues testing on s390x with the NVMe, RoCE and
> NETD devices, but I could have missed something.
Well, just taking a coarse look at how some other PCI device drivers use the
Command register (this being non-s390 specific after all); some of them base
decisions on whether either/or these flags are set in config space. Now that
this sets both flags, this might have surprising side-effects.
On the other hand, iterating over the resources might not even be enough
with some device-drivers, since they base their decision on whether to enable
either/or on other knowledge.
So I don't know. Enabling both just in case might be a good compromise.
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt / Geschäftsführung: David Faller
Sitz der Ges.: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
Powered by blists - more mailing lists