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

[Parav Pandit <parav@nvidia.com>]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Friday, November 10, 2023 11:52 AM
> 
> 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,

If you think it is layer violation, than suspend bit for sure is not needed. PCI PM interface should suspend/resume the device on D0<->D3 state transitions.

> and what about MMIO and CCW?

They have largely lacked the richness of PCI transport. So those transport needs to evolve.
Otherwise, PCI offers rich transport facilities compared to MMIO, hence, it will continue wider use.

> This should be a basic facility.
Other transport can also offer like PCI.

> >
> > 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.
> 
I believe the series posted in v3 can support vdpa use case as well.
So I will progress to post v4.

> As explained before, freeze/stop the device by PCI is a layer violation.
I am afraid, we have different vision.
I donât see any layer violation.
Suspend is enough in the PCI PM.
Our vision is more aligned with rest of the hypervisor knobs that owns the migration framework.

> 
> 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).
When it is pass-through, it is controlled by the guest, so for example, if the guest resets the device, hypervisor has lost the control of migration context etc.
Hence, hypervisor needs a channel which is not guest owned.

Same channel can work when trap+emulation is done.

> >
> >> 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.

I just donât see how a suspend bit some debug feature.
Almost everything with that regard is a debug feature to me.