From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id 2CC359863E0 for ; Tue, 26 Apr 2022 09:52:26 +0000 (UTC) Date: Tue, 26 Apr 2022 05:52:17 -0400 From: "Michael S. Tsirkin" Message-ID: <20220426055149-mutt-send-email-mst@kernel.org> References: <1650785333.6800807-1-xuanzhuo@linux.alibaba.com> MIME-Version: 1.0 In-Reply-To: Subject: Re: [virtio-dev] queue_reset register polarity to improve Content-Type: text/plain; charset=us-ascii Content-Disposition: inline To: Stefan Hajnoczi Cc: Xuan Zhuo , Parav Pandit , "virtio-dev@lists.oasis-open.org" List-ID: On Tue, Apr 26, 2022 at 09:48:29AM +0100, Stefan Hajnoczi wrote: > On Sun, Apr 24, 2022 at 03:28:53PM +0800, Xuan Zhuo wrote: > > On Sun, 24 Apr 2022 00:49:19 +0000, Parav Pandit wrote: > > > Hi, > > > > > > A recently defined queue_reset register has a little weird definition that we should improve. > > > When driver initiate queue reset, it writes queue_reset = 1. > > > When device is busy resetting the queue, on this driver request, it is expected to return queue_reset=0. > > > Once queue reset is completed it is expected to return queue_reset = 1. > > > (Polarity changed twice to same value as what was driver set). See more below. > > > > > > So state wise, > > > # q_enable, q_reset represents : > > > a) 0,0 -> device init time value > > > b) 1,0 -> vq is enabled and working > > > c) 1,1 -> vq is enabled, driver initiated reset > > > d) 1,0 -> vq is enabled, but device is busy doing the reset (conflicting definition with above #b ) > > > e) 0,1 -> vq reset is complete in the device and VQ is now disabled (again conflict with #a above ) > > > > > > Instead, I think we should have below better, consistent definition, no matter how queue reset occurs (init time or later). > > > > > > 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 (communicating that its working on resetting, consistent with #C) > > > E) 0, 0 -> q_reset by device is completed, q got disabled (now matches the state same as device init time #A) > > > > Seems to me to be two different designs, I don't see any particular value in the > > conflict mentioned here, and under what circumstances would it cause any > > trouble? > > > > The design here is similar to many scenarios, such as device reset. > > > > So if you want to change to your design, I want to know if there are other > > reasons. > > A benefit of Parav's suggested definition is that reading the value of > q_enable and q_reset from the device tells you the state. > > The current definition relies on the driver maintaining internal state > (although this could just be assumptions in the code rather than > variables stored in memory) because it cannot determine the state by > reading q_enable and q_reset. > > Introspection may not be important for regular drivers because they know > which previous operations they performed, but in some cases like driver > crash recovery or live migration introspection can be useful. That said, > they can reset the entire device to return to a known state, if > necessary. > > Stefan Sounds like a valid reason, probably a good idea to include in the commit log. -- MST --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org