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 49776CD37B0 for ; Mon, 18 Sep 2023 06:51:15 +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 BC90371C8E for ; Mon, 18 Sep 2023 06:51:14 +0000 (UTC) Received: from lists.oasis-open.org (oasis-open.org [10.110.1.242]) by lists.oasis-open.org (Postfix) with ESMTP id B5AA898642C for ; Mon, 18 Sep 2023 06:51:14 +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 AA3B09863A4; Mon, 18 Sep 2023 06:51:14 +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 94B9C98638E; Mon, 18 Sep 2023 06:51:05 +0000 (UTC) X-Virus-Scanned: amavisd-new at kavi.com X-IronPort-AV: E=McAfee;i="6600,9927,10836"; a="369908200" X-IronPort-AV: E=Sophos;i="6.02,156,1688454000"; d="scan'208";a="369908200" X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10836"; a="869453019" X-IronPort-AV: E=Sophos;i="6.02,156,1688454000"; d="scan'208";a="869453019" Message-ID: <39bff9d1-86c6-e790-87cc-e69c992e7a7e@intel.com> Date: Mon, 18 Sep 2023 14:50:58 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0 Thunderbird/102.15.1 Content-Language: en-US From: "Zhu, Lingshan" To: "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: <20230906081637.32185-1-lingshan.zhu@intel.com> <20230906081637.32185-3-lingshan.zhu@intel.com> <20230914073023-mutt-send-email-mst@kernel.org> <258989c9-8c5f-c72e-c03d-aabf11f9823d@intel.com> <20230915070504-mutt-send-email-mst@kernel.org> <3ff7e79f-8fbb-4e3e-eb12-17ed8943fe6f@intel.com> In-Reply-To: <3ff7e79f-8fbb-4e3e-eb12-17ed8943fe6f@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: [virtio-dev] Re: [PATCH 2/5] virtio: introduce SUSPEND bit in device status On 9/18/2023 10:56 AM, Zhu, Lingshan wrote: > > > On 9/15/2023 7:10 PM, Michael S. Tsirkin wrote: >> On Fri, Sep 15, 2023 at 10:57:33AM +0800, Zhu, Lingshan wrote: >>> >>> On 9/14/2023 7:34 PM, Michael S. Tsirkin wrote: >>>> On Wed, Sep 06, 2023 at 04:16:34PM +0800, Zhu Lingshan wrote: >>>>> This patch introduces a new status bit in the device status: SUSPEND. >>>>> >>>>> This SUSPEND bit can be used by the driver to suspend a device, >>>>> in order to stabilize the device states and virtqueue states. >>>>> >>>>> Its main use case is live migration. >>>>> >>>>> Signed-off-by: Zhu Lingshan >>>>> Signed-off-by: Jason Wang >>>>> Signed-off-by: Eugenio Pérez >>>>> --- >>>>>    content.tex | 31 +++++++++++++++++++++++++++++++ >>>>>    1 file changed, 31 insertions(+) >>>>> >>>>> diff --git a/content.tex b/content.tex >>>>> index 0e492cd..0fab537 100644 >>>>> --- a/content.tex >>>>> +++ b/content.tex >>>>> @@ -47,6 +47,9 @@ \section{\field{Device Status} >>>>> Field}\label{sec:Basic Facilities of a Virtio Dev >>>>>    \item[DRIVER_OK (4)] Indicates that the driver is set up and >>>>> ready to >>>>>      drive the device. >>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, >>>>> indicates that the >>>>> +  device has been suspended by the driver. >>>>> + >>>>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has >>>>> experienced >>>>>      an error from which it can't recover. >>>>>    \end{description} >>>>> @@ -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,26 @@ \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. >>>> why? let's just forbid driver from setting it. >>> OK >>>>> + >>>>> +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. >>>>> + >>>> sorry what? >>> In case of a failed or cancelled Live Migration, the device needs to >>> resume >>> operation. >>> However the spec forbids the driver to clear a device status bit, so >>> re-writing >>> DRIVER_OK is expected to clear SUSPEND and the device resume operation. >> No, DRIVER_OK is already set. Setting a bit that is already set should >> not have side effects. In fact auto-clearing suspend is problematic too. > The spec says: Set the DRIVER_OK status bit. At this point the device > is “live”. > > So semantically DRIVER_OK can bring the device to live even from SUSPEND. > > In the implementation, the device can check whether SUSPEND is set, then > decide what to do. Just don't ignore DRIVER_OK if it is already > set, and the driver should not clear a device status bit. > >> >> >>>>> +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 descritors 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. >>>> flush how? >>> This is device-type-specific, and we will include tracking inflight >>> descriptors(buffers) in V2. >>>>> +\item Record Virtqueue State of each enabled virtqueue, see >>>>> section \ref{sec:Virtqueues / Virtqueue State} >>>> record where? >>> This is transport specific, for PCI, patch 5 introduces two new >>> fields for >>> avail and used state >> they clearly can't store state for all vqs, these are just two 16 bit >> fields. > vq states filed can work with queue_select like other vq fields. > I will document this in the comment. >> >>>>> +\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} >>>> pause in what sense? completely? this does not seem realistic. >>>> e.g. pci express link has to stay active or device will die. >>> only pause virtio, I will rephrase the sentence as "pause its virtio >>> operation". >> that is vague too. for example what happens to link state of >> a networking device? > Then how about we say: pause operation in both data-path and > control-path? > > Or do you have any suggestion? >> >>> Others like PCI link in the example is out of the spec and we don't >>> need >>> to migrate them. >>>> >>>> also, presumably here it is except a bunch of other fields. >>>> e.g. what about queue select and all related queue fields? >>> For now they are forbidden. >>> >>> As SiWei suggested, we will introduce a new feature bit to control >>> whether >>> allowing resetting a VQ after SUSPEND. We can use more feature bits if >>> there are requirements to perform anything after SUSPEND. But for now >>> they are forbidden. >> I don't know how this means, but whatever. you need to make >> all this explicit though. > a new feature bit: VIRTIO_F_RING_SUSPEND_RESET. If this feature > bit has been negotiated then the device allow reset a vq after SUSPEND. Hi Michael, Rethink of this, as you suggested before, In V2, I will forbid resetting VQs after SUSPEND. Thanks >> >>>>> +\end{itemize} >>>>> + >>>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio >>>>> Device / Feature Bits} >>>>>    Each virtio device offers all the features it understands.  During >>>>> @@ -937,6 +964,10 @@ \chapter{Reserved Feature >>>>> Bits}\label{sec:Reserved Feature Bits} >>>>>        \ref{devicenormative:Basic Facilities of a Virtio Device / >>>>> Feature Bits} for >>>>>        handling features reserved for future use. >>>>> +  \item[VIRTIO_F_SUSPEND(42)] This feature indicates that the >>>>> driver can >>>>> +   SUSPEND the device. >>>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device >>>>> Status Field}. >>>>> + >>>>>    \end{description} >>>>>    \drivernormative{\section}{Reserved Feature Bits}{Reserved >>>>> Feature Bits} >>>>> -- >>>>> 2.35.3 > --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org