Re: [virtio-comment] Re: [PATCH V2 1/2] virtio: introduce virtqueue state as basic facility

[Jason Wang <jasowang@redhat.com>]

å 2021/3/22 äå4:19, Eugenio Perez Martin åé:
> On Mon, Mar 22, 2021 at 4:47 AM Jason Wang <jasowang@redhat.com> wrote:
> > This patch adds new device facility to save and restore virtqueue
> > state. The virtqueue state is split into two parts:
> >
> > - The available state: The state that is used for read the next
> >    available buffer.
> > - The used state: The state that is used for make buffer used.
> >
> > This will simply the transport specific method implemention. E.g two
> > le16 could be used instead of a single le32). For split virtqueue, we
> > only need the available state since the used state is implemented in
> > the virtqueue itself (the used index).
> > For packed virtqueue, we need
> > both the available state and the used state.
> >
> > Those states are required to implement live migration support for
> > virtio device.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >   content.tex | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 81 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 620c0e2..af39111 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -385,6 +385,83 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
> >   types. It is RECOMMENDED that devices generate version 4
> >   UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
> >
> > +\section{Virtqueue State}\label{sec:Virtqueues / Virtqueue State}
> > +
> > +When VIRTIO_F_QUEUE_STATE is negotiated, the driver can set and
> > +get the device internal virtqueue state through the following
> > +fields. The way to access those fields is transport specific.
> > +
> > +\subsection{\field{Available State} Field}
> > +
> > +The availabe state field is two bytes virtqueue state that is used by
> > +the device to read for the next available buffer.
> > +
> > +When VIRTIO_RING_F_PACKED is not negotiated, it contains:
> > +
> > +\begin{lstlisting}
> > +le16 last_avail_idx;
> > +\end{lstlisting}
> > +
> > +The \field{last_avail_idx} field is the location where the device read
> > +for the next index from the virtqueue available ring.
> > +
> > +When VIRTIO_RING_F_PACKED is negotiated, it contains:
> > +
> > +\begin{lstlisting}
> > +le16 {
> > +  last_avail_idx : 15;
> > +  last_avail_wrap_counter : 1;
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{last_avail_idx} field is the location where the device read
> > +for the next descriptor from the virtqueue descriptor ring.
> > +
> > +The \field{last_avail_wrap_counter} field is the last driver ring wrap
> > +counter that is observed by the device.
> > +
> > +See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> > +
> > +\subsection{\field{Used State} Field}
> > +
> > +The availabe state field is two bytes virtqueue state that is used by
> > +the device to make buffer used.
> > +
> > +When VIRTIO_RING_F_PACKED is negotiated, the used state contains:
> > +
> > +\begin{lstlisting}
> > +le16 {
> > +  used_idx : 15;
> > +  used_wrap_counter : 1;
> > +};
> > +\end{lstlisting}
> > +
> > +The \field{used_idx} field is the location where the device write next
> > +used descriptor to the descriptor ring.
> > +
> > +The \field{used_wrap_counter} field is the wrap counter that is used
> > +by the device.
> > +
> > +The used state is unnecessary when VIRTIO_RING_F_PACKED is not
> > +negotiated.
> Maybe we could be more specific with "unnecessary"? Either to say:
> 1) "For split virtqueue the device/driver must ignore this field at
> start/recover state and use used_idx in the used ring.", so use of
> this field is totally unspecified.


RIght, this could be added as normative part.

An interesting part is the timing of the read, the current semantics 
depends on reset command to stop the device (to avoid introduing new 
status).

So in order to get the state, driver needs:

1) reset the device
2) read the avail and used states

Technically, after step 1, driver doesn't know whether nor not it's a 
split virtqueue.

For write, it should be fine since we mandate it can only be done after 
FEATURE_OK so driver know it's a split or not.

So how about something like in the driver normative:

To get the virtqueue state, the driver MUST remember the queue format 
that is used and read the virtqueue state accordingly after resetting 
the device. E.g if the split virtqueue is used before resetting the 
device, the driver MUST NOT acces used state after restting the device.

To write the virtqueue state, the driver MUST NOT write to the used 
state if VIRTIO_F_RING_PACKED is not negotiated.

Thanks


> 2) "For split virtqueue, the device/driver must prefer to get/set
> state from virtqueue's used ring in case of discrepancy with this
> value".
>
> I prefer the former so there is only one place to set/retrieve it, but
> something like this would avoid confusion in my opinion. I find it
> equally convenient to prefer the virtqueue memory or the
> transport-specific used state field, though.