[PATCH v19 0/4] [PCI] Error recovery for vfio-pci devices on s390x

[PATCH v19 0/4] [PCI] Error recovery for vfio-pci devices on s390x

[Farhan Ali]

Hi Bjorn,

This patch set includes only the PCI patches of the original series for
error recovery for vfio-pci devices on s390x [1]. Breaking up the patch
series into PCI and VFIO only patches to make merging easier based on
discussion with Alex [2].

Thanks
Farhan

[1] https://lore.kernel.org/all/20260520171113.1111-1-alifm@linux.ibm.com/
[2] https://lore.kernel.org/all/20260602163344.1eda12d2@shazbot.org/

ChangeLog
---------
v18 https://lore.kernel.org/all/20260603181647.2215-1-alifm@linux.ibm.com/
v18 -> v19
  - Move config space accessible check to pcie_flr() function (based on
  discussion of Sashiko review)

  - Fix a gap in MSI-X restoration (patch 4).

  - Rebase on 7.1-rc7

v17 -> v18
  - Rebase on 7.1-rc6.

Farhan Ali (4):
  PCI: Allow per function PCI slots to fix slot reset on s390
  PCI: Avoid saving config space state if inaccessible
  PCI: Fail FLR when config space is inaccessible
  PCI/MSI: Enable memory decoding before restoring MSI-X messages

 drivers/pci/hotplug/rpaphp_slot.c |  2 +-
 drivers/pci/msi/msi.c             |  9 +++++++++
 drivers/pci/pci.c                 | 32 ++++++++++++++++++++++++++++--
 drivers/pci/slot.c                | 33 +++++++++++++++++++++++--------
 include/linux/pci.h               |  8 ++++++--
 5 files changed, 71 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH v19 1/4] PCI: Allow per function PCI slots to fix slot reset on s390

[Farhan Ali]

On s390 systems, which use a machine level hypervisor, PCI devices are
always accessed through a form of PCI pass-through which fundamentally
operates on a per PCI function granularity. This is also reflected in the
s390 PCI hotplug driver which creates hotplug slots for individual PCI
functions. Its reset_slot() function, which is a wrapper for
zpci_hot_reset_device(), thus also resets individual functions.

Currently, the kernel's PCI_SLOT() macro assigns the same pci_slot object
to multifunction devices. This approach worked fine on s390 systems that
only exposed virtual functions as individual PCI domains to the operating
system.  Since commit 44510d6fa0c0 ("s390/pci: Handling multifunctions")
s390 supports exposing the topology of multifunction PCI devices by
grouping them in a shared PCI domain. This creates a problem when resetting
a function through the hotplug driver's slot_reset() interface.

When attempting to reset a function through the hotplug driver, the shared
slot assignment causes the wrong function to be reset instead of the
intended one. It also leaks memory as we do create a pci_slot object for
the function, but don't correctly free it in pci_slot_release().

Add a flag for struct pci_slot to allow per function PCI slots for
functions managed through a hypervisor, which exposes individual PCI
functions while retaining the topology. Since we can use all 8 bits
for slot 'number' (for ARI devices), change slot 'number' u16 to
account for special values -1 and PCI_SLOT_ALL_DEVICES.

Fixes: 44510d6fa0c0 ("s390/pci: Handling multifunctions")
Cc: stable@vger.kernel.org
Suggested-by: Niklas Schnelle <schnelle@linux.ibm.com>
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/pci/hotplug/rpaphp_slot.c |  2 +-
 drivers/pci/pci.c                 |  5 +++--
 drivers/pci/slot.c                | 33 +++++++++++++++++++++++--------
 include/linux/pci.h               |  8 ++++++--
 4 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index 67362e5b9971..92eabf5f61b9 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -84,7 +84,7 @@ int rpaphp_register_slot(struct slot *slot)
 	struct hotplug_slot *php_slot = &slot->hotplug_slot;
 	u32 my_index;
 	int retval;
-	int slotno = -1;
+	int slotno = PCI_SLOT_PLACEHOLDER;
 
 	dbg("%s registering slot:path[%pOF] index[%x], name[%s] pdomain[%x] type[%d]\n",
 		__func__, slot->dn, slot->index, slot->name,
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index d34266651ad0..f5f8291482b0 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4865,8 +4865,9 @@ static int pci_reset_hotplug_slot(struct hotplug_slot *hotplug, bool probe)
 
 static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe)
 {
-	if (dev->multifunction || dev->subordinate || !dev->slot ||
-	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET)
+	if (dev->subordinate || !dev->slot ||
+	    dev->dev_flags & PCI_DEV_FLAGS_NO_BUS_RESET ||
+	    (dev->multifunction && !dev->slot->per_func_slot))
 		return -ENOTTY;
 
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 6d5cd37bfb1e..894d6213ed30 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -37,7 +37,7 @@ static const struct sysfs_ops pci_slot_sysfs_ops = {
 
 static ssize_t address_read_file(struct pci_slot *slot, char *buf)
 {
-	if (slot->number == 0xff)
+	if (slot->number == (u16)PCI_SLOT_PLACEHOLDER)
 		return sysfs_emit(buf, "%04x:%02x\n",
 				  pci_domain_nr(slot->bus),
 				  slot->bus->number);
@@ -72,6 +72,23 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
 	return bus_speed_read(slot->bus->cur_bus_speed, buf);
 }
 
+static bool pci_dev_matches_slot(struct pci_dev *dev, struct pci_slot *slot)
+{
+	if (slot->per_func_slot)
+		return dev->devfn == slot->number;
+
+	return slot->number == PCI_SLOT_ALL_DEVICES ||
+		PCI_SLOT(dev->devfn) == slot->number;
+}
+
+static bool pci_slot_enabled_per_func(void)
+{
+	if (IS_ENABLED(CONFIG_S390))
+		return true;
+
+	return false;
+}
+
 static void pci_slot_release(struct kobject *kobj)
 {
 	struct pci_dev *dev;
@@ -82,8 +99,7 @@ static void pci_slot_release(struct kobject *kobj)
 
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &slot->bus->devices, bus_list)
-		if (slot->number == PCI_SLOT_ALL_DEVICES ||
-		    PCI_SLOT(dev->devfn) == slot->number)
+		if (pci_dev_matches_slot(dev, slot))
 			dev->slot = NULL;
 	up_read(&pci_bus_sem);
 
@@ -187,8 +203,7 @@ void pci_dev_assign_slot(struct pci_dev *dev)
 
 	mutex_lock(&pci_slot_mutex);
 	list_for_each_entry(slot, &dev->bus->slots, list)
-		if (slot->number == PCI_SLOT_ALL_DEVICES ||
-		    PCI_SLOT(dev->devfn) == slot->number)
+		if (pci_dev_matches_slot(dev, slot))
 			dev->slot = slot;
 	mutex_unlock(&pci_slot_mutex);
 }
@@ -267,7 +282,7 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 
 	mutex_lock(&pci_slot_mutex);
 
-	if (slot_nr == -1)
+	if (slot_nr == PCI_SLOT_PLACEHOLDER)
 		goto placeholder;
 
 	/*
@@ -298,6 +313,9 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 	slot->bus = pci_bus_get(parent);
 	slot->number = slot_nr;
 
+	if (pci_slot_enabled_per_func())
+		slot->per_func_slot = 1;
+
 	slot->kobj.kset = pci_slots_kset;
 
 	slot_name = make_slot_name(name);
@@ -318,8 +336,7 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 
 	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &parent->devices, bus_list)
-		if (slot_nr == PCI_SLOT_ALL_DEVICES ||
-		    PCI_SLOT(dev->devfn) == slot_nr)
+		if (pci_dev_matches_slot(dev, slot))
 			dev->slot = slot;
 	up_read(&pci_bus_sem);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2c4454583c11..d58982aa8730 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -78,14 +78,18 @@
  * and, if ARI Forwarding is enabled, functions may appear to be on multiple
  * devices.
  */
-#define PCI_SLOT_ALL_DEVICES	0xfe
+#define PCI_SLOT_ALL_DEVICES	0xfeff
+
+/* Used to identify a slot as a placeholder */
+#define PCI_SLOT_PLACEHOLDER	-1
 
 /* pci_slot represents a physical slot */
 struct pci_slot {
 	struct pci_bus		*bus;		/* Bus this slot is on */
 	struct list_head	list;		/* Node in list of slots */
 	struct hotplug_slot	*hotplug;	/* Hotplug info (move here) */
-	unsigned char		number;		/* Device nr, or PCI_SLOT_ALL_DEVICES */
+	u16			number;		/* Device nr, or PCI_SLOT_ALL_DEVICES */
+	unsigned int		per_func_slot:1; /* Allow per function slot */
 	struct kobject		kobj;
 };
 
-- 
2.43.0

[PATCH v19 2/4] PCI: Avoid saving config space state if inaccessible

[Farhan Ali]

The current reset process saves the device's config space state before
reset and restores it afterward. However errors may occur unexpectedly and
it may then be impossible to save config space because the device may be
inaccessible (e.g. DPC). This results in saving invalid values that get
written back to the device during state restoration.

With a reset we want to recover/restore the device into a functional state.
So avoid saving the state of the config space when the device config space
is inaccessible.

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/pci/pci.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f5f8291482b0..973d23e41c48 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -722,6 +722,27 @@ u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec)
 }
 EXPORT_SYMBOL_GPL(pci_find_dvsec_capability);
 
+static bool pci_dev_config_accessible(struct pci_dev *dev, char *msg)
+{
+	u32 val;
+
+	/*
+	 * If device's config space is inaccessible it can return ~0 for
+	 * any reads. Since VFs can also return ~0 for Device and Vendor ID
+	 * check Command and Status registers. Note that this is racy
+	 * because the device may become inaccessible partway through
+	 * next access.
+	 */
+	pci_read_config_dword(dev, PCI_COMMAND, &val);
+	if (PCI_POSSIBLE_ERROR(val)) {
+		pci_warn(dev, "Device config space inaccessible; unable to %s\n",
+				msg);
+		return false;
+	}
+
+	return true;
+}
+
 /**
  * pci_find_parent_resource - return resource region of parent bus of given
  *			      region
@@ -5027,6 +5048,9 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
 	 */
 	pci_set_power_state(dev, PCI_D0);
 
+	if (!pci_dev_config_accessible(dev, "save state"))
+		return;
+
 	pci_save_state(dev);
 	/*
 	 * Disable the device by clearing the Command register, except for
-- 
2.43.0

[PATCH v19 3/4] PCI: Fail FLR when config space is inaccessible

[Farhan Ali]

If a device is in an error state, then it's config space may not be
accssible. Add additional check to validate if a device's config space is
accessible before doing an FLR reset.

Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 973d23e41c48..521a4bb189d7 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4352,6 +4352,9 @@ int pcie_flr(struct pci_dev *dev)
 {
 	int ret;
 
+	if (!pci_dev_config_accessible(dev, "FLR"))
+		return -ENOTTY;
+
 	if (!pci_wait_for_pending_transaction(dev))
 		pci_err(dev, "timed out waiting for pending transaction; performing function level reset anyway\n");
 
-- 
2.43.0

[PATCH v19 4/4] PCI/MSI: Enable memory decoding before restoring MSI-X messages

[Farhan Ali]

The current MSI-X restoration path assumes the Command register Memory bit
is enabled when writing MSI-X messages. But its possible the last saved and
restored state of device may not have the Memory bit enabled, even if a
device driver later enables Memory bit and MSI-X. Attempting to access
Memory space without Memory bit enabled can lead to Unsupported Request
(UR) from the device. Fix this by enabling Memory bit if we write MSI-X
messages, and restore it afterwards.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 drivers/pci/msi/msi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
index 81d24a270a79..d8d3c8a911ac 100644
--- a/drivers/pci/msi/msi.c
+++ b/drivers/pci/msi/msi.c
@@ -874,6 +874,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
 {
 	struct msi_desc *entry;
 	bool write_msg;
+	u16 cmd;
 
 	if (!dev->msix_enabled)
 		return;
@@ -884,6 +885,11 @@ void __pci_restore_msix_state(struct pci_dev *dev)
 				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
 
 	write_msg = arch_restore_msi_irqs(dev);
+	if (write_msg) {
+		pci_read_config_word(dev, PCI_COMMAND, &cmd);
+		pci_write_config_word(dev, PCI_COMMAND,
+				      cmd | PCI_COMMAND_MEMORY);
+	}
 
 	scoped_guard (msi_descs_lock, &dev->dev) {
 		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
@@ -893,6 +899,9 @@ void __pci_restore_msix_state(struct pci_dev *dev)
 		}
 	}
 
+	if (write_msg)
+		pci_write_config_word(dev, PCI_COMMAND, cmd);
+
 	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
 }
 
-- 
2.43.0

Re: [PATCH v19 4/4] PCI/MSI: Enable memory decoding before restoring MSI-X messages

[Niklas Schnelle]

On Mon, 2026-06-15 at 11:35 -0700, Farhan Ali wrote:
> The current MSI-X restoration path assumes the Command register Memory bit
> is enabled when writing MSI-X messages. But its possible the last saved and
                                              ^ it's

> restored state of device may not have the Memory bit enabled, even if a
                   ^a

> device driver later enables Memory bit and MSI-X. Attempting to access
> Memory space without Memory bit enabled can lead to Unsupported Request
> (UR) from the device. Fix this by enabling Memory bit if we write MSI-X
> messages, and restore it afterwards.

Don't we have the same issue in __pci_restore_msi_state()?

> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  drivers/pci/msi/msi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> index 81d24a270a79..d8d3c8a911ac 100644
> --- a/drivers/pci/msi/msi.c
> +++ b/drivers/pci/msi/msi.c
> @@ -874,6 +874,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  {
>  	struct msi_desc *entry;
>  	bool write_msg;
> +	u16 cmd;
>  
>  	if (!dev->msix_enabled)
>  		return;
> @@ -884,6 +885,11 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
>  
>  	write_msg = arch_restore_msi_irqs(dev);
> +	if (write_msg) {
> +		pci_read_config_word(dev, PCI_COMMAND, &cmd);

Sashiko complains that the read cmd may be all 0xFF…F in case of
transient errors. Though I don't see how we'd handle that here without
a larger refactor.

> +		pci_write_config_word(dev, PCI_COMMAND,
> +				      cmd | PCI_COMMAND_MEMORY);
> +	}
>  
>  	scoped_guard (msi_descs_lock, &dev->dev) {
>  		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
> @@ -893,6 +899,9 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>  		}
>  	}
>  
> +	if (write_msg)
> +		pci_write_config_word(dev, PCI_COMMAND, cmd);

I wonder if it may be safer to have the Command register reads/writes
within the msic_descs_lock critical section in case this gets executed
concurrently and so the restore above from one thread could slip
between the Memory bit enable and the beginning of the critical
section.

> +
>  	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>  }
> 
> 

Re: [PATCH v19 4/4] PCI/MSI: Enable memory decoding before restoring MSI-X messages

[Farhan Ali]

On 6/15/2026 11:59 AM, Niklas Schnelle wrote:
> On Mon, 2026-06-15 at 11:35 -0700, Farhan Ali wrote:
>> The current MSI-X restoration path assumes the Command register Memory bit
>> is enabled when writing MSI-X messages. But its possible the last saved and
>                                                ^ it's
>
>> restored state of device may not have the Memory bit enabled, even if a
>                     ^a
>
>> device driver later enables Memory bit and MSI-X. Attempting to access
>> Memory space without Memory bit enabled can lead to Unsupported Request
>> (UR) from the device. Fix this by enabling Memory bit if we write MSI-X
>> messages, and restore it afterwards.
> Don't we have the same issue in __pci_restore_msi_state()?

I think for MSI it's done through config space registers and so it 
doesn't need to access MMIO.


>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>   drivers/pci/msi/msi.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
>> index 81d24a270a79..d8d3c8a911ac 100644
>> --- a/drivers/pci/msi/msi.c
>> +++ b/drivers/pci/msi/msi.c
>> @@ -874,6 +874,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>>   {
>>   	struct msi_desc *entry;
>>   	bool write_msg;
>> +	u16 cmd;
>>   
>>   	if (!dev->msix_enabled)
>>   		return;
>> @@ -884,6 +885,11 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>>   				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
>>   
>>   	write_msg = arch_restore_msi_irqs(dev);
>> +	if (write_msg) {
>> +		pci_read_config_word(dev, PCI_COMMAND, &cmd);
> Sashiko complains that the read cmd may be all 0xFF…F in case of
> transient errors. Though I don't see how we'd handle that here without
> a larger refactor.
>
>> +		pci_write_config_word(dev, PCI_COMMAND,
>> +				      cmd | PCI_COMMAND_MEMORY);
>> +	}
>>   
>>   	scoped_guard (msi_descs_lock, &dev->dev) {
>>   		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
>> @@ -893,6 +899,9 @@ void __pci_restore_msix_state(struct pci_dev *dev)
>>   		}
>>   	}
>>   
>> +	if (write_msg)
>> +		pci_write_config_word(dev, PCI_COMMAND, cmd);
> I wonder if it may be safer to have the Command register reads/writes
> within the msic_descs_lock critical section in case this gets executed
> concurrently and so the restore above from one thread could slip
> between the Memory bit enable and the beginning of the critical
> section.

Hmm I am not sure if its necessary, AFAIU the msi_desc_lock is used to 
protect the descriptor list. Other config space changes in this function 
is also done outside of this critical section.

OTOH Sashiko does mention correctly pci_msix_write_vector_ctrl() can 
happen even when write_msg is false and which I missed. But this got me 
curious, as to why would we write the vector control for an MSI-X entry 
if we are not writing the message (Address and Data). AFAICT the change 
to write message based on arch_restore_msi_irq() was originally 
introduced with 76ccc297018d ("x86/PCI: Expand the x86_msi_ops to have a 
restore MSIs."). Prior to that we always restored the message and vector 
control mask for an entry. So I am not sure if write_msg is false and we 
don't write a message, why would we want to write the vector control 
mask? If we have a valid reason to write the vector control, even though 
we don't write the message then we should we enable the Memory bit 
unconditionally? Thanks

Farhan

>
>> +
>>   	pci_msix_clear_and_set_ctrl(dev, PCI_MSIX_FLAGS_MASKALL, 0);
>>   }
>>
>>

Re: [PATCH v19 4/4] PCI/MSI: Enable memory decoding before restoring MSI-X messages

[Niklas Schnelle]

On Mon, 2026-06-15 at 15:17 -0700, Farhan Ali wrote:
> On 6/15/2026 11:59 AM, Niklas Schnelle wrote:
> > On Mon, 2026-06-15 at 11:35 -0700, Farhan Ali wrote:
> > > The current MSI-X restoration path assumes the Command register Memory bit
> > > is enabled when writing MSI-X messages. But its possible the last saved and
> >                                                ^ it's
> > 
> > > restored state of device may not have the Memory bit enabled, even if a
> >                     ^a
> > 
> > > device driver later enables Memory bit and MSI-X. Attempting to access
> > > Memory space without Memory bit enabled can lead to Unsupported Request
> > > (UR) from the device. Fix this by enabling Memory bit if we write MSI-X
> > > messages, and restore it afterwards.
> > Don't we have the same issue in __pci_restore_msi_state()?
> 
> I think for MSI it's done through config space registers and so it 
> doesn't need to access MMIO.

Ah you're right, I was confused by the call to
__pci_restore_msi_state() in __pci_restore_msi_state() but looking at
the code paths it does seem like for MSI's it won't do MMIO.

> 
> > 
> > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> > > ---
> > >   drivers/pci/msi/msi.c | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/pci/msi/msi.c b/drivers/pci/msi/msi.c
> > > index 81d24a270a79..d8d3c8a911ac 100644
> > > --- a/drivers/pci/msi/msi.c
> > > +++ b/drivers/pci/msi/msi.c
> > > @@ -874,6 +874,7 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> > >   {
> > >   	struct msi_desc *entry;
> > >   	bool write_msg;
> > > +	u16 cmd;
> > >   
> > >   	if (!dev->msix_enabled)
> > >   		return;
> > > @@ -884,6 +885,11 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> > >   				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
> > >   
> > >   	write_msg = arch_restore_msi_irqs(dev);
> > > +	if (write_msg) {
> > > +		pci_read_config_word(dev, PCI_COMMAND, &cmd);
> > Sashiko complains that the read cmd may be all 0xFF…F in case of
> > transient errors. Though I don't see how we'd handle that here without
> > a larger refactor.
> > 
> > > +		pci_write_config_word(dev, PCI_COMMAND,
> > > +				      cmd | PCI_COMMAND_MEMORY);
> > > +	}
> > >   
> > >   	scoped_guard (msi_descs_lock, &dev->dev) {
> > >   		msi_for_each_desc(entry, &dev->dev, MSI_DESC_ALL) {
> > > @@ -893,6 +899,9 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> > >   		}
> > >   	}
> > >   
> > > +	if (write_msg)
> > > +		pci_write_config_word(dev, PCI_COMMAND, cmd);
> > I wonder if it may be safer to have the Command register reads/writes
> > within the msic_descs_lock critical section in case this gets executed
> > concurrently and so the restore above from one thread could slip
> > between the Memory bit enable and the beginning of the critical
> > section.
> 
> Hmm I am not sure if its necessary, AFAIU the msi_desc_lock is used to 
> protect the descriptor list. Other config space changes in this function 
> is also done outside of this critical section.
> 
> OTOH Sashiko does mention correctly pci_msix_write_vector_ctrl() can 
> happen even when write_msg is false and which I missed. But this got me 
> curious, as to why would we write the vector control for an MSI-X entry 
> if we are not writing the message (Address and Data). AFAICT the change 
> to write message based on arch_restore_msi_irq() was originally 
> introduced with 76ccc297018d ("x86/PCI: Expand the x86_msi_ops to have a 
> restore MSIs."). Prior to that we always restored the message and vector 
> control mask for an entry. So I am not sure if write_msg is false and we 
> don't write a message, why would we want to write the vector control 
> mask? If we have a valid reason to write the vector control, even though 
> we don't write the message then we should we enable the Memory bit 
> unconditionally? Thanks
> 
> Farhan

It seems to me that the only place where arch_restore_msi_irqs() even
returns false is in xen_initdom_restore_msi() and from what I can tell
from the Xen code this then ends up doing the MSI write message in the
Xen hypervisor so for MSI-X also does MMIO. And yes I'd think that if
write_msg is false the pci_msix_write_vector_ctrl() would potentially
also cause an UR if the Memory bit remains off. So from my basic
understanding I'd think it should be enabled unconditionally. As a
bonus it would make the code easier to read too.

Thanks,
Niklas