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

[Steven Sistare <steven.sistare@oracle.com>]

On 5/27/2025 5:03 PM, Michael S. Tsirkin wrote:
> 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.

Thanks very much for the feedback.  How about:

hw/vfio/pci.c
vfio_instance_init()
     /*
      * A device that is resuming for cpr is already configured, so do not
      * reset it during qemu_system_reset prior to cpr load, else interrupts
      * may be lost.
      */
     pci_dev->skip_reset_on_cpr = true

hw/pci/pci.c
pci_do_device_reset()
     if (dev->skip_reset_on_cpr && cpr_is_incoming()) {
         return;
     }

- Steve