Re: [virtio-dev][RFC PATCH v1 2/2] virtio-gpu: add the ability to export resources

2020-01-09 Thread Gurchetan Singh
I like the idea of having one central place in the kernel where
virtio-devices get their uuid from -- i.e, no separate VM specific,
device specific implementations calling into uuid_gen().

On Wed, Jan 8, 2020 at 3:20 AM David Stevens  wrote:
>
> > Is there a specific reason why you want the host pick the uuid?  I would
> > let the guest define the uuid, i.e. move the uuid fields to
> > virtio_gpu_export_resource and scratch virtio_gpu_resp_export_resource.
>
> Sending the uuid in the original request doesn't really buy us
> anything, at least in terms of asynchronicity. The guest would still
> need to wait for the response to arrive before it could safely pass
> the uuid to any other virtio devices, to prevent a race where the
> import fails because it is processed before virtio-gpu processes the
> export. Perhaps this wouldn't be the case if we supported sharing
> fences between virtio devices, but even then, fences are more of a
> thing for the operation of a pipeline, not for the setup of a
> pipeline.
>
> At that point, I think it's just a matter of aesthetics. I lean
> slightly towards returning the uuid from the host, since that rules
> out any implementation with the aforementioned race. That being said,
> if there are any specific reasons or preferences to assigning the uuid
> from the guest, I can switch to that direction.
>
> -David

-
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] Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3

2020-01-09 Thread Michael S. Tsirkin
On Fri, Jan 10, 2020 at 12:06:06AM +0800, Liu, Jiang wrote:
> On Jan 5, 2020, at 6:42 PM, Michael S. Tsirkin  wrote:
> > 
> > On Thu, Dec 26, 2019 at 09:16:19PM +0800, Liu, Jiang wrote:
> >>> 2) The mask and unmask control is missed
> >>> 
> >>> 
>  but the extension doesn’t support 3) because
>  we noticed that the Linux virtio subsystem doesn’t really make use of 
>  interrupt masking/unmasking.
> > 
> > Linux uses masking/unmasking in order to migrate interrupts between
> > CPUs.
> This is a limitation of the PCI MSI/MSIx spec.
> To update the MSI/MSIx vector configuration, we need to write to 
> msg_high/msg_low/msg_data registers.
> Because write to three 32-bit registers is not an atomic operation on PCI 
> bus, so it may cause incorrect interrupt delivery if interrupt happens after 
> writing 1 or 2 registers.
> When Intel remapping is enabled on x86 platforms, we don’t need to 
> mask/unmask PCI MSI/MSIx interrupts when setting affinity.
> For MMIO MSI extension, we have special design to solve this race window. The 
> flow to update MMIO MSI vector configuration is:
> 1) write msg_high
> 2) write msg_low
> 3) write msg_data
> 4) write the command register to update the vector configuration.
> During step 1-3, the hardware/device backend driver only caches the value 
> written. And update the vector configuration in step 4, so it’s an atomic 
> operation now.
> So mask/unmask becomes optional for MMIO MSI interrupts.

Oh I see. That needs some documentation I guess.

One question though: how do you block VQ from generating interrupts?

There's value in doing that for performance since then device can avoid
re-checking interrupt enable/event idx values in memory.



> > 
> > -- 
> > MST


-
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] Re: [RFC] virtio video driver

2020-01-09 Thread Dmitry Sepp
Hi Keiichi, Tomasz,

On Montag, 6. Januar 2020 07:44:51 CET Keiichi Watanabe wrote:
> Hi Dmitry,
> 
> On Fri, Dec 27, 2019 at 11:05 PM Dmitry Sepp
> 
>  wrote:
> > Hi Keiichi,
> > 
> > On Mittwoch, 25. Dezember 2019 12:38:55 CET Keiichi Watanabe wrote:
> > > Hi Dmitry,
> > > 
> > > Thanks for sharing the driver implementation.
> > > Since the virtio protocol specification is still under discussion in
> > > virtio-dev@ ML separately, let me add comments only about
> > > implementation that are irrelevant to the protocol details.
> > > Please find my comments inline.
> > > 
> > > On Fri, Dec 6, 2019 at 1:33 AM Dmitry Sepp 
> > 
> > wrote:
> > > > From: Kiran Pawar 
> > > > 
> > > > This adds a Virtio based video driver for video streaming device that
> > > > operates input and/or output data buffers to share video devices with
> > > > several guests. The current implementation consist of V4L2 based video
> > > > driver supporting video functions of decoder and encoder. This can be
> > > > extended for other functions e.g. processor, capture and output.
> > > > The device uses descriptor structures to advertise and negotiate
> > > > stream
> > > > formats and controls. This allows the driver to modify the processing
> > > > logic of the device on a per stream basis.
> > > > 
> > > > Signed-off-by: Dmitry Sepp 
> > > > Signed-off-by: Kiran Pawar 
> > > > Signed-off-by: Nikolay Martyanov 
> > > > ---
> > > > 
> > > >  drivers/media/Kconfig  |1 +
> > > >  drivers/media/Makefile |2 +
> > > >  drivers/media/virtio/Kconfig   |   11 +
> > > >  drivers/media/virtio/Makefile  |   12 +
> > > >  drivers/media/virtio/virtio_video.h|  372 +++
> > > >  drivers/media/virtio/virtio_video_caps.c   |  618 +++
> > > >  drivers/media/virtio/virtio_video_dec.c|  947 +
> > > >  drivers/media/virtio/virtio_video_dec.h|   30 +
> > > >  drivers/media/virtio/virtio_video_device.c |  747 +
> > > >  drivers/media/virtio/virtio_video_driver.c |  278 +
> > > >  drivers/media/virtio/virtio_video_enc.c| 1124
> > > >  
> > > >  drivers/media/virtio/virtio_video_enc.h|   30 +
> > > >  drivers/media/virtio/virtio_video_vq.c |  950 +
> > > >  include/uapi/linux/virtio_ids.h|2 +
> > > >  include/uapi/linux/virtio_video.h  |  456 
> > > >  15 files changed, 5580 insertions(+)
> > > >  create mode 100644 drivers/media/virtio/Kconfig
> > > >  create mode 100644 drivers/media/virtio/Makefile
> > > >  create mode 100644 drivers/media/virtio/virtio_video.h
> > > >  create mode 100644 drivers/media/virtio/virtio_video_caps.c
> > > >  create mode 100644 drivers/media/virtio/virtio_video_dec.c
> > > >  create mode 100644 drivers/media/virtio/virtio_video_dec.h
> > > >  create mode 100644 drivers/media/virtio/virtio_video_device.c
> > > >  create mode 100644 drivers/media/virtio/virtio_video_driver.c
> > > >  create mode 100644 drivers/media/virtio/virtio_video_enc.c
> > > >  create mode 100644 drivers/media/virtio/virtio_video_enc.h
> > > >  create mode 100644 drivers/media/virtio/virtio_video_vq.c
> > > >  create mode 100644 include/uapi/linux/virtio_video.h
> > > > 
> > > > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > > > index b36a41332867..cc95806e8f8b 100644
> > > > --- a/drivers/media/Kconfig
> > > > +++ b/drivers/media/Kconfig
> > > > @@ -170,6 +170,7 @@ source "drivers/media/pci/Kconfig"
> > > > 
> > > >  source "drivers/media/platform/Kconfig"
> > > >  source "drivers/media/mmc/Kconfig"
> > > >  source "drivers/media/radio/Kconfig"
> > > > 
> > > > +source "drivers/media/virtio/Kconfig"
> > > > 
> > > >  comment "Supported FireWire (IEEE 1394) Adapters"
> > > >  
> > > > depends on DVB_CORE && FIREWIRE
> > > > 
> > > > diff --git a/drivers/media/Makefile b/drivers/media/Makefile
> > > > index f215f0a89f9e..9517a6f724d1 100644
> > > > --- a/drivers/media/Makefile
> > > > +++ b/drivers/media/Makefile
> > > > @@ -25,6 +25,8 @@ obj-y += rc/
> > > > 
> > > >  obj-$(CONFIG_CEC_CORE) += cec/
> > > > 
> > > > +obj-$(CONFIG_VIDEO_VIRTIO)  += virtio/
> > > > +
> > > > 
> > > >  #
> > > >  # Finally, merge the drivers that require the core
> > > >  #
> > > > 
> > > > diff --git a/drivers/media/virtio/Kconfig
> > > > b/drivers/media/virtio/Kconfig
> > > > new file mode 100644
> > > > index ..3289bcf233f0
> > > > --- /dev/null
> > > > +++ b/drivers/media/virtio/Kconfig
> > > > @@ -0,0 +1,11 @@
> > > > +# SPDX-License-Identifier: GPL-2.0+
> > > > +# Video driver for virtio
> > > > +
> > > > +config VIDEO_VIRTIO
> > > 
> > > "VIRTIO_VIDEO" would be better like VIRTIO_NET and VIRTIO_PCI.
> > 
> > Ok, I agree.
> > 
> > > > +   tristate "Virtio video V4L2 driver"
> > > > +   depends on VIRTIO && VIDEO_DEV && VIDEO_V4L2
> > > > +   select VIDEOBUF2_DMA_SG
> > > > +   select V4L2_MEM2MEM_DEV
> > > 
> > >

[virtio-dev] Re: [PATCH v2 0/1] VirtIO video device specification

2020-01-09 Thread Dmitry Sepp
Hi,

On Dienstag, 7. Januar 2020 11:25:56 CET Keiichi Watanabe wrote:
> Hi Dmitry,
> 
> On Mon, Jan 6, 2020 at 8:28 PM Dmitry Sepp  
wrote:
> > Hi,
> > 
> > On Montag, 6. Januar 2020 11:30:22 CET Keiichi Watanabe wrote:
> > > Hi Dmitry, Tomasz,
> > > 
> > > On Fri, Jan 3, 2020 at 10:05 PM Dmitry Sepp
> > > 
> > 
> > wrote:
> > > > Hi Tomasz, Keiichi,
> > > > 
> > > > On Samstag, 21. Dezember 2019 07:19:23 CET Tomasz Figa wrote:
> > > > > On Sat, Dec 21, 2019 at 3:18 PM Tomasz Figa  
wrote:
> > > > > > On Sat, Dec 21, 2019 at 1:36 PM Keiichi Watanabe
> > > > > > 
> > > > 
> > > > wrote:
> > > > > > > Hi Dmitry,
> > > > > > > 
> > > > > > > On Sat, Dec 21, 2019 at 12:59 AM Dmitry Sepp
> > > > > > > 
> > > > > > >  wrote:
> > > > > > > > Hi Keiichi,
> > > > > > > > 
> > > > > > > > On Mittwoch, 18. Dezember 2019 14:02:13 CET Keiichi Watanabe
> > 
> > wrote:
> > > > > > > > > Hi,
> > > > > > > > > This is the 2nd version of virtio-video patch. The PDF is
> > > > > > > > > available
> > > > > > > > > in [1].
> > > > > > > > > The first version was sent at [2].
> > > > > > > > > 
> > > > > > > > > Any feedback would be appreciated. Thank you.
> > > > > > > > > 
> > > > > > > > > Best,
> > > > > > > > > Keiichi
> > > > > > > > > 
> > > > > > > > > [1]:
> > > > > > > > > https://drive.google.com/drive/folders/1eT5fEckBoor2iHZR4f4G
> > > > > > > > > LxYz
> > > > > > > > > FMVa
> > > > > > > > > pOFx?us
> > > > > > > > > p=sharing [2]: https://markmail.org/message/gc6h25acct22niut
> > > > > > > > > 
> > > > > > > > > Change log:
> > > > > > > > > 
> > > > > > > > > v2:
> > > > > > > > > * Removed functionalities except encoding and decoding.
> > > > > > > > > * Splited encoder and decoder into different devices that
> > > > > > > > > use
> > > > > > > > > the
> > > > > > > > > same
> > > > > > > > > protocol. * Replaced GET_FUNCS with GET_CAPABILITY.
> > > > > > > > > * Updated structs for capabilities.
> > > > > > > > > 
> > > > > > > > >   - Defined new structs and enums such as image formats,
> > > > > > > > >   profiles,
> > > > > > > > >   range
> > > > > > > > > 
> > > > > > > > > (min, max, step), etc
> > > > > > > > > 
> > > > > > > > > * For virtio_video_pixel_format, chose a naming
> > > > > > > > > convention
> > > > > > > > > that
> > > > > > > > > is used
> > > > > > > > > 
> > > > > > > > >   in DRM. We removed XBGR, NV21 and I422, as they are
> > > > > > > > >   not
> > > > > > > > >   used
> > > > > > > > >   in the
> > > > > > > > >   current draft implementation.
> > > > > > > > >   https://lwn.net/Articles/806416/
> > > > > > > > >   
> > > > > > > > >   - Removed virtio_video_control, whose usage was not
> > > > > > > > >   documented
> > > > > > > > >   yet
> > > > > > > > >   and
> > > > > > > > > 
> > > > > > > > > which is not necessary for the simplest decoding scenario.
> > > > > > > > > 
> > > > > > > > >   - Removed virtio_video_desc, as it is no longer needed.
> > > > > > > > > 
> > > > > > > > > * Updated struct virtio_video_config for changes around
> > > > > > > > > capabilities.
> > > > > > > > > * Added a way to represent supported combinations of
> > > > > > > > > formats.
> > > > > > > > > 
> > > > > > > > >   - A field "mask" in virtio_video_format_desc plays this
> > > > > > > > >   role.
> > > > > > > > > 
> > > > > > > > > * Removed VIRTIO_VIDEO_T_STREAM_{START,STOP} because they
> > > > > > > > > don't
> > > > > > > > > play
> > > > > > > > > any
> > > > > > > > > meaningful roles. * Removed VIRTIO_VIDEO_T_STREAM_{ATTACH,
> > > > > > > > > DETACH}_BACKING
> > > > > > > > > and merged them into RESOURCE_{CREATE, DESTROY}. * Added a
> > > > > > > > > way
> > > > > > > > > to
> > > > > > > > > notify/specify resource creation method.
> > > > > > > > > 
> > > > > > > > >   - Added a feature flag.
> > > > > > > > >   - Defined enum virtio_video_mem_type.
> > > > > > > > >   - Added new fields in video_stream_create.
> > > > > > > > > 
> > > > > > > > > * Modified fields in virtio_video_params.
> > > > > > > > > 
> > > > > > > > >   - Added crop information.
> > > > > > > > > 
> > > > > > > > > * Removed enum virtio_video_channel_type because we can get
> > > > > > > > > this
> > > > > > > > > information by image format.
> > > > > > > > 
> > > > > > > > Could you please explain this? How do you get the information?
> > > > > > > 
> > > > > > > It means that if image formats are well-defined, channel
> > > > > > > information
> > > > > > > (e.g. the order of channels) is uniquely determined.
> > > > > > > 
> > > > > > > > Suppose you have some piece of HW on the host side that wants
> > > > > > > > I420
> > > > > > > > as
> > > > > > > > one
> > > > > > > > contig buffer w/ some offsets. But on the driver side, say,
> > > > > > > > gralloc
> > > > > > > > gives you three separate buffers, one per channel. How do we
> > > > > > > > pass
> > > > > > > > those to the device then?
> > > > > > > 
> > > > > > > You're talking about CrOS use cas

Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification

2020-01-09 Thread Tomasz Figa
On Wed, Jan 8, 2020 at 10:52 PM Keiichi Watanabe  wrote:
>
> Hi Gerd,
>
> On Thu, Dec 19, 2019 at 10:12 PM Gerd Hoffmann  wrote:
> >
> >   Hi,
> >
> > > > However that still doesn't let the driver know which buffers will be
> > > > dequeued when. A simple example of this scenario is when the guest is
> > > > done displaying a frame and requeues the buffer back to the decoder.
> > > > Then the decoder will not choose it for decoding next frames into as
> > > > long as the frame in that buffer is still used as a reference frame,
> > > > even if one sends the drain request.
> > > It might be that I'm getting your point wrong, but do you mean some 
> > > hardware
> > > can mark a buffer as ready to be displayed yet still using the underlying
> > > memory to decode other frames?
> >
> > Yes, this is how I understand Tomasz Figa.
> >
> > > This means, if you occasionally/intentionally
> > > write to the buffer you mess up the whole decoding pipeline.
> >
> > And to avoid this the buffer handling aspect must be clarified in the
> > specification.  Is the device allowed to continue using the buffer after
> > finishing decoding and completing the queue request?  If so, how do we
> > hand over buffer ownership back to the driver so it can free the pages?
> > drain request?  How do we handle re-using buffers?  Can the driver
> > simply re-queue them and expect the device figures by itself whenever it
> > can use the buffer or whenever it is still needed as reference frame?
>
> The device shouldn't be able to access buffers after it completes a
> queue request.
> The device can touch buffer contents from when a queue request is sent
> until the device responds it.
> In contrast, the driver must not modify buffer contents in that period.
>
> Regarding re-using, the driver can simply re-queue buffers returned by
> the device. If the device needs a buffer as reference frame, it must
> not return the buffer.

I think that might not be what we expect. We want the decoder to
return a decoded frame as soon as possible, but that decoded frame may
be also needed for decoding next frames. In V4L2 stateful decoder, the
API is defined that the client must not modify the decoded
framebuffer, otherwise decoding next frames may not be correct. We may
need something similar, with an explicit operation that makes the
buffers not accessible to the host anymore. I think the queue flush
operation could work as such.

> I'll describe this rule in the next version of the patch.
>
> Best regards,
> Keiichi
>
> >
> > cheers,
> >   Gerd
> >

-
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] [PATCH v5] virtio-snd: add virtio sound device specification

2020-01-09 Thread Anton Yakovlev
This patch proposes virtio specification for a new virtio sound device,
that may be useful in case when having audio is required but a device
passthrough or emulation is not an option.

Signed-off-by: Anton Yakovlev 
---

v4 -> v5 changes:
1. Insert the virtio_snd_hdr to the virtio_snd_event structure.
2. Rephrase field description in structures.
3. Resize the features and rates fields in a stream configuration.
4. Replace MUST with SHOULD in queued buffer size requirements.

 conformance.tex  |  24 ++
 content.tex  |   1 +
 introduction.tex |   3 +
 virtio-sound.tex | 700 +++
 4 files changed, 728 insertions(+)
 create mode 100644 virtio-sound.tex

diff --git a/conformance.tex b/conformance.tex
index 50969e5..b8c6836 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -191,6 +191,17 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
 \item \ref{drivernormative:Device Types / RPMB Device / Device Operation}
 \end{itemize}
 
+\conformance{\subsection}{Sound Driver Conformance}\label{sec:Conformance / 
Driver Conformance / Sound Driver Conformance}
+
+A sound driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / Sound Device / Device Initialization}
+\item \ref{drivernormative:Device Types / Sound Device / Device Operation / 
PCM Stream Parameters}
+\item \ref{drivernormative:Device Types / Sound Device / Device Operation / 
PCM Output Stream}
+\item \ref{drivernormative:Device Types / Sound Device / Device Operation / 
PCM Input Stream}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device 
Conformance}
 
 A device MUST conform to the following normative statements:
@@ -360,6 +371,19 @@ \section{Conformance Targets}\label{sec:Conformance / 
Conformance Targets}
 \item \ref{devicenormative:Device Types / RPMB Device / Device Operation}
 \end{itemize}
 
+\conformance{\subsection}{Sound Device Conformance}\label{sec:Conformance / 
Device Conformance / Sound Device Conformance}
+
+A sound device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / Sound Device / Device Initialization}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / 
Jack Configuration}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / 
PCM Stream Configuration}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / 
PCM Stream Parameters}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / 
PCM Output Stream}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / 
PCM Input Stream}
+\end{itemize}
+
 \conformance{\section}{Legacy Interface: Transitional Device and Transitional 
Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional 
Device and Transitional Driver Conformance}
 A conformant implementation MUST be either transitional or
 non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index d68cfaf..70ad066 100644
--- a/content.tex
+++ b/content.tex
@@ -5739,6 +5739,7 @@ \subsubsection{Legacy Interface: Framing 
Requirements}\label{sec:Device
 \input{virtio-vsock.tex}
 \input{virtio-fs.tex}
 \input{virtio-rpmb.tex}
+\input{virtio-sound.tex}
 
 \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
diff --git a/introduction.tex b/introduction.tex
index 33da3ec..07674f8 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -66,6 +66,9 @@ \section{Normative References}\label{sec:Normative References}
 \phantomsection\label{intro:eMMC}\textbf{[eMMC]} &
 eMMC Electrical Standard (5.1), JESD84-B51,
 
\newline\url{http://www.jedec.org/sites/default/files/docs/JESD84-B51.pdf}\\
+   \phantomsection\label{intro:HDA}\textbf{[HDA]} &
+   High Definition Audio Specification,
+   
\newline\url{https://www.intel.com/content/dam/www/public/us/en/documents/product-specifications/high-definition-audio-specification.pdf}\\
 
 \end{longtable}
 
diff --git a/virtio-sound.tex b/virtio-sound.tex
new file mode 100644
index 000..2cd3f2e
--- /dev/null
+++ b/virtio-sound.tex
@@ -0,0 +1,700 @@
+\section{Sound Device}\label{sec:Device Types / Sound Device}
+
+The virtio sound card is a virtual audio device supporting input, output and
+bidirectional PCM streams.
+
+A device is managed by control requests and can send various notifications
+through dedicated queues. A driver can transmit PCM frames using message-based
+transport or shared memory.
+
+\subsection{Device ID}\label{sec:Device Types / Sound Device / Device ID}
+
+25
+
+\subsection{Virtqueues}\label{sec:Device Types / Sound Device / Virtqueues}
+
+\begin{description}
+\item[0] controlq
+\item[1] eventq
+\item[2] txq
+\item[3] rxq
+\end{description}
+
+The control queue is used for sending control messages from the driver to
+the device.
+
+The event q

[virtio-dev] Re: [PATCH v4] virtio-snd: add virtio sound device specification

2020-01-09 Thread Gerd Hoffmann
  Hi,

> > Repeating "device-readable" or "device-writable" for each struct field
> > looks a bit odd because this applies to the whole struct.  Not so much
> > for these structs with a single field only, but there are structs with
> > more fields further down the spec ...
> 
> Well, I'm not sure here. Previously, I was told to mark explicitly every
> field.

Was that for virtqueue structs, or for config space?

For config space which allows random access it surely makes sense.  In
contrast the virtqueue has in and out buffers, so you requests structs
are placed in "out" buffers and are device-readable. whereas response
structs are placed in "in" buffers and are device-writable.

> Also, regarding requirement for populating tx/rx queues with period size
> buffers. Maybe it's better to replace MUST with SHOULD? It would give more
> flexibility for the driver. What do you think?

I think relaxing this to SHOULD is fine.  I can't think of any bad side
effects of that.   The driver obviously can't use the buffer completion
as period notification then.  But that is the driver's choice and it
wouldn't break the virtio protocol ...

cheers,
  Gerd


-
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] Re: [PATCH RFC v4 00/13] virtio-mem: paravirtualized memory

2020-01-09 Thread David Hildenbrand
On 12.12.19 18:11, David Hildenbrand wrote:
> This series is based on latest linux-next. The patches are located at:
> https://github.com/davidhildenbrand/linux.git virtio-mem-rfc-v4
> 
> The basic idea of virtio-mem is to provide a flexible,
> cross-architecture memory hot(un)plug solution that avoids many limitations
> imposed by existing technologies, architectures, and interfaces. More
> details can be found below and in linked material.
> 
> This RFC is limited to x86-64, however, should theoretically work on any
> architecture that supports virtio and implements memory hot(un)plug under
> Linux - like s390x, powerpc64 and arm64. On x86-64, it is currently
> possible to add/remove memory to the system in >= 4MB granularity.
> Memory hotplug works very reliably. For memory unplug, there are no
> guarantees how much memory can actually get unplugged, it depends on the
> setup (especially: fragmentation of (unmovable) memory). I have plans to
> improve that in the future.
> 
> --
> 1. virtio-mem
> --
> 
> The basic idea behind virtio-mem was presented at KVM Forum 2018. The
> slides can be found at [1]. The previous RFC can be found at [2]. The
> first RFC can be found at [3]. However, the concept evolved over time. The
> KVM Forum slides roughly match the current design.
> 
> Patch #2 ("virtio-mem: Paravirtualized memory hotplug") contains quite some
> information, especially in "include/uapi/linux/virtio_mem.h":
> 
>   Each virtio-mem device manages a dedicated region in physical address
>   space. Each device can belong to a single NUMA node, multiple devices
>   for a single NUMA node are possible. A virtio-mem device is like a
>   "resizable DIMM" consisting of small memory blocks that can be plugged
>   or unplugged. The device driver is responsible for (un)plugging memory
>   blocks on demand.
> 
>   Virtio-mem devices can only operate on their assigned memory region in
>   order to (un)plug memory. A device cannot (un)plug memory belonging to
>   other devices.
> 
>   The "region_size" corresponds to the maximum amount of memory that can
>   be provided by a device. The "size" corresponds to the amount of memory
>   that is currently plugged. "requested_size" corresponds to a request
>   from the device to the device driver to (un)plug blocks. The
>   device driver should try to (un)plug blocks in order to reach the
>   "requested_size". It is impossible to plug more memory than requested.
> 
>   The "usable_region_size" represents the memory region that can actually
>   be used to (un)plug memory. It is always at least as big as the
>   "requested_size" and will grow dynamically. It will only shrink when
>   explicitly triggered (VIRTIO_MEM_REQ_UNPLUG).
> 
>   Memory in the usable region can usually be read, however, there are no
>   guarantees. It can happen that the device cannot process a request,
>   because it is busy. The device driver has to retry later.
> 
>   Usually, during system resets all memory will get unplugged, so the
>   device driver can start with a clean state. However, in specific
>   scenarios (if the device is busy) it can happen that the device still
>   has memory plugged. The device driver can request to unplug all memory
>   (VIRTIO_MEM_REQ_UNPLUG) - which might take a while to succeed if the
>   device is busy.
> 
> --
> 2. Linux Implementation
> --
> 
> This RFC reuses quite some existing MM infrastructure, however, has to
> expose some additional functionality.
> 
> Memory blocks (e.g., 128MB) are added/removed on demand. Within these
> memory blocks, subblocks (e.g., 4MB) are plugged/unplugged. The sizes
> depend on the target architecture, MAX_ORDER + pageblock_order, and
> the block size of a virtio-mem device.
> 
> add_memory()/try_remove_memory() is used to add/remove memory blocks.
> virtio-mem will not online memory blocks itself. This has to be done by
> user space, or configured into the kernel
> (CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE). virtio-mem will only unplug memory
> that was online to the ZONE_NORMAL. Memory is suggested to be onlined to
> the ZONE_NORMAL for now.
> 
> The memory hotplug notifier is used to properly synchronize against
> onlining/offlining of memory blocks and to track the states of memory
> blocks (including the zone memory blocks are onlined to).
> 
> The set_online_page() callback is used to keep unplugged subblocks
> of a memory block fake-offline when onlining the memory block.
> generic_online_page() is used to fake-online plugged subblocks. This
> handling is similar to the Hyper-V balloon driver.
> 
> PG_offline is used to mark unplugged subblocks as offline, so e.g.,
> dumping tools (makedumpfile) will skip these pages. This is similar to

Re: [virtio-dev][RFC PATCH v1 1/2] content: define what exporting a resource is

2020-01-09 Thread David Stevens
> > that isn't just a leaf node of the spec. I think it's better to define
> > 'resource' as a top level concept for virtio devices, even if the specifics
> > of what a 'resource' is are defined by individual device types.
>
> Your patch doesn't define what a resource is though.  It only refers to
> something it calls 'resource' ...

Reading it again, what I wrote was a little ambiguous. Stating things
more clearly, the top level defines an 'exported resource' as a
'resource' associated with a uuid for the purpose of sharing between
different virtio devices. It leaves the definition of what constitutes
a 'resource' to individual device types. Perhaps it would be better to
use 'object' or something instead of 'resource', to avoid the
collision with virtio-gpu resources.

-David

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



Re: [virtio-dev] Re: [PATCH v2 1/1] virtio-video: Add virtio video device specification

2020-01-09 Thread Gerd Hoffmann
  Hi,

> Regarding re-using, the driver can simply re-queue buffers returned by
> the device. If the device needs a buffer as reference frame, it must
> not return the buffer.

Ok, that'll work.

> I'll describe this rule in the next version of the patch.

Good.  You should also add a note about ordering.  If the device returns
the buffers as soon as they are no longer needed for decoding they
might be completed out-of-order, specifically B-Frames might complete
before the reference I/P frame.  Is the device allowed to do that or
should it complete the buffers in playback order?

cheers,
  Gerd


-
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] Re: [PATCH v1 2/2] virtio-mmio: add features for virtio-mmio specification version 3

2020-01-09 Thread Michael S. Tsirkin
On Thu, Jan 09, 2020 at 02:15:51PM +0800, Liu, Jing2 wrote:
> 
> On 1/5/2020 7:04 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 25, 2019 at 10:50:23AM +0800, Zha Bin wrote:
> > > From: Liu Jiang
> > > 
> > > Userspace VMMs (e.g. Qemu microvm, Firecracker) take advantage of using
> > > virtio over mmio devices as a lightweight machine model for modern
> > > cloud. The standard virtio over MMIO transport layer only supports one
> > > legacy interrupt, which is much heavier than virtio over PCI transport
> > > layer using MSI. Legacy interrupt has long work path and causes specific
> > > VMExits in following cases, which would considerably slow down the
> > > performance:
> > > 
> > > 1) read interrupt status register
> > > 2) update interrupt status register
> > > 3) write IOAPIC EOI register
> > > 
> > > We proposed to update virtio over MMIO to version 3[1] to add the
> > > following new features and enhance the performance.
> > > 
> > > 1) Support Message Signaled Interrupt(MSI), which increases the
> > > interrupt performance for virtio multi-queue devices
> > > 2) Support per-queue doorbell, so the guest kernel may directly write
> > > to the doorbells provided by virtio devices.
> > Do we need to come up with new "doorbell" terminology?
> > virtio spec calls these available event notifications,
> > let's stick to this.
> 
> Yes, let's keep virtio words, which just calls notifications.
> 
> > > The following is the network tcp_rr performance testing report, tested
> > > with virtio-pci device, vanilla virtio-mmio device and patched
> > > virtio-mmio device (run test 3 times for each case):
> > > 
> > >   netperf -t TCP_RR -H 192.168.1.36 -l 30 -- -r 32,1024
> > > 
> > >   Virtio-PCIVirtio-MMIO   Virtio-MMIO(MSI)
> > >   trans/s 953669399500
> > >   trans/s 973470299749
> > >   trans/s 989470959318
> > > 
> > > [1]https://lkml.org/lkml/2019/12/20/113
> > > 
> > > Signed-off-by: Liu Jiang
> > > Signed-off-by: Zha Bin
> > > Signed-off-by: Chao Peng
> > > Signed-off-by: Jing Liu
> > Do we need a new version though? What is wrong with
> > a feature bit? This way we can create compatible devices
> > and drivers.
> 
> We considered using 1 feature bit of 24~37 to specify MSI capability, but
> 
> this feature bit only means for mmio transport layer, but not representing
> 
> comment feature negotiation of the virtio device. So we're not sure if this
> is a good choice.

We are not short on bits, just don't use bits below 32
since these are for legacy devices.


> > > [...]
> > > +static void mmio_write_msi_msg(struct msi_desc *desc, struct msi_msg 
> > > *msg)
> > > +{
> > > + struct device *dev = desc->dev;
> > > + struct virtio_device *vdev = dev_to_virtio(dev);
> > > + struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> > > + void __iomem *pos = vm_dev->base;
> > > + uint16_t cmd = VIRTIO_MMIO_MSI_CMD(VIRTIO_MMIO_MSI_CMD_UPDATE,
> > > + desc->platform.msi_index);
> > > +
> > > + writel(msg->address_lo, pos + VIRTIO_MMIO_MSI_ADDRESS_LOW);
> > > + writel(msg->address_hi, pos + VIRTIO_MMIO_MSI_ADDRESS_HIGH);
> > > + writel(msg->data, pos + VIRTIO_MMIO_MSI_DATA);
> > > + writew(cmd, pos + VIRTIO_MMIO_MSI_COMMAND);
> > > +}
> > All this can happen when IRQ affinity changes while device
> > is sending interrupts. An interrupt sent between the writel
> > operations will then be directed incorrectly.
> 
> When investigating kernel MSI behavior, I found in most case there's no
> action during IRQ affinity changes to avoid the interrupt coming.
> 
> For example, when migrate_one_irq, it masks the irq before
> irq_do_set_affinity. But for others, like user setting any irq affinity
> 
> via /proc/, it only holds desc->lock instead of disable/mask irq. In such
> case, how can it ensure the interrupt sending between writel ops?

Could be a bug too. E.g. PCI spec explicitly says it's illegal to
change non-masked interrupts exactly for this reason.



> 
> > > [...]
> > > +
> > > +/* RO: MSI feature enabled mask */
> > > +#define VIRTIO_MMIO_MSI_ENABLE_MASK  0x8000
> > I don't understand the comment. Is this a way for
> > a version 3 device to say "I want/do not want MSI"?
> > Why not just use a feature bit? We are not short on these.
> 
> This is just used for current MSI enabled/disabled status, after all MSI
> configurations setup finished.
> 
> Not for showing MSI capability.
> 
> In other words, since the concern of feature bit, we choose to update the
> virtio mmio
> 
> version that devices with v3 have MSI capability and notifications.
> 
> 
> Thanks,
> 
> Jing

MSI looks like an optimization. I don't see how that
justifies incrementing a major version and breaking
compat with all existing guests.

-- 
MST


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

Re: [virtio-dev][RFC PATCH v1 2/2] virtio-gpu: add the ability to export resources

2020-01-09 Thread Gerd Hoffmann
  Hi,

> At that point, I think it's just a matter of aesthetics. I lean
> slightly towards returning the uuid from the host, since that rules
> out any implementation with the aforementioned race.

Ok, design the API in a way that you can't get it wrong.  Makes sense.
I'd still name it ressource_assign_uuid though.

cheers,
  Gerd


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



Re: [virtio-dev][RFC PATCH v1 1/2] content: define what exporting a resource is

2020-01-09 Thread Gerd Hoffmann
  Hi,

> that isn't just a leaf node of the spec. I think it's better to define
> 'resource' as a top level concept for virtio devices, even if the specifics
> of what a 'resource' is are defined by individual device types.

Your patch doesn't define what a resource is though.  It only refers to
something it calls 'resource' ...

cheers,
  Gerd


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