Re: [PATCH V2 3/6] virtio: dont reset vqs when SUSPEND

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 11/9/2023 6:15 PM, Parav Pandit wrote:
> > From: Zhu, Lingshan <lingshan.zhu@intel.com>
> > Sent: Thursday, November 9, 2023 3:28 PM
> >
> > On 11/9/2023 1:46 AM, Michael S. Tsirkin wrote:
> > > On Tue, Nov 07, 2023 at 05:27:23PM +0800, Zhu, Lingshan wrote:
> > > > On 11/6/2023 5:49 PM, Michael S. Tsirkin wrote:
> > > > > On Fri, Nov 03, 2023 at 06:34:34PM +0800, Zhu Lingshan wrote:
> > > > > > When SUSPEND is set, device states and virtqueue states should be
> > > > > > stablized, therefore the driver should not reset vqs when SUSPEND
> > > > > > is set in device status.
> > > > > >
> > > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > > ---
> > > > > >     content.tex | 3 +++
> > > > > >     1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex index bcc9d4b..060b5c2
> > > > > > 100644
> > > > > > --- a/content.tex
> > > > > > +++ b/content.tex
> > > > > > @@ -444,6 +444,9 @@ \subsubsection{Virtqueue Reset}\label{sec:Basic
> > Facilities of a Virtio Device /
> > > > > >     The device MUST reset any state of a virtqueue to the default state,
> > > > > >     including the available state and the used state.
> > > > > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set in
> > > > > > +\field{device status}, the driver SHOULD NOT reset any virtqueues.
> > > > > > +
> > > > > >     \drivernormative{\paragraph}{Virtqueue Reset}{Basic Facilities of a
> > Virtio Device / Virtqueues / Virtqueue Reset / Virtqueue Reset}
> > > > > >     After the driver tells the device to reset a queue, the driver
> > > > > > MUST verify that
> > > > > Seems somewhat arbitrary and breaks the claim that the feature is
> > > > > orthogonal and can have uses besides migration.
> > > > when suspended, the device is frozen.
> > > > The driver is aware of this process and so should not reset the vqs I think.
> > > Again that is only true because you want to use it for migration.
> > > But then you can't claim it's a generic facility.
> > I don't get it. The device status is a basic facility.
> >
> > We need to SUSPEND the device by setting SUSPEND bit, to stabilize the device
> > states for migration.
> Is the PCI's PM time not enough to suspend the device?
> For large device I could imagine it could be short.
As you see, PCI PM, so this is a layer violation, virtio should be self 
contained, and what about MMIO and CCW?
This should be a basic facility.
> In that case if there is suspend the device available, it will be used by the guest driver itself, hypervisor wouldnât know about it when those registers are not trapped.
> So we need two ways to suspend.
> One is guest visible, and guest controlled.
> Second is hypervisor control to fulfill the device migration needs.
The guest can eve reset the device.
> So if you can please take a look if the proposed admin command to freeze/stop mode can be used in the emulated register case or not.
> It helps to have the suspend bit in guest control as well with/without emulation mode.
Parav, please believe I have read your series, I didn't comment there 
because I want to avoid further
conflicts/debating, we have done these enough.

As explained before, freeze/stop the device by PCI is a layer violation.

And device status can be pass-through(without emulation, just map it to 
guest) to the guest or trapped(trap and emulate by the hypervisor, for 
example set_status in vDPA).
> > This can also be used for debugging I think.
> As Michael listed, a dedicated debug interface is usually more useful instead of in-band.
re-using another facility without extra efforts is not a bad thing anyway.