[virtio-dev] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands

[Parav Pandit]

> From: Michael S. Tsirkin 
> Sent: Wednesday, May 17, 2023 1:45 AM
> 
> On Sat, May 06, 2023 at 03:01:34AM +0300, Parav Pandit wrote:
> > +\begin{lstlisting}
> > +struct virtio_admin_cmd_lreg_wr_data {
> > +   u8 offset; /* Starting byte offset of the register(s) to write */
> > +   u8 register[];
> > +};
> > +\end{lstlisting}
> 
> So to summarize, I think the main comment here was to have separate
> commands for access to common versus device config. Just today I thought
> some more and came up with another reason why this is a good idea: if we ever
> want legacy MMIO emulation, we can reuse the device config command.
> 
MMIO is actually simpler than PCI because all the registers are contiguous 
regardless of msix.
So single command with offset can service just fine.
More below.

> Jason also feels the common config command can be shared with vq transport
> effort. We can discuss this, for sure, I guess its putting code in hardware 
> versus
> in hypervisor, but if there are hardware vendors who strictly want this code 
> in
> hardware I don't have a big problem with this even if I don't exactly get why.
> 
> With this idea all hypervisor has to do is subtract the offset from device 
> config
> access, sounds like a small price to pay.  
It is more than the subtraction cost.

Hypervisor needs to inspect MSI-X enable/disable settings to decide when to 
issue command #1 vs #3 for the requested offset.

> Does this sound like a reasonable
> compromize to you?

Splitting proposed one command to two commands,
1. one for accessing legacy common config
2. second for accessing legacy device specific config 

seems fine to me as below.

So we will have total 5 commands (instead of 3).

1. legacy common config read
2. legacy common config write

3. legacy device config read
4. legacy device config write
5. query device notification area

#1 and #3 same cmd signature but different opcode.
#2 and #4 same cmd signature but different opcode.



-
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: [RFC PATCH v6] virtio-video: Add virtio video device specification

[Cornelia Huck]

>>> I also hope to be able to update virtio-video at a faster pace. Please
>>> let me try.
>>
>> Please hold on a bit - there are two things here.
>>
>> 1) I'd like to settle the virtio-v4l2/virtio-video argument first to
>> make sure we don't get two things clashing head-on. As far as codecs
>> are concerned we certainly don't need both. Cornelia, I think we'll
>> need you to make a call on this, or at least tell us what you need to
>> make the call. If it helps I can send a draft of what the virtio-v4l2
>> spec would look like, it should be relatively short.
>>
>> 2) I (and other ChromeOS contributors) have been driving this spec so
>> far and while I think virtio-v4l2 is a better solution, I have not
>> said I would give up on virtio-video if virtio-v4l2 was not adopted
>> and will keep iterating on it in that case.
>
> It actually very much looks like you gave up. I mean, you developed it
> for years, and now you'd like to throw it away. Well, I meant something
> like "please have some faith in my ability to update virtio-video at a
> faster pace". I think I already have the permission to continue the
> development. You know, OpenSynergy also spent some time on the
> virtio-video. This includes the draft v1 version and the V4L2 driver. I
> think this was kind of an informal agreement between us, that you do the
> spec and we do the driver. It didn't work well enough since draft v4, I
> think. Now as our interests are not aligned anymore, it is fine to
> continue separately. I'm not going to wait until you change your mind on
> virtio-v4l2. Obviously you're very busy with it right now. If I stop and
> wait now, we won't have the video device in the 1.3 release for sure. I
> think when we are ready to develop virtio-video together again, we'll
> need a better agreement.

Please, can we just calm down here? This thread is painful to read
already, and pointing at each other is not going to help us to come up
with something that is generally agreeable. In the end, I want _a_ spec,
and I don't really have a horse in V4L2 vs. video race, but having to
dig through all of this in the hope of moderating things is just
impossible if you're arguing in circles, and much of it seemingly being
a rehashing about who said/did something or not.

> That said, I very much appreciate your efforts with the spec. It
> definitely received a lot of improvements.


-
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 v2 1/2] can: virtio: Initial virtio CAN driver.

[Harald Mommer]

Hello,

(still sorting my E-Mail mess)

On 07.11.22 08:35, Vincent Mailhol wrote:

On Mon. 7 Nov. 2022 at 16:15, Alexander Stein
 wrote:

Am Freitag, 4. November 2022, 18:24:20 CET schrieb Harald Mommer:

From: Harald Mommer 

- CAN Control

   - "ip link set up can0" starts the virtual CAN controller,
   - "ip link set up can0" stops the virtual CAN controller

- CAN RX

   Receive CAN frames. CAN frames can be standard or extended, classic or
   CAN FD. Classic CAN RTR frames are supported.

- CAN TX

   Send CAN frames. CAN frames can be standard or extended, classic or
   CAN FD. Classic CAN RTR frames are supported.

- CAN Event indication (BusOff)

   The bus off handling is considered code complete but until now bus off
   handling is largely untested.

This is version 2 of the driver after having gotten review comments.

Signed-off-by: Harald Mommer 
Cc: Damir Shaikhutdinov 

[...]


diff --git a/include/uapi/linux/virtio_can.h
b/include/uapi/linux/virtio_can.h new file mode 100644
index ..0ca75c7a98ee
--- /dev/null
+++ b/include/uapi/linux/virtio_can.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Copyright (C) 2021 OpenSynergy GmbH
+ */
+#ifndef _LINUX_VIRTIO_VIRTIO_CAN_H
+#define _LINUX_VIRTIO_VIRTIO_CAN_H
+
+#include 
+#include 
+#include 
+#include 
+
+/* Feature bit numbers */
+#define VIRTIO_CAN_F_CAN_CLASSIC0u
+#define VIRTIO_CAN_F_CAN_FD 1u
+#define VIRTIO_CAN_F_LATE_TX_ACK2u
+#define VIRTIO_CAN_F_RTR_FRAMES 3u
+
+/* CAN Result Types */
+#define VIRTIO_CAN_RESULT_OK0u
+#define VIRTIO_CAN_RESULT_NOT_OK1u
+
+/* CAN flags to determine type of CAN Id */
+#define VIRTIO_CAN_FLAGS_EXTENDED   0x8000u
+#define VIRTIO_CAN_FLAGS_FD 0x4000u
+#define VIRTIO_CAN_FLAGS_RTR0x2000u
+
+/* TX queue message types */
+struct virtio_can_tx_out {
+#define VIRTIO_CAN_TX   0x0001u
+ __le16 msg_type;
+ __le16 reserved;
+ __le32 flags;
+ __le32 can_id;
+ __u8 sdu[64u];

64u is CANFD_MAX_DLEN, right? Is it sensible to use that define instead?
I guess if CAN XL support is to be added at some point, a dedicated struct is
needed, to fit for CANXL_MAX_DLEN (2048 [1]).

[1] 
https://ddec1-0-en-ctp.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2flinux.git%2f=127a2be7-331e-4823-805f-4578232f5495=53c7c7de28b92dfd96e93d9dd61a23e634d2fbec-52b59e6b756f1b97e14cc9439116d9b7a4ad93da
commit/?id=1a3e3034c049503ec6992a4a7d573e7fff31fac4


1.) This is CANFD_MAX_DLEN. It is not sensible to use that define 
instead as this is a Linux define and this header here is not Linux 
specific.


BTW, a microcontroller implementation supporting CAN classic only may 
allocate only 8u bytes (CAN_MAX_DLEN) for the payload if memory usage 
was an issue. However I doubt that very small controllers are an 
audience for virtio.


The data structure has been updated in the meantime in the sources, spec 
still to be sent. The newer structure should be prepared for CAN XL 
later however CAN XL is currently not supported.



To add to Alexander's comment, what is the reason to have the msg_type
flag? The struct can_frame, canfd_frame and canxl_frame are done such
that it is feasible to decide the type (Classic, FD, XL) from the
content of the structure. Why not just reusing the current structures?


The message type costs 2 bytes, if additional alignment is needed it 
causes a cost of 4 bytes in the worst case.


 * Virtio uses shared memory to transfer those messages which is very
   fast. From the speed perspective it costs almost nothing to have the
   message type. It's not a slow serial line where every byte sent counts.
 * The target machines for Virtio contain usually many megabytes if not
   gigabytes. The additional message identifier plays no role for those
   machines.

So the cost for the message type is relatively small. You may think 
we're always transferring CAN messages on Rxq and Txq so we can save the 
message type anyway. This is true for today and may be totally wrong 
already in a few months.


Those queues are communication channels which can transport any 
information. After the specification has been published some day someone 
may want to extend the specification having to transfer a complete 
different message over Txq or Rxq.


Easy with message type:

1. Add a feature flag to indicate that the device now also understands
   msg_type FOO_NEW
2. Define the structure for FOO_NEW having __le_16 msg_type as first
   member. The rest can be defined freely without any restrictions.

Without message type 2. can become ugly. The implementer is forced to 
get into the existing scheme, this may be difficult and the result may 
not look nice.


Better we spend the few bytes now to have better options in the future. 
We don't know what is in 5 years or even next year.


===

The question "why not reusing exactly 

Re: [virtio-dev] Re: [RFC PATCH v6] virtio-video: Add virtio video device specification

[Alexander Gordeev]

On 12.05.23 06:09, Alexandre Courbot wrote:

On Thu, May 11, 2023 at 5:50 PM Alexander Gordeev
 wrote:


3. We can have things, that V4L2 doesn't support in their stateful UAPI.
For example, dequeuing output buffers in decoder order.


... provided the host can do that (i.e. has a stateless decoder
interface).


Indeed dequeuing in decoding order is obviously possible with stateless
interfaces. But it is often possible with stateful interfaces too AFAIK,
just not with V4L2. For example, we have this option on some of our
targets because the codecs are exposed using OMX with some vendor
extensions. This is a stateful interface.

--
Alexander Gordeev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone: +49 30 60 98 54 0 - 88
Fax: +49 (30) 60 98 54 0 - 99
EMail: alexander.gord...@opensynergy.com

www.opensynergy.com

Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Régis Adjamah

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

Re: [virtio-dev] Re: [RFC PATCH v6] virtio-video: Add virtio video device specification

[Alexander Gordeev]

On 12.05.23 06:15, Alexandre Courbot wrote:

On Thu, May 11, 2023 at 6:00 PM Alexander Gordeev
 wrote:


On 11.05.23 10:50, Alexander Gordeev wrote:

On 08.05.23 06:55, Alexandre Courbot wrote:

On Fri, May 5, 2023 at 8:55 PM Alexander Gordeev
 wrote:


On 03.05.23 16:04, Cornelia Huck wrote:

On Fri, Apr 28 2023, Alexander Gordeev
 wrote:


On 27.04.23 15:16, Alexandre Courbot wrote:

But in any case, that's irrelevant to the guest-host interface, and I
think a big part of the disagreement stems from the misconception
that
V4L2 absolutely needs to be used on either the guest or the host,
which is absolutely not the case.


I understand this, of course. I'm arguing, that it is harder to
implement it, get it straight and then maintain it over years. Also it
brings limitations, that sometimes can be workarounded in the virtio
spec, but this always comes at a cost of decreased readability and
increased complexity. Overall it looks clearly as a downgrade compared
to virtio-video for our use-case. And I believe it would be the
same for
every developer, that has to actually implement the spec, not just do
the pass through. So if we think of V4L2 UAPI pass through as a
compatibility device (which I believe it is), then it is fine to have
both and keep improving the virtio-video, including taking the best
ideas from the V4L2 and overall using it as a reference to make
writing
the driver simpler.


Let me jump in here and ask another question:

Imagine that, some years in the future, somebody wants to add a virtio
device for handling video encoding/decoding to their hypervisor.

Option 1: There are different devices to chose from. How is the person
implementing this supposed to pick a device? They might have a narrow
use case, where it is clear which of the devices is the one that
needs to
be supported; but they also might have multiple, diverse use cases, and
end up needing to implement all of the devices.


I think in this case virtio-v4l2 should be used as a compatibility
device exclusively. This means discouraging increasing its complexity
even more with more patches in the spec. virtio-video should eventually
cover all the use-cases of V4L2, so I think it is reasonable to use it
in both complex use-cases and in simple use-cases, where there is no
decoder/encoder V4L2 device on the host.


Option 2: There is one device with various optional features. The
person
implementing this can start off with a certain subset of features
depending on their expected use cases, and add to it later, if needed;
but the upfront complexity might be too high for specialized use cases.


I don't see that many negociable features we can provide for a
decoder/encoder device - at least not many that are not considered
basic (like guest buffers). In terms of provided features for codecs
virtio-video and virtio-v4l2 are essentially equivalent.


Actually I see a lot of potential in using the virtio feature flag
negotiation for virtio-video:

1. We already have some feature flags related to memory management.

2. I think it would be great to take V4L2 controls negotiation and turn
it into the feature flags negotiation. I really like this idea and I'd
like to implement it. Not all the controls at once, of course. Still it
would be very easy to port more given the existing process. They
correspond well enough to each other, I think. This way we don't need to
introduce something like the VIDIOC_QUERYCTRL/VIDIOC_QUERY_EXT_CTRL, we
don't need two mechanisms for feature negotiations (like it would be
with virtio-v4l2, right?), also all the features would be in one place.
Then we can directly reference some enums from V4L2, like
v4l2_mpeg_video_h264_profile or v4l2_mpeg_video_h264_level. That's what
I call taking the best from V4L2.

3. We can have things, that V4L2 doesn't support in their stateful UAPI.
For example, dequeuing output buffers in decoder order.


I'd like to also share my current roadmap for the draft v7 of
virtio-video (or maybe the draft v8 in some cases). The significant
changes would be:

1. Making querying the capabilities fully compatible with V4L2 but in 1
round-trip over virtio, not 10+. This is what I'm actively working on
right now.


That sounds good, while not essential since the capability negotiation
occurs before we start streaming so these extra trips should not be
perceptible by the user. But the current capability command was not
suitable and needs to be improved anyway.


Great!
Well, there are a lot of steps in capability negotiations in V4L2:
1. enumeration of coded formats,
2. setting the coded format on OUTPUT/CAPTURE,
3. enumeration of raw formats,
4. enumeration of resolutions.
5. enumeration of intervals (optional),
6. enumeration of controls and their ranges (if the range is a menu, it
needs its own enumeration).

Each enumeration is 1 API call per element + 1 more in the end. So it
may require up to like 200 round trips overall if there are many
controls I guess. I believe only the first step can