Re: [virtio-comment] Re: [PATCH] virtio-pci: introduce VIRITO_F_QUEUE_STATE

[Jason Wang <jasowang@redhat.com>]

在 2021/3/15 下午11:24, Cornelia Huck 写道:
> On Mon, 15 Mar 2021 10:58:46 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> This patch adds the ability to save and restore virtqueue state via a
>> new field in the common configuration infrastructure.
>>
>> To simply the implementation, no new device status is introduced. For
>> device, the requirements is not to forget the queue state after
>> virtio reset and clear the virtqueue state upon ACKNOWLEDGE. For
>> driver, it must set the virtqueue state before setting DRIVER_OK.
>>
>> To save a virtqueue state, the driver then need:
>>
>> 1) reset device
>> 2) read virtqueue statue
>>
>> To restore a virtqueue state, the driver need:
>>
>> 1) reset device
>> 2) perform necessary setups (e.g features negotiation)
>> 3) write virtqueue state
>> 4) set DRIVER_OK
>>
>> The main user should be live migration.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   content.tex                 | 38 +++++++++++++++++++++++++++++++++++++
>>   virtqueue-state-packed-le.c |  7 +++++++
>>   virtqueue-state-split-le.c  |  4 ++++
>>   3 files changed, 49 insertions(+)
>>   create mode 100644 virtqueue-state-packed-le.c
>>   create mode 100644 virtqueue-state-split-le.c
>>
>> diff --git a/content.tex b/content.tex
>> index 620c0e2..d7bff25 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -837,6 +837,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           le64 queue_driver;              /* read-write */
>>           le64 queue_device;              /* read-write */
>>           le16 queue_notify_data;         /* read-only for driver */
>> +        le64 queue_state;               /* read-write */
>>   };
>>   \end{lstlisting}
>>   
>> @@ -916,6 +917,29 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>           may benefit from providing another value, for example an internal virtqueue
>>           identifier, or an internal offset related to the virtqueue number.
>>           \end{note}
>> +
>> +\item[\field{queue_state}]
>> +        This field exists only if VIRTIO_F_QUEUE_STATE has been
>> +        negotiated. The driver will use this field to get or set the
>> +        virtqueue state by reading or writing the 64bit from the
>> +        field.
>> +        When VIRTIO_F_RING_PACKED has not been negotiated, the driver
>> +        can set and get the following states:
>> +        \lstinputlisting{virtqueue-state-split-le.c}
>> +        The field \field{last_avail_idx} is the location where the
>> +        device read for next index from the available ring.
>> +        When VIRTIO_F_RING_PACKED has been negotiated, the driver can
>> +        set and get the following states:
>> +        \lstinputlisting{virtqueue-state-packed-le.c}
>> +        The field \field{last_avail_idx} is the next location where
>> +        device read for the next descriptor from the descriptor
>> +        ring. The field \field{last_avail_wrap_counter} is the last
>> +        driver ring wrap counter that is observed by the device. The
>> +        field \field{used_idx} is the next location where device write
>> +        used descriptor do descriptor ring. The field
>> +        \field{used_wrap_counter} is the wrap counter that is used by
>> +        the device. See also \ref{sec:Packed Virtqueues / Driver and Device Ring Wrap Counters}.
> While queue_state is a pci-specific field, I don't think any of this is
> transport-specific. I think the description of the layout for the queue
> state should move into a generic section, and this part only reference
> it.


Yes, will move it, probably "basic facility" part.


>
>> +
>>   \end{description}
>>   
>>   \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
>> @@ -964,6 +988,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>   present either a value of 0 or a power of 2 in
>>   \field{queue_size}.
>>   
>> +If VIRTIO_F_QUEUE_STATE has been negotiated, a device MUST NOT clear
>> +the queue state upon reset and MUST reset the queue state when
>> +ACKNOWLEDGE has been set through \field{device status} bit.
> What happens if a driver tries to read the queue status outside of this
> window? Should it get zeroes? Unpredictable values?


I'm not sure having normative like this can help. Can we leave it to 
device? I had a driver normative to clarify when should driver read or 
write to the value.


>
>> +
>>   \drivernormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
>>   
>>   The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
>> @@ -981,6 +1009,13 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>>   
>>   The driver MUST NOT write a 0 to \field{queue_enable}.
>>   
>> +If VIRTIO_F_QUEUE_STATE has been negotiated, a driver SHOULD set the
>> +state of each virtqueue through \field{queue_state} before setting the
>> +DRIVER_OK \field{device status} bit and SHOULD NOT write to
>> +\field{queue_state} after setting the DRIVER_OK \field{device status}
>> +bit. If a driver want to get the virtqueue state, it MUST first reset
>> +the device then read state from \field{queue_state}.
> What should the driver do with a 'fresh' device? Does it need to start
> out with a reset, read the (zero) state, and then write it back?


If 'fresh' means a normal probe procedure, in this case we don't need to 
get the virtqueue state. What we need is to set a proper state.  For 
split virtqueue, the driver should write 0 (as last_avail_idx). For 
packed virtqueue, the driver shoudl write:

{.last_avail_idx = 0, .last_avail_wrap_counter=1, .used_idx=0, 
used_wrap_counter=1}.

If 'fresh' means start device after migration, we need to set the 
virtqueue state to what source gives us:

in src:

1) reset device
2) read virtqueue states
3) pass virtqueue states to dst

in dst:

1) receivce virtqueue states from src
2) reset device
3) perform necesssary setup ( feature neogitaion etc.)
4) set the virtqueue states we received from src.

Btw, it looks to me we need to clearify that "The driver MUST write to 
queue_state after FEATURE_OK but before DRIVER_OK)

Thanks


>
>> +
>>   \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
>>   
>>   The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
>> @@ -6596,6 +6631,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>     transport specific.
>>     For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>>   
>> +\item[VIRTIO_F_QUEUE_STATE(40)] This feature indicates that the driver
>> +  can set and get the virtqueue state.
> Here is probably the best place to put the layout description from the
> pci section above, and to refer to the pci-specific implementation
> (just as it is done for the driver notifications right above.)
>
>> +
>>   \end{description}
>>   
>>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> diff --git a/virtqueue-state-packed-le.c b/virtqueue-state-packed-le.c
>> new file mode 100644
>> index 0000000..f21f9c2
>> --- /dev/null
>> +++ b/virtqueue-state-packed-le.c
>> @@ -0,0 +1,7 @@
>> +le64 {
>> +	last_avail_idx : 15;
>> +	last_avail_wrap_counter : 1;
>> +	used_idx : 15;
>> +	used_wrap_counter : 1;
>> +	reserved : 32;
>> +};
>> diff --git a/virtqueue-state-split-le.c b/virtqueue-state-split-le.c
>> new file mode 100644
>> index 0000000..daeb4a3
>> --- /dev/null
>> +++ b/virtqueue-state-split-le.c
>> @@ -0,0 +1,4 @@
>> +le64 {
>> +	last_avail_idx : 16;
>> +	reserved: 48;
>> +};
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/