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

[Alex Williamson]

On Tue, 23 Jun 2026 09:36:39 -0700
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:

> On 6/23/2026 4:28 AM, Niklas Schnelle wrote:
> > On Mon, 2026-06-22 at 13:49 -0700, Farhan Ali wrote:  
> >> On 6/22/2026 1:29 PM, Thomas Gleixner wrote:  
> >>> On Mon, Jun 22 2026 at 10:18, 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
> >>>> 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 and restore
> >>>> it afterwards.
> >>>>
> >>>> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> >>>> ---
> >>>>  
> > --- 8< ---  
> >>>> @@ -882,6 +883,8 @@ void __pci_restore_msix_state(struct pci_dev *dev)
> >>>>    	pci_intx_for_msi(dev, 0);
> >>>>    	pci_msix_clear_and_set_ctrl(dev, 0,
> >>>>    				PCI_MSIX_FLAGS_ENABLE | PCI_MSIX_FLAGS_MASKALL);
> >>>> +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
> >>>> +	pci_write_config_word(dev, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);  
> >>> Can we please have a comment there which explains this? Three month down
> >>> the road this will results in head scratching otherwise.
> >>>
> >>> I agree with Niklas that this wants a Fixes and a Cc:stable tag.
> >>>
> >>> Other than that:
> >>>
> >>> Reviewed-by: Thomas Gleixner <tglx@xxxxxxxxxx>
> >>>
> >>> Thanks,
> >>>
> >>>           tglx
> >>>  
> >> I can add a comment, how about something like below?
> >>
> >> "The restored device state may not have Memory decoding enabled in
> >> Command register. Since the MSI-X was enabled for the device, enable
> >> Memory decoding before restoring MSI-X"  
> > Missing "the" before "Command register" other than that this sounds
> > good to me and I agree with Thomas that a comment makes sense here.
> >  
> >> For the Fixes tag, do you have a suggestion for a commit? This behavior
> >> has been present since 41017f0cac92 ("[PATCH] PCI: MSI(X) save/restore
> >> for suspend/resume" which introduced these restore functions. So should
> >> be Fixes against 41017f0cac92?  
> > I'm not sure but my guess would be that for suspend/resume this isn't
> > an issue since the suspend part would save the state with Memory Space
> > enabled. So it wouldn't be broken in this original use-case. I think
> > the problem only occurs in case restore is done for error recovery.
> > Might it make sense to have the Fixes tag point to a2f1e22390ac
> > ("PCI/ERR: Ensure error recoverability at all times") even though that
> > only helps to expose the issue?
> >
> > Thanks,
> > Niklas  
> 
> Yeah, commit a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all 
> times") exposes this issue. I am okay if we want Fixes tag to point to 
> this change, unless someone objects. I will also add a note in the 
> commit message to specify a2f1e22390ac exposes this issue.

I'm not following how we come to that limited conclusion.  vfio-pci has
long relied on the save/restore in PCI core and it doesn't seem
unreasonable that if a guest triggers a pci_reset_function() using a
method that vfio-pci traps and implements through another
pci_reset_function() in the host, that it's done with memory disabled.

It would be atypical for a guest to trigger a reset with MSI-X enabled,
but I don't see what prevents it.  I'd attribute the fix to the
introduction for better stable coverage.  This might also be pulled out
of the series as an independent fix.  Thanks,

Alex