From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from ws5-mx01.kavi.com (ws5-mx01.kavi.com [34.193.7.191]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 29C5AC5475B for ; Tue, 20 Feb 2024 04:06:18 +0000 (UTC) Received: from lists.oasis-open.org (oasis.ws5.connectedcommunity.org [10.110.1.242]) by ws5-mx01.kavi.com (Postfix) with ESMTP id 1E0D67817B for ; Tue, 20 Feb 2024 04:06:16 +0000 (UTC) Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id D72FF98666C for ; Tue, 20 Feb 2024 04:06:15 +0000 (UTC) Received: from host09.ws5.connectedcommunity.org (host09.ws5.connectedcommunity.org [10.110.1.97]) by lists.oasis-open.org (Postfix) with QMQP id BEC8C98648B; Tue, 20 Feb 2024 04:06:15 +0000 (UTC) Mailing-List: contact virtio-dev-help@lists.oasis-open.org; run by ezmlm List-ID: Sender: Precedence: bulk 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 9F5B19866ED for ; Tue, 20 Feb 2024 04:06:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at kavi.com X-IronPort-AV: E=McAfee;i="6600,9927,10989"; a="13199440" X-IronPort-AV: E=Sophos;i="6.06,171,1705392000"; d="scan'208";a="13199440" X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.06,171,1705392000"; d="scan'208";a="27810558" Message-ID: Date: Tue, 20 Feb 2024 12:06:07 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: David Stevens , "Michael S. Tsirkin" Cc: jasowang@redhat.com, eperezma@redhat.com, cohuck@redhat.com, stefanha@redhat.com, virtio-comment@lists.oasis-open.org, virtio-dev@lists.oasis-open.org References: <20240218132306.83456-1-lingshan.zhu@intel.com> <20240218090935-mutt-send-email-mst@kernel.org> From: "Zhu, Lingshan" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status On 2/19/2024 2:46 PM, David Stevens wrote: > On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin wrote: >> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote: >>> This commit allows the driver to suspend the device by >>> introducing a new status bit SUSPEND in device_status. >>> >>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND >>> which indicating whether the device support SUSPEND. >>> >>> This SUSPEND bit is transport-independent. >>> >>> Signed-off-by: Zhu Lingshan >>> Signed-off-by: Jason Wang >>> Signed-off-by: Eugenio Pérez >> Could we get some kind of dscription how this has taken into >> consideration the proposal from David Stevens? >> >> I find it really tiring when there are competing patches with authors >> ignoring each other's work and leaving it up to reviewers to >> figure out how do the patches compare. > This patch looks like it could be used to implement my use case. > However, parts of it are a bit vague and imprecise, so it's hard to > actually say whether my use case would actually be covered by a > specific implementation of this proposal. I am on vacation till this Friday. Shall we co-work on this and post something new together? > > To give specifics on what I'm trying to do, I need to allow a guest > with virtio-pci devices (including stateful devices like virtio-fs and > virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices > to wake up the guest (currently via an ACPI GPE, but eventually via a > native PCI PME). This is all done on a consumer device, so there is no > need for snapshotting or for live migration. Suspending is not dedicated only for live migration. For your use case, shall we add a new PCI section like saying: when entering SUSPEND, the device should get into S1/S3 and other PCI specific steps in PCI transport charter. That is because PCI is a transport layer of virtio, controlling virtio states by PCI sounds like a layer violation. > >>> --- >>> content.tex | 34 ++++++++++++++++++++++++++++++++-- >>> 1 file changed, 32 insertions(+), 2 deletions(-) >>> >>> diff --git a/content.tex b/content.tex >>> index 0a62dce..3d656b5 100644 >>> --- a/content.tex >>> +++ b/content.tex >>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev >>> >>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced >>> an error from which it can't recover. >>> + >>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the >>> + device has been suspended by the driver. >>> \end{description} >>> >>> The \field{device status} field starts out as 0, and is reinitialized to 0 by >>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev >>> recover by issuing a reset. >>> \end{note} >>> >>> +The driver SHOULD NOT set SUSPEND if FEATURES_OK is not set. >>> + >>> +When setting SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set. >>> + >>> \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field} >>> >>> The device MUST NOT consume buffers or send any used buffer >>> @@ -82,6 +89,25 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev >>> that a reset is needed. If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device >>> MUST send a device configuration change notification to the driver. >>> >>> +The device MUST ignore SUSPEND if FEATURES_OK is not set. >>> + >>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated. >>> + >>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set. >>> + >>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND >>> +and resumes operation upon DRIVER_OK. >>> + >>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND, >>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}: >>> + >>> +\begin{itemize} >>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used. >>> +\item Wait until all descriptors that being processed to finish and mark them as used. >>> +\item Flush all used buffer and send used buffer notifications to the driver. > What's the difference between the previous three lines? They might be > trying to say different things, but there is a lot of overlap in how > they're phrased. That makes it hard to figure out exactly what they're > mandating. Is it something like: "stop processing new buffers", > "finish processing any buffers that are currently in flight", "mark > all finished buffers as used and send a used buffer notification"? sure, the "mark used" parts can merge. > > Also, what does this mean for devices where the driver places empty > buffers into the virtqueue that the device then holds on to for an > indeterminate period of time (e.g. network receiveq, balloon statsq, > etc)? The device doesn't process them. Oh, I should add: The driver should not make any more buffers available to the device. > >>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space} > What does "Pause its operation except device status" mean? The word > "operation" is vague, what exactly does it include? Also what does > "operate its device status" mean? Because we need to resume the device after suspending it by setting DRIVER_OK, so we need the device status filed keel alive. Thanks, Zhu Lingshan > > -David --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org