Re: [PATCH v2] balloon: transitional device support

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Sun, 26 Apr 2015 20:40:10 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Virtio 1.0 cs02 doesn't include a modern balloon device.  At some
> point we'll likely define an incompatible interface with a
> different ID and different semantics.  But for now, it's not a
> big effort to support a transitional balloon device: this has the
> advantage of supporting existing drivers, transparently, as well
> as transports that don't allow mixing virtio 0 and virtio 1
> devices. And balloon is an easy device to test, so it's also
> useful for people to test virtio core handling of transitional
> devices.
> 
> Three issues with legacy hypervisors have been identified:
> 1. Actual value is actually used, and is necessary for management
>    to work. Luckily 4 byte config space writes are now atomic.
>    When using old guests, hypervisors can detect access to the last byte.
>    When using old hypervisors, drivers can use atomic 4-byte accesses.
> 2. Hypervisors actually didn't ignore the stats from the first
>    buffer supplied. This means the values there would be
>    incorrect until hypervisor resends the request.
>    Add a note suggesting hypervisors ignore the 1st buffer.
> 3. QEMU simply over-writes stats from each buffer it gets.
>    Thus if driver supplies a different subset of stats
>    on each request, stale values will be there.
>    Require drivers to supply the same subset on each
>    request. This also gives us a simple way to figure out
>    which stats are supported.
> 
> Please note the issues are there in legacy mode only:
> we need to fix them even if we don't implement a modern
> interface for balloon.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> changes from v1:
> 	dropped stats format change
> 	added a bunch of normative statements
> 	documented work-arounds for legacy hypervisor bugs
> 
>  conformance.tex |  24 +++++++-
>  content.tex     | 173 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 170 insertions(+), 27 deletions(-)
> 
> diff --git a/conformance.tex b/conformance.tex
> index 80b333f..c51287a 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -15,7 +15,7 @@ Conformance targets:
>    \begin{itemize}
>      \item Clause \ref{sec:Conformance / Driver Conformance},
>      \item One of clauses \ref{sec:Conformance / Driver Conformance / PCI Driver Conformance}, \ref{sec:Conformance / Driver Conformance / MMIO Driver Conformance} or \ref{sec:Conformance / Driver Conformance / Channel I/O Driver Conformance}.
> -    \item One of clauses \ref{sec:Conformance / Driver Conformance / Network Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Block Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Console Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Entropy Driver Conformance} or \ref{sec:Conformance / Driver Conformance / SCSI Host Driver Conformance}.
> +    \item One of clauses \ref{sec:Conformance / Driver Conformance / Network Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Block Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Console Driver Conformance}, \ref{sec:Conformance / Driver Conformance / Entropy Driver Conformance}   or \ref{sec:Conformance / Driver Conformance / SCSI Host Driver Conformance}.

Unrelated whitespace change?

>    \end{itemize}
>  \item[Device] A device MUST conform to three conformance clauses:
>    \begin{itemize}

(...)

> diff --git a/content.tex b/content.tex
> index 11015a5..3f728e4 100644
> --- a/content.tex
> +++ b/content.tex

(...)

> @@ -4393,6 +4392,26 @@ guest memory statistics to the host.
>      memory statistics is present.
>  \end{description}
> 
> +\drivernormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
> +The driver SHOULD accept the VIRTIO_BALLOON_F_MUST_TELL_HOST
> +feature if offered by the device.
> +
> +\devicenormative{\subsubsection}{Feature bits}{Device Types / Memory Balloon Device / Feature bits}
> +If the device offers the VIRTIO_BALLOON_F_MUST_TELL_HOST feature
> +bit, and if the driver did not accept this feature bit, the
> +device MAY signal failure by failing to set FEATURES_OK
> +\field{device status} bit when the driver writes FEATURES_OK into
> +\field{device status}.

I'd drop "...when the driver writes...", as this should be clear.

> +\subparagraph{Legacy Interface: Feature bits}\label{sec:Device
> +Types / Memory Balloon Device / Feature bits / Legacy Interface:
> +Feature bits}
> +As the legacy interface does not have a way to gracefully report feature
> +negotiation failure, when using the legacy interface,
> +transitional devices MUST support guests which do not negotiate
> +VIRTIO_BALLOON_F_MUST_TELL_HOST feature, and SHOULD
> +allow guest to use memory before notifying host if
> +VIRTIO_BALLOON_F_MUST_TELL_HOST is not negotiated.
> +
>  \subsection{Device configuration layout}\label{sec:Device Types / Memory Balloon Device / Device configuration layout}
>    Both fields of this configuration
>    are always available.
> @@ -4404,25 +4423,32 @@ struct virtio_balloon_config {
>  };
>  \end{lstlisting}
> 
> -Note that these fields are always little endian, despite convention
> +\subparagraph{Legacy Interface: Device configuration layout}\label{sec:Device Types / Memory Balloon Device / Device
> +configuration layout / Legacy Interface: Device configuration layout}
> +When using the legacy interface, transitional devices and drivers
> +MUST format the fields in struct virtio_balloon_config

Hm... is using MUST in a non-normative section ok?

> +according to the little-endian format.
> +\begin{note}
> +This is unlike the usual convention that was used for other legacy devices

Drop "that was used for other legacy devices"?

>  that legacy device fields are guest endian.
> +\end{note}
> 
>  \subsection{Device Initialization}\label{sec:Device Types / Memory Balloon Device / Device Initialization}
> 
> +The device initialization process is outlined below:
> +
>  \begin{enumerate}
>  \item The inflate and deflate virtqueues are identified.
> 
>  \item If the VIRTIO_BALLOON_F_STATS_VQ feature bit is negotiated:
>    \begin{enumerate}
>    \item Identify the stats virtqueue.
> -
> -  \item Add one empty buffer to the stats virtqueue and notify the
> -    device.
> +  \item Add one empty buffer to the stats virtqueue.
> +  \item DRIVER_OK is set: device operation begins.
> +  \item Notify the device about the stats virtqueue buffer.
>    \end{enumerate}
>  \end{enumerate}
> 
> -Device operation begins immediately.
> -
>  \subsection{Device Operation}\label{sec:Device Types / Memory Balloon Device / Device Operation}
> 
>  The device is driven by the receipt of a

(...)

> @@ -4488,7 +4540,27 @@ When using the legacy interface, the driver SHOULD ignore the \field{len} value
>  Historically, some devices put the total descriptor length there,
>  even though no data was actually written.
>  \end{note}
> +When using the legacy interface, the driver MUST write out all
> +4 bytes each time it updates the \field{actual} value in the
> +configuration space, using a single atomic operation.
> +\begin{note}
> +Historically, devices used the \field{actual} value, even though
> +the device-specific configuration space was never guaranteed

s/never/not/ ?

> +to be atomic.
> +\end{note}
> +When using the legacy interface, the device MUST NOT use the
> +\field{actual} value written by the driver in the configuration
> +space, until the last, most-significant byte of the value has been
> +written.
> +\begin{note}
> +Historically, drivers wrote the \field{actual} value
> +by using multiple single-byte writes in order, from the
> +least-significant to the most-significant value.

But this is only true of _some_ drivers (virtio-pci), no?

> +\end{note}
> 
> +\footnote{As updates to device-specific configuration space are not atomic, this field
> +isn't particularly reliable, but can be used to diagnose buggy guests.

Does this footnote still make sense for non-legacy? I guess not, as we
mandate atomic updates?

> +}
>  \subsubsection{Memory Statistics}\label{sec:Device Types / Memory Balloon Device / Device Operation / Memory Statistics}
> 
>  The stats virtqueue is atypical because communication is driven