Re: [virtio-dev] Re: [RFC PATCH v6] virtio-video: Add virtio video device specification
On Mon, Apr 24, 2023 at 4:52 PM Alexander Gordeev
wrote:
>
> On 21.04.23 18:01, Alexander Gordeev wrote:
>
> Hi Alexandre,
>
> On 21.04.23 06:02, Alexandre Courbot wrote:
>
> * I am still not convinced that V4L2 is lacking from a security
> perspective. It would take just one valid example to change my mind
> (and no, the way the queues are named is not valid). And btw, if it
> really introduces security issues, then this makes it invalid for
> inclusion in virtio entirely, just not OpSy's hypervisor.
>
>
> I'd like to start with this and then answer everything else later.
>
> Let's compare VIRTIO_VIDEO_CMD_RESOURCE_QUEUE with
> VIDIOC_QBUF+VIDIOC_DQBUF. Including the parameters, of course. First,
> let's compare the word count to get a very rough estimate of complexity.
> I counted 585 words for VIRTIO_VIDEO_CMD_RESOURCE_QUEUE, including the
> parameters. VIDIOC_QBUF+VIDIOC_DQBUF are defined together and take 1206
> words, they both use struct v4l2_buffer as a parameter. The struct takes
> 2716 words to be described. So the whole thing takes 3922 words. This is
> 6.7 times more, than VIRTIO_VIDEO_CMD_RESOURCE_QUEUE. If we check the
> definitions of the structs, it is also very obvious, that V4L2 UAPI is
> almost like an order of magnitude more complex.
>
>
> I think, it is best to add all the steps necessary to reproduce my
> calculations just in case.
>
> VIRTIO_VIDEO_CMD_RESOURCE_QUEUE is doing essentially the same thing as
> VIDIOC_QBUF+VIDIOC_DQBUF, so we're comparing apples to apples (if we don't
> forget to compare their parameters too).
>
> To get the word count for the VIRTIO_VIDEO_CMD_RESOURCE_QUEUE I opened the
> rendered PDF of video section only from the first email in this thread. Here
> is the link:
> https://drive.google.com/file/d/1Sm6LSqvKqQiwYmDE9BXZ0po3XTKnKYlD/view?usp=sharing
> . Then I scrolled to page 11 and copied everything related a text file. This
> is around two pages in the PDF. Then I removed page numbers from the copied
> text and used 'wc -w' to count words.
>
> To get the word count for VIDIOC_QBUF+VIDIOC_DQBUF I opened this link:
> https://docs.kernel.org/userspace-api/media/v4l/vidioc-qbuf.html . Then I
> selected all the text except table of contents and did followed the same
> procedure.
>
> To get the word count for struct v4l2_buffer and other types, that are
> referenced from it, I opened this link:
> https://docs.kernel.org/userspace-api/media/v4l/buffer.html#struct-v4l2-buffer
> . Then I selected all the text except the table of contents and the text
> above struct v4l2_buffer definition. The rest is the same.
>
> Also it's quite obvious if you look at them how much bigger struct
> v4l2_buffer (including the referenced types) is compared to struct
> virtio_video_resource_queue.
You are comparing not the complexity of the structures but the
verbosity of their documentation, which are written in a different
style, format, and by different people. And the V4L2 page also
contains the description of memory types, which is part of another
section in the virtio-video spec. There is no way to draw a meaningful
conclusion from this.
If you want to compare, do it with how the structures are actually
used. Here is how you would queue an input buffer with virtio-video:
struct virtio_video_resource_queue queue_buf = {
.cmd_type = VIRTIO_VIDEO_CMD_RESOURCE_QUEUE,
.stream_id = 42,
.queue_type = VIRTIO_VIDEO_QUEUE_TYPE_INPUT,
.resource_id = 1,
.timestamp = 0x10,
.data_sizes = {
[0] = 0x1000,
},
};
Now the same with virtio-v4l2:
struct virtio_v4l2_queue_buf queue_buf = {
.cmd = VIRTIO_V4L2_CMD_IOCTL,
.code = VIDIOC_QBUF,
.session_id = 42,
.buffer.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
.buffer.index = 1,
.buffer.timestamp.tv_usec = 0x10,
.buffer.memory = V4L2_MEMORY_MMAP,
.planes = {
[0] = { .bytesused = 0x1000 },
}
};
In both cases, you pass a structure with some members set, and the
rest to 0. The host receives basically the same thing - it's the same
data! The only difference is how it is laid out.
Also as mentioned by Bart, the apparent simplicity of
VIRTIO_VIDEO_CMD_RESOURCE_QUEUE, which does not require a dequeue
operation as the dequeue is sent with its response, is actually a
fallacy: that design choice makes the specification simpler, but at
the cost of more complexity on the device side and the potential to
starve on command descriptors. By contrast, adopting the V4L2 model
resulted in simpler code on both sides and no possibility to deadlock.
That point could be addressed by revising the virtio-video spec, but
then you get even closer to V4L2.
>
> Do we agree now, that V4L2 UAPI is not only marginally more complex?
No, and I rest my case on the code samples above.
>
>
> Also please read:
>
> https://medium.com/starting-up-security/evidence-of-absence-8148958da092
>
>
> This reference probably needs a clarification. You
[virtio-dev] Re: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
On Wed, Apr 26, 2023 at 04:27:34AM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, April 26, 2023 12:12 AM > > > > > > No, this is ROM, not RAM. > > > For VFs it is not ROM because one might configure VF with different feature > bits. Hmm interesting. If you want to block a specific feature from VF you probably want to do it for things like LM compat, and I feel the way to do it is in software not in hardware because you want e.g. cross-vendor compatibility, which specific vendors rarely care about. No? For example, we have all the crazy amount of flexibility with layout of pci config space (not virtio config) that we have zero control over unless we want to go and mandate all that in virtio - which would put us on a treadmill of chasing each new pci capability added. BTW I generally wonder about why this aggressive fight against memory mapped registers does not seem to be reflected in the pci spec at all, which seems to keep happily adding more memory mapped stuff for control plane things in each revision. If anyone, I would expect these guys to know a thing or two about pci hardware. > > > CVQ already exists and provides the SET command. There is no reason to do > > GET in some other way. > > > > Spoken looking at just hardware cost :) > Not really. The CVQ is already there, no extra overheads. > > The reason is that this is device specific. Maintainance overhead and system > > RAM cost for the code to support this should not be ignored. > Sure. As above its not a new and such query occurs only once or extremely > rare. It will have to be done each time user runs ethtool -k, no? Unless you cache this in software then it's extra RAM :) > > > > > Device has single channel to provide a value and hence it doesn't need any > > internal synchronization between two paths. > > > > > > So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an > > improvement. > > > But it still comes with a cost which device cannot mitigate. > > > The cost is, > > > 1. a driver may not negotiate such feature bit, and device ends up burning > > memory. > > > 1.b Device provisioning becomes a factor of what some guests may use/not > > use/already using on the live system. > > > > > > 2. Every device needs AQ even when the CVQ exists. > > > > > > Hence, better to avoid expanding current structure for any new fields, > > specially which has the SET equivalent. > > > > > > But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine. > > > More things can evolve for generic things like config space over AQ. > > > > I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are > > these costs. > What I had in mind is all the rarely used and/or one time used config > registers are queries over Q interface. > A device exposes a feature bit indicating that it offers it via a q interface > instead of MMIO. > A net device already has a CVQ and its almost always there, so utilizing an > existing object to query makes perfect sense. > It can be argued other way that, hey other devices cannot benefit for it > because they missed the CVQ. > So may be a AQ can service for all the device types. Exactly. > > What I had in mind is extending proposed vq transport to support > > sriov. I don't see why we can not have exactly 0 bytes of memory per VF. > > > VFs other than legacy purposes do not undergo mediation via PF. > Legacy is the only exception; newer things is direct communication for a PCI > device PF, SRIOV VFs and SIOV devices. > > > And if we care about single bytes of PF memory (do we? there's only one PF > > per > > SRIOV device ...), what we should do is a variant of pci transport that > > aggressively saves memory. > Users already deploy 16 to 30 PFs coming from single physical card today in > single system. > Building things uniformly for PF, VF, SIOV devices is coming for free. > > It is not only this byte, but we also see that device needs to offer many > capabilities more than boolean flag, so putting non latency sensitive item on > the MMIO area hurts overall. -- 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 v13] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, April 26, 2023 12:12 AM > > > No, this is ROM, not RAM. > For VFs it is not ROM because one might configure VF with different feature bits. > > CVQ already exists and provides the SET command. There is no reason to do > GET in some other way. > > Spoken looking at just hardware cost :) Not really. The CVQ is already there, no extra overheads. > The reason is that this is device specific. Maintainance overhead and system > RAM cost for the code to support this should not be ignored. Sure. As above its not a new and such query occurs only once or extremely rare. > > > Device has single channel to provide a value and hence it doesn't need any > internal synchronization between two paths. > > > > So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an > improvement. > > But it still comes with a cost which device cannot mitigate. > > The cost is, > > 1. a driver may not negotiate such feature bit, and device ends up burning > memory. > > 1.b Device provisioning becomes a factor of what some guests may use/not > use/already using on the live system. > > > > 2. Every device needs AQ even when the CVQ exists. > > > > Hence, better to avoid expanding current structure for any new fields, > specially which has the SET equivalent. > > > > But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine. > > More things can evolve for generic things like config space over AQ. > > I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are > these costs. What I had in mind is all the rarely used and/or one time used config registers are queries over Q interface. A device exposes a feature bit indicating that it offers it via a q interface instead of MMIO. A net device already has a CVQ and its almost always there, so utilizing an existing object to query makes perfect sense. It can be argued other way that, hey other devices cannot benefit for it because they missed the CVQ. So may be a AQ can service for all the device types. > What I had in mind is extending proposed vq transport to support > sriov. I don't see why we can not have exactly 0 bytes of memory per VF. > VFs other than legacy purposes do not undergo mediation via PF. Legacy is the only exception; newer things is direct communication for a PCI device PF, SRIOV VFs and SIOV devices. > And if we care about single bytes of PF memory (do we? there's only one PF per > SRIOV device ...), what we should do is a variant of pci transport that > aggressively saves memory. Users already deploy 16 to 30 PFs coming from single physical card today in single system. Building things uniformly for PF, VF, SIOV devices is coming for free. It is not only this byte, but we also see that device needs to offer many capabilities more than boolean flag, so putting non latency sensitive item on the MMIO area hurts overall. - 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 v13] virtio-net: support inner header hash
On Tue, Apr 25, 2023 at 09:39:28PM +, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, April 25, 2023 5:06 PM
> > > On 4/23/2023 3:35 AM, Heng Qi wrote:
> > > > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > > Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > > > @@
> > -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device
> > Types / Network Device
> > > > u8 rss_max_key_size;
> > > > le16 rss_max_indirection_table_length;
> > > > le32 supported_hash_types;
> > > > +le32 supported_tunnel_hash_types;
> >
> > this needs a comment explaining it only exists with some feature bits.
> >
> Yes, it is already there.
> +Field \field{supported_tunnel_hash_types} only exists if the device supports
> inner header hash, i.e. if VIRTIO_NET_F_HASH_TUNNEL is set.
> +
> I think it should be changed from "device supports" to "driver negotiated".
>
> > > > };
> > > In v12 I was asking this to move to above field from the config area
> > > to the GET command in comment [1] as,
> > >
> > > "With that no need to define two fields at two different places in
> > > config area and also in cvq."
> >
> > I think I disagree.
> > the proposed design is consistent with regular tunneling.
> >
> Sure.
> I understand how config space has evolved from 0.9.5 to know without much
> attention, but really expanding this way is not helpful.
> It requires building more and more RAM based devices even for PCI PFs, which
> is sub optimal.
No, this is ROM, not RAM.
> CVQ already exists and provides the SET command. There is no reason to do GET
> in some other way.
Spoken looking at just hardware cost :)
The reason is that this is device specific. Maintainance overhead and
system RAM cost for the code to support this should not be ignored.
> Device has single channel to provide a value and hence it doesn't need any
> internal synchronization between two paths.
>
> So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an
> improvement.
> But it still comes with a cost which device cannot mitigate.
> The cost is,
> 1. a driver may not negotiate such feature bit, and device ends up burning
> memory.
> 1.b Device provisioning becomes a factor of what some guests may use/not
> use/already using on the live system.
>
> 2. Every device needs AQ even when the CVQ exists.
>
> Hence, better to avoid expanding current structure for any new fields,
> specially which has the SET equivalent.
>
> But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine.
> More things can evolve for generic things like config space over AQ.
I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are
these costs. What I had in mind is extending proposed vq transport to
support sriov. I don't see why we can not have exactly 0 bytes of memory
per VF.
And if we care about single bytes of PF memory (do we? there's only one
PF per SRIOV device ...), what we should do is a variant of pci
transport that aggressively saves 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: [virtio-comment] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
On Tue, Apr 25, 2023 at 10:09 PM Stefan Hajnoczi wrote: > > On Mon, Apr 24, 2023 at 11:40:02AM +0800, Jason Wang wrote: > > On Sun, Apr 23, 2023 at 7:31 PM zhenwei pi wrote: > > > I develop an kernel initiator(unstable, WIP version, currently TCP/RDMA > > > supported): > > > https://github.com/pizhenwei/linux/tree/virtio-of-github > > > > A quick glance at the code told me it's a mediation layer that convert > > descriptors in the vring to the fabric specific packet. This is the > > vDPA way. > > > > If we agree virtio of fabic is useful, we need invent facilities to > > allow building packet directly without bothering the virtqueue (the > > API is layout independent anyhow). > > I agree. vrings makes sense for RDMA, but I think virtio_fabrics.c > should not be dependent on vrings. > > Linux struct virtqueue is independent of vrings but the implementation > currently lives in virtio_ring.c because there has never been a > non-vring transport before. > > It would be nice to implement virtqueue_add_sgs() specifically for > virtio_tcp.c without the use of vrings. Is a new struct > virtqueue_ops needed with with .add_sgs() and related callbacks? > > Luckily the API already supports this abstraction and > changes to existing device drivers should be unnecessary or minimal. That's my understanding. Btw, I'm also ok if we start with vring and optimize on top. Thanks > > Stefan - 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: Re: [virtio-comment] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
On 4/25/23 21:55, Stefan Hajnoczi wrote: On Mon, Apr 24, 2023 at 11:40:02AM +0800, Jason Wang wrote: On Sun, Apr 23, 2023 at 7:31 PM zhenwei pi wrote: "Virtio Over Fabrics" aims at "reuse virtio device specifications", and provides network defined peripheral devices. And this protocol also could be used in virtualization environment, typically hypervisor(or vhost-user process) handles request from virtio PCI/MMIO/CCW, remaps request and forwards to target by fabrics. This requires meditation in the datapath, isn't it? - Protocol The detail protocol definition see: https://github.com/pizhenwei/linux/blob/virtio-of-github/include/uapi/linux/virtio_of.h I'd say a RFC patch for virtio spec is more suitable than the codes. VIRTIO over TCP has long been anticipated but so far no one posted an implementation. There are probably mentions of it from 10+ years ago. I'm excited to see this! Both the VIRTIO spec and the Linux drivers provide an abstraction that allows fabrics (e.g. TCP) to fit in as a VIRTIO Transport. vrings are not the only way to implement virtqueues. Many VIRTIO devices will work fine over a message passing transport like TCP. A few devices like the balloon device may not make sense. Shared Memory Regions won't work. Fully agree. Please define VIRTIO over Fabrics as a Transport in the VIRTIO spec so that the integration with the VIRTIO device model is seamless. I look forward to discussing spec patches. Stefan Thanks, I'm working on it. -- zhenwei pi - 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 v13] virtio-net: support inner header hash
> From: Michael S. Tsirkin
> Sent: Tuesday, April 25, 2023 5:06 PM
> > On 4/23/2023 3:35 AM, Heng Qi wrote:
> > > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > Types / Network Device / Feature bits / Legacy Interface: Feature bits} @@
> -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device
> Types / Network Device
> > > u8 rss_max_key_size;
> > > le16 rss_max_indirection_table_length;
> > > le32 supported_hash_types;
> > > +le32 supported_tunnel_hash_types;
>
> this needs a comment explaining it only exists with some feature bits.
>
Yes, it is already there.
+Field \field{supported_tunnel_hash_types} only exists if the device supports
inner header hash, i.e. if VIRTIO_NET_F_HASH_TUNNEL is set.
+
I think it should be changed from "device supports" to "driver negotiated".
> > > };
> > In v12 I was asking this to move to above field from the config area
> > to the GET command in comment [1] as,
> >
> > "With that no need to define two fields at two different places in
> > config area and also in cvq."
>
> I think I disagree.
> the proposed design is consistent with regular tunneling.
>
Sure.
I understand how config space has evolved from 0.9.5 to know without much
attention, but really expanding this way is not helpful.
It requires building more and more RAM based devices even for PCI PFs, which is
sub optimal.
CVQ already exists and provides the SET command. There is no reason to do GET
in some other way.
Device has single channel to provide a value and hence it doesn't need any
internal synchronization between two paths.
So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an
improvement.
But it still comes with a cost which device cannot mitigate.
The cost is,
1. a driver may not negotiate such feature bit, and device ends up burning
memory.
1.b Device provisioning becomes a factor of what some guests may use/not
use/already using on the live system.
2. Every device needs AQ even when the CVQ exists.
Hence, better to avoid expanding current structure for any new fields,
specially which has the SET equivalent.
But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine.
More things can evolve for generic things like config space over AQ.
-
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: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference
On Tue, Apr 25, 2023 at 06:20:08PM +0200, Halil Pasic wrote: > On Tue, 25 Apr 2023 09:15:14 -0400 > "Michael S. Tsirkin" wrote: > > > On Tue, Apr 25, 2023 at 03:02:15PM +0200, Halil Pasic wrote: > > > On Tue, 25 Apr 2023 04:10:00 + > > > Parav Pandit wrote: > [..] > > > > Sure. All you wrote is correct. > > > > > > > > > > I'm happy we agree. All I say we may want to rewrite the > > > > > > "Each virtqueue is identified by a virtqueue index; virtqueue index > > > range is from 0 to 65535 inclusive." > > > as > > > "Each virtqueue is uniquely identified by a virtqueue index. The number > > > of supported virtqueues is device dependent, but can never exceed 65536. > > > Thus 16 bit is sufficient to represent virtqueue indexes. If the number > > > of virtqueues currently supported by some device is N, each of it is > > > virtqueues is uniquely identified by a single index from the range > > > [0..N-1]." > > > > Seems to be repeating same thing over and over. > > Nod. > > > This redundancy has cost, e.g. more places to change when we > > talk about admin queues. > > Yes it can not be any number 0 to 65535 but this kind of nitpicking > > belongs in conformance statements not in general description. > > > > I tend to agree. I would prefer to have it in some sort of conformance > statement, but I would also prefer to have it in exactly one place (and > not all over the place). > > Regards, > Halil > > [..] I don't feel there is much to say in one place. We did not in the past specify what happens if drivers e.g. set queue select to a large number. So some drivers might do just that for a variety of reasons. In other words, given a queue there's an index and i feel Parav's text explains what it is pretty unambigously and also succintly. I would even maybe drop the 0 to 65535 thing and just say that it is 0 based. What are the specific limitations on the index is already explained elsewhere. -- 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 v13] virtio-net: support inner header hash
On Tue, Apr 25, 2023 at 04:28:33PM -0400, Parav Pandit wrote:
>
>
> On 4/23/2023 3:35 AM, Heng Qi wrote:
> > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types /
> > Network Device / Feature bits / Legacy Interface: Feature bits}
> > @@ -198,6 +202,7 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device
> > u8 rss_max_key_size;
> > le16 rss_max_indirection_table_length;
> > le32 supported_hash_types;
> > +le32 supported_tunnel_hash_types;
this needs a comment explaining it only exists with some
feature bits.
> > };
> In v12 I was asking this to move to above field from the config area to the
> GET command in comment [1] as,
>
> "With that no need to define two fields at two different places in config
> area and also in cvq."
I think I disagree.
the proposed design is consistent with regular tunneling.
however I feel we should limit this to 1-2 legacy protocols.
with that, we do not really need a new field at all,
they can fit in supported_hash_types.
> I am sorry if that was not clear enough.
>
> [1]
> https://lore.kernel.org/virtio-dev/569cbaf9-f1fb-0e1f-a2ef-b1d7cd7db...@nvidia.com/
>
> > \subparagraph{Supported/enabled hash types}
> > \label{sec:Device Types / Network Device / Device Operation / Processing
> > of Incoming Packets / Hash calculation for incoming packets /
> > Supported/enabled hash types}
> > +This paragraph relies on definitions from \hyperref[intro:IP]{[IP]},
> > \hyperref[intro:UDP]{[UDP]} and \hyperref[intro:TCP]{[TCP]}.
> > Hash types applicable for IPv4 packets:
> > \begin{lstlisting}
> > #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> > @@ -980,6 +993,152 @@ \subsubsection{Processing of Incoming
> > Packets}\label{sec:Device Types / Network
> > (see \ref{sec:Device Types / Network Device / Device Operation /
> > Processing of Incoming Packets / Hash calculation for incoming packets /
> > IPv6 packets without extension header}).
> > \end{itemize}
> > +\paragraph{Inner Header Hash}
> > +\label{sec:Device Types / Network Device / Device Operation / Processing
> > of Incoming Packets / Inner Header Hash}
> > +
> > +If VIRTIO_NET_F_HASH_TUNNEL has been negotiated, the device supports inner
> > header hash and the driver can send
> > +commands VIRTIO_NET_CTRL_TUNNEL_HASH_SET and
> > VIRTIO_NET_CTRL_TUNNEL_HASH_GET for the inner header hash configuration.
> > +
> > +struct virtio_net_hash_tunnel_config {
> Please move field from the config struct to here. Both are RO fields.
>
> le32 supported_hash_tunnel_types;
> > +le32 hash_tunnel_types;
> > +};
> > +
> > +#define VIRTIO_NET_CTRL_TUNNEL_HASH 7
> > + #define VIRTIO_NET_CTRL_TUNNEL_HASH_SET 0
> > + #define VIRTIO_NET_CTRL_TUNNEL_HASH_GET 1
> > +
> > +Filed \field{hash_tunnel_types} contains a bitmask of configured hash
> > tunnel types as
> > +defined in \ref{sec:Device Types / Network Device / Device Operation /
> > Processing of Incoming Packets / Hash calculation for incoming packets /
> > Supported/enabled hash tunnel types}.
> > +
> > +The class VIRTIO_NET_CTRL_TUNNEL_HASH has the following commands:
> > +\begin{itemize}
> > +\item VIRTIO_NET_CTRL_TUNNEL_HASH_SET: set the \field{hash_tunnel_types}
> > to configure the inner header hash calculation for the device.
> > +\item VIRTIO_NET_CTRL_TUNNEL_HASH_GET: get the \field{hash_tunnel_types}
> > from the device.
> > +\end{itemize}
> > +
> > +For the command VIRTIO_NET_CTRL_TUNNEL_HASH_SET, the structure
> > virtio_net_hash_tunnel_config is write-only for the driver.
> > +For the command VIRTIO_NET_CTRL_TUNNEL_HASH_GET, the structure
> > virtio_net_hash_tunnel_config is read-only for the driver.
> > +
> You need to split the structures to two, one for get and one for set in
> above description as get and set contains different fields.
> > +
> > +If VIRTIO_NET_HASH_TUNNEL_TYPE_NONE is set or the encapsulation type is
> > not included in \field{hash_tunnel_types},
> > +the hash of the outer header is calculated for the received encapsulated
> > packet.
> > +
> > +
> > +For scenarios with sufficient external entropy or no internal hashing
> > requirements, inner header hash may not be needed:
> > +A tunnel is often expected to isolate the external network from the
> > internal one. By completely ignoring entropy
> > +in the external header and replacing it with entropy from the internal
> > header, for hash calculations, this expectation
> You wanted to say inner here like rest of the places.
>
> s/internal header/inner header
>
> > +The driver MUST NOT set any VIRTIO_NET_HASH_TUNNEL_TYPE_ flags that are
> > not supported by the device.
> Multiple flags so,
>
> s/flags that are/flags which are/
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive,
[virtio-dev] Re: [virtio-comment] [PATCH v13] virtio-net: support inner header hash
On Sun, Apr 23, 2023 at 03:35:32PM +0800, Heng Qi wrote:
> 1. Currently, a received encapsulated packet has an outer and an inner
> header, but
> the virtio device is unable to calculate the hash for the inner header. The
> same
> flow can traverse through different tunnels, resulting in the encapsulated
> packets being spread across multiple receive queues (refer to the figure
> below).
> However, in certain scenarios, we may need to direct these encapsulated
> packets of
> the same flow to a single receive queue. This facilitates the processing
> of the flow by the same CPU to improve performance (warm caches, less
> locking, etc.).
>
>client1client2
> |+---+ |
> +--->|tunnels|<+
>+---+
> | |
> v v
> +-+
> | monitoring host |
> +-+
>
> To achieve this, the device can calculate a symmetric hash based on the inner
> headers
> of the same flow.
>
> 2. For legacy systems, they may lack entropy fields which modern protocols
> have in
> the outer header, resulting in multiple flows with the same outer header but
> different inner headers being directed to the same receive queue. This
> results in
> poor receive performance.
>
> To address this limitation, inner header hash can be used to enable the
> device to advertise
> the capability to calculate the hash for the inner packet, regaining better
> receive performance.
>
> Signed-off-by: Heng Qi
> Reviewed-by: Xuan Zhuo
grammar in new text is still pretty bad, lots of typos too.
Don't have time to fix it for you right now sorry, it's
a holiday here.
> ---
> v12->v13:
> 1. Add a GET command for hash_tunnel_types. @Parav Pandit
> 2. Add tunneling protocol explanation. @Jason Wang
> 3. Add comments on some usage scenarios for inner hash.
>
> v11->v12:
> 1. Add a command VIRTIO_NET_CTRL_MQ_TUNNEL_CONFIG.
> 2. Refine the commit log. @Michael S . Tsirkin
> 3. Add some tunnel types.
>
> v10->v11:
> 1. Revise commit log for clarity for readers.
> 2. Some modifications to avoid undefined terms. @Parav Pandit
> 3. Change VIRTIO_NET_F_HASH_TUNNEL dependency. @Parav Pandit
> 4. Add the normative statements. @Parav Pandit
>
> v9->v10:
> 1. Removed hash_report_tunnel related information. @Parav Pandit
> 2. Re-describe the limitations of QoS for tunneling.
> 3. Some clarification.
>
> v8->v9:
> 1. Merge hash_report_tunnel_types into hash_report. @Parav Pandit
> 2. Add tunnel security section. @Michael S . Tsirkin
> 3. Add VIRTIO_NET_F_HASH_REPORT_TUNNEL.
> 4. Fix some typos.
> 5. Add more tunnel types. @Michael S . Tsirkin
>
> v7->v8:
> 1. Add supported_hash_tunnel_types. @Jason Wang, @Parav Pandit
> 2. Change hash_report_tunnel to hash_report_tunnel_types. @Parav Pandit
> 3. Removed re-definition for inner packet hashing. @Parav Pandit
> 4. Fix some typos. @Michael S . Tsirkin
> 5. Clarify some sentences. @Michael S . Tsirkin
>
> v6->v7:
> 1. Modify the wording of some sentences for clarity. @Michael S. Tsirkin
> 2. Fix some syntax issues. @Michael S. Tsirkin
>
> v5->v6:
> 1. Fix some syntax and capitalization issues. @Michael S. Tsirkin
> 2. Use encapsulated/encaptulation uniformly. @Michael S. Tsirkin
> 3. Move the links to introduction section. @Michael S. Tsirkin
> 4. Clarify some sentences. @Michael S. Tsirkin
>
> v4->v5:
> 1. Clarify some paragraphs. @Cornelia Huck
> 2. Fix the u8 type. @Cornelia Huck
>
> v3->v4:
> 1. Rename VIRTIO_NET_F_HASH_GRE_VXLAN_GENEVE_INNER to
> VIRTIO_NET_F_HASH_TUNNEL. @Jason Wang
> 2. Make things clearer. @Jason Wang @Michael S. Tsirkin
> 3. Keep the possibility to use inner hash for automatic receive
> steering. @Jason Wang
> 4. Add the "Tunnel packet" paragraph to avoid repeating the GRE etc.
> many times. @Michael S. Tsirkin
>
> v2->v3:
> 1. Add a feature bit for GRE/VXLAN/GENEVE inner hash. @Jason Wang
> 2. Chang \field{hash_tunnel} to \field{hash_report_tunnel}. @Jason
> Wang, @Michael S. Tsirkin
>
> v1->v2:
> 1. Remove the patch for the bitmask fix. @Michael S. Tsirkin
> 2. Clarify some paragraphs. @Jason Wang
> 3. Add \field{hash_tunnel} and VIRTIO_NET_HASH_REPORT_GRE. @Yuri
> Benditovich
>
> device-types/net/description.tex| 159
> device-types/net/device-conformance.tex | 1 +
> device-types/net/driver-conformance.tex | 1 +
> introduction.tex| 44 +++
> 4 files changed, 205 insertions(+)
>
> diff --git a/device-types/net/description.tex
> b/device-types/net/description.tex
> index 0500bb6..48e41f1
[virtio-dev] Re: [PATCH v12 03/10] admin: introduce group administration commands
On Tue, Apr 25, 2023 at 08:18:54PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Tuesday, April 25, 2023 4:05 PM > > > Ok. I feel it is still worth to have commit log updated. If you happen to > > > revise > > v13, please do. > > > Else its fine. > > > > > > v12 surely doesn't merge cleanly. So v13 is needed anyway. > > > > > > Can you please generate? Patch-5 of v12 fails to apply. > > > > should apply on top of 985bbf397db4228faf3ec464e838687dc4cb4904. master > > will fail because of the file movement. I will need to rebase to apply on > > latest > > master, I will do it before we start voting. > > Ok. sounds good. I assume you will post rebased v13 with commit log updated? > When do you plan to do? > I request that it is better to rebase now because legacy related patches need > to touch the add the PCI extended capability on top of your changes. I can rebase and push to a branch. Day after tomorrow at the earliest due to a holiday. I don't want to repost right now, spamming is not polite to reviewers, several days need to pass. > This results in further manual merges for me. > Better to do the work once with one time rebase, instead of both of us. > > Let me know if I should generate v13 fixing above two items as you have > holiday this week. > It helps me build on top of AQ patches with one time rebase. I pushed the rebase to tag mst-admin-v13 on github. Don't have time for tweaks it's a holiday. I usually leave patches on list for a week or so. If no comments I will post v13 and we will roll a vote. -- 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 v13] virtio-net: support inner header hash
On 4/23/2023 3:35 AM, Heng Qi wrote:
\subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network Device / Feature bits / Legacy Interface: Feature bits}
@@ -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device
Types / Network Device
u8 rss_max_key_size;
le16 rss_max_indirection_table_length;
le32 supported_hash_types;
+le32 supported_tunnel_hash_types;
};
In v12 I was asking this to move to above field from the config area to
the GET command in comment [1] as,
"With that no need to define two fields at two different places in
config area and also in cvq."
I am sorry if that was not clear enough.
[1]
https://lore.kernel.org/virtio-dev/569cbaf9-f1fb-0e1f-a2ef-b1d7cd7db...@nvidia.com/
\subparagraph{Supported/enabled hash types}
\label{sec:Device Types / Network Device / Device Operation / Processing of
Incoming Packets / Hash calculation for incoming packets / Supported/enabled
hash types}
+This paragraph relies on definitions from \hyperref[intro:IP]{[IP]},
\hyperref[intro:UDP]{[UDP]} and \hyperref[intro:TCP]{[TCP]}.
Hash types applicable for IPv4 packets:
\begin{lstlisting}
#define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
@@ -980,6 +993,152 @@ \subsubsection{Processing of Incoming
Packets}\label{sec:Device Types / Network
(see \ref{sec:Device Types / Network Device / Device Operation / Processing
of Incoming Packets / Hash calculation for incoming packets / IPv6 packets
without extension header}).
\end{itemize}
+\paragraph{Inner Header Hash}
+\label{sec:Device Types / Network Device / Device Operation / Processing of
Incoming Packets / Inner Header Hash}
+
+If VIRTIO_NET_F_HASH_TUNNEL has been negotiated, the device supports inner
header hash and the driver can send
+commands VIRTIO_NET_CTRL_TUNNEL_HASH_SET and VIRTIO_NET_CTRL_TUNNEL_HASH_GET
for the inner header hash configuration.
+
+struct virtio_net_hash_tunnel_config {
Please move field from the config struct to here. Both are RO fields.
le32 supported_hash_tunnel_types;
+le32 hash_tunnel_types;
+};
+
+#define VIRTIO_NET_CTRL_TUNNEL_HASH 7
+ #define VIRTIO_NET_CTRL_TUNNEL_HASH_SET 0
+ #define VIRTIO_NET_CTRL_TUNNEL_HASH_GET 1
+
+Filed \field{hash_tunnel_types} contains a bitmask of configured hash tunnel
types as
+defined in \ref{sec:Device Types / Network Device / Device Operation /
Processing of Incoming Packets / Hash calculation for incoming packets /
Supported/enabled hash tunnel types}.
+
+The class VIRTIO_NET_CTRL_TUNNEL_HASH has the following commands:
+\begin{itemize}
+\item VIRTIO_NET_CTRL_TUNNEL_HASH_SET: set the \field{hash_tunnel_types} to
configure the inner header hash calculation for the device.
+\item VIRTIO_NET_CTRL_TUNNEL_HASH_GET: get the \field{hash_tunnel_types} from
the device.
+\end{itemize}
+
+For the command VIRTIO_NET_CTRL_TUNNEL_HASH_SET, the structure
virtio_net_hash_tunnel_config is write-only for the driver.
+For the command VIRTIO_NET_CTRL_TUNNEL_HASH_GET, the structure
virtio_net_hash_tunnel_config is read-only for the driver.
+
You need to split the structures to two, one for get and one for set in
above description as get and set contains different fields.
+
+If VIRTIO_NET_HASH_TUNNEL_TYPE_NONE is set or the encapsulation type is not
included in \field{hash_tunnel_types},
+the hash of the outer header is calculated for the received encapsulated
packet.
+
+
+For scenarios with sufficient external entropy or no internal hashing
requirements, inner header hash may not be needed:
+A tunnel is often expected to isolate the external network from the internal
one. By completely ignoring entropy
+in the external header and replacing it with entropy from the internal header,
for hash calculations, this expectation
You wanted to say inner here like rest of the places.
s/internal header/inner header
+The driver MUST NOT set any VIRTIO_NET_HASH_TUNNEL_TYPE_ flags that are not
supported by the device.
Multiple flags so,
s/flags that are/flags which are/
-
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 v12 03/10] admin: introduce group administration commands
> From: Michael S. Tsirkin > Sent: Tuesday, April 25, 2023 4:05 PM > > Ok. I feel it is still worth to have commit log updated. If you happen to > > revise > v13, please do. > > Else its fine. > > > > v12 surely doesn't merge cleanly. So v13 is needed anyway. > > > > Can you please generate? Patch-5 of v12 fails to apply. > > should apply on top of 985bbf397db4228faf3ec464e838687dc4cb4904. master > will fail because of the file movement. I will need to rebase to apply on > latest > master, I will do it before we start voting. Ok. sounds good. I assume you will post rebased v13 with commit log updated? When do you plan to do? I request that it is better to rebase now because legacy related patches need to touch the add the PCI extended capability on top of your changes. This results in further manual merges for me. Better to do the work once with one time rebase, instead of both of us. Let me know if I should generate v13 fixing above two items as you have holiday this week. It helps me build on top of AQ patches with one time rebase. - 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 v12 03/10] admin: introduce group administration commands
On Tue, Apr 25, 2023 at 01:38:21PM +, Parav Pandit wrote: > > > > From: Michael S. Tsirkin > > Sent: Tuesday, April 25, 2023 9:32 AM > > > > I am requesting to drop the commit point that limits the usage of the AQ. > > > Do you agree that AQ can be extended for use beyond VF and SIOV group > > management commands? > > > Or one should create a new VQ type? > > > If it is the later, I don't see the need of multiple AQ. > > > > not management. that is old term and indeed too narrow. > > administration as in hypervisor (admin) access through PF while guest has > > access through VF. > > > > legacy access you link to below seems to fall within scope: > > we access VF through a PF. > > > > So what is the problem? > > > Commit log confused me. You clarified above. > It is clear now. Hence no problem. > > > maybe we'll change the meaning down the road. I do not see the immediate > > need currently though. > > > As long as we agree that it is open to widen the scope, it is acceptable. > > > > [1] > > > https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html > > > > Then I don't see a problem. It's just commit log, not spec. Let it slide is > > my > > suggestion, this is spec text not code. > > Ok. I feel it is still worth to have commit log updated. If you happen to > revise v13, please do. > Else its fine. > > v12 surely doesn't merge cleanly. So v13 is needed anyway. > > Can you please generate? Patch-5 of v12 fails to apply. should apply on top of 985bbf397db4228faf3ec464e838687dc4cb4904. master will fail because of the file movement. I will need to rebase to apply on latest master, I will do it before we start voting. -- MST - 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: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference
On Tue, 25 Apr 2023 09:15:14 -0400 "Michael S. Tsirkin" wrote: > On Tue, Apr 25, 2023 at 03:02:15PM +0200, Halil Pasic wrote: > > On Tue, 25 Apr 2023 04:10:00 + > > Parav Pandit wrote: [..] > > > Sure. All you wrote is correct. > > > > > > > I'm happy we agree. All I say we may want to rewrite the > > > > "Each virtqueue is identified by a virtqueue index; virtqueue index > > range is from 0 to 65535 inclusive." > > as > > "Each virtqueue is uniquely identified by a virtqueue index. The number > > of supported virtqueues is device dependent, but can never exceed 65536. > > Thus 16 bit is sufficient to represent virtqueue indexes. If the number > > of virtqueues currently supported by some device is N, each of it is > > virtqueues is uniquely identified by a single index from the range > > [0..N-1]." > > Seems to be repeating same thing over and over. Nod. > This redundancy has cost, e.g. more places to change when we > talk about admin queues. > Yes it can not be any number 0 to 65535 but this kind of nitpicking > belongs in conformance statements not in general description. > I tend to agree. I would prefer to have it in some sort of conformance statement, but I would also prefer to have it in exactly one place (and not all over the place). Regards, Halil [..] - 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
[I'm replying here, as that seems to be the last message in the thread, and my reply hopefully catches everyone interested here.] To do a very high level summary, we have (at least) two use cases for virtio-video, that unfortunately have quite different requirements. Both want to encode/decode video, but in different environments. - The "restricted" case: Priority is on security, and the attack surface should be kept as small as possible, for example, by avoiding unneded complexity in the interface. Fancy allocations and management should be avoided. The required functionality is also quite clearly defined. - The "feature-rich" case: Priority is on enabling features, and being able to re-use existing V4L2 support is considered a big plus. Both device and driver implementations will be implemented in a full OS environment, so all kind of helpers are already available. (This is not to say that one case does not care about functionality or security; it's mostly a case of different priorities and environments.) I had been hoping that it would be possible to find kind of a common ground between the two cases, but reading the thread, I'm not quite as hopeful anymore... if we really don't manage to find an approach to make the different requirements co-exist, a separate virtio-v4l2 device might be the way to go -- but I've not totally given up hope yet. Some remarks from my side: - I'm not totally convinced that counting words is always a good proxy for complexity -- an interface might be simple on paper, but if the actual implementation would need to be quite involved to get it right, we'd again have a lot of opportunity for mistakes. - How much of v4l2 does actually need to be in the device specification for a driver to make potentially good use of it? Sure, being able to directly map to v4l2 really gives a huge benefit, but is there a way to extract a subset that's not too complex, but can be easily wrapped for interfacing with v4l2? (Both interface and functionality wise.) Even if that means that a driver would need to implement some kind of shim, a layer that easily maps to v4l2 concepts would still be much easier to implement than one that needs to map two quite different interfaces. [I'm really relying on the good judgement of people familiar with the interfaces here :)] - To which extent does security need to be baked into the device specification? We should avoid footguns, and avoiding needless complication is also a good idea, but while every new functionality means more attack surface, it also enables more use cases. That tension is hard to resolve; how much of it can we alleviate by making things optional? I hope I have not muddied the waters here, but I'd really like to see an agreement on how to continue (with two different devices, if there is really no other way.) - 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] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
On Mon, Apr 24, 2023 at 11:40:02AM +0800, Jason Wang wrote: > On Sun, Apr 23, 2023 at 7:31 PM zhenwei pi wrote: > > I develop an kernel initiator(unstable, WIP version, currently TCP/RDMA > > supported): > > https://github.com/pizhenwei/linux/tree/virtio-of-github > > A quick glance at the code told me it's a mediation layer that convert > descriptors in the vring to the fabric specific packet. This is the > vDPA way. > > If we agree virtio of fabic is useful, we need invent facilities to > allow building packet directly without bothering the virtqueue (the > API is layout independent anyhow). I agree. vrings makes sense for RDMA, but I think virtio_fabrics.c should not be dependent on vrings. Linux struct virtqueue is independent of vrings but the implementation currently lives in virtio_ring.c because there has never been a non-vring transport before. It would be nice to implement virtqueue_add_sgs() specifically for virtio_tcp.c without the use of vrings. Is a new struct virtqueue_ops needed with with .add_sgs() and related callbacks? Luckily the API already supports this abstraction and changes to existing device drivers should be unnecessary or minimal. Stefan signature.asc Description: PGP signature
[virtio-dev] Re: [virtio-comment] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
On Mon, Apr 24, 2023 at 11:40:02AM +0800, Jason Wang wrote: > On Sun, Apr 23, 2023 at 7:31 PM zhenwei pi wrote: > > "Virtio Over Fabrics" aims at "reuse virtio device specifications", and > > provides network defined peripheral devices. > > And this protocol also could be used in virtualization environment, > > typically hypervisor(or vhost-user process) handles request from virtio > > PCI/MMIO/CCW, remaps request and forwards to target by fabrics. > > This requires meditation in the datapath, isn't it? > > > > > - Protocol > > The detail protocol definition see: > > https://github.com/pizhenwei/linux/blob/virtio-of-github/include/uapi/linux/virtio_of.h > > I'd say a RFC patch for virtio spec is more suitable than the codes. VIRTIO over TCP has long been anticipated but so far no one posted an implementation. There are probably mentions of it from 10+ years ago. I'm excited to see this! Both the VIRTIO spec and the Linux drivers provide an abstraction that allows fabrics (e.g. TCP) to fit in as a VIRTIO Transport. vrings are not the only way to implement virtqueues. Many VIRTIO devices will work fine over a message passing transport like TCP. A few devices like the balloon device may not make sense. Shared Memory Regions won't work. Please define VIRTIO over Fabrics as a Transport in the VIRTIO spec so that the integration with the VIRTIO device model is seamless. I look forward to discussing spec patches. Stefan signature.asc Description: PGP signature
[virtio-dev] RE: [PATCH v12 03/10] admin: introduce group administration commands
> From: Michael S. Tsirkin > Sent: Tuesday, April 25, 2023 9:32 AM > > I am requesting to drop the commit point that limits the usage of the AQ. > > Do you agree that AQ can be extended for use beyond VF and SIOV group > management commands? > > Or one should create a new VQ type? > > If it is the later, I don't see the need of multiple AQ. > > not management. that is old term and indeed too narrow. > administration as in hypervisor (admin) access through PF while guest has > access through VF. > > legacy access you link to below seems to fall within scope: > we access VF through a PF. > > So what is the problem? > Commit log confused me. You clarified above. It is clear now. Hence no problem. > maybe we'll change the meaning down the road. I do not see the immediate > need currently though. > As long as we agree that it is open to widen the scope, it is acceptable. > > [1] > > https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html > > Then I don't see a problem. It's just commit log, not spec. Let it slide is my > suggestion, this is spec text not code. Ok. I feel it is still worth to have commit log updated. If you happen to revise v13, please do. Else its fine. v12 surely doesn't merge cleanly. So v13 is needed anyway. Can you please generate? Patch-5 of v12 fails to apply. - 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 v12 03/10] admin: introduce group administration commands
On Tue, Apr 25, 2023 at 01:25:24PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Tuesday, April 25, 2023 2:20 AM > > > > Below paragraph is no longer applies as we already discussed more use > > > cases of the AQ such as accessing the PCI VF's legacy config space > > > registers. > > > Please drop below paragraph. > > > > Look - at one point you want commit log to record design process, an another > > you turn around and ask me to drop it. > > I feel like keeping this around frankly. Maybe I will add a sentence or two > > along > > the lines of "Note that nothing limits us from extending this down the road > > though - but let's focus on what we already know for now". > > > I am requesting to drop the commit point that limits the usage of the AQ. > Do you agree that AQ can be extended for use beyond VF and SIOV group > management commands? > Or one should create a new VQ type? > If it is the later, I don't see the need of multiple AQ. not management. that is old term and indeed too narrow. administration as in hypervisor (admin) access through PF while guest has access through VF. legacy access you link to below seems to fall within scope: we access VF through a PF. So what is the problem? maybe we'll change the meaning down the road. I do not see the immediate need currently though. > > This extreme focus on commit log is poinless anyway - it makes much more > > sense for code since commit log describes the change in english. Here the > > whole change is english anyway. > > > > > > Without this the admin vq would become a "whatever" vq which does > > > > not do anything specific at all, just a general transport like thing. > > > > I feel going this way opens the design space to the point where we > > > > no longer know what belongs in e.g. config space what in the control > > > > q and what in the admin q. > > > > As it is, whatever deals with groups is in the admin q; other things > > > > not in the admin q. > > > > > > > > > > A PF can use same or different AQ with a new struct > > > virtio_legacy_register_access with a different opcode range. > > > > We'll get to that bridge and we'll cross it, the proposal is not on list > > even yet. > It is at [1]. > [1] https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html > I cannot draft the legacy command using AQ because AQ patch-5 in v12 fails to > merge. > > Before I draft it, I sent [1] on the design way forward. > > > I > > actually think that yes, you need it in a separate function. If PF is passed > > through to guest then PF can not also safely DMA into host memory. > That is fine. Only some user tests would pass the PF and VF to the VMs. > One can create N combinations to make it not work. > In all practical purposes, PF is owned by the hypervisor, trusted sw as > listed in the PCI spec. > > > Alternatively, we could use an MMIO based mechanism for admin commands. > > And maybe that would be a good fit. > We discussed that MMIO is not a good fit for the VFs at scale as listed in > [1]. > > [1] https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html Then I don't see a problem. It's just commit log, not spec. Let it slide is my suggestion, this is spec text not code. -- 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] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
> From: Jason Wang > Sent: Tuesday, April 25, 2023 2:31 AM > >> 2, Typical virtualization environment. The workloads run in a guest, > >> and QEMU handles virtio-pci(or MMIO), and forwards requests to target. > >> +--+ +--+ +--+ > >> |Map-Reduce| | nginx | ... | processes| > >> +--+ +--+ +--+ > >> > >> Guest | | | Kernel +---+ > >> +---+ +---+ > >> | ext4 | | LKCF | | HWRNG | > >> +---+ +---+ +---+ > >> | | | > >> +---+ +---+ +---+ > >> | vdx | |vCrypto| | vRNG | > >> +---+ +---+ +---+ > >> | | | PCI > >> > >> | > >> QEMU +--+ > >> |virtio backend| > >> +--+ > >> | > >> +--+ > >> |NIC/IB| > >> +--+ > >> | +-+ > >> +->|virtio target| > >> +-+ > >> > > Use case 1 and 2 can be achieved directly without involving any > > mediation layer or any other translation layer (for example virtio to > > nfs). > > > Not for at least use case 2? It said it has a virtio backend in Qemu. Or the > only > possible way is to have virtio of in the guest. > Front end and back end both are virtio. So There is some layer of mediation/translation from PCI to fabric commands. But not as radical as virtio blk to nfs or virtio blk to nvme.
[virtio-dev] RE: [PATCH v12 03/10] admin: introduce group administration commands
> From: Michael S. Tsirkin > Sent: Tuesday, April 25, 2023 2:20 AM > > Below paragraph is no longer applies as we already discussed more use > > cases of the AQ such as accessing the PCI VF's legacy config space > > registers. > > Please drop below paragraph. > > Look - at one point you want commit log to record design process, an another > you turn around and ask me to drop it. > I feel like keeping this around frankly. Maybe I will add a sentence or two > along > the lines of "Note that nothing limits us from extending this down the road > though - but let's focus on what we already know for now". > I am requesting to drop the commit point that limits the usage of the AQ. Do you agree that AQ can be extended for use beyond VF and SIOV group management commands? Or one should create a new VQ type? If it is the later, I don't see the need of multiple AQ. > This extreme focus on commit log is poinless anyway - it makes much more > sense for code since commit log describes the change in english. Here the > whole change is english anyway. > > > > Without this the admin vq would become a "whatever" vq which does > > > not do anything specific at all, just a general transport like thing. > > > I feel going this way opens the design space to the point where we > > > no longer know what belongs in e.g. config space what in the control > > > q and what in the admin q. > > > As it is, whatever deals with groups is in the admin q; other things > > > not in the admin q. > > > > > > > A PF can use same or different AQ with a new struct > > virtio_legacy_register_access with a different opcode range. > > We'll get to that bridge and we'll cross it, the proposal is not on list even > yet. It is at [1]. [1] https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html I cannot draft the legacy command using AQ because AQ patch-5 in v12 fails to merge. Before I draft it, I sent [1] on the design way forward. > I > actually think that yes, you need it in a separate function. If PF is passed > through to guest then PF can not also safely DMA into host memory. That is fine. Only some user tests would pass the PF and VF to the VMs. One can create N combinations to make it not work. In all practical purposes, PF is owned by the hypervisor, trusted sw as listed in the PCI spec. > Alternatively, we could use an MMIO based mechanism for admin commands. > And maybe that would be a good fit. We discussed that MMIO is not a good fit for the VFs at scale as listed in [1]. [1] https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html - 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: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference
On Tue, Apr 25, 2023 at 03:02:15PM +0200, Halil Pasic wrote: > On Tue, 25 Apr 2023 04:10:00 + > Parav Pandit wrote: > > > > From: Halil Pasic > > > Sent: Monday, April 24, 2023 9:29 AM > [..] > > > > > > In [1] you state the following: "There is nothing like a zero based > > > index. A VQ > > > can be any u16 _number_ in range of 0 to 65534. > > > Number can also start from zero. So zero based index = zero based number." > > > > > > I don't really understand what do you mean by this, > > Like you described the vq index range is device specific and it is already > > documented in each device type. > > > > Yes, but we don't actually spell it out. In virtio 1.1 "index" or > "virtqueue number" can be found in chapter 5 "Device Types". > > Yes each device has a section "5.X.2 Virtqueues", but we don't spell it > out that the numbers 0, 1, 2 are actually virtqueue indexes. > > > A driver can choose to enable its "first VQ" located at vq index 2 for a > > specific device type. > > So, for device the first VQ is vq index 2 instead of 0. > > You read "first queue" as the queue that was "enabled" first. I tend to > agree, the queues can be "enabled" in any particular order. But I don't > agree that the queue that was "first enabled" is the what the spec > currently refers to as "first queue". > > I don't really agree with your statement. For example, IMHO, it does not > work if the device has just one queue (like block, entropy) or just two > queues. > > I'm convinced, for the device the first vq is always the queue with > index 0, if there is such a thing as "first vq for the device". I believe > the specification does not need "first vq for the device" and similar, > because what we actually need is "the queue with virtqueue index N". > > > > > PCI spec wrote "first queue is 0", this was little misleading how driver > > and device see it. > > > > So vq index starts from 0 but it need not be the first VQ, that's what I > > meant by above line and range description. > > > > > IMHO we should make sure the spec can not be read in this way. Because > if one was to choose the vq index 5 from the allowed range of [0..65535]. > And try to "enable" the vq with index 5 as the first and only queue of > the virtio-blk device, and use it as such: well that would not work out > well. > > > > but I'm afraid it is not > > > consistent with my understanding. Can not be any number in range of 0 to > > > 65534. > > > > > > Rather in general it depends on the device type, and on the device how > > > many > > > queues it support. Let us call this number dev_vq_max. And if dev_vq_max > > > > N > > > > 0 index is associated with a queue the N-1 index is also associated > > > > with a > > > queue. > > > > > > Let me also note that the number of queues supported/provided by the > > > device > > > is a matter of the device. For many devices the number of queues is a > > > constant > > > given in the specification. A Network Device tells its driver how may > > > queues it > > > supports via max_virtqueue_pairs, Crypto Device via max_dataqueues, and > > > Console Device via max_dataqueues. > > > The transports PCI also has num_queues which is documented as "The device > > > specifies the maximum number of virtqueues supported here." while CCW and > > > MMIO have no transport specific means to tell the driver about the number > > > of > > > queues supported by the device. In any case, if the number of queues > > > provided > > > by the device is N, each of those queues is uniquely identified by > > > exactly one > > > index from the range [0..N-1]. Furthermore the role a certain queue plays > > > is > > > determined by its index. E.g. for the Console Device virtqueue identified > > > by the > > > index 3 is the "control receiveq". > > > > > Sure. All you wrote is correct. > > > > I'm happy we agree. All I say we may want to rewrite the > > "Each virtqueue is identified by a virtqueue index; virtqueue index > range is from 0 to 65535 inclusive." > as > "Each virtqueue is uniquely identified by a virtqueue index. The number > of supported virtqueues is device dependent, but can never exceed 65536. > Thus 16 bit is sufficient to represent virtqueue indexes. If the number > of virtqueues currently supported by some device is N, each of it is > virtqueues is uniquely identified by a single index from the range > [0..N-1]." Seems to be repeating same thing over and over. This redundancy has cost, e.g. more places to change when we talk about admin queues. Yes it can not be any number 0 to 65535 but this kind of nitpicking belongs in conformance statements not in general description. > And it may be ester than doing a separate issue+ballot for it. > > Regards, > Halil > > > > > - > > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org > > For additional commands, e-mail:
Re: [virtio-dev] RE: [virtio-comment] [PATCH v14 04/11] transport-pci: Avoid first vq index reference
On Tue, 25 Apr 2023 04:10:00 + Parav Pandit wrote: > > From: Halil Pasic > > Sent: Monday, April 24, 2023 9:29 AM [..] > > > > In [1] you state the following: "There is nothing like a zero based index. > > A VQ > > can be any u16 _number_ in range of 0 to 65534. > > Number can also start from zero. So zero based index = zero based number." > > > > I don't really understand what do you mean by this, > Like you described the vq index range is device specific and it is already > documented in each device type. > Yes, but we don't actually spell it out. In virtio 1.1 "index" or "virtqueue number" can be found in chapter 5 "Device Types". Yes each device has a section "5.X.2 Virtqueues", but we don't spell it out that the numbers 0, 1, 2 are actually virtqueue indexes. > A driver can choose to enable its "first VQ" located at vq index 2 for a > specific device type. > So, for device the first VQ is vq index 2 instead of 0. You read "first queue" as the queue that was "enabled" first. I tend to agree, the queues can be "enabled" in any particular order. But I don't agree that the queue that was "first enabled" is the what the spec currently refers to as "first queue". I don't really agree with your statement. For example, IMHO, it does not work if the device has just one queue (like block, entropy) or just two queues. I'm convinced, for the device the first vq is always the queue with index 0, if there is such a thing as "first vq for the device". I believe the specification does not need "first vq for the device" and similar, because what we actually need is "the queue with virtqueue index N". > > PCI spec wrote "first queue is 0", this was little misleading how driver and > device see it. > > So vq index starts from 0 but it need not be the first VQ, that's what I > meant by above line and range description. > IMHO we should make sure the spec can not be read in this way. Because if one was to choose the vq index 5 from the allowed range of [0..65535]. And try to "enable" the vq with index 5 as the first and only queue of the virtio-blk device, and use it as such: well that would not work out well. > > but I'm afraid it is not > > consistent with my understanding. Can not be any number in range of 0 to > > 65534. > > > > Rather in general it depends on the device type, and on the device how many > > queues it support. Let us call this number dev_vq_max. And if dev_vq_max > > > N > > > 0 index is associated with a queue the N-1 index is also associated with > > > a > > queue. > > > > Let me also note that the number of queues supported/provided by the device > > is a matter of the device. For many devices the number of queues is a > > constant > > given in the specification. A Network Device tells its driver how may > > queues it > > supports via max_virtqueue_pairs, Crypto Device via max_dataqueues, and > > Console Device via max_dataqueues. > > The transports PCI also has num_queues which is documented as "The device > > specifies the maximum number of virtqueues supported here." while CCW and > > MMIO have no transport specific means to tell the driver about the number of > > queues supported by the device. In any case, if the number of queues > > provided > > by the device is N, each of those queues is uniquely identified by exactly > > one > > index from the range [0..N-1]. Furthermore the role a certain queue plays is > > determined by its index. E.g. for the Console Device virtqueue identified > > by the > > index 3 is the "control receiveq". > > > Sure. All you wrote is correct. > I'm happy we agree. All I say we may want to rewrite the "Each virtqueue is identified by a virtqueue index; virtqueue index range is from 0 to 65535 inclusive." as "Each virtqueue is uniquely identified by a virtqueue index. The number of supported virtqueues is device dependent, but can never exceed 65536. Thus 16 bit is sufficient to represent virtqueue indexes. If the number of virtqueues currently supported by some device is N, each of it is virtqueues is uniquely identified by a single index from the range [0..N-1]." And it may be ester than doing a separate issue+ballot for it. Regards, Halil > > - > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org > - 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] can: virtio-can: cleanups
On Tue, Apr 25, 2023 at 11:17:20AM +0200, Marc Kleine-Budde wrote: > On 24.04.2023 17:09:23, Michael S. Tsirkin wrote: > > On Mon, Apr 24, 2023 at 09:47:58PM +0200, Marc Kleine-Budde wrote: > > > Address the topics raised in > > > > > > https://lore.kernel.org/20230424-footwear-daily-9339bd0ec428-...@pengutronix.de > > > > > > Signed-off-by: Marc Kleine-Budde > > > > given base patch is rfc this should be too? > > This is an incremental patch that fixes the topics I raised in the > review of "[RFC PATCH v2] can: virtio: Initial virtio CAN driver.", see > linked discussion thread. > > regards, > Marc and that's fine, just pls put RFC in the subject. -- 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] [PATCH v14 06/11] transport-mmio: Avoid referring to zero based index
On Wed, 19 Apr 2023 04:46:34 +0300
Parav Pandit wrote:
> VQ range is already described in the first patch in basic virtqueue
> section. Hence remove the duplicate reference to it.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/163
> Signed-off-by: Parav Pandit
Acked-by: Halil Pasic
Same comments apply as for patch #4.
>
> ---
> changelog:
> v12->v13:
> - corrected number to index
> v11->v12:
> - remove changes related to 'vq number'
> v8->v9:
> - added 'by' at two places
> - replaced 'queue number' with 'vq number'
>
> v6->v7:
> - remove text around first vq as it is already covered in the basic
> virtqueues facility section
> ---
> transport-mmio.tex | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/transport-mmio.tex b/transport-mmio.tex
> index 164e640..2d24b4c 100644
> --- a/transport-mmio.tex
> +++ b/transport-mmio.tex
> @@ -113,8 +113,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio
> Transport Options / Vi
> following operations on \field{QueueSizeMax},
> \field{QueueSize}, \field{QueueReady},
> \field{QueueDescLow}, \field{QueueDescHigh}, \field{QueueDriverlLow},
> \field{QueueDriverHigh},
> -\field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset}
> apply to. The index
> -number of the first queue is zero (0x0).
> +\field{QueueDeviceLow}, \field{QueueDeviceHigh} and \field{QueueReset}
> apply to.
>}
>\hline
>\mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
> @@ -363,8 +362,7 @@ \subsubsection{Virtqueue Configuration}\label{sec:Virtio
> Transport Options / Vir
> The driver will typically initialize the virtual queue in the following way:
>
> \begin{enumerate}
> -\item Select the queue writing its index (first queue is 0) to
> - \field{QueueSel}.
> +\item Select the queue by writing its index to \field{QueueSel}.
>
> \item Check if the queue is not already in use: read \field{QueueReady},
> and expect a returned value of zero (0x0).
> @@ -474,9 +472,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport
> Options / Virtio Over M
> Writing to this register selects the virtual queue that the
> following operations on the \field{QueueSizeMax},
> \field{QueueSize}, \field{QueueAlign}
> -and \field{QueuePFN} registers apply to. The index
> -number of the first queue is zero (0x0).
> -.
> +and \field{QueuePFN} registers apply to.
>}
>\hline
>\mmioreg{QueueSizeMax}{Maximum virtual queue size}{0x034}{R}{%
> @@ -550,8 +546,7 @@ \subsection{Legacy interface}\label{sec:Virtio Transport
> Options / Virtio Over M
>
> The virtual queue is configured as follows:
> \begin{enumerate}
> -\item Select the queue writing its index (first queue is 0) to
> - \field{QueueSel}.
> +\item Select the queue by writing its index to \field{QueueSel}.
>
> \item Check if the queue is not already in use: read \field{QueuePFN},
> expecting a returned value of zero (0x0).
-
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] can: virtio-can: cleanups
On 24.04.2023 17:09:23, Michael S. Tsirkin wrote: > On Mon, Apr 24, 2023 at 09:47:58PM +0200, Marc Kleine-Budde wrote: > > Address the topics raised in > > > > https://lore.kernel.org/20230424-footwear-daily-9339bd0ec428-...@pengutronix.de > > > > Signed-off-by: Marc Kleine-Budde > > given base patch is rfc this should be too? This is an incremental patch that fixes the topics I raised in the review of "[RFC PATCH v2] can: virtio: Initial virtio CAN driver.", see linked discussion thread. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | signature.asc Description: PGP signature
[virtio-dev] Re: Re: [virtio-comment] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
On Mon, Apr 24, 2023 at 9:38 PM zhenwei pi wrote: > > > > On 4/24/23 11:40, Jason Wang wrote: > > On Sun, Apr 23, 2023 at 7:31 PM zhenwei pi wrote: > >> > >> Hi, > >> > >> In the past years, virtio supports lots of device specifications by > >> PCI/MMIO/CCW. These devices work fine in the virtualization environment, > >> and we have a chance to support virtio device family for the > >> container/host scenario. > > > > PCI can work for containers for sure (or does it meet any issue like > > scalability?). It's better to describe what problems you met and why > > you choose this way to solve it. > > > > It's better to compare this with > > > > 1) hiding the fabrics details via DPU > > 2) vDPA > > > Hi, > > Sorry, I missed this part. "Network defined peripheral devices of virtio > family" is the main purpose of this proposal, This can be achieved by either DPU or vDPA. I think the advantages is, if we standardize this in the spec, it avoids vendor specific protocol. > this allows us to use many > types of remote resources which are provided by virtio target. > > From the point of my view, there are 3 cases: > 1, Host/container scenario. For example, host kernel connects a virtio > target block service, maps it as a vdx(virtio-blk) device(used by > Map-Reduce service which needs a fast/large size disk). The host kernel > also connects a virtio target crypto service, maps it as virtio crypto > device(used by nginx to accelarate HTTPS). And so on. > > +--++--+ +--+ > |Map-Reduce|| nginx | ... | processes| > +--++--+ +--+ > > Host | | | > Kernel +---+ +---+ +---+ > | ext4 | | LKCF | | HWRNG | > +---+ +---+ +---+ > | | | > +---+ +---+ +---+ > | vdx | |vCrypto| | vRNG | > +---+ +---+ +---+ > | | | > | ++ | > +-->|TCP/RDMA|<+ > ++ > | > +--+ > |NIC/IB| > +--+ > | +-+ > +->|virtio target| > +-+ > > 2, Typical virtualization environment. The workloads run in a guest, and > QEMU handles virtio-pci(or MMIO), and forwards requests to target. > +--++--+ +--+ > |Map-Reduce|| nginx | ... | processes| > +--++--+ +--+ > > Guest| | | > Kernel +---+ +---+ +---+ > | ext4 | | LKCF | | HWRNG | > +---+ +---+ +---+ > | | | > +---+ +---+ +---+ > | vdx | |vCrypto| | vRNG | > +---+ +---+ +---+ > | | | > PCI > | > QEMU +--+ > |virtio backend| > +--+ > | > +--+ > |NIC/IB| > +--+ > | +-+ > +->|virtio target| > +-+ > So it's the job of the Qemu to do the translation from virtqueue to packet here? > 3, SmartNIC/DPU/vDPA environment. It's possible to convert virtio-pci > request to virtio-of request by hardware, and forward request to virtio > target directly. > +--++--+ +--+ > |Map-Reduce|| nginx | ... | processes| > +--++--+ +--+ > > Host | | | > Kernel +---+ +---+ +---+ > | ext4 | | LKCF | | HWRNG | > +---+ +---+ +---+ > | | | > +---+ +---+ +---+
[virtio-dev] Re: [virtio-comment] [PROPOSAL] Virtio Over Fabrics(TCP/RDMA)
在 2023/4/25 13:03, Parav Pandit 写道: On 4/24/2023 9:38 AM, zhenwei pi wrote: From the point of my view, there are 3 cases: 1, Host/container scenario. For example, host kernel connects a virtio target block service, maps it as a vdx(virtio-blk) device(used by Map-Reduce service which needs a fast/large size disk). The host kernel also connects a virtio target crypto service, maps it as virtio crypto device(used by nginx to accelarate HTTPS). And so on. +--+ +--+ +--+ |Map-Reduce| | nginx | ... | processes| +--+ +--+ +--+ Host | | | Kernel +---+ +---+ +---+ | ext4 | | LKCF | | HWRNG | +---+ +---+ +---+ | | | +---+ +---+ +---+ | vdx | |vCrypto| | vRNG | +---+ +---+ +---+ | | | | ++ | +-->|TCP/RDMA|<+ ++ | +--+ |NIC/IB| +--+ | +-+ +->|virtio target| +-+ 2, Typical virtualization environment. The workloads run in a guest, and QEMU handles virtio-pci(or MMIO), and forwards requests to target. +--+ +--+ +--+ |Map-Reduce| | nginx | ... | processes| +--+ +--+ +--+ Guest | | | Kernel +---+ +---+ +---+ | ext4 | | LKCF | | HWRNG | +---+ +---+ +---+ | | | +---+ +---+ +---+ | vdx | |vCrypto| | vRNG | +---+ +---+ +---+ | | | PCI | QEMU +--+ |virtio backend| +--+ | +--+ |NIC/IB| +--+ | +-+ +->|virtio target| +-+ Example #3 enables to implement virtio backend over fabrics initiator in the user space, which is also a good use case. It can be also be done in non native virtio backend. More below. 3, SmartNIC/DPU/vDPA environment. It's possible to convert virtio-pci request to virtio-of request by hardware, and forward request to virtio target directly. +--+ +--+ +--+ |Map-Reduce| | nginx | ... | processes| +--+ +--+ +--+ Host | | | Kernel +---+ +---+ +---+ | ext4 | | LKCF | | HWRNG | +---+ +---+ +---+ | | | +---+ +---+ +---+ | vdx | |vCrypto| | vRNG | +---+ +---+ +---+ | | | PCI | SmartNIC +---+ |virtio HW queue| +---+ | +--+ |NIC/IB| +--+ | +-+ +->|virtio target| +-+ All 3 seems a valid use cases. Use case 1 and 2 can be achieved directly without involving any mediation layer or any other translation layer (for example virtio to nfs). Not for at least use case 2? It said it has a virtio backend in Qemu. Or the only possible way is to have virtio of in the guest. Thanks Many blk and file protocols outside of the virtio already exists which achieve this. I don't see virtio being any different to support this in native manner, mainly the blk, fs, crypto device. use
[virtio-dev] Re: [PATCH v12 03/10] admin: introduce group administration commands
On Mon, Apr 24, 2023 at 06:07:12PM -0400, Parav Pandit wrote:
>
>
> On 4/24/2023 12:44 PM, Michael S. Tsirkin wrote:
> > This introduces a general structure for group administration commands,
> > used to control device groups through their owner.
> >
> > Following patches will introduce specific commands and an interface for
> > submitting these commands to the owner.
> >
> > Note that the commands are focused on controlling device groups:
> > this is why group related fields are in the generic part of
> > the structure.
>
> Below paragraph is no longer applies as we already discussed more use cases
> of the AQ such as accessing the PCI VF's legacy config space registers.
> Please drop below paragraph.
Look - at one point you want commit log to record design process,
an another you turn around and ask me to drop it.
I feel like keeping this around frankly. Maybe I will add
a sentence or two along the lines of "Note that nothing limits us from
extending this down the road though - but let's focus
on what we already know for now".
This extreme focus on commit log is poinless anyway - it makes
much more sense for code since commit log describes the
change in english. Here the whole change is english anyway.
> > Without this the admin vq would become a "whatever" vq which does not do
> > anything specific at all, just a general transport like thing.
> > I feel going this way opens the design space to the point where
> > we no longer know what belongs in e.g. config space
> > what in the control q and what in the admin q.
> > As it is, whatever deals with groups is in the admin q; other
> > things not in the admin q.
> >
>
> A PF can use same or different AQ with a new struct
> virtio_legacy_register_access with a different opcode range.
We'll get to that bridge and we'll cross it, the proposal is not
on list even yet. I actually think that yes, you need
it in a separate function. If PF is passed through to guest
then PF can not also safely DMA into host memory.
Alternatively, we could use an MMIO based mechanism for
admin commands. And maybe that would be a good fit.
> AQ doesn't replace the control vq, so that part description is fine.
>
> > There are specific exceptions such as query but that's an exception that
> > proves the rule ;)
> >
> > Signed-off-by: Michael S. Tsirkin
> > Reviewed-by: Stefan Hajnoczi
> > Reviewed-by: Zhu Lingshan
> > ---
> >
> > changes since v11:
> > acks by Stefan and Lingshan
> >
> > changes since v10:
> > explain the role of status vs status_qualifier, addresses
> > multiple comments
> > add more status values and appropriate qualifiers,
> > as suggested by Parav
> > clarify reserved command opcodes as suggested by Stefan
> > better formatting for ranges, comment by Jiri
> > make sure command-specific-data is a multiple of qword,
> > so things are aligned, suggested by Jiri
> > add Q_OK qualifier for success
> > ---
> > admin.tex| 121 +++
> > introduction.tex | 3 ++
> > 2 files changed, 124 insertions(+)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 05d506a..d6042e4 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -60,4 +60,125 @@ \section{Device groups}\label{sec:Basic Facilities of a
> > Virtio Device / Device g
> > PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI
> > Bus}).
> > \end{description}
> > +\subsection{Group administration commands}\label{sec:Basic Facilities of a
> > Virtio Device / Device groups / Group administration commands}
> > +The driver sends group administration commands to the owner device of
> > +a group to control member devices of the group.
> > +This mechanism can
> > +be used, for example, to configure a member device before it is
> > +initialized by its driver.
> > +\footnote{The term "administration" is intended to be interpreted
> > +widely to include any kind of control. See specific commands
> > +for detail.}
> > +
> > +All the group administration commands are of the following form:
> > +
> > +\begin{lstlisting}
> > +struct virtio_admin_cmd {
> > +/* Device-readable part */
> > +le16 opcode;
> > +/*
> > + * 1 - SR-IOV
> > + * 2-65535 - reserved
> > + */
> > +le16 group_type;
> > +/* unused, reserved for future extensions */
> > +u8 reserved1[12];
> > +le64 group_member_id;
> > +le64 command_specific_data[];
> > +
> > +/* Device-writable part */
> > +le16 status;
> > +le16 status_qualifier;
> > +/* unused, reserved for future extensions */
> > +u8 reserved2[4];
> > +u8 command_specific_result[];
> > +};
> > +\end{lstlisting}
> > +
> > +For all commands, \field{opcode}, \field{group_type} and if
> > +necessary \field{group_member_id} and \field{command_specific_data} are
> > +set by the driver, and the owner device sets
