Re: [virtio-dev] Re: [virtio-comment] Re: [virtio-dev] Re: [Proposal] Relationship between XDP and rx-csum in virtio-net
On Wed, May 24, 2023 at 02:07:14AM -0400, Michael S. Tsirkin wrote:
> On Tue, May 23, 2023 at 09:51:44PM +0800, Heng Qi wrote:
> > On Tue, May 23, 2023 at 09:30:28AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, May 23, 2023 at 05:18:20PM +0800, Heng Qi wrote:
> > > > On Tue, May 23, 2023 at 03:15:37AM -0400, Michael S. Tsirkin wrote:
> > > > > On Tue, May 23, 2023 at 10:41:18AM +0800, Heng Qi wrote:
> > > > > > On Mon, May 22, 2023 at 03:10:05PM -0400, Michael S. Tsirkin wrote:
> > > > > > > On Mon, May 22, 2023 at 08:12:00PM +0800, Heng Qi wrote:
> > > > > > > > 1) Add a feature bit to the virtio specification to tell the
> > > > > > > > sender that a fully
> > > > > > > > csumed packet must be sent.
> > > > > > >
> > > > > > > Who is the sender in this picture? The driver?
> > > > > >
> > > > > > The device or the driver.
> > > > > >
> > > > > > When the device is hw, the sender is more likely to be a device.
> > > > > > When the device is sw, the sender can be a device or a driver.
> > > > > >
> > > > > > But in general, this feature is inclined to constrain the behavior
> > > > > > of the device and
> > > > > > the driver from the receiving side.
> > > > >
> > > > > Based on above I am guessing you are talking about driver getting
> > > > > packets from device, I wish you used terms from virtio spec.
> > > >
> > > > Yes, I'm going to use the terminology of the virtio spec.
> > > >
> > > > >
> > > > > > For example:
> > > > > > VIRTIO_NET_F_UNNECESSARY_CSUM : The driver tells the device that
> > > > > > you must send me a fully csumed packet.
> > > > > >
> > > > > > Then the specific implementation can be
> > > > > >
> > > > > > (1) the sender sends a fully csumed packet;
> > > > > > (2) the receiver receives a CHECKSUM_PARTIAL packet, and the device
> > > > > > helps calculate the fully csum
> > > > > > (because the two parties in the communication are located on
> > > > > > the same host, the packet is trusted.).
> > > > > >
> > > > > > In summary, if VIRTIO_NET_F_UNNECESSARY_CSUM is negotiated, the
> > > > > > driver will no longer receive any packets marked CHECKSUM_PARTIAL.
> > > > > >
> > > > > > Thanks.
> > > > >
> > > > > This is what clearing VIRTIO_NET_F_GUEST_CSUM does.
> > > >
> > > > Yes, but with VIRTIO_NET_F_GUEST_CSUM cleared, although the device can
> > > > receive a fully checksummed packet, we can no longer enjoy
> > > > the device's ability to validate the packet checksum. That is, the value
> > > > of \field{flags} in the virtio_net_hdr structure is set to 0, which
> > > > means
> > > > that the packet received by the driver will not be marked as
> > > > VIRTIO_NET_HDR_F_DATA_VALID.
> > > >
> > > > So, we need a feature bit (let's say VIRTIO_NET_F_UNNECESSARY_CSUM).
> > > > If VIRTIO_NET_F_UNNECESSARY_CSUM is negotiated, the device must give the
> > > > driver a fully checksummed packet, and the packet is validated by the
> > > > device with \field{flags} set to VIRTIO_NET_HDR_F_DATA_VALID.
> > > >
> > > > >
> > > > > I feel you are trying to say that clearing VIRTIO_NET_F_GUEST_CSUM
> > > > > disables all offloads but you want to keep some of them?
> > > > >
> > > >
> > > > No, what I mean is that a feature VIRTIO_NET_F_UNNECESSARY_CSUM is
> > > > needed
> > > > in addition to VIRTIO_NET_F_GUEST_CSUM, if both features are negotiated,
> > > > then the driver may always receive packets marked as
> > > > VIRTIO_NET_HDR_F_DATA_VALID, which means that we can now load XDP at the
> > > > same time.
> > >
> > > Makes no sense to me. VIRTIO_NET_F_GUEST_CSUM set already allows
> > > VIRTIO_NET_HDR_F_DATA_VALID:
> >
> > We need to focus on what happens when the XDP program is loaded:
> >
> > The current virtio-net needs to turn off the VIRTIO_NET_F_GUEST_CSUM
> > feature when loading XDP. The main reason for doing this is because
> > VIRTIO_NET_F_GUEST_CSUM allows to receive packets marked as
> > VIRTIO_NET_HDR_F_NEEDS_CSUM. Such packets are not compatible with
> > XDP programs, because we cannot guarantee that the csum_{start, offset}
> > fields are correct after XDP modifies the packets.
>
> What do you want the device to do with these packets?
> Just clear VIRTIO_NET_HDR_F_NEEDS_CSUM?
> Then ... why don't you have the driver clear that bit?
Just clearing VIRTIO_NET_HDR_F_NEEDS_CSUM will cause the stack to drop
packets when validating the checksum.
>From [1] we know that after negotiating VIRTIO_NET_F_GUEST_CSUM, the
device can choose to save some work, thus generating packets carrying
VIRTIO_NET_HDR_F_NEEDS_CSUM. The new feature just keeping the device from saving
the work it's supposed to be doing. Of course, this will bring some
device calculating overhead, but if the device has sufficient calculating
resources, it can offer the new feature.
[1] “The converse features are also available: a driver can save the
virtual device some work by negotiating these features. Note: For
example, a network packet transported between two guests on the same
system
Re: [virtio-dev][PATCH 2/2] virtio-spi: add the device specification
Hello Cornelia Huck,
Thank you so much for your comments. I respond these comments,
could you please help check again. Really appreciate.
I will raise next proposal to fix these comments.
Best Regards
Haixu Cui
On 4/18/2023 5:10 PM, Cornelia Huck wrote:
On Mon, Apr 17 2023, Haixu Cui wrote:
Meta comment: I think you want to send to virtio-comment (feel free to
keep virtio-dev on cc:).
Yes, I will send to virtio-comment and cc to virtio-dev in subsequent
commits.
virtio-spi is a virtual SPI master and it allows a guset to operate and
use the physical SPI master controlled by the host.
---
device-types/spi/description.tex| 155
device-types/spi/device-conformance.tex | 7 ++
device-types/spi/driver-conformance.tex | 7 ++
3 files changed, 169 insertions(+)
create mode 100644 device-types/spi/description.tex
create mode 100644 device-types/spi/device-conformance.tex
create mode 100644 device-types/spi/driver-conformance.tex
diff --git a/device-types/spi/description.tex b/device-types/spi/description.tex
new file mode 100644
index 000..a68e5f4
--- /dev/null
+++ b/device-types/spi/description.tex
@@ -0,0 +1,155 @@
+\section{SPI Master Device}\label{sec:Device Types / SPI Master Device}
+
+virtio-spi is a virtual SPI(Serial Peripheral Interface) master and it allows a
Nit: missing blank after "SPI"
Got it. Will add blank after SPI.
s/it//
+guest to operate and use the physical SPI master devices controlled by the
host.
+By attaching the host SPI controlled nodes to the virtual SPI master device,
the
+guest can communicate with them without changing or adding extra drivers for
these
+controlled SPI devices.
Can we rewrite this paragraph without relying on the host/guest
terminology? Does
"allows a driver to operate and use the physical SPI master devices
controlled by the device"
capture the essence of what this is doing?
I don't quite understand the second sentence, maybe someone familiar
with SPI can come up with something.
This statement is referred to the Virtio-I2C Chapter, because Virtio-SPI
is very similar to Virtio-SPI in architecture. The guest can communicate
through the physical SPI master without operating the hardware directly,
but calling the physical SPI master driver running on the host. So the
physical SPI driver on the host doesn't need changes any more.
+
+In a typical host and guest architecture with Virtio SPI, Virtio SPI driver
+is the front-end and exists in the guest kernel, Virtio SPI device acts as
s/guest kernel/guest/ ?
Yes, I will update in next commit.
+the back-end and exists in the host. And VirtQueue is used for communication
+between the front-end and the back-end.
"A virtqueue is used for communication between the front-end and the
back-end." ?
[I *think* "virtqueue" is the term we've agreed on?]
Yes, I will remove this line.
+
+\subsection{Device ID}\label{sec:Device Types / SPI Master Device / Device ID}
+45
+
+\subsection{Virtqueues}\label{sec:Device Types / SPI Master Device /
Virtqueues}
+
+\begin{description}
+\item[0] requestq
+\end{description}
+
+\subsection{Feature bits}\label{sec:Device Types / SPI Master Device / Feature
bits}
+
+None.
+
+\subsection{Device configuration layout}\label{sec:Device Types / SPI Master
Device / Device configuration layout}
+
+All fields of this configuration are always available and read-only for the
Virtio SPI driver.
+
+\begin{lstlisting}
+struct virtio_spi_config {
+u32 bus_num;
+u32 chip_select_max_number;
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{bus_num}] is the physical spi master assigned to guset, and this
is SOC-specific.
Maybe better "the physical SPI master controled by the device"?
Because this item is set by host and used by guest, so it seems that
this is assigned to the guset by the host.
+
+\item[\field{chip_select_max_number}] is the number of chipselect the physical
spi master supported.
+\end{description}
+
+\subsection{Device Initialization}\label{sec:Device Types / SPI Master Device
/ Device Initialization}
+
+\begin{itemize}
+\item The Virtio SPI driver configures and initializes the virtqueue.
+
+\item The Virtio SPI driver allocates and registers the virtual SPI master.
What does this mean? Shouldn't we rather only require the driver to set
up the virtqueue and leave details on how to operate beyond the device
<-> driver interface to the implementation?
Yes, I will remove this line in next commit.
+\end{itemize}
+
+\subsection{Device Operation}\label{sec:Device Types / SPI Master Device /
Device Operation}
+
+\subsubsection{Device Operation: Request Queue}\label{sec:Device Types / SPI
Master Device / Device Operation: Request Queue}
+
+The Virtio SPI driver queues requests to the virtqueue, and they are used by
the
+Virtio SPI device. Each request represents one SPI transfer and it is of the
form:
+
+\begin{lstlisting}
+struct virtio_spi_transfer_head {
+
[virtio-dev] Re: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands
On Tue, May 23, 2023 at 10:22:27PM +, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, May 23, 2023 2:49 PM
> > > > iokit is from my previous work experience not virtio.
> > > > For virtio we had a lot of trouble with hacks like this on windows.
> > > I don't know why windows driver need to talk to PF driver until now when
> > > the
> > AQ doesn't exists.
> >
> > What is it you want to know?
> >
> Why a VF driver needs to talk to a PF driver in windows without AQ?
I don't know- it doesn't? If it did, it would be hard as we learned
when trying to support pci-bridge-seat. Doable, but annoying.
> > > > That's an important platform.
> > > >
> > > I am not sure how important is it.
> > > What I learnt that a decade has passed and even after that virtio drivers
> > > are
> > not part of the OS distro.
> >
> > Because with QEMU they are so easy to side-load. So we never bothered.
> >
> I don't understand your comment.
> I was saying to my knowledge a Windows OS do not have a virtio drivers in
> their distro which you said is important.
It's important for windows to work well with virtio. And it does because
we supply an install disk with virtio drivers through QEMU
> > > It doesn't need to because guest is anyway going to do whatever it does.
> >
> > yes. But hypervisor can do work-arounds or detect broken functionality and
> > stop cleanly.
> >
> You mentioned this several times.
> Below requirement still holds fine.
>
> > > The clear requirement is: to not fix legacy but to support legacy.
> > > Fixing legacy is role of 1.x. Not the role of legacy interface itself
> > > using
> > AQ/MMIO.
> >
> > And for purpose of BAR mapping, such BAR in the VF can be mapped too.
> > > You didn't answer why VF BAR is a problem and suggest PF BAR.
> >
> > VF BAR by itself is not a problem. Discovering VF BAR mapping offset
> > through PF
> > is.
> >
> At the end of the email you right that "AQ is fine" but here you write
> discovering that through the PF AQ is.
> How to interpret your comment?
>
> > > If notification for VF is in VF -> hence other config registers also in
> > > VF, such
> > blanket statements does not help.
> >
> > not other config registers. what's needed in VF driver should be retrievable
> > through VF.
> >
> This applies to the modern device, for legacy emulation, I don't see this a
> mandatory.
>
> > > Your comments and suggestions are helpful.
> > > Some of them follow the loop that doesn't help.
> > > Again, thanks a lot for the inputs and reviews.
> > >
> > > The design discussion helped to produce two solutions here than what was
> > started in v0.
> >
> > OK good. Now I suggest you make an effort to come up with some compromise
> > that will make this thread stop. I really can't say I even remember what we
> > said
> > in the beginning.
>
> > You had this idea of starting with a problem statement document how about
> > doing that? Because I feel every time people bring up some idea and a
> > problem
> > this idea would solve, you go "this is out of scope". Let's decide what the
> > scope
> > is?
> >
> The requirements are simple and straightforward at start of the cover letter.
> i.e. for quick reference in short: A PCI VF passthrough to guest VM to
> support legacy + 1.x.
>
> I propose:
> Method-1:
> 1. VF legacy registers access over AQ of PF
> 2. VF driver notifications using MMIO of VF
>
> Method-2:
> 1. legacy registers access using two MMIO registers (data and addr)
> 2. VF driver notifications using MMIO of VF
I think I prefer Method-1, I didn't exactly see Method-2 to judge
though.
> > > I am asking a 1.x PCI device has the VF notification BAR already, why we
> > > don't
> > use it, why to build an additional one in device?
> >
> > Following this logic, a 1.x PCI device also has modern common config.
> > All you really truly need is a flag to make it behave like legacy, from POV
> > of
> > device config and VQs.
> >
> Dual definition to same area in device is not elegant way to achieve it.
> We already discussed that some area is RW, some is not. It moves location.
>
> And it still doesn't solve the problem of reset.
> Hence, either doing via AQ or 2 registers method would work fine.
>
> It keeps legacy also isolated from mainstream.
>
> > So let's try to keep drivers self-contained as much as possible.
> >
> You said "AQ is fine" at the end of email.
> Hard to understand your mixed suggestion in context of sending v3.
I am saying a simple thing actually:
- notifications are through VF
- rest of config is through PF (AQ)
seems like a consistent model to me.
> > > And notification also works well that reuses transitional device's
> > > notification.
> >
> > I don't know what these are. You had some weird
> > +struct virtio_admin_cmd_lq_notify_query_result {
> > + u8 bar; /* PCI BAR number of the member VF */
> > + u8 reserved[7];
> > + le64 offset; /* Byte offset within the BAR */ };
> > if you want to r
[virtio-dev] Re: [RFC 1/4] vdpa: add VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag
On Wed, May 24, 2023 at 12:06:47PM +0800, Jason Wang wrote:
> On Tue, May 23, 2023 at 1:55 PM Eugenio Perez Martin
> wrote:
> >
> > On Mon, May 22, 2023 at 11:22 PM Shannon Nelson
> > wrote:
> > >
> > > On 5/22/23 1:07 PM, Eugenio Perez Martin wrote:
> > > >
> > > > On Mon, May 22, 2023 at 9:23 PM Michael S. Tsirkin
> > > > wrote:
> > > >>
> > > >> On Mon, May 22, 2023 at 08:57:27PM +0200, Eugenio Pérez wrote:
> > > >>> Signed-off-by: Eugenio Pérez
> > > >>> ---
> > > >>> include/uapi/linux/vhost_types.h | 2 ++
> > > >>> 1 file changed, 2 insertions(+)
> > > >>>
> > > >>> diff --git a/include/uapi/linux/vhost_types.h
> > > >>> b/include/uapi/linux/vhost_types.h
> > > >>> index c5690a8992d8..a1b316f49b38 100644
> > > >>> --- a/include/uapi/linux/vhost_types.h
> > > >>> +++ b/include/uapi/linux/vhost_types.h
> > > >>> @@ -165,5 +165,7 @@ struct vhost_vdpa_iova_range {
> > > >>> #define VHOST_BACKEND_F_SUSPEND 0x4
> > > >>> /* Device can be resumed */
> > > >>> #define VHOST_BACKEND_F_RESUME 0x5
> > > >>> +/* Device can enable virtqueues after DRIVER_OK */
> > > >>
> > > >> A bit more detail on what does this mean?
> > > >> It's normally driver that enables VQs not device itself ...
> > > >>
> > > >
> > > > I agree, it is not well explained.
> > > >
> > > > What about "Device supports the driver to enable virtqueues after
> > > > DRIVER_OK"?
> > > >
> > > >>> +#define VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK 0x6
> > >
> > > Does this mean it is possible only after DRIVER_OK, or does it mean in
> > > addition to before DRIVER_OK? It isn't clear to me.
> > >
> > > Maybe something like
> > > "Device supports virtqueue enable only after DRIVER_OK"
> > > or
> > > "Device supports virtqueue enable both before and after DRIVER_OK"
> > >
> >
> > I agree too,
> >
> > With the previous suggestion it would be:
> >
> > "Device supports the driver to enable virtqueues both before and after
> > DRIVER_OK"
>
> Btw, I think it might be worth clarifying whether or not this is
> supported by the current virtio spec. Spec seems to be unclear on
> this:
>
> 1) The driver MUST NOT write a 0 to queue_enable.
> 2) if reset is support, driver can wrote 1 to re-enable a virtqueue
>
> But it doesn't say if the driver can write 1 after DRIVER_OK without reset.
>
> Thanks
I don't think it can. Do any drivers do this?
>
> >
> > Does it look better?
> >
> > Thanks!
> >
> > > sln
> > >
> > >
> > > >>>
> > > >>> #endif
> > > >>> --
> > > >>> 2.31.1
> > > >>
> > > >
> > >
> >
-
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 PATCH v7 1/1] virtio-video: Add virtio video device specification
Add the specification of the video decoder and encoder devices, which
can be used to provide host-accelerated video operations to the guest.
Signed-off-by: Alexander Gordeev
Co-authored-by: Keiichi Watanabe
Co-authored-by: Alexandre Courbot
---
conformance.tex |4 +
content.tex |1 +
device-types/video/description.tex| 2005 +
device-types/video/device-conformance.tex | 24 +
device-types/video/driver-conformance.tex | 18 +
introduction.tex | 21 +
6 files changed, 2073 insertions(+)
create mode 100644 device-types/video/description.tex
create mode 100644 device-types/video/device-conformance.tex
create mode 100644 device-types/video/driver-conformance.tex
diff --git a/conformance.tex b/conformance.tex
index 01ccd69..d719eda 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -34,6 +34,7 @@ \section{Conformance Targets}\label{sec:Conformance /
Conformance Targets}
\ref{sec:Conformance / Driver Conformance / SCMI Driver Conformance},
\ref{sec:Conformance / Driver Conformance / GPIO Driver Conformance} or
\ref{sec:Conformance / Driver Conformance / PMEM Driver Conformance}.
+\ref{sec:Conformance / Driver Conformance / Video Driver Conformance},
\item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device
and Transitional Driver Conformance}.
\end{itemize}
@@ -61,6 +62,7 @@ \section{Conformance Targets}\label{sec:Conformance /
Conformance Targets}
\ref{sec:Conformance / Device Conformance / SCMI Device Conformance},
\ref{sec:Conformance / Device Conformance / GPIO Device Conformance} or
\ref{sec:Conformance / Device Conformance / PMEM Device Conformance}.
+\ref{sec:Conformance / Device Conformance / Video Device Conformance},
\item Clause \ref{sec:Conformance / Legacy Interface: Transitional Device
and Transitional Driver Conformance}.
\end{itemize}
@@ -152,6 +154,7 @@ \section{Conformance Targets}\label{sec:Conformance /
Conformance Targets}
\input{device-types/scmi/driver-conformance.tex}
\input{device-types/gpio/driver-conformance.tex}
\input{device-types/pmem/driver-conformance.tex}
+\input{device-types/video/driver-conformance.tex}
\conformance{\section}{Device Conformance}\label{sec:Conformance / Device
Conformance}
@@ -238,6 +241,7 @@ \section{Conformance Targets}\label{sec:Conformance /
Conformance Targets}
\input{device-types/scmi/device-conformance.tex}
\input{device-types/gpio/device-conformance.tex}
\input{device-types/pmem/device-conformance.tex}
+\input{device-types/video/device-conformance.tex}
\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
diff --git a/content.tex b/content.tex
index cff548a..1255e6e 100644
--- a/content.tex
+++ b/content.tex
@@ -710,6 +710,7 @@ \chapter{Device Types}\label{sec:Device Types}
\input{device-types/scmi/description.tex}
\input{device-types/gpio/description.tex}
\input{device-types/pmem/description.tex}
+\input{device-types/video/description.tex}
\chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
diff --git a/device-types/video/description.tex
b/device-types/video/description.tex
new file mode 100644
index 000..43d368d
--- /dev/null
+++ b/device-types/video/description.tex
@@ -0,0 +1,2005 @@
+\section{Video Device}
+\label{sec:Device Types / Video Device}
+
+The virtio video encoder and decoder devices provide support for
+host-accelerated video encoding and decoding. Despite being different
+device types, they use the same protocol and general flow.
+
+\subsection{Device ID}
+\label{sec:Device Types / Video Device / Device ID}
+
+\begin{description}
+\item[30]
+ encoder device
+\item[31]
+ decoder device
+\end{description}
+
+\subsection{Virtqueues}
+\label{sec:Device Types / Video Device / Virtqueues}
+
+\begin{description}
+\item[0]
+ commandq - queue for driver commands and device responses to these commands
+\item[1]
+ eventq - queue for events sent by the device to the driver
+\end{description}
+
+\subsection{Feature bits}
+\label{sec:Device Types / Video Device / Feature bits}
+
+\begin{description}
+\item[VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES (0)]
+ Guest pages can be used as the backing memory of resources.
+\item[VIRTIO_VIDEO_F_RESOURCE_NON_CONTIG (1)]
+ The device can use non-contiguous guest memory as the backing memory of
+ resources. Only meaningful if VIRTIO_VIDEO_F_RESOURCE_GUEST_PAGES is also
+ set.
+\item[VIRTIO_VIDEO_F_RESOURCE_DYNAMIC (2)]
+ The device supports re-attaching memory to resources while streaming.
+% TODO this flag is not used anywhere at the moment.
+% Might be necessary with Android.
+\item[VIRTIO_VIDEO_F_RESOURCE_VIRTIO_OBJECT (3)]
+ Objects exported by another virtio device can be used as the ba
[virtio-dev] [RFC PATCH v7 0/1] Virtio video device specification
Hi, This is the 7th version of virtio-video device patch. Feedback would be much appreciated. It is still under discussion on virtio-dev mailing list whether the video device specification should be continued as it is done here, or the V4L2 stateful decoder API should be used instead. There are many arguments. Also different parties have different priorities and use-cases. Anyway I hope, that in this draft I was able to solve some of the long standing issues of the previous drafts. There're still things to be done like introducing completion events for asynchronous commands, but these changes are not going to be controversial, I think. The biggest conceptual change here is that I reintroduced device parameters discovery, but in a (hopefully) simpler way. The idea is that the QUERY_CAPS command should return all the supported parameters and the device capabilities in one go and in a way compatible with V4L2, so that it is easy to write the driver. This way the need to query the default values of the parameters before changing them is reduced because the capabilities are much better known from the start. This means an extensive use of bitsets in the capabilities. Here I took some ideas from the crypto device specification. Also I turned GET_PARAM + SET_PARAM command into a single SET_PARAMS command again with the help of parameter bitsets. This is a big change, it still has to be polished. I'm going to do it in the next versions together with updating the virtio-video driver. I tried to use directly or reference the existing definitions from V4L2 for the formats and encoder settings as much as possible. It turns out the raw formats are defined in DRM much better, than in V4L2. (For example, V4L2 names RGBA 32 bit format "BGRA32" for some reason.) So for raw formats I gave both references to DRM and V4L2. The size of the resulting video section is 23 pages. I'm not sure if it is possible to this much simple conceptually. Any ideas for shorter and more precise wording would be much appreciated. Full PDF: https://drive.google.com/file/d/1Va3LxbFdQ3Odeib4FD-B1CPYSz21Ihha/view?usp=sharing PDF with the video section only: https://drive.google.com/file/d/1qANJDOpt0R_PvC0QuGaxYXC_Uq70MagC/view?usp=sharing Changelog v6 -> v7: * Used changes from Alexandre Courbot's repository on GitHub: https://github.com/Gnurou/virtio-video-spec They have the same license file as the virtio-spec repository. The changes addressed some of the comments to the draft v6. * The big change in discovering and setting the parameters using QUERY_CAPS and SET_PARAMS commands. * Restored the encoder settings and some of the formats. * Addressed the remaining comments to the draft v6 or added todo comments. * Finished renaming STOP to RESET. * Added references to V4L2 and DRM. * Added conformance sections. * Fixed a lot of small issues. * Reformatted for better readability. Alexander Gordeev (1): virtio-video: Add virtio video device specification conformance.tex |4 + content.tex |1 + device-types/video/description.tex| 2005 + device-types/video/device-conformance.tex | 24 + device-types/video/driver-conformance.tex | 18 + introduction.tex | 21 + 6 files changed, 2073 insertions(+) create mode 100644 device-types/video/description.tex create mode 100644 device-types/video/device-conformance.tex create mode 100644 device-types/video/driver-conformance.tex -- 2.34.1 - 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 PATCH v3] can: virtio: Initial virtio CAN driver.
Hello Vincent,
On 15.05.23 08:31, Vincent MAILHOL wrote:
if (err) {
Opened Eclipse, searched for "if (err != 0)" in the kernel code. 290
matches. For "if (ret != 0)" I found now 1970 matches.
Read my comment again. I never mentioned err vs. ret.
I am asking to replace "if (err != 0)" by "if (err)". We are not
using MISRA and there is no concept of essential boolean type here.
You can pass an integer to an if ().
I do not use eclipse, but git can give a few relevant statistics:
$ git grep "if (err != 0)" | wc -l
277
$ git grep "if (err)" | wc -l
34307
And while this is not the topic, "ret" is more popular than "err":
$ git grep "if (ret != 0)" | wc -l
1956
$ git grep "if (ret)" | wc -l
67927
but both are well established usage so I do not really care which one
of "ret" or "err" you use.
Looks like major practice is indeed to omit the != 0 for error
indicating return codes so I will adapt to the majority but adding an
unlikely. The block is not expected to be entered at all as the comment
says however it needs to be there due to defensive programming practice.
=> if (unlikely(ret))
+ /* Not expected to happen */
+ dev_err(dev, "%s(): virtqueue_add_sgs() failed\n", __func__);
+ }
+
+ if (!virtqueue_kick(vq)) {
+ /* Not expected to happen */
+ dev_err(dev, "%s(): Kick failed\n", __func__);
+ }
+
+ while (!virtqueue_get_buf(vq, &len) && !virtqueue_is_broken(vq))
+ wait_for_completion(&priv->ctrl_done);
+
+ mutex_unlock(&priv->ctrl_lock);
+
+ return priv->cpkt_in.result;
+}
...
+static netdev_tx_t virtio_can_start_xmit(struct sk_buff *skb,
+struct net_device *dev)
+{
+ struct virtio_can_priv *priv = netdev_priv(dev);
+ struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+ struct virtio_can_tx *can_tx_msg;
+ struct virtqueue *vq = priv->vqs[VIRTIO_CAN_QUEUE_TX];
+ struct scatterlist sg_out[1];
+ struct scatterlist sg_in[1];
+ struct scatterlist *sgs[2];
Instead declaring two times an array of 1, can we do:
struct scatterlist sgs[2];
Ooopsy on my side. sgs is an array of pointers so the above is not equivalent.
And doing this:
struct scatterlist *sgs[2];
Would be problematic as the memory of the two elements would not be allocated.
and then use sgs[0] for out and sgs[1] for in?
Or, if you really want to keep sg_out and sg_in, at least do:
struct scatterlist sg_out, sg_in;
struct scatterlist *sgs[] = {&sg_out, &sg_in};
N.B. The same comment also applies to the other places where you are
doing some sg[1] declarations.
Makes thing worse. I'm not even sure whether this is a null change only
or introduces a problem.
virtio strictly separates devices readable and device writeable data.
Therefore I want really to have here 2 separate definitions. The one is
data to the device, the other is data from the device.
ACK. My second example does that:
struct scatterlist sg_out, sg_in;
struct scatterlist *sgs[] = {&sg_out, &sg_in};
First I remove the [1] from sg_out and sg_in to make those scalars
instead of arrays of [1].
Your 2nd example above violates the reverse xmas tree rule so I cannot
take this one as is.
But got the thing through the compiler with the following construct:
struct scatterlist sg_out, sg_in, *sgs[2] = { &sg_out, &sg_in };
If this had any advantage, I could separate the data further. For
example I could separate the payload from the preceding data. In this
case I had struct scatterlist sg_out[2]. As long as the payload is
small the memcpy for the payload can be justified and [1] is good. In
fact, those are still arrays even if by coincident now the number of
elements is 1.
sg_out and sg_in are only passed to one function: sg_init_one(). And
as the name suggests, sg_init_one expects a single scatterlist, not an
array.
A look at:
$ git grep sg_init_one
show me that doing as "sg_init_one(&foo[0], ...)" is not a popular
solution. The majority does sg_init_one(&foo, ...).
ACK. Checked this also now, saw &foo[0] once or so.
I do get that sgs is an array of arrays. I am just not comfortable
with sg_out and sg_in being declared as arrays because these never get
used as such.
+ unsigned long flags;
+ u32 can_flags;
+ int err;
+ int putidx;
+ netdev_tx_t xmit_ret = NETDEV_TX_OK;
+ const unsigned int hdr_size = offsetof(struct virtio_can_tx_out, sdu);
+
+ if (can_dev_dropped_skb(dev, skb))
+ goto kick; /* No way to return NET_XMIT_DROP here */
+
+ /* No local check for CAN_RTR_FLAG or FD frame against negotiated
+* features. The device will reject those anyway if not supported.
+*/
+
+ can_tx_msg = kzalloc(sizeof(*can_tx_msg), GFP_ATOMIC);
+ if (!can_tx_msg)
+
[virtio-dev] Re: [RFC 1/4] vdpa: add VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag
On Wed, May 24, 2023 at 6:07 AM Jason Wang wrote:
>
> On Tue, May 23, 2023 at 1:55 PM Eugenio Perez Martin
> wrote:
> >
> > On Mon, May 22, 2023 at 11:22 PM Shannon Nelson
> > wrote:
> > >
> > > On 5/22/23 1:07 PM, Eugenio Perez Martin wrote:
> > > >
> > > > On Mon, May 22, 2023 at 9:23 PM Michael S. Tsirkin
> > > > wrote:
> > > >>
> > > >> On Mon, May 22, 2023 at 08:57:27PM +0200, Eugenio Pérez wrote:
> > > >>> Signed-off-by: Eugenio Pérez
> > > >>> ---
> > > >>> include/uapi/linux/vhost_types.h | 2 ++
> > > >>> 1 file changed, 2 insertions(+)
> > > >>>
> > > >>> diff --git a/include/uapi/linux/vhost_types.h
> > > >>> b/include/uapi/linux/vhost_types.h
> > > >>> index c5690a8992d8..a1b316f49b38 100644
> > > >>> --- a/include/uapi/linux/vhost_types.h
> > > >>> +++ b/include/uapi/linux/vhost_types.h
> > > >>> @@ -165,5 +165,7 @@ struct vhost_vdpa_iova_range {
> > > >>> #define VHOST_BACKEND_F_SUSPEND 0x4
> > > >>> /* Device can be resumed */
> > > >>> #define VHOST_BACKEND_F_RESUME 0x5
> > > >>> +/* Device can enable virtqueues after DRIVER_OK */
> > > >>
> > > >> A bit more detail on what does this mean?
> > > >> It's normally driver that enables VQs not device itself ...
> > > >>
> > > >
> > > > I agree, it is not well explained.
> > > >
> > > > What about "Device supports the driver to enable virtqueues after
> > > > DRIVER_OK"?
> > > >
> > > >>> +#define VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK 0x6
> > >
> > > Does this mean it is possible only after DRIVER_OK, or does it mean in
> > > addition to before DRIVER_OK? It isn't clear to me.
> > >
> > > Maybe something like
> > > "Device supports virtqueue enable only after DRIVER_OK"
> > > or
> > > "Device supports virtqueue enable both before and after DRIVER_OK"
> > >
> >
> > I agree too,
> >
> > With the previous suggestion it would be:
> >
> > "Device supports the driver to enable virtqueues both before and after
> > DRIVER_OK"
>
> Btw, I think it might be worth clarifying whether or not this is
> supported by the current virtio spec. Spec seems to be unclear on
> this:
>
> 1) The driver MUST NOT write a 0 to queue_enable.
> 2) if reset is support, driver can wrote 1 to re-enable a virtqueue
>
> But it doesn't say if the driver can write 1 after DRIVER_OK without reset.
>
To double check, you mean to write 1 even if the queue is live?
"without reset *the virtqueue*"?
I'd add another combination that might be worth clarifying: ack
VIRTIO_F_RING_RESET, write 1 to queue_enable, then write 1 to
queue_reset. Set state DRIVER_OK, and then enable the queue
afterwards. Is this valid?
I guess it is not, but I don't see the standard forbidding it, isn't it?
Thanks!
> Thanks
>
>
> >
> > Does it look better?
> >
> > Thanks!
> >
> > > sln
> > >
> > >
> > > >>>
> > > >>> #endif
> > > >>> --
> > > >>> 2.31.1
> > > >>
> > > >
> > >
> >
>
-
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 v2 0/2] transport-pci: Introduce legacy registers access using AQ
> From: Michael S. Tsirkin > Sent: Wednesday, May 24, 2023 1:57 AM > I am frankly lost at this point, this thread is too big. > > So where do you want to go for this project? I would like something specific, > email threads are growing too big. > I think the reason is that there is actually a host of small related > subprojects. So > when you propose just this small feature, everyone jumps in with their own pet > project proposing addressing that too. > > For example: > - SIOV guys were adding transport over VQ. It would seem that > can include support for legacy. Yes. SIOV can support legacy and modern emulation. We need to have the SIOV devices without this emulation support so that they natively good. > - For migration we need ability to report member events to owner. Which events you have in mind? The AQ is design to carry the migration of the VFs and would be controlled via the AQ so it knows whats going on. > It would seem that can be used to support INT#x. > - Ability to stick capabilities in extended config space > is a good idea, for a variety of reasons. > > and so on. Understandably frustrating, and it is easy to go overboard with > generality. > > If you feel your path for progress is clear, that's good. The path seems clear to me for supporting legacy registers access for the VFs and future SIOV. Let's conclude it in other thread we are discussing of patch 1/2. > if not, here are ideas for getting this unstuck: > - start a TODO.md file with a list of things we need to address, > and for each one a list of options > - everyone will add to this file then we get a higher level > picture and can discuss "this will be addressed by A, that by B" This is certainly present in my personal todo list to discuss, I am happy to maintain in the public virtio doc at https://github.com/oasis-tcs/virtio-docs/ > - have a chat at the upstream developers meeting (May 31) > so far it's about vdpa blk > - anyone coming to the kvm forum to have a chat there? > > Hope this helps. Definitely a good suggestion. I will send PR for virtio-docs to maintain our TODO list. Since some of the discussions are ongoing basis if we can meet monthly on more practically on need basis, it will help to make progress faster. - 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: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands
> From: Michael S. Tsirkin > Sent: Wednesday, May 24, 2023 6:08 AM > > On Tue, May 23, 2023 at 10:22:27PM +, Parav Pandit wrote: > > > > > From: Michael S. Tsirkin > > > Sent: Tuesday, May 23, 2023 2:49 PM > > > > > iokit is from my previous work experience not virtio. > > > > > For virtio we had a lot of trouble with hacks like this on windows. > > > > I don't know why windows driver need to talk to PF driver until > > > > now when the > > > AQ doesn't exists. > > > > > > What is it you want to know? > > > > > Why a VF driver needs to talk to a PF driver in windows without AQ? > > I don't know- it doesn't? If it did, it would be hard as we learned when > trying to > support pci-bridge-seat. Doable, but annoying. If it does today, something likely is missing in that OS. Their model for sriov has not evolved as much as Linux from what I see. So not to worry about it, as they may have to implement more things. > > > > > > That's an important platform. > > > > > > > > > I am not sure how important is it. > > > > What I learnt that a decade has passed and even after that virtio > > > > drivers are > > > not part of the OS distro. > > > > > > Because with QEMU they are so easy to side-load. So we never bothered. > > > > > I don't understand your comment. > > I was saying to my knowledge a Windows OS do not have a virtio drivers in > their distro which you said is important. > > It's important for windows to work well with virtio. And it does because we > supply an install disk with virtio drivers through QEMU Ok. So WDM can use their IPR model or will be able to build infra as/if needed. > > I propose: > > Method-1: > > 1. VF legacy registers access over AQ of PF 2. VF driver notifications > > using MMIO of VF > > > > Method-2: > > 1. legacy registers access using two MMIO registers (data and addr) 2. > > VF driver notifications using MMIO of VF > > I think I prefer Method-1, I didn't exactly see Method-2 to judge though. > Ok. so lets step forward in v3 using 4 commands for registers access. 2 RW commands for common config. 2 RW commands for device config. Notification on below. > > > > > I am asking a 1.x PCI device has the VF notification BAR already, > > > > why we don't > > > use it, why to build an additional one in device? > > > > > > Following this logic, a 1.x PCI device also has modern common config. > > > All you really truly need is a flag to make it behave like legacy, > > > from POV of device config and VQs. > > > > > Dual definition to same area in device is not elegant way to achieve it. > > We already discussed that some area is RW, some is not. It moves location. > > > > And it still doesn't solve the problem of reset. > > Hence, either doing via AQ or 2 registers method would work fine. > > > > It keeps legacy also isolated from mainstream. > > > > > So let's try to keep drivers self-contained as much as possible. > > > > > You said "AQ is fine" at the end of email. > > Hard to understand your mixed suggestion in context of sending v3. > > I am saying a simple thing actually: > - notifications are through VF > - rest of config is through PF (AQ) > seems like a consistent model to me. > Sounds good to me. What about discovery the notification place for the VF? We have two options. 1. discover them via AQ like config registers access 2. discover it via extended PCI capability #1 is far more programable and does not need to bake in the PCI device layout. So this way one day in future we can get rid of it more easily for those PCI devices who may prefer everything in the hw. > > > if you want to re-use the normal notifications for VFs we already > > > describe them using capabilities. > > > > > That capability is intermixed with 1.x register queue_notify_off. > > Hmm yes annoying I forgot about this. > > > So, I see following options. > > a. either to create a new legacy specific extended capability, or b. > > say queue_notify_off doesn't apply for legacy notifications because > > this register doesn't exist for legacy c. exchange it via the AQ like above > struct. > > a feels ok to me... maybe test the waters with express capability as well, it > will > come handy down the road I think. Yes, if this is really needed for some OS, this addition will be possible in future in the spec and also of the devices in CC list here. > > > > However, this creates a weird mix where there's this command that > > > mostly follows legacy pci config layout except notifications which > > > are there in the legacy pci config are actually somewhere else. And > > > this is just inconsistent and confusing, and the reason for this is > implementation defined. > > Depends on how you see it. > > It is at same level of inconsistency as 1.x for a legit reason which is to > > split > config registers from data path register. > > > > It is not implementation defined. Notification over AQ cannot reach same > > perf > level as MMIO. > > I will avoid repeating it. > > yes, no need to repea
[virtio-dev] Re: [PATCH v2 0/2] transport-pci: Introduce legacy registers access using AQ
On Wed, May 24, 2023 at 06:57:30PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, May 24, 2023 1:57 AM > > > > I am frankly lost at this point, this thread is too big. > > > > So where do you want to go for this project? I would like something > > specific, > > email threads are growing too big. > > I think the reason is that there is actually a host of small related > > subprojects. So > > when you propose just this small feature, everyone jumps in with their own > > pet > > project proposing addressing that too. > > > > For example: > > - SIOV guys were adding transport over VQ. It would seem that > > can include support for legacy. > Yes. SIOV can support legacy and modern emulation. > We need to have the SIOV devices without this emulation support so that they > natively good. > > > - For migration we need ability to report member events to owner. > Which events you have in mind? memory faults/dirtying for example. > The AQ is design to carry the migration of the VFs and would be controlled > via the AQ so it knows whats going on. > > > It would seem that can be used to support INT#x. > > - Ability to stick capabilities in extended config space > > is a good idea, for a variety of reasons. > > > > and so on. Understandably frustrating, and it is easy to go overboard with > > generality. > > > > If you feel your path for progress is clear, that's good. > The path seems clear to me for supporting legacy registers access for the VFs > and future SIOV. > Let's conclude it in other thread we are discussing of patch 1/2. > > > if not, here are ideas for getting this unstuck: > > - start a TODO.md file with a list of things we need to address, > > and for each one a list of options > > - everyone will add to this file then we get a higher level > > picture and can discuss "this will be addressed by A, that by B" > This is certainly present in my personal todo list to discuss, I am happy to > maintain in the public virtio doc at > https://github.com/oasis-tcs/virtio-docs/ > > > - have a chat at the upstream developers meeting (May 31) > > so far it's about vdpa blk > > - anyone coming to the kvm forum to have a chat there? > > > > Hope this helps. > Definitely a good suggestion. > I will send PR for virtio-docs to maintain our TODO list. Hmm. This is just a dumping ground then. Fine by me, but what I had in mind is a common list of known problems to address by TC as a whole. If you want that we want patches on list and discussion, maybe even voting. > Since some of the discussions are ongoing basis if we can meet monthly > on more practically on need basis, it will help to make progress > faster. That meeting is byweekly actually I think. -- 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: [PATCH v2 0/2] transport-pci: Introduce legacy registers access using AQ
> From: Michael S. Tsirkin > Sent: Wednesday, May 24, 2023 3:58 PM > > > Hope this helps. > > Definitely a good suggestion. > > I will send PR for virtio-docs to maintain our TODO list. > > Hmm. This is just a dumping ground then. Fine by me, but what I had in mind > is > a common list of known problems to address by TC as a whole. > If you want that we want patches on list and discussion, maybe even voting. > It is not a dumping ground, it is the list of things to track and document without searching emails. It is a patch on the list for the doc in virtio-docs area that if needed can be voted. - 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: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands
On Wed, May 24, 2023 at 07:18:56PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, May 24, 2023 6:08 AM > > > > On Tue, May 23, 2023 at 10:22:27PM +, Parav Pandit wrote: > > > > > > > From: Michael S. Tsirkin > > > > Sent: Tuesday, May 23, 2023 2:49 PM > > > > > > iokit is from my previous work experience not virtio. > > > > > > For virtio we had a lot of trouble with hacks like this on windows. > > > > > I don't know why windows driver need to talk to PF driver until > > > > > now when the > > > > AQ doesn't exists. > > > > > > > > What is it you want to know? > > > > > > > Why a VF driver needs to talk to a PF driver in windows without AQ? > > > > I don't know- it doesn't? If it did, it would be hard as we learned when > > trying to > > support pci-bridge-seat. Doable, but annoying. > If it does today, something likely is missing in that OS. Their model for > sriov has not evolved as much as Linux from what I see. > So not to worry about it, as they may have to implement more things. > > > > > > > > > That's an important platform. > > > > > > > > > > > I am not sure how important is it. > > > > > What I learnt that a decade has passed and even after that virtio > > > > > drivers are > > > > not part of the OS distro. > > > > > > > > Because with QEMU they are so easy to side-load. So we never bothered. > > > > > > > I don't understand your comment. > > > I was saying to my knowledge a Windows OS do not have a virtio drivers in > > their distro which you said is important. > > > > It's important for windows to work well with virtio. And it does because we > > supply an install disk with virtio drivers through QEMU > > Ok. So WDM can use their IPR model or will be able to build infra as/if > needed. > > > > I propose: > > > Method-1: > > > 1. VF legacy registers access over AQ of PF 2. VF driver notifications > > > using MMIO of VF > > > > > > Method-2: > > > 1. legacy registers access using two MMIO registers (data and addr) 2. > > > VF driver notifications using MMIO of VF > > > > I think I prefer Method-1, I didn't exactly see Method-2 to judge though. > > > Ok. so lets step forward in v3 using 4 commands for registers access. > 2 RW commands for common config. > 2 RW commands for device config. > > Notification on below. > > > > > > > > I am asking a 1.x PCI device has the VF notification BAR already, > > > > > why we don't > > > > use it, why to build an additional one in device? > > > > > > > > Following this logic, a 1.x PCI device also has modern common config. > > > > All you really truly need is a flag to make it behave like legacy, > > > > from POV of device config and VQs. > > > > > > > Dual definition to same area in device is not elegant way to achieve it. > > > We already discussed that some area is RW, some is not. It moves location. > > > > > > And it still doesn't solve the problem of reset. > > > Hence, either doing via AQ or 2 registers method would work fine. > > > > > > It keeps legacy also isolated from mainstream. > > > > > > > So let's try to keep drivers self-contained as much as possible. > > > > > > > You said "AQ is fine" at the end of email. > > > Hard to understand your mixed suggestion in context of sending v3. > > > > I am saying a simple thing actually: > > - notifications are through VF > > - rest of config is through PF (AQ) > > seems like a consistent model to me. > > > Sounds good to me. > What about discovery the notification place for the VF? > We have two options. > 1. discover them via AQ like config registers access > 2. discover it via extended PCI capability > > #1 is far more programable and does not need to bake in the PCI device layout. I think 2 is easier for drivers to use. > So this way one day in future we can get rid of it more easily for those PCI > devices who may prefer everything in the hw. I think these devices are better off with a new transport using drastically less registers. > > > > if you want to re-use the normal notifications for VFs we already > > > > describe them using capabilities. > > > > > > > That capability is intermixed with 1.x register queue_notify_off. > > > > Hmm yes annoying I forgot about this. > > > > > > So, I see following options. > > > a. either to create a new legacy specific extended capability, or b. > > > say queue_notify_off doesn't apply for legacy notifications because > > > this register doesn't exist for legacy c. exchange it via the AQ like > > > above > > struct. > > > > a feels ok to me... maybe test the waters with express capability as well, > > it will > > come handy down the road I think. > Yes, if this is really needed for some OS, this addition will be possible in > future in the spec and also of the devices in CC list here. hmm not sure what do you mean. I meant to put the capabilities with the new legacy bar in express config space. > > > > > > However, this creates a weird mix where there's this command that > > > > mostly follows
[virtio-dev] Re: [PATCH v2 0/2] transport-pci: Introduce legacy registers access using AQ
On Wed, May 24, 2023 at 08:01:18PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, May 24, 2023 3:58 PM > > > > > Hope this helps. > > > Definitely a good suggestion. > > > I will send PR for virtio-docs to maintain our TODO list. > > > > Hmm. This is just a dumping ground then. Fine by me, but what I had in > > mind is > > a common list of known problems to address by TC as a whole. > > If you want that we want patches on list and discussion, maybe even voting. > > > It is not a dumping ground, it is the list of things to track and document > without searching emails. virtio-doc is a dumping ground though :) > It is a patch on the list for the doc in virtio-docs area that if needed can > be voted. Let's start with nvidia-todo.md in virtio-doc indeed. People can at least read it, good starting point. Next I would like us to proceed to a todo list under virtio-spec, so that TC at least knows what are current problems before us. -- 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: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands
> From: Michael S. Tsirkin
> Sent: Wednesday, May 24, 2023 4:13 PM
[..]
> > Sounds good to me.
> > What about discovery the notification place for the VF?
> > We have two options.
> > 1. discover them via AQ like config registers access 2. discover it
> > via extended PCI capability
> >
> > #1 is far more programable and does not need to bake in the PCI device
> layout.
>
> I think 2 is easier for drivers to use.
Yes.
Its trade off.
But not a huge difference from sw point of view as they use underlying apis.
As opposed to do that baking pci capabilities is slightly hard as it needs to
intermix control path over registers..
Typically, CVQ and AQ etc a PCI device would do using some sort of firmware, so
adding commands are easier than low level PCI capabilities based tunneling.
>
> > So this way one day in future we can get rid of it more easily for those PCI
> devices who may prefer everything in the hw.
>
> I think these devices are better off with a new transport using drastically
> less
> registers.
>
That way AQ scores more.
> > > a feels ok to me... maybe test the waters with express capability as
> > > well, it will come handy down the road I think.
> > Yes, if this is really needed for some OS, this addition will be possible
> > in future
> in the spec and also of the devices in CC list here.
>
> hmm not sure what do you mean. I meant to put the capabilities with the new
> legacy bar in express config space.
>
I mean to say we should do AQ.
And if an OS can never implement two driver communication for legacy, may be at
that future point a spec should evolve to provide this capability-based access.
> > The new capability in mind is:
> > VIRTIO_PCI_EXT_CAP_PCI_LEGACY_REG
> >
> > struct virtio_pci_legacy_cfg_cap {
> > struct virtio_pci_cap cap;
> > le32 addr; /* register offset, width, op=RD/WR,
> status:0=pending,1=done)
> > le32 reg_data;
> > };
>
> I would use cap64, cap is there because we did not think about 64 bit early
> enough.
>
Yes.
> > This will be hard to do for SIOV in future, so I prefer AQ cmd for config
> register access and notification discovery.
> > WDYT?
>
> I need to go back and look at how does SIOV tvq proposal manage notifications
> then. Do not remember off the top of my head.
SIOV tvq proposal does not tunnel the driver notifications.
It exposes the address and size of db region like how I had in similar command.
This is why it is not a transport vq and I suggest to rename to aq :)
Snippet of it below.
+The virtqueue notification area information can be get through the following
commands:
+
+\begin{lstlisting}
+#define VIRTIO_TRANSPTQ_CTRL_VQ_NOTIFY11
+ #define VIRTIO_TRANSPTQ_CTRL_VQ_NOTIFY_GET 0
+
+struct virtio_transportq_ctrl_vq_notification_area {
+ u64 address;
+ u64 size;
+};
+\end{lstlisting}
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
