Re: [PATCH V3 14/42] pci: skip reset during cpr

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, May 27, 2025 at 04:42:16PM -0400, Steven Sistare wrote:
> 
> 
> On 5/24/2025 5:34 AM, Michael S. Tsirkin wrote:
> > On Fri, May 16, 2025 at 10:19:09AM +0200, Cédric Le Goater wrote:
> > > On 5/12/25 17:32, Steve Sistare wrote:
> > > > Do not reset a vfio-pci device during CPR.
> > > > 
> > > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > > > ---
> > > >    hw/pci/pci.c | 13 +++++++++++++
> > > >    1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > index fe38c4c..2ba2e0f 100644
> > > > --- a/hw/pci/pci.c
> > > > +++ b/hw/pci/pci.c
> > > > @@ -32,6 +32,8 @@
> > > >    #include "hw/pci/pci_host.h"
> > > >    #include "hw/qdev-properties.h"
> > > >    #include "hw/qdev-properties-system.h"
> > > > +#include "migration/cpr.h"
> > > > +#include "migration/misc.h"
> > > >    #include "migration/qemu-file-types.h"
> > > >    #include "migration/vmstate.h"
> > > >    #include "net/net.h"
> > > > @@ -537,6 +539,17 @@ static void pci_reset_regions(PCIDevice *dev)
> > > >    static void pci_do_device_reset(PCIDevice *dev)
> > > >    {
> > > > +    /*
> > > > +     * A PCI device that is resuming for cpr is already configured, so do
> > > > +     * not reset it here when we are called from qemu_system_reset prior to
> > > > +     * cpr load, else interrupts may be lost for vfio-pci devices.  It is
> > > > +     * safe to skip this reset for all PCI devices, because vmstate load will
> > > > +     * set all fields that would have been set here.
> > > > +     */
> > > > +    if (cpr_is_incoming()) {
> > > 
> > > Why can't we use cpr_is_incoming() in vfio instead of using an heuristic
> > > on saved fds?
> > > 
> > > Thanks,
> > > 
> > > C.
> > 
> > Think I agree.
> 
> OK.  I will delete the "reused" variable everywhere, and use cpr_is_incoming.
> 
> Michael, since I already use cpr_is_incoming in this pci patch, can I have
> your RB or ack?
> 
> - Steve

My problem is not with cpr_is_incoming as such.

First this comment is a very low level thing to say in common pci code.
vfio will change and we will not remember to keep this up to date.


Second, do we really know vmload for all devices sets all fields as
opposed to assume that qemu_system_reset cleared them?  If not this
introduces an information leak.

It feels safer to just add a way for VFIO to opt out of
(all or part of) reset, instead.

> > 
> > > 
> > > 
> > > > +        return;
> > > > +    }
> > > > +
> > > >        pci_device_deassert_intx(dev);
> > > >        assert(dev->irq_state == 0);
> >