[virtio-dev] [RFC] snd: Add virtio sound device specification

[Mikhail Golubev]

This patch proposes virtio specification for new virtio sound device. Any
feedback would be greatly appreciated. Previous version of the proposal: [1].

Reference implementation is now available and located here:

 * QEMU v4.1.0-rc3 device:
   https://github.com/OpenSynergy/qemu/tree/virtio-snd-draft

 * Linux kernel 5.2-rc7 driver:
   https://github.com/OpenSynergy/linux/tree/virtio-snd-draft

How to use:

1) Set the SND_VIRTIO configuration option in Linux kernel config to
  enable virtio-snd in a guest.

2) QEMU command line must contain virtio-snd-pci device configuration:

  a) ALSA backend:
-audiodev id=sndbe,driver=alsa,out.try-poll=off,\
-device virtio-snd-pci

Polling mode must be disabled because QEMU will freeze until
playback is stopped.

  b) PulseAudio backend:
-audiodev id=sndbe,driver=pa,server=/run/user/1000/pulse/native,\
-device virtio-snd-pci

[1]: https://lists.oasis-open.org/archives/virtio-dev/201905/msg00013.html

Signed-off-by: Anton Yakovlev 
Signed-off-by: Mikhail Golubev 


Please mind our privacy 
notice
 pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 
DSGVO finden Sie 
hier.

-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org

[virtio-dev] [RFC] snd: Add virtio sound device specification

[Mikhail Golubev]

From: Anton Yakovlev 

The virtio sound device is an extendable virtual audio device. It
provides playback and capture PCM streams to a guest.

The device is implemented as a collection of a device-specific functions
that provide dedicated audio-related functionality. At the moment it
supports the following:

1. Base function (generic device management);
2. PCM function (a playback/capture PCM stream).

Signed-off-by: Anton Yakovlev 
Signed-off-by: Mikhail Golubev 
---
 content.tex  |   1 +
 virtio-sound.tex | 432 +++
 2 files changed, 433 insertions(+)
 create mode 100644 virtio-sound.tex

diff --git a/content.tex b/content.tex
index ee0d7c9..6a7f9a3 100644
--- a/content.tex
+++ b/content.tex
@@ -5677,6 +5677,7 @@ \subsubsection{Legacy Interface: Framing 
Requirements}\label{sec:Device
 \input{virtio-input.tex}
 \input{virtio-crypto.tex}
 \input{virtio-vsock.tex}
+\input{virtio-sound.tex}

 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}

diff --git a/virtio-sound.tex b/virtio-sound.tex
new file mode 100644
index 000..4039d64
--- /dev/null
+++ b/virtio-sound.tex
@@ -0,0 +1,432 @@
+\section{Sound Device}\label{sec:Device Types / Sound Device}
+
+The virtio sound card is an extendable virtual audio device. It provides its
+functionality in a form of functions that can be configured and managed in an
+independent way.
+
+\subsection{Device ID}\label{sec:Device Types / Sound Device / Device ID}
+
+25
+
+\subsection{Virtqueues}\label{sec:Device Types / Sound Device / Virtqueues}
+
+Depending on provided functionality, a device can use one or more virtqueues
+described in a configuration space. A virtqueue assigned to the base function
+is mandatory and referred as a control queue.
+
+\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
+
+There are currently no feature bits defined for this device.
+
+\subsection{Device configuration layout}\label{sec:Device Types / Sound Device 
/ Device configuration layout}
+
+Configuration space provides information about available virtqueues and their
+assignments to enabled functions using following layout structure and 
definitions:
+
+\begin{lstlisting}
+enum {
+/* function identifiers for virtqueue assignments */
+VIRTIO_SND_FN_BASE = 0,
+VIRTIO_SND_FN_PCM
+};
+
+struct virtio_snd_queue_info {
+/* VIRTIO_SND_FN_* */
+le16 function;
+/* a function device id */
+u8 device_id;
+/* a function subdevice id */
+u8 subdevice_id;
+};
+
+struct virtio_snd_config {
+/* maximum # of available virtqueues */
+le32 nqueues;
+};
+\end{lstlisting}
+
+The \field{nqueues} field contains a maximum amount of available virtqueues and
+is followed by an \field{nqueues}-sized array of \field{struct 
virtio_snd_queue_info}
+structures. \field{struct virtio_snd_queue_info} with index \field{i} describes
+a device function assigned to a virtqueue with index \field{i}. 
\field{device_id}
+and \field{subdevice_id} fields are used to distinguish different instances of
+the same function type.
+
+\subsection{Device Initialization}\label{sec:Device Types / Sound Device / 
Device Initialization}
+
+\begin{enumerate}
+\item Initialize all available virtqueues.
+\item Retrieve a device configuration.
+\item Initialize all functions enabled in the device configuration.
+\end{enumerate}
+
+A device configuration is represented with a set of descriptors having
+a fixed header using the following layout structure and definitions:
+
+\begin{lstlisting}
+enum {
+/* the PCM function descriptor types */
+VIRTIO_SND_DESC_PCM = 0,
+VIRTIO_SND_DESC_PCM_STREAM
+};
+
+struct virtio_snd_desc {
+u8 length;
+u8 type;
+
+u16 padding;
+};
+\end{lstlisting}
+
+The fixed header \field{struct virtio_snd_desc} in each
+descriptor includes the following fields:
+
+\begin{description}
+\item[\field{length}] specifies the total number of bytes in the descriptor.
+\item[\field{type}] identifies the descriptor type (VIRTIO_SND_DESC_*).
+\end{description}
+
+With the exception of the base function, all implemented functions MUST add
+their descriptors in configuration only if a particular function (or part of
+its functionality) is enabled in the device.
+
+\subsection{Device Operation}\label{sec:Device Types / Sound Device / Device 
Operation}
+
+All control messages are placed into a single control queue.
+
+All requests and responses on the queue consist of or are preceded by
+a header using the following layout structure and definitions:
+
+\begin{lstlisting}
+enum {
+/* the base function request types */
+VIRTIO_SND_R_BASE_GET_CFG = 0,
+
+/* the PCM function request types */
+VIRTIO_SND_R_PCM_SET_FORMAT = 0x0100,
+VIRTIO_SND_R_PCM_PREPARE,
+VIRTIO_SND_R_PCM_START,
+VIRTIO_SND_R_PCM_STOP,
+VIRTIO_SND_R_PCM_PAUSE,
+VIRTIO_SND_R_PCM_UNPAUSE,
+VIRTIO_SND_R_PCM_QUERY_CHMAP,
+VIRTIO_SND_R_PCM_USE_CHMAP,
+
+/* response 

[virtio-dev] Re: [RFC][Patch v12 1/2] mm: page_reporting: core infrastructure

[Nitesh Narayan Lal]

On 8/15/19 7:00 PM, Alexander Duyck wrote:
> On Thu, Aug 15, 2019 at 12:23 PM Nitesh Narayan Lal  wrote:
[...]
>>> +}
>>> +
>>> +/**
>>> + * __page_reporting_enqueue - tracks the freed page in the respective 
>>> zone's
>>> + * bitmap and enqueues a new page reporting job to the workqueue if 
>>> possible.
>>> + */
>>> +void __page_reporting_enqueue(struct page *page)
>>> +{
>>> +   struct page_reporting_config *phconf;
>>> +   struct zone *zone;
>>> +
>>> +   rcu_read_lock();
>>> +   /*
>>> +* We should not process this page if either page reporting is 
>>> not
>>> +* yet completely enabled or it has been disabled by the 
>>> backend.
>>> +*/
>>> +   phconf = rcu_dereference(page_reporting_conf);
>>> +   if (!phconf)
>>> +   return;
>>> +
>>> +   zone = page_zone(page);
>>> +   bitmap_set_bit(page, zone);
>>> +
>>> +   /*
>>> +* We should not enqueue a job if a previously enqueued 
>>> reporting work
>>> +* is in progress or we don't have enough free pages in the 
>>> zone.
>>> +*/
>>> +   if (atomic_read(>free_pages) >= phconf->max_pages &&
>>> +   !atomic_cmpxchg(>refcnt, 0, 1))
>> This doesn't make any sense to me. Why are you only incrementing the
>> refcount if it is zero? Combining this with the assignment above, this
>> isn't really a refcnt. It is just an oversized bitflag.
> The intent for having an extra variable was to ensure that at a time only 
> one
> reporting job is enqueued. I do agree that for that purpose I really 
> don't need
> a reference counter and I should have used something like bool
> 'page_hinting_active'. But with bool, I think there could be a possible 
> chance
> of race. Maybe I should rename this variable and keep it as atomic.
> Any thoughts?
 You could just use a bitflag to achieve what you are doing here. That
 is the primary use case for many of the test_and_set_bit type
 operations. However one issue with doing it as a bitflag is that you
 have no way of telling that you took care of all requesters.
>>> I think you are right, I might end up missing on certain reporting
>>> opportunities in some special cases. Specifically when the pages which are
>>> part of this new reporting request belongs to a section of the bitmap which
>>> has already been scanned. Although, I have failed to reproduce this kind of
>>> situation in an actual setup.
>>>
  That is
 where having an actual reference count comes in handy as you know
 exactly how many zones are requesting to be reported on.
>>> True.
>>>
>> Also I am pretty sure this results in the opportunity to miss pages
>> because there is nothing to prevent you from possibly missing a ton of
>> pages you could hint on if a large number of pages are pushed out all
>> at once and then the system goes idle in terms of memory allocation
>> and freeing.
> I was looking at how you are enqueuing/processing reporting jobs for each 
> zone.
> I am wondering if I should also consider something on similar lines as 
> having
> that I might be able to address the concern which you have raised above. 
> But it
> would also mean that I have to add an additional flag in the zone_flags. 
> :)
 You could do it either in the zone or outside the zone as yet another
 bitmap. I decided to put the flags inside the zone because there was a
 number of free bits there and it should be faster since we were
 already using the zone structure.
>>> There are two possibilities which could happen while I am reporting:
>>> 1. Another request might come in for a different zone.
>>> 2. Another request could come in for the same zone and the pages belong to a
>>> section of the bitmap which has already been scanned.
>>>
>>> Having a per zone flag to indicate reporting status will solve the first
>>> issue and to an extent the second as well. Having refcnt will possibly solve
>>> both of them. What I am wondering about is that in my case I could easily
>>> impact the performance negatively by performing more bitmap scanning.
>>>
>>>
>> I realized that it may not be possible for me to directly adopt either refcnt
>> or zone flags just because of the way I have page reporting setup right now.
>>
>> For now, I will just replace the refcnt with a bitflag as that should work
>> for most of the cases.  Nevertheless, I will also keep looking for a better 
>> way.
> If nothing else something you could consider is a refcnt for the
> number of bits you have set in your bitfield. Then all you would need
> to be doing is replace the cmpxchg with just a atomic_fetch_inc and
> what you would need to do is have your worker thread track how many
> bits it has cleared and subtract that from