Re: [PATCH net-next v5 3/6] vhost/vsock: support MSG_EOR bit processing
On Fri, Sep 03, 2021 at 03:32:35PM +0300, Arseny Krasnov wrote:
'MSG_EOR' handling has similar logic as 'MSG_EOM' - if bit present
in packet's header, reset it to 0. Then restore it back if packet
processing wasn't completed. Instead of bool variable for each
flag, bit mask variable was added: it has logical OR of 'MSG_EOR'
and 'MSG_EOM' if needed, to restore flags, this variable is ORed
with flags field of packet.
Signed-off-by: Arseny Krasnov
---
drivers/vhost/vsock.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index feaf650affbe..938aefbc75ec 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -114,7 +114,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
size_t nbytes;
size_t iov_len, payload_len;
int head;
- bool restore_flag = false;
+ u32 flags_to_restore = 0;
spin_lock_bh(>send_pkt_list_lock);
if (list_empty(>send_pkt_list)) {
@@ -179,15 +179,20 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
* created dynamically and are initialized with header
* of current packet(except length). But in case of
* SOCK_SEQPACKET, we also must clear message delimeter
-* bit(VIRTIO_VSOCK_SEQ_EOM). Otherwise, instead of one
-* packet with delimeter(which marks end of message),
-* there will be sequence of packets with delimeter
-* bit set. After initialized header will be copied to
-* rx buffer, this bit will be restored.
+* bit (VIRTIO_VSOCK_SEQ_EOM) and MSG_EOR bit
+* (VIRTIO_VSOCK_SEQ_EOR) if set. Otherwise,
+* there will be sequence of packets with these
+* bits set. After initialized header will be copied to
+* rx buffer, these required bits will be restored.
*/
if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOM)
{
pkt->hdr.flags &=
~cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
- restore_flag = true;
+ flags_to_restore |= VIRTIO_VSOCK_SEQ_EOM;
+
+ if (le32_to_cpu(pkt->hdr.flags) &
VIRTIO_VSOCK_SEQ_EOR) {
+ pkt->hdr.flags &=
~cpu_to_le32(VIRTIO_VSOCK_SEQ_EOR);
+ flags_to_restore |=
VIRTIO_VSOCK_SEQ_EOR;
+ }
}
}
@@ -224,8 +229,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
* to send it with the next available buffer.
*/
if (pkt->off < pkt->len) {
- if (restore_flag)
- pkt->hdr.flags |=
cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
+ pkt->hdr.flags |= cpu_to_le32(flags_to_restore);
/* We are queueing the same virtio_vsock_pkt to
handle
* the remaining bytes, and we want to deliver it
--
2.25.1
Reviewed-by: Stefano Garzarella
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 3/6] vhost/vsock: support MSG_EOR bit processing
On Fri, Sep 03, 2021 at 08:55:39AM +0200, Stefano Garzarella wrote:
On Fri, Sep 03, 2021 at 09:15:38AM +0300, Arseny Krasnov wrote:
'MSG_EOR' handling has similar logic as 'MSG_EOM' - if bit present
in packet's header, reset it to 0. Then restore it back if packet
processing wasn't completed. Instead of bool variable for each
flag, bit mask variable was added: it has logical OR of 'MSG_EOR'
and 'MSG_EOM' if needed, to restore flags, this variable is ORed
with flags field of packet.
Signed-off-by: Arseny Krasnov
---
drivers/vhost/vsock.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index feaf650affbe..93e8d635e18f 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -114,7 +114,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
size_t nbytes;
size_t iov_len, payload_len;
int head;
- bool restore_flag = false;
+ u32 flags_to_restore = 0;
spin_lock_bh(>send_pkt_list_lock);
if (list_empty(>send_pkt_list)) {
@@ -179,15 +179,20 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
* created dynamically and are initialized with header
* of current packet(except length). But in case of
* SOCK_SEQPACKET, we also must clear message delimeter
-* bit(VIRTIO_VSOCK_SEQ_EOM). Otherwise, instead of one
-* packet with delimeter(which marks end of message),
-* there will be sequence of packets with delimeter
-* bit set. After initialized header will be copied to
-* rx buffer, this bit will be restored.
+* bit (VIRTIO_VSOCK_SEQ_EOM) and MSG_EOR bit
+* (VIRTIO_VSOCK_SEQ_EOR) if set. Otherwise,
+* there will be sequence of packets with these
+* bits set. After initialized header will be copied to
+* rx buffer, these required bits will be restored.
*/
if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOM)
{
pkt->hdr.flags &=
~cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
- restore_flag = true;
+ flags_to_restore |= VIRTIO_VSOCK_SEQ_EOM;
+
+ if (le32_to_cpu(pkt->hdr.flags &
VIRTIO_VSOCK_SEQ_EOR)) {
^
Ooops, le32_to_cpu() should close before bitwise and operator.
I missed this, but kernel test robot discovered :-)
+ pkt->hdr.flags &=
~cpu_to_le32(VIRTIO_VSOCK_SEQ_EOR);
+ flags_to_restore |=
VIRTIO_VSOCK_SEQ_EOR;
+ }
}
}
@@ -224,8 +229,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
* to send it with the next available buffer.
*/
if (pkt->off < pkt->len) {
- if (restore_flag)
- pkt->hdr.flags |=
cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
+ pkt->hdr.flags |= cpu_to_le32(flags_to_restore);
/* We are queueing the same virtio_vsock_pkt to handle
* the remaining bytes, and we want to deliver it
-- 2.25.1
Reviewed-by: Stefano Garzarella
NACK
Please resend fixing the issue.
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-comment] Re: [PATCH v5] virtio-vsock: add description for datagram type
On Fri, Sep 03, 2021 at 03:57:24AM -0400, Michael S. Tsirkin wrote:
On Fri, Sep 03, 2021 at 09:22:24AM +0200, Stefano Garzarella wrote:
On Thu, Sep 02, 2021 at 05:08:01PM -0700, Jiang Wang . wrote:
> On Thu, Sep 2, 2021 at 7:07 AM Stefan Hajnoczi wrote:
> > On Thu, Jun 10, 2021 at 06:12:03PM +, Jiang Wang wrote:
[...]
> > >
> > > -There are currently no feature bits defined for this device.
> > > +\begin{description}
> > > +\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket
type.
> > > +\end{description}
> > > +
> > > +\begin{description}
> > > +\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
type.
> >
> > Is this really bit 2 or did you mean bit 1 (value 0x2)?
> >
> I left bit 1 for SEQPACKET feature bit. That will probably merge
> before this patch.
Yep, SEQPACKET implementation is also merged in the linux kernel using the
feature bit 1 (0x2), bit 0 (0x1) was left free for stream.
>
> > What happens to the virtqueue layout when VIRTIO_VSOCK_F_DGRAM is
> > present and VIRTIO_VSOCK_F_STREAM is absent? The virtqueue section above
> > implies that VIRTIO_VSOCK_F_STREAM is always present.
> >
> yeah, good question. I think then it means the first two queues will be used
> for dgram?
>
> > > +\end{description}
> > > +
> > > +If no feature bits are defined, assume device only supports stream
socket type.
> >
> > It's cleaner to define VIRTIO_VSOCK_F_NO_STREAM (0) instead. When the
> > bit is set the stream socket type is not available and the stream_rx/tx
> > virtqueues are absent.
> >
> > This way it's not necessary to define special behavior depending on
> > certain combinations of feature bits.
> >
> Agree. I went back and read old emails again and found I missed the
> negative bit part. So repeating the previous question, if
> VIRTIO_VSOCK_F_NO_STREAM and VIRTIO_VSOCK_F_DGRAM
> present, then we will only have 3 queues and the first two are for dgram,
right?
>
> Also, I am wondering what if an implementation only sets
> VIRTIO_VSOCK_F_NO_STREAM bit, but somehow forgot (or for whatever
> reason) to set VIRTIO_VSOCK_F_DGRAM bit? Does that mean there will
> be no virtqueues? The implementation is a mistake? Because it will not
> do anything.
> Do we need to explicitly add a sentence in the spec to say something like
> "Don't set VIRTIO_VSOCK_F_NO_STREAM alone" etc?
Good point.
IIRC we thought to add F_STREAM to allow devices for example that support
only DGRAM, but we said to consider stream supported if no feature was set
for backward compatibility.
With F_NO_STREAM we can do the same, but actually we could have this strange
case. I don't think it's a big problem, in the end it's a configuration
error. Maybe F_NO_STREMA is clearer since the original device spec supports
streams without any feature bit defined.
Stefano
How about that instead of VIRTIO_VSOCK_F_NO_STREAM we do
VIRTIO_VSOCK_F_TYPE /* device supports multiple socket types */
then with VIRTIO_VSOCK_F_TYPE clear we only have stream.
For SEQPACKET it should be okay, since it depends on stream queues, but
DGRAM will have its own queues, so with F_TYPE it seems to me more
difficult to handle the case in which a device supports DGRAM but not
STREAM.
We should also make SEQPACKET depend on this VIRTIO_VSOCK_F_TYPE -
linux guests do not validate that right now but
it's probably not too late to add such a patch to linux
as a bugfix.
Yep, also with F_NO_STREAM we should do a validation, since F_SEQPACKET
depends on !F_NO_STREAM.
I'll take care of this when we decide what flag to use.
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] virtio-blk: remove unneeded "likely" statements
On Thu, Sep 02, 2021 at 10:13:04PM +0300, Max Gurtovoy wrote: > > On 9/2/2021 6:00 PM, Stefan Hajnoczi wrote: > > On Mon, Aug 30, 2021 at 03:01:11PM +0300, Max Gurtovoy wrote: > > > Usually we use "likely/unlikely" to optimize the fast path. Remove > > > redundant "likely" statements in the control path to ease on the code. > > > > > > Signed-off-by: Max Gurtovoy > > > --- > > > drivers/block/virtio_blk.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > It would be nice to tweak the commit description before merging this. I > > had trouble parsing the second sentence. If I understand correctly the > > purpose of this patch is to make the code simpler and easier to read: > > > >s/ease on the code/simplify the code and make it easier to read/ > > I'm ok with this change in commit message. > > MST, > > can you apply this change if you'll pick this commit ? > > -Max. Just repost with a fixed commit log pls, easier for me. Thanks! > > > > Reviewed-by: Stefan Hajnoczi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [External] Re: [virtio-comment] [PATCH v5] virtio-vsock: add description for datagram type
On Tue, Aug 31, 2021 at 06:24:34PM -0700, Jiang Wang . wrote:
> On Tue, Aug 31, 2021 at 6:13 PM Michael S. Tsirkin wrote:
> >
> > On Thu, Jun 10, 2021 at 06:12:03PM +, Jiang Wang wrote:
> > > Add supports for datagram type for virtio-vsock. Datagram
> > > sockets are connectionless and unreliable. To avoid contention
> > > with stream and other sockets, add two more virtqueues and
> > > a new feature bit to identify if those two new queues exist or not.
> > >
> > > Also add descriptions for resource management of datagram, which
> > > does not use the existing credit update mechanism associated with
> > > stream sockets.
> > >
> > > Signed-off-by: Jiang Wang
> >
> > Is this going anywhere? Linux with this included was just released but
> > if no one has the cycles to work on the spec then it's not too late to
> > disable the guest code in a stable@ patch.
> >
>
> Hi Michael,
>
> I am still working on this (fixing the migration issue), but would
> like to know if there are any more
> comments or not. I did not notice any commits related to vsock
> dgram merged to Linux kernel, could you show me the commit link?
>
> Thanks and regards,
>
> Jiang
My bad I was thinking of seqpacket. I see that description
was not posted as well, so all is well.
> > > ---
> > >
> > > V2: addressed the comments for the previous version.
> > > V3: add description for the mergeable receive buffer.
> > > V4: add a feature bit for stream and reserver a bit for seqpacket.
> > > Fix mrg_rxbuf related sentences.
> > > V5: removed mergeable rx buffer part. It will go to a
> > > separate patch. Fixed comments about tx, rx, feature bit etc.
> > >
> > > virtio-vsock.tex | 71
> > > +++-
> > > 1 file changed, 60 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > index da7e641..26a62ac 100644
> > > --- a/virtio-vsock.tex
> > > +++ b/virtio-vsock.tex
> > > @@ -9,14 +9,37 @@ \subsection{Device ID}\label{sec:Device Types / Socket
> > > Device / Device ID}
> > >
> > > \subsection{Virtqueues}\label{sec:Device Types / Socket Device /
> > > Virtqueues}
> > > \begin{description}
> > > -\item[0] rx
> > > -\item[1] tx
> > > +\item[0] stream rx
> > > +\item[1] stream tx
> > > +\item[2] datagram rx
> > > +\item[3] datagram tx
> > > +\item[4] event
> > > +\end{description}
> > > +The virtio socket device uses 5 queues if feature bit
> > > VIRTIO_VSOCK_F_DRGAM is set. Otherwise, it
> > > +only uses 3 queues, as the following.
> > > +
> > > +\begin{description}
> > > +\item[0] stream rx
> > > +\item[1] stream tx
> > > \item[2] event
> > > \end{description}
> > >
> > > +When behavior differs between stream and datagram rx/tx virtqueues
> > > +their full names are used. Common behavior is simply described in
> > > +terms of rx/tx virtqueues and applies to both stream and datagram
> > > +virtqueues.
> > > +
> > > \subsection{Feature bits}\label{sec:Device Types / Socket Device /
> > > Feature bits}
> > >
> > > -There are currently no feature bits defined for this device.
> > > +\begin{description}
> > > +\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket
> > > type.
> > > +\end{description}
> > > +
> > > +\begin{description}
> > > +\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
> > > type.
> > > +\end{description}
> > > +
> > > +If no feature bits are defined, assume device only supports stream
> > > socket type.
> > >
> > > \subsection{Device configuration layout}\label{sec:Device Types / Socket
> > > Device / Device configuration layout}
> > >
> > > @@ -107,6 +130,9 @@ \subsection{Device Operation}\label{sec:Device Types
> > > / Socket Device / Device Op
> > >
> > > \subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket
> > > Device / Device Operation / Virtqueue Flow Control}
> > >
> > > +Flow control applies to stream sockets; datagram sockets do not have
> > > +flow control.
> > > +
> > > The tx virtqueue carries packets initiated by applications and replies to
> > > received packets. The rx virtqueue carries packets initiated by the
> > > device and
> > > replies to previously transmitted packets.
> > > @@ -140,12 +166,15 @@ \subsubsection{Addressing}\label{sec:Device Types /
> > > Socket Device / Device Opera
> > > consists of a (cid, port number) tuple. The header fields used for this
> > > are
> > > \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
> > >
> > > -Currently only stream sockets are supported. \field{type} is 1 for stream
> > > -socket types.
> > > +Currently stream and datagram (dgram) sockets are supported.
> > > \field{type} is 1 for stream
> > > +socket types. \field{type} is 3 for dgram socket types.
> > >
> > > Stream sockets provide in-order, guaranteed, connection-oriented delivery
> > > without message boundaries.
> > >
> > > +Datagram sockets provide unordered, unreliable, connectionless messages
> > >
Re: [virtio-comment] Re: [PATCH v5] virtio-vsock: add description for datagram type
On Fri, Sep 03, 2021 at 09:22:24AM +0200, Stefano Garzarella wrote:
> On Thu, Sep 02, 2021 at 05:08:01PM -0700, Jiang Wang . wrote:
> > On Thu, Sep 2, 2021 at 7:07 AM Stefan Hajnoczi wrote:
> > >
> > > On Thu, Jun 10, 2021 at 06:12:03PM +, Jiang Wang wrote:
> > > > Add supports for datagram type for virtio-vsock. Datagram
> > > > sockets are connectionless and unreliable. To avoid contention
> > > > with stream and other sockets, add two more virtqueues and
> > > > a new feature bit to identify if those two new queues exist or not.
> > > >
> > > > Also add descriptions for resource management of datagram, which
> > > > does not use the existing credit update mechanism associated with
> > > > stream sockets.
> > > >
> > > > Signed-off-by: Jiang Wang
> > > > ---
> > >
> > > Overall this looks good. The tricky thing will be implementing dgram
> > > sockets in a way that minimizes dropped packets and provides some degree
> > > of fairness between senders. Those are implementation issues though and
> > > not visible at the device specification level.
> > >
> > > > diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > > index da7e641..26a62ac 100644
> > > > --- a/virtio-vsock.tex
> > > > +++ b/virtio-vsock.tex
> > > > @@ -9,14 +9,37 @@ \subsection{Device ID}\label{sec:Device Types /
> > > > Socket Device / Device ID}
> > > >
> > > > \subsection{Virtqueues}\label{sec:Device Types / Socket Device /
> > > > Virtqueues}
> > > > \begin{description}
> > > > -\item[0] rx
> > > > -\item[1] tx
> > > > +\item[0] stream rx
> > > > +\item[1] stream tx
> > > > +\item[2] datagram rx
> > > > +\item[3] datagram tx
> > > > +\item[4] event
> > > > +\end{description}
> > > > +The virtio socket device uses 5 queues if feature bit
> > > > VIRTIO_VSOCK_F_DRGAM is set. Otherwise, it
> > > > +only uses 3 queues, as the following.
> > >
> > > s/as the following/as follows:/
> > >
> > Will do.
> >
> > > > +
> > > > +\begin{description}
> > > > +\item[0] stream rx
> > > > +\item[1] stream tx
> > > > \item[2] event
> > > > \end{description}
> > > >
> > > > +When behavior differs between stream and datagram rx/tx virtqueues
> > > > +their full names are used. Common behavior is simply described in
> > > > +terms of rx/tx virtqueues and applies to both stream and datagram
> > > > +virtqueues.
> > > > +
> > > > \subsection{Feature bits}\label{sec:Device Types / Socket Device /
> > > > Feature bits}
> > > >
> > > > -There are currently no feature bits defined for this device.
> > > > +\begin{description}
> > > > +\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket
> > > > type.
> > > > +\end{description}
> > > > +
> > > > +\begin{description}
> > > > +\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket
> > > > type.
> > >
> > > Is this really bit 2 or did you mean bit 1 (value 0x2)?
> > >
> > I left bit 1 for SEQPACKET feature bit. That will probably merge
> > before this patch.
>
> Yep, SEQPACKET implementation is also merged in the linux kernel using the
> feature bit 1 (0x2), bit 0 (0x1) was left free for stream.
>
> >
> > > What happens to the virtqueue layout when VIRTIO_VSOCK_F_DGRAM is
> > > present and VIRTIO_VSOCK_F_STREAM is absent? The virtqueue section above
> > > implies that VIRTIO_VSOCK_F_STREAM is always present.
> > >
> > yeah, good question. I think then it means the first two queues will be
> > used
> > for dgram?
> >
> > > > +\end{description}
> > > > +
> > > > +If no feature bits are defined, assume device only supports stream
> > > > socket type.
> > >
> > > It's cleaner to define VIRTIO_VSOCK_F_NO_STREAM (0) instead. When the
> > > bit is set the stream socket type is not available and the stream_rx/tx
> > > virtqueues are absent.
> > >
> > > This way it's not necessary to define special behavior depending on
> > > certain combinations of feature bits.
> > >
> > Agree. I went back and read old emails again and found I missed the
> > negative bit part. So repeating the previous question, if
> > VIRTIO_VSOCK_F_NO_STREAM and VIRTIO_VSOCK_F_DGRAM
> > present, then we will only have 3 queues and the first two are for dgram,
> > right?
> >
> > Also, I am wondering what if an implementation only sets
> > VIRTIO_VSOCK_F_NO_STREAM bit, but somehow forgot (or for whatever
> > reason) to set VIRTIO_VSOCK_F_DGRAM bit? Does that mean there will
> > be no virtqueues? The implementation is a mistake? Because it will not
> > do anything.
> > Do we need to explicitly add a sentence in the spec to say something like
> > "Don't set VIRTIO_VSOCK_F_NO_STREAM alone" etc?
>
> Good point.
>
> IIRC we thought to add F_STREAM to allow devices for example that support
> only DGRAM, but we said to consider stream supported if no feature was set
> for backward compatibility.
> With F_NO_STREAM we can do the same, but actually we could have this strange
> case. I don't think it's a big problem, in the end it's a configuration
> error. Maybe F_NO_STREMA is clearer since
Re: [virtio-comment] Re: [PATCH v5] virtio-vsock: add description for datagram type
On Thu, Sep 02, 2021 at 05:08:01PM -0700, Jiang Wang . wrote:
On Thu, Sep 2, 2021 at 7:07 AM Stefan Hajnoczi wrote:
On Thu, Jun 10, 2021 at 06:12:03PM +, Jiang Wang wrote:
> Add supports for datagram type for virtio-vsock. Datagram
> sockets are connectionless and unreliable. To avoid contention
> with stream and other sockets, add two more virtqueues and
> a new feature bit to identify if those two new queues exist or not.
>
> Also add descriptions for resource management of datagram, which
> does not use the existing credit update mechanism associated with
> stream sockets.
>
> Signed-off-by: Jiang Wang
> ---
Overall this looks good. The tricky thing will be implementing dgram
sockets in a way that minimizes dropped packets and provides some degree
of fairness between senders. Those are implementation issues though and
not visible at the device specification level.
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index da7e641..26a62ac 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -9,14 +9,37 @@ \subsection{Device ID}\label{sec:Device Types / Socket
Device / Device ID}
>
> \subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
> \begin{description}
> -\item[0] rx
> -\item[1] tx
> +\item[0] stream rx
> +\item[1] stream tx
> +\item[2] datagram rx
> +\item[3] datagram tx
> +\item[4] event
> +\end{description}
> +The virtio socket device uses 5 queues if feature bit VIRTIO_VSOCK_F_DRGAM
is set. Otherwise, it
> +only uses 3 queues, as the following.
s/as the following/as follows:/
Will do.
> +
> +\begin{description}
> +\item[0] stream rx
> +\item[1] stream tx
> \item[2] event
> \end{description}
>
> +When behavior differs between stream and datagram rx/tx virtqueues
> +their full names are used. Common behavior is simply described in
> +terms of rx/tx virtqueues and applies to both stream and datagram
> +virtqueues.
> +
> \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature
bits}
>
> -There are currently no feature bits defined for this device.
> +\begin{description}
> +\item[VIRTIO_VSOCK_F_STREAM (0)] Device has support for stream socket type.
> +\end{description}
> +
> +\begin{description}
> +\item[VIRTIO_VSOCK_F_DGRAM (2)] Device has support for datagram socket type.
Is this really bit 2 or did you mean bit 1 (value 0x2)?
I left bit 1 for SEQPACKET feature bit. That will probably merge
before this patch.
Yep, SEQPACKET implementation is also merged in the linux kernel using
the feature bit 1 (0x2), bit 0 (0x1) was left free for stream.
What happens to the virtqueue layout when VIRTIO_VSOCK_F_DGRAM is
present and VIRTIO_VSOCK_F_STREAM is absent? The virtqueue section above
implies that VIRTIO_VSOCK_F_STREAM is always present.
yeah, good question. I think then it means the first two queues will be used
for dgram?
> +\end{description}
> +
> +If no feature bits are defined, assume device only supports stream socket
type.
It's cleaner to define VIRTIO_VSOCK_F_NO_STREAM (0) instead. When the
bit is set the stream socket type is not available and the stream_rx/tx
virtqueues are absent.
This way it's not necessary to define special behavior depending on
certain combinations of feature bits.
Agree. I went back and read old emails again and found I missed the
negative bit part. So repeating the previous question, if
VIRTIO_VSOCK_F_NO_STREAM and VIRTIO_VSOCK_F_DGRAM
present, then we will only have 3 queues and the first two are for dgram, right?
Also, I am wondering what if an implementation only sets
VIRTIO_VSOCK_F_NO_STREAM bit, but somehow forgot (or for whatever
reason) to set VIRTIO_VSOCK_F_DGRAM bit? Does that mean there will
be no virtqueues? The implementation is a mistake? Because it will not
do anything.
Do we need to explicitly add a sentence in the spec to say something like
"Don't set VIRTIO_VSOCK_F_NO_STREAM alone" etc?
Good point.
IIRC we thought to add F_STREAM to allow devices for example that
support only DGRAM, but we said to consider stream supported if no
feature was set for backward compatibility.
With F_NO_STREAM we can do the same, but actually we could have this
strange case. I don't think it's a big problem, in the end it's a
configuration error. Maybe F_NO_STREMA is clearer since the original
device spec supports streams without any feature bit defined.
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 3/3] virtio-mem: disallow mapping virtio-mem memory via /dev/mem
On Thu, Sep 02, 2021 at 06:09:19PM +0200, David Hildenbrand wrote:
> We don't want user space to be able to map virtio-mem device memory
> directly (e.g., via /dev/mem) in order to have guarantees that in a sane
> setup we'll never accidentially access unplugged memory within the
> device-managed region of a virtio-mem device, just as required by the
> virtio-spec.
>
> As soon as the virtio-mem driver is loaded, the device region is visible
> in /proc/iomem via the parent device region. From that point on user space
> is aware of the device region and we want to disallow mapping anything
> inside that region (where we will dynamically (un)plug memory) until
> the driver has been unloaded cleanly and e.g., another driver might take
> over.
>
> By creating our parent IORESOURCE_SYSTEM_RAM resource with
> IORESOURCE_EXCLUSIVE, we will disallow any /dev/mem access to our
> device region until the driver was unloaded cleanly and removed the
> parent region. This will work even though only some memory blocks are
> actually currently added to Linux and appear as busy in the resource tree.
>
> So access to the region from user space is only possible
> a) if we don't load the virtio-mem driver.
> b) after unloading the virtio-mem driver cleanly.
>
> Don't build virtio-mem if access to /dev/mem cannot be restricticted --
> if we have CONFIG_DEVMEM=y but CONFIG_STRICT_DEVMEM is not set.
>
> Reviewed-by: Dan Williams
> Signed-off-by: David Hildenbrand
> ---
> drivers/virtio/Kconfig | 1 +
> drivers/virtio/virtio_mem.c | 4 +++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index ce1b3f6ec325..ff80cd03f1d1 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -101,6 +101,7 @@ config VIRTIO_MEM
> depends on MEMORY_HOTPLUG_SPARSE
> depends on MEMORY_HOTREMOVE
> depends on CONTIG_ALLOC
> + depends on !DEVMEM || STRICT_DEVMEM
> help
>This driver provides access to virtio-mem paravirtualized memory
>devices, allowing to hotplug and hotunplug memory.
It would be nicer if there was a symbol in the MEMORY_ namespace
we can depend on exported by mm and depending on !DEVMEM ||
STRICT_DEVMEM.
E.g.
config MEMORY_EXCLUSIVE
def_bool y
depends on !DEVMEM || STRICT_DEVMEM
and then in virtio
depends on MEMORY_EXCLUSIVE
the virtio change itself is ok though:
Acked-by: Michael S. Tsirkin
for merging through -mm.
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index b91bc810a87e..c2d93492cf0f 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2523,8 +2523,10 @@ static int virtio_mem_create_resource(struct
> virtio_mem *vm)
> if (!name)
> return -ENOMEM;
>
> + /* Disallow mapping device memory via /dev/mem completely. */
> vm->parent_resource = __request_mem_region(vm->addr, vm->region_size,
> -name, IORESOURCE_SYSTEM_RAM);
> +name, IORESOURCE_SYSTEM_RAM |
> +IORESOURCE_EXCLUSIVE);
> if (!vm->parent_resource) {
> kfree(name);
> dev_warn(>vdev->dev, "could not reserve device region\n");
> --
> 2.31.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next v4 3/6] vhost/vsock: support MSG_EOR bit processing
On Fri, Sep 03, 2021 at 09:15:38AM +0300, Arseny Krasnov wrote:
'MSG_EOR' handling has similar logic as 'MSG_EOM' - if bit present
in packet's header, reset it to 0. Then restore it back if packet
processing wasn't completed. Instead of bool variable for each
flag, bit mask variable was added: it has logical OR of 'MSG_EOR'
and 'MSG_EOM' if needed, to restore flags, this variable is ORed
with flags field of packet.
Signed-off-by: Arseny Krasnov
---
drivers/vhost/vsock.c | 22 +-
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index feaf650affbe..93e8d635e18f 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -114,7 +114,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
size_t nbytes;
size_t iov_len, payload_len;
int head;
- bool restore_flag = false;
+ u32 flags_to_restore = 0;
spin_lock_bh(>send_pkt_list_lock);
if (list_empty(>send_pkt_list)) {
@@ -179,15 +179,20 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
* created dynamically and are initialized with header
* of current packet(except length). But in case of
* SOCK_SEQPACKET, we also must clear message delimeter
-* bit(VIRTIO_VSOCK_SEQ_EOM). Otherwise, instead of one
-* packet with delimeter(which marks end of message),
-* there will be sequence of packets with delimeter
-* bit set. After initialized header will be copied to
-* rx buffer, this bit will be restored.
+* bit (VIRTIO_VSOCK_SEQ_EOM) and MSG_EOR bit
+* (VIRTIO_VSOCK_SEQ_EOR) if set. Otherwise,
+* there will be sequence of packets with these
+* bits set. After initialized header will be copied to
+* rx buffer, these required bits will be restored.
*/
if (le32_to_cpu(pkt->hdr.flags) & VIRTIO_VSOCK_SEQ_EOM)
{
pkt->hdr.flags &=
~cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
- restore_flag = true;
+ flags_to_restore |= VIRTIO_VSOCK_SEQ_EOM;
+
+ if (le32_to_cpu(pkt->hdr.flags &
VIRTIO_VSOCK_SEQ_EOR)) {
+ pkt->hdr.flags &=
~cpu_to_le32(VIRTIO_VSOCK_SEQ_EOR);
+ flags_to_restore |=
VIRTIO_VSOCK_SEQ_EOR;
+ }
}
}
@@ -224,8 +229,7 @@ vhost_transport_do_send_pkt(struct vhost_vsock *vsock,
* to send it with the next available buffer.
*/
if (pkt->off < pkt->len) {
- if (restore_flag)
- pkt->hdr.flags |=
cpu_to_le32(VIRTIO_VSOCK_SEQ_EOM);
+ pkt->hdr.flags |= cpu_to_le32(flags_to_restore);
/* We are queueing the same virtio_vsock_pkt to handle
* the remaining bytes, and we want to deliver it
--
2.25.1
Reviewed-by: Stefano Garzarella
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
