Re: [RFC PATCH 4/5] virtqueue: constraints for virtqueue state

[Eugenio Perez Martin <eperezma@redhat.com>]

On Fri, Aug 18, 2023 at 11:44âAM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 8/17/2023 11:19 PM, Eugenio Perez Martin wrote:
> > On Tue, Aug 15, 2023 at 1:30âPM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 8/15/2023 8:34 AM, Jason Wang wrote:
> >>> On Mon, Aug 14, 2023 at 7:29âPM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> This commit specifies the constraints of the virtqueue state,
> >>>> and the actions should be taken by the device when SUSPEND
> >>>> and DRIVER_OK is set
> >>>>
> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> ---
> >>>>    content.tex | 31 +++++++++++++++++++++++++++++++
> >>>>    1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/content.tex b/content.tex
> >>>> index 43bd5de..f6ac581 100644
> >>>> --- a/content.tex
> >>>> +++ b/content.tex
> >>>> @@ -587,6 +587,37 @@ \subsection{\field{Used State} Field}
> >>>>
> >>>>    See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> >>>>
> >>>> +\drivernormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated, the driver MUST set SUSPEND in \field{device status}
> >>>> +first before getting or setting Virtqueue State of any virtqueues.
> >>> I don't get why this is a must. It could be useful for debugging.
> >> To avoid race conditions with the device and make the device
> >> implementation easier
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged but VIRTIO_RING_F_PACKED not been negotiated,
> >>> typo
> >> yes
> >>>> +the driver MUST NOT access \field{Used State} of any virtqueues, it should use the
> >>>> +used index in the used ring.
> >>>> +
> >>>> +\devicenormative{\subsection}{Virtqueue State}{Basic Facilities of a Virtio Device / Virtqueue State}
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but SUSPEND is not set in \field{device status},
> >>>> +the device MUST ignore any accesses against Virtqueue State of any virtqueues.
> >>> Btw, do we need to clarify the behavior of ring reset after suspending?
> >> I think once suspended, the device should ignore resetting a queue
> > Actually shadow virtqueue could benefit from the ability to change vq
> > properties (addresses) while the device is suspended, and then just
> > resume it. I've been told that ring reset is overkill for that.
> If ring reset is overkill, is SUSPEND even more overkill?

It depends on the cost of recreating the vq in the device I think. But
it has more to do with *what* is changed in the vq, as it seems some
parameters (vq size) has more impact than others like vq address. The
way to stop the device does not affect, but ring reset offers the
possibility of change all of the parameters already.

Adding Si-Wei and Dragos here, as they pointed it out in the
virtio-networking upstream meeting.

> >
> > But probably it is better to address it on top, with another feature flag.
> I think if we want to changing the vq properties, there must be a
> mechanism to
> stop the queue then resume the queue.
>
> How about allow setting queue_enable = 0 to stop it and =1 to resume and
> force it reinitialize?
>

Yes, I think that is better suited. But maybe this is better to be
added on top, so we maintain this series small.

Thanks!

> Thanks
> Zhu Lingshan
> >
> >>>> +
> >>>> +When VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED is not,
> >>>> +the device MUST ignore any accesses against \field{Used State}.
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiaged, the device MUST reset
> >>>> +the Virtqueue State of every virtqueue upon a reset.
> >>> Need to define the meaning of "reset" this is important for packed virtqueue.
> >> I will remove this as Stefan suggested.
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE and VIRTIO_RING_F_PACKED have been negotiaged, when SUSPEND is set,
> >>>> +the device MUST record the Virtqueue State of every enabled virtqueue
> >>>> +in \field{Available State} and \field{Used State} respectively,
> >>>> +and correspondingly restore the Virtqueue State of every enabled virtqueue
> >>>> +from \field{Avaiable State} and \field{Used State} when DRIVER_OK is set.
> >>> We can just let the device report those states in any case then we
> >>> don't need to care about those details, or did you see any blockers?
> >> Agree, I will add the definition of used_state of splitted vq in the
> >> next version
> >>
> >> Thanks
> >>> Thanks
> >>>
> >>>> +
> >>>> +If VIRTIO_F_QUEUE_STATE has been negotiated but VIRTIO_RING_F_PACKED has been not, when SUSPEND is set,
> >>>> +the device MUST record the available state of every enabled virtqueue in \field{Available State},
> >>>> +and restore the available state of every enabled virtqueue from \field{Avaiable State}
> >>>> +when DRIVER_OK is set.
> >>>> +
> >>>>    \input{admin.tex}
> >>>>
> >>>>    \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
> >>>> --
> >>>> 2.35.3
> >>>>
>