[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v3] virtio: Improve queue_reset polarity to match to default reset state
On Thu, Apr 28, 2022 at 04:50:41AM +0300, Parav Pandit wrote: > Currently when driver initiates a queue reset, device is expected > to communicate reset status to the driver by changing the value of the > queue_reset register twice. First to return value other than 1 when > reset is ongoing, later to return 1 when queue reset is completed. > > However initially during the device reset time the queue reset value > is zero. queue_reset changes the value of the register to a different > value on reset completion. Yet another time queue_reset value is > expected to change when queue_select is reprogrammed. > > For example in below flow, a created virtqueue, which is disabled > by driver leaves the queue state as > queue_enable = 0, queue_reset = 1. > > example flow: > a) 0,0 -> device init time value > b) 1,0 -> vq is enabled by driver and working > c) 1,1 -> vq is enabled, driver initiated reset > d) 0,0 -> queue reset is ongoing > conflicts with #a, because queue is still enabled in device > whose reset is in progress. > External entity has no knowledge if VQ is undergoing reset or > VQ is never enabled. > e) 0,1 -> queue reset is completed (conflicts with #a initial value) > > On next iteration, when queue_select selects the same VQ again, > without enablement, device is confused to return 1 or 0 because > it was reset once before via queue_reset register. > > This demands complex device implementation to understand what > should be returned for a VQ that is reset using queue_reset register > vs other means. > > Additionally, external entity that may have to manage a virtio device > cannot distinguish between #a and #d, whether a VQ is not yet enabled > or it is in use and undergoing a reset. > > Instead, it is better and efficient to maintain the same VQ state > on the device when queue reset is completed. > > new proposed flow: > q_enable, q_reset > A) 0, 0 -> default, device init time > B) 1, 0 -> driver has enabled vq > C) 1, 1 -> driver started q reset > D) 1, 1 -> q_reset stays 1 until device is busy resetting vq > (device communicates that its working on resetting VQ, consistent with #C) > E) 0, 0 -> q_reset by device is completed, q got disabled > (consistent with device init time #A) > > Hence, this patch proposes a simple change to have reset register > polarity to be same as that of initial reset value. > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/139 > Fixes: 12998e738621 ("virtio: pci support virtqueue reset") > Fixes: a4ce81a83780 ("virtio: mmio support virtqueue reset") > Fixes: 3b5378d70a42 ("virtio: introduce virtqueue reset as basic facility") > Signed-off-by: Parav Pandit <parav@nvidia.com> > > --- > changelog: > v2->v3: > - Updated the commit log to capture the problem more accurately > - Fixed below comments from Michael and Cornelia > - Fixed adding article 'the' before driver > - Change wording in driver requirements section to reflect driver > requirements reflecting device behavior > - Added device compliance statement on what to return for queue_reset > register to indicate ongoing reset in progress > - Moved device specific description adjacent to other device related > queue reset description > v1->v2: > - Fixed below comment from Xuan Zhuo > - Fixed the configuration and register layout text to reflect the polarity > v0->v1: > - Fixed below comments from Michael > - Removed example device side pseudo code for old and new polarity > - Removed unwanted articles 'a' and 'the' > - Dropped extra empty line > --- > content.tex | 44 ++++++++++++++++++-------------------------- > 1 file changed, 18 insertions(+), 26 deletions(-) > > diff --git a/content.tex b/content.tex > index 060bdab..c0b3f15 100644 > --- a/content.tex > +++ b/content.tex > @@ -1029,19 +1029,15 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport > The device MUST present a 0 in \field{queue_enable} on reset. > > If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in > -\field{queue_enable} after the driver has reset the virtqueue via > -\field{queue_reset}. > - > -If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in > \field{queue_reset} on reset. > > If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in > \field{queue_reset} after the virtqueue is enabled with \field{queue_enable}. > > -The device MUST reset the queue when 1 is written to \field{queue_reset}, and > -present a 1 in \field{queue_reset} after the queue has been reset, until the > -driver re-enables the queue via \field{queue_enable} or the device is reset. > -The device MUST present consistent default values after queue reset. > +The device MUST reset the queue when 1 is written to \field{queue_reset}. The > +device MUST continue to present 1 in \field{queue_reset} when the queue reset > +is ongoing. The device MUST present 0 in \field{queue_reset} and \field{queue_enable} > +when queue reset completes. > (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > The device MUST present a 0 in \field{queue_size} if the virtqueue > @@ -1069,11 +1065,11 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport > The driver MUST NOT write a 0 to \field{queue_enable}. > > If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to > -\field{queue_reset} to reset the queue, it MUST verify that the queue > -has been reset by reading back \field{queue_reset} and ensuring that it > -is 1. The driver MAY re-enable the queue by writing a 1 to > -\field{queue_enable} after ensuring that the other virtqueue fields have > -been set up correctly. The driver MAY set driver-writeable queue configuration > +\field{queue_reset} to reset the queue, the driver MUST NOT consider queue > +reset to be complete until it reads back 0 in both \field{queue_reset} and > +\field{queue_enable}. two reads are now required, this is unfortunate. why is it not enough to read back 0 in queue_reset? > The driver MAY re-enable the queue by writing 1 to > +\field{queue_enable} after ensuring that other virtqueue fields have been > +set up correctly. The driver MAY set driver-writeable queue configuration > values to different values than those that were used before the queue reset. > (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > @@ -2005,19 +2001,15 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi > The device MUST NOT access virtual queue contents when \field{QueueReady} is zero (0x0). > > If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in > -\field{QueueReady} after the driver has reset the virtqueue via > -\field{QueueReset}. > - > -If VIRTIO_F_RING_RESET has been negotiated, the device MUST present a 0 in > \field{QueueReset} on reset. > > If VIRTIO_F_RING_RESET has been negotiated, The device MUST present a 0 in > \field{QueueReset} after the virtqueue is enabled with \field{QueueReady}. > > -The device MUST reset the queue when 1 is written to \field{QueueReset}, and > -present a 1 in \field{QueueReset} after the queue has been reset, until the > -driver re-enables the queue via \field{QueueReady} or until the device is reset. > -The device MUST present consistent default values after queue reset. > +The device MUST reset the queue when 1 is written to \field{QueueReset}. The > +device MUST continue to present 1 in \field{QueueReset} when the queue reset > +is ongoing. The device MUST present 0 in \field{QueueReset} and \field{QueueReady} > +when queue reset completes. > (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > \drivernormative{\subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > @@ -2063,11 +2055,11 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi > it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value. > > If VIRTIO_F_RING_RESET has been negotiated, after the driver writes 1 to > -\field{QueueReset} to reset the queue, it MUST verify that the queue > -has been reset by reading back \field{QueueReset} and ensuring that it > -is 1. The driver MAY re-enable the queue by writing a 1 to > -\field{QueueReady} after ensuring that the other virtqueue fields have > -been set up correctly. The driver MAY set driver-writeable queue configuration > +\field{QueueReset} to reset the queue, the driver MUST NOT consider queue > +reset to be complete until it reads back 0 in both \field{QueueReset} and > +\field{QueueReady}. The driver MAY re-enable the queue by writing 1 to > +\field{QueueReady} after ensuring that other virtqueue fields have been > +set up correctly. The driver MAY set driver-writeable queue configuration > values to different values than those that were used before the queue reset. > (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > -- > 1.8.3.1 > > > 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/
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]