linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address
@ 2020-12-01 15:25 Andra Paraschiv
  2020-12-01 15:25 ` [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure Andra Paraschiv
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Andra Paraschiv @ 2020-12-01 15:25 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski,
	Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov,
	Andra Paraschiv

vsock enables communication between virtual machines and the host they are
running on. Nested VMs can be setup to use vsock channels, as the multi
transport support has been available in the mainline since the v5.5 Linux kernel
has been released.

Implicitly, if no host->guest vsock transport is loaded, all the vsock packets
are forwarded to the host. This behavior can be used to setup communication
channels between sibling VMs that are running on the same host. One example can
be the vsock channels that can be established within AWS Nitro Enclaves
(see Documentation/virt/ne_overview.rst).

To be able to explicitly mark a connection as being used for a certain use case,
add a flag field in the vsock address data structure. The "svm_reserved1" field
has been repurposed to be the flag field. The value of the flag will then be
taken into consideration when the vsock transport is assigned.

This way can distinguish between nested VMs / local communication and sibling
VMs use cases. And can also setup one or more types of communication at the same
time.

Thank you.

Andra

---

Patch Series Changelog

The patch series is built on top of v5.10-rc6.

GitHub repo branch for the latest version of the patch series:

* https://github.com/andraprs/linux/tree/vsock-flag-sibling-comm-v1

---

Andra Paraschiv (3):
  vm_sockets: Include flag field in the vsock address data structure
  virtio_transport_common: Set sibling VMs flag on the receive path
  af_vsock: Assign the vsock transport considering the vsock address
    flag

 include/uapi/linux/vm_sockets.h         | 18 +++++++++++++++++-
 net/vmw_vsock/af_vsock.c                | 15 +++++++++++----
 net/vmw_vsock/virtio_transport_common.c |  8 ++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)

-- 
2.20.1 (Apple Git-117)




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-01 15:25 [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Andra Paraschiv
@ 2020-12-01 15:25 ` Andra Paraschiv
  2020-12-01 16:09   ` Stefano Garzarella
  2020-12-03  9:21   ` Stefan Hajnoczi
  2020-12-01 15:25 ` [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path Andra Paraschiv
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 21+ messages in thread
From: Andra Paraschiv @ 2020-12-01 15:25 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski,
	Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov,
	Andra Paraschiv

vsock enables communication between virtual machines and the host they
are running on. With the multi transport support (guest->host and
host->guest), nested VMs can also use vsock channels for communication.

In addition to this, by default, all the vsock packets are forwarded to
the host, if no host->guest transport is loaded. This behavior can be
implicitly used for enabling vsock communication between sibling VMs.

Add a flag field in the vsock address data structure that can be used to
explicitly mark the vsock connection as being targeted for a certain
type of communication. This way, can distinguish between nested VMs and
sibling VMs use cases and can also setup them at the same time. Till
now, could either have nested VMs or sibling VMs at a time using the
vsock communication stack.

Use the already available "svm_reserved1" field and mark it as a flag
field instead. This flag can be set when initializing the vsock address
variable used for the connect() call.

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
index fd0ed7221645d..58da5a91413ac 100644
--- a/include/uapi/linux/vm_sockets.h
+++ b/include/uapi/linux/vm_sockets.h
@@ -114,6 +114,22 @@
 
 #define VMADDR_CID_HOST 2
 
+/* This sockaddr_vm flag value covers the current default use case:
+ * local vsock communication between guest and host and nested VMs setup.
+ * In addition to this, implicitly, the vsock packets are forwarded to the host
+ * if no host->guest vsock transport is set.
+ */
+#define VMADDR_FLAG_DEFAULT_COMMUNICATION	0x0000
+
+/* Set this flag value in the sockaddr_vm corresponding field if the vsock
+ * channel needs to be setup between two sibling VMs running on the same host.
+ * This way can explicitly distinguish between vsock channels created for nested
+ * VMs (or local communication between guest and host) and the ones created for
+ * sibling VMs. And vsock channels for multiple use cases (nested / sibling VMs)
+ * can be setup at the same time.
+ */
+#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION	0x0001
+
 /* Invalid vSockets version. */
 
 #define VM_SOCKETS_INVALID_VERSION -1U
@@ -145,7 +161,7 @@
 
 struct sockaddr_vm {
 	__kernel_sa_family_t svm_family;
-	unsigned short svm_reserved1;
+	unsigned short svm_flag;
 	unsigned int svm_port;
 	unsigned int svm_cid;
 	unsigned char svm_zero[sizeof(struct sockaddr) -
-- 
2.20.1 (Apple Git-117)




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path
  2020-12-01 15:25 [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Andra Paraschiv
  2020-12-01 15:25 ` [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure Andra Paraschiv
@ 2020-12-01 15:25 ` Andra Paraschiv
  2020-12-01 16:22   ` Stefano Garzarella
  2020-12-01 15:25 ` [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag Andra Paraschiv
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Andra Paraschiv @ 2020-12-01 15:25 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski,
	Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov,
	Andra Paraschiv

The vsock flag can be set during the connect() setup logic, when
initializing the vsock address data structure variable. Then the vsock
transport is assigned, also considering this flag.

The vsock transport is also assigned on the (listen) receive path. The
flag needs to be set considering the use case.

Set the vsock flag of the remote address to the one targeted for sibling
VMs communication if the following conditions are met:

* The source CID of the packet is higher than VMADDR_CID_HOST.
* The destination CID of the packet is higher than VMADDR_CID_HOST.

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 5956939eebb78..871c84e0916b1 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -1062,6 +1062,14 @@ virtio_transport_recv_listen(struct sock *sk, struct virtio_vsock_pkt *pkt,
 	vsock_addr_init(&vchild->remote_addr, le64_to_cpu(pkt->hdr.src_cid),
 			le32_to_cpu(pkt->hdr.src_port));
 
+	/* If the packet is coming with the source and destination CIDs higher
+	 * than VMADDR_CID_HOST, then a vsock channel should be established for
+	 * sibling VMs communication.
+	 */
+	if (vchild->local_addr.svm_cid > VMADDR_CID_HOST &&
+	    vchild->remote_addr.svm_cid > VMADDR_CID_HOST)
+		vchild->remote_addr.svm_flag = VMADDR_FLAG_SIBLING_VMS_COMMUNICATION;
+
 	ret = vsock_assign_transport(vchild, vsk);
 	/* Transport assigned (looking at remote_addr) must be the same
 	 * where we received the request.
-- 
2.20.1 (Apple Git-117)




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag
  2020-12-01 15:25 [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Andra Paraschiv
  2020-12-01 15:25 ` [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure Andra Paraschiv
  2020-12-01 15:25 ` [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path Andra Paraschiv
@ 2020-12-01 15:25 ` Andra Paraschiv
  2020-12-01 16:23   ` Stefano Garzarella
  2020-12-01 16:27 ` [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Stefano Garzarella
  2020-12-02 13:37 ` Stefano Garzarella
  4 siblings, 1 reply; 21+ messages in thread
From: Andra Paraschiv @ 2020-12-01 15:25 UTC (permalink / raw)
  To: netdev
  Cc: linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski,
	Stefano Garzarella, Stefan Hajnoczi, Vitaly Kuznetsov,
	Andra Paraschiv

The vsock flag has been set in the connect and (listen) receive paths.

When the vsock transport is assigned, the remote CID is used to
distinguish between types of connection.

Use the vsock flag (in addition to the CID) from the remote address to
decide which vsock transport to assign. For the sibling VMs use case,
all the vsock packets need to be forwarded to the host, so always assign
the guest->host transport if the vsock flag is set. For the other use
cases, the vsock transport assignment logic is not changed.

Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
---
 net/vmw_vsock/af_vsock.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index d10916ab45267..bafc1cb20abd4 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -419,16 +419,21 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
  * (e.g. during the connect() or when a connection request on a listener
  * socket is received).
  * The vsk->remote_addr is used to decide which transport to use:
- *  - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST if
- *    g2h is not loaded, will use local transport;
- *  - remote CID <= VMADDR_CID_HOST will use guest->host transport;
- *  - remote CID > VMADDR_CID_HOST will use host->guest transport;
+ *  - remote flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION, will always
+ *    forward the vsock packets to the host and use guest->host transport;
+ *  - otherwise, going forward with the remote flag default value:
+ *    - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST
+ *      if g2h is not loaded, will use local transport;
+ *    - remote CID <= VMADDR_CID_HOST or h2g is not loaded, will use
+ *      guest->host transport;
+ *    - remote CID > VMADDR_CID_HOST will use host->guest transport;
  */
 int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 {
 	const struct vsock_transport *new_transport;
 	struct sock *sk = sk_vsock(vsk);
 	unsigned int remote_cid = vsk->remote_addr.svm_cid;
+	unsigned short remote_flag = vsk->remote_addr.svm_flag;
 	int ret;
 
 	switch (sk->sk_type) {
@@ -438,6 +443,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	case SOCK_STREAM:
 		if (vsock_use_local_transport(remote_cid))
 			new_transport = transport_local;
+		else if (remote_flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION)
+			new_transport = transport_g2h;
 		else if (remote_cid <= VMADDR_CID_HOST || !transport_h2g)
 			new_transport = transport_g2h;
 		else
-- 
2.20.1 (Apple Git-117)




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-01 15:25 ` [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure Andra Paraschiv
@ 2020-12-01 16:09   ` Stefano Garzarella
  2020-12-01 18:15     ` Paraschiv, Andra-Irina
  2020-12-03  9:21   ` Stefan Hajnoczi
  1 sibling, 1 reply; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-01 16:09 UTC (permalink / raw)
  To: Andra Paraschiv
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
>vsock enables communication between virtual machines and the host they
>are running on. With the multi transport support (guest->host and
>host->guest), nested VMs can also use vsock channels for communication.
>
>In addition to this, by default, all the vsock packets are forwarded to
>the host, if no host->guest transport is loaded. This behavior can be
>implicitly used for enabling vsock communication between sibling VMs.
>
>Add a flag field in the vsock address data structure that can be used to
>explicitly mark the vsock connection as being targeted for a certain
>type of communication. This way, can distinguish between nested VMs and
>sibling VMs use cases and can also setup them at the same time. Till
>now, could either have nested VMs or sibling VMs at a time using the
>vsock communication stack.
>
>Use the already available "svm_reserved1" field and mark it as a flag
>field instead. This flag can be set when initializing the vsock address
>variable used for the connect() call.

Maybe we can split this patch in 2 patches, one to rename the svm_flag 
and one to add the new flags.

>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
>diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
>index fd0ed7221645d..58da5a91413ac 100644
>--- a/include/uapi/linux/vm_sockets.h
>+++ b/include/uapi/linux/vm_sockets.h
>@@ -114,6 +114,22 @@
>
> #define VMADDR_CID_HOST 2
>
>+/* This sockaddr_vm flag value covers the current default use case:
>+ * local vsock communication between guest and host and nested VMs setup.
>+ * In addition to this, implicitly, the vsock packets are forwarded to the host
>+ * if no host->guest vsock transport is set.
>+ */
>+#define VMADDR_FLAG_DEFAULT_COMMUNICATION	0x0000

I think we don't need this macro, since the next one can be used to 
check if it a sibling communication (flag 0x1 set) or not (flag 0x1 
not set).

>+
>+/* Set this flag value in the sockaddr_vm corresponding field if the vsock
>+ * channel needs to be setup between two sibling VMs running on the same host.
>+ * This way can explicitly distinguish between vsock channels created for nested
>+ * VMs (or local communication between guest and host) and the ones created for
>+ * sibling VMs. And vsock channels for multiple use cases (nested / sibling VMs)
>+ * can be setup at the same time.
>+ */
>+#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION	0x0001

What do you think if we shorten in VMADDR_FLAG_SIBLING?

Thanks,
Stefano

>+
> /* Invalid vSockets version. */
>
> #define VM_SOCKETS_INVALID_VERSION -1U
>@@ -145,7 +161,7 @@
>
> struct sockaddr_vm {
> 	__kernel_sa_family_t svm_family;
>-	unsigned short svm_reserved1;
>+	unsigned short svm_flag;
> 	unsigned int svm_port;
> 	unsigned int svm_cid;
> 	unsigned char svm_zero[sizeof(struct sockaddr) -
>-- 
>2.20.1 (Apple Git-117)
>
>
>
>
>Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path
  2020-12-01 15:25 ` [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path Andra Paraschiv
@ 2020-12-01 16:22   ` Stefano Garzarella
  2020-12-01 19:01     ` Paraschiv, Andra-Irina
  0 siblings, 1 reply; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-01 16:22 UTC (permalink / raw)
  To: Andra Paraschiv
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

On Tue, Dec 01, 2020 at 05:25:04PM +0200, Andra Paraschiv wrote:
>The vsock flag can be set during the connect() setup logic, when
>initializing the vsock address data structure variable. Then the vsock
>transport is assigned, also considering this flag.
>
>The vsock transport is also assigned on the (listen) receive path. The
>flag needs to be set considering the use case.
>
>Set the vsock flag of the remote address to the one targeted for sibling
>VMs communication if the following conditions are met:
>
>* The source CID of the packet is higher than VMADDR_CID_HOST.
>* The destination CID of the packet is higher than VMADDR_CID_HOST.
>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
>index 5956939eebb78..871c84e0916b1 100644
>--- a/net/vmw_vsock/virtio_transport_common.c
>+++ b/net/vmw_vsock/virtio_transport_common.c
>@@ -1062,6 +1062,14 @@ virtio_transport_recv_listen(struct sock *sk, struct virtio_vsock_pkt *pkt,
> 	vsock_addr_init(&vchild->remote_addr, le64_to_cpu(pkt->hdr.src_cid),
> 			le32_to_cpu(pkt->hdr.src_port));
>

Maybe is better to create an helper function that other transports can 
use for the same purpose or we can put this code in the 
vsock_assign_transport() and set this flag only when the 'psk' argument 
is not NULL (this is the case when it's called by the transports when we 
receive a new connection request and 'psk' is the listener socket).

The second way should allow us to support all the transports without 
touching them.

>+	/* If the packet is coming with the source and destination CIDs higher
>+	 * than VMADDR_CID_HOST, then a vsock channel should be established for
>+	 * sibling VMs communication.
>+	 */
>+	if (vchild->local_addr.svm_cid > VMADDR_CID_HOST &&
>+	    vchild->remote_addr.svm_cid > VMADDR_CID_HOST)
>+		vchild->remote_addr.svm_flag = VMADDR_FLAG_SIBLING_VMS_COMMUNICATION;

svm_flag is always initialized to 0 in vsock_addr_init(), so this 
assignment is the first one and it's okay, but to avoid future issues 
I'd use |= here to set the flag.

Thanks,
Stefano

>+
> 	ret = vsock_assign_transport(vchild, vsk);
> 	/* Transport assigned (looking at remote_addr) must be the same
> 	 * where we received the request.
>-- 2.20.1 (Apple Git-117)
>
>
>
>
>Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag
  2020-12-01 15:25 ` [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag Andra Paraschiv
@ 2020-12-01 16:23   ` Stefano Garzarella
  2020-12-01 19:06     ` Paraschiv, Andra-Irina
  0 siblings, 1 reply; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-01 16:23 UTC (permalink / raw)
  To: Andra Paraschiv
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

On Tue, Dec 01, 2020 at 05:25:05PM +0200, Andra Paraschiv wrote:
>The vsock flag has been set in the connect and (listen) receive paths.
>
>When the vsock transport is assigned, the remote CID is used to
>distinguish between types of connection.
>
>Use the vsock flag (in addition to the CID) from the remote address to
>decide which vsock transport to assign. For the sibling VMs use case,
>all the vsock packets need to be forwarded to the host, so always assign
>the guest->host transport if the vsock flag is set. For the other use
>cases, the vsock transport assignment logic is not changed.
>
>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>---
> net/vmw_vsock/af_vsock.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index d10916ab45267..bafc1cb20abd4 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -419,16 +419,21 @@ static void vsock_deassign_transport(struct vsock_sock *vsk)
>  * (e.g. during the connect() or when a connection request on a listener
>  * socket is received).
>  * The vsk->remote_addr is used to decide which transport to use:
>- *  - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST if
>- *    g2h is not loaded, will use local transport;
>- *  - remote CID <= VMADDR_CID_HOST will use guest->host transport;
>- *  - remote CID > VMADDR_CID_HOST will use host->guest transport;
>+ *  - remote flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION, will always
>+ *    forward the vsock packets to the host and use guest->host transport;
>+ *  - otherwise, going forward with the remote flag default value:
>+ *    - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or VMADDR_CID_HOST
>+ *      if g2h is not loaded, will use local transport;
>+ *    - remote CID <= VMADDR_CID_HOST or h2g is not loaded, will use
>+ *      guest->host transport;
>+ *    - remote CID > VMADDR_CID_HOST will use host->guest transport;
>  */
> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> {
> 	const struct vsock_transport *new_transport;
> 	struct sock *sk = sk_vsock(vsk);
> 	unsigned int remote_cid = vsk->remote_addr.svm_cid;
>+	unsigned short remote_flag = vsk->remote_addr.svm_flag;
> 	int ret;
>
> 	switch (sk->sk_type) {
>@@ -438,6 +443,8 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
> 	case SOCK_STREAM:
> 		if (vsock_use_local_transport(remote_cid))
> 			new_transport = transport_local;
>+		else if (remote_flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION)

Others flags can be added, so here we should use the bitwise AND 
operator to check if this flag is set.

And what about merging with the next if clause?


Thanks,
Stefano

>+			new_transport = transport_g2h;
> 		else if (remote_cid <= VMADDR_CID_HOST || 
> 		!transport_h2g)
> 			new_transport = transport_g2h;
> 		else
>-- 
>2.20.1 (Apple Git-117)
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address
  2020-12-01 15:25 [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Andra Paraschiv
                   ` (2 preceding siblings ...)
  2020-12-01 15:25 ` [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag Andra Paraschiv
@ 2020-12-01 16:27 ` Stefano Garzarella
  2020-12-01 18:02   ` Paraschiv, Andra-Irina
  2020-12-02 13:37 ` Stefano Garzarella
  4 siblings, 1 reply; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-01 16:27 UTC (permalink / raw)
  To: Andra Paraschiv
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

Hi Andra,

On Tue, Dec 01, 2020 at 05:25:02PM +0200, Andra Paraschiv wrote:
>vsock enables communication between virtual machines and the host they are
>running on. Nested VMs can be setup to use vsock channels, as the multi
>transport support has been available in the mainline since the v5.5 Linux kernel
>has been released.
>
>Implicitly, if no host->guest vsock transport is loaded, all the vsock packets
>are forwarded to the host. This behavior can be used to setup communication
>channels between sibling VMs that are running on the same host. One example can
>be the vsock channels that can be established within AWS Nitro Enclaves
>(see Documentation/virt/ne_overview.rst).
>
>To be able to explicitly mark a connection as being used for a certain use case,
>add a flag field in the vsock address data structure. The "svm_reserved1" field
>has been repurposed to be the flag field. The value of the flag will then be
>taken into consideration when the vsock transport is assigned.
>
>This way can distinguish between nested VMs / local communication and sibling
>VMs use cases. And can also setup one or more types of communication at the same
>time.

Thanks to work on this, I've left you a few comments, but I think this 
is the right way to support nested and sibling communication together.

Thank you,
Stefano


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address
  2020-12-01 16:27 ` [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Stefano Garzarella
@ 2020-12-01 18:02   ` Paraschiv, Andra-Irina
  0 siblings, 0 replies; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-01 18:02 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov



On 01/12/2020 18:27, Stefano Garzarella wrote:
>
>
> Hi Andra,
>
> On Tue, Dec 01, 2020 at 05:25:02PM +0200, Andra Paraschiv wrote:
>> vsock enables communication between virtual machines and the host 
>> they are
>> running on. Nested VMs can be setup to use vsock channels, as the multi
>> transport support has been available in the mainline since the v5.5 
>> Linux kernel
>> has been released.
>>
>> Implicitly, if no host->guest vsock transport is loaded, all the 
>> vsock packets
>> are forwarded to the host. This behavior can be used to setup 
>> communication
>> channels between sibling VMs that are running on the same host. One 
>> example can
>> be the vsock channels that can be established within AWS Nitro Enclaves
>> (see Documentation/virt/ne_overview.rst).
>>
>> To be able to explicitly mark a connection as being used for a 
>> certain use case,
>> add a flag field in the vsock address data structure. The 
>> "svm_reserved1" field
>> has been repurposed to be the flag field. The value of the flag will 
>> then be
>> taken into consideration when the vsock transport is assigned.
>>
>> This way can distinguish between nested VMs / local communication and 
>> sibling
>> VMs use cases. And can also setup one or more types of communication 
>> at the same
>> time.
>
> Thanks to work on this, I've left you a few comments, but I think this
> is the right way to support nested and sibling communication together.

Hi Stefano,

Thanks also for taking time to review and both you and Stefan for 
sharing an overview of this proposed option.

I'm going through the comments and will send out the v2 of the patch 
series as I have the changes done and validated.

Thanks,
Andra



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-01 16:09   ` Stefano Garzarella
@ 2020-12-01 18:15     ` Paraschiv, Andra-Irina
  2020-12-02  8:32       ` Stefano Garzarella
  0 siblings, 1 reply; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-01 18:15 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov



On 01/12/2020 18:09, Stefano Garzarella wrote:
>
> On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
>> vsock enables communication between virtual machines and the host they
>> are running on. With the multi transport support (guest->host and
>> host->guest), nested VMs can also use vsock channels for communication.
>>
>> In addition to this, by default, all the vsock packets are forwarded to
>> the host, if no host->guest transport is loaded. This behavior can be
>> implicitly used for enabling vsock communication between sibling VMs.
>>
>> Add a flag field in the vsock address data structure that can be used to
>> explicitly mark the vsock connection as being targeted for a certain
>> type of communication. This way, can distinguish between nested VMs and
>> sibling VMs use cases and can also setup them at the same time. Till
>> now, could either have nested VMs or sibling VMs at a time using the
>> vsock communication stack.
>>
>> Use the already available "svm_reserved1" field and mark it as a flag
>> field instead. This flag can be set when initializing the vsock address
>> variable used for the connect() call.
>
> Maybe we can split this patch in 2 patches, one to rename the svm_flag
> and one to add the new flags.

Sure, I can split this in 2 patches, to have a bit more separation of 
duties.

>
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/vm_sockets.h 
>> b/include/uapi/linux/vm_sockets.h
>> index fd0ed7221645d..58da5a91413ac 100644
>> --- a/include/uapi/linux/vm_sockets.h
>> +++ b/include/uapi/linux/vm_sockets.h
>> @@ -114,6 +114,22 @@
>>
>> #define VMADDR_CID_HOST 2
>>
>> +/* This sockaddr_vm flag value covers the current default use case:
>> + * local vsock communication between guest and host and nested VMs 
>> setup.
>> + * In addition to this, implicitly, the vsock packets are forwarded 
>> to the host
>> + * if no host->guest vsock transport is set.
>> + */
>> +#define VMADDR_FLAG_DEFAULT_COMMUNICATION     0x0000
>
> I think we don't need this macro, since the next one can be used to
> check if it a sibling communication (flag 0x1 set) or not (flag 0x1
> not set).

Right, that's not particularly the use of the flag value, as by default 
comes as 0. It was more for sharing the cases this covers. But I can 
remove the define and keep this kind of info, with regard to the default 
case, in the commit message / comments.

>
>> +
>> +/* Set this flag value in the sockaddr_vm corresponding field if the 
>> vsock
>> + * channel needs to be setup between two sibling VMs running on the 
>> same host.
>> + * This way can explicitly distinguish between vsock channels 
>> created for nested
>> + * VMs (or local communication between guest and host) and the ones 
>> created for
>> + * sibling VMs. And vsock channels for multiple use cases (nested / 
>> sibling VMs)
>> + * can be setup at the same time.
>> + */
>> +#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION 0x0001
>
> What do you think if we shorten in VMADDR_FLAG_SIBLING?
>

Yup, this seems ok as well for me. I'll update the naming.

Thanks,
Andra

>
>> +
>> /* Invalid vSockets version. */
>>
>> #define VM_SOCKETS_INVALID_VERSION -1U
>> @@ -145,7 +161,7 @@
>>
>> struct sockaddr_vm {
>>       __kernel_sa_family_t svm_family;
>> -      unsigned short svm_reserved1;
>> +      unsigned short svm_flag;
>>       unsigned int svm_port;
>>       unsigned int svm_cid;
>>       unsigned char svm_zero[sizeof(struct sockaddr) -
>> -- 
>> 2.20.1 (Apple Git-117)
>>
>>
>>
>>
>> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. 
>> Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. 
>> Registered in Romania. Registration number J22/2621/2005.
>>
>




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path
  2020-12-01 16:22   ` Stefano Garzarella
@ 2020-12-01 19:01     ` Paraschiv, Andra-Irina
  2020-12-02  8:53       ` Stefano Garzarella
  0 siblings, 1 reply; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-01 19:01 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov



On 01/12/2020 18:22, Stefano Garzarella wrote:
>
> On Tue, Dec 01, 2020 at 05:25:04PM +0200, Andra Paraschiv wrote:
>> The vsock flag can be set during the connect() setup logic, when
>> initializing the vsock address data structure variable. Then the vsock
>> transport is assigned, also considering this flag.
>>
>> The vsock transport is also assigned on the (listen) receive path. The
>> flag needs to be set considering the use case.
>>
>> Set the vsock flag of the remote address to the one targeted for sibling
>> VMs communication if the following conditions are met:
>>
>> * The source CID of the packet is higher than VMADDR_CID_HOST.
>> * The destination CID of the packet is higher than VMADDR_CID_HOST.
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/net/vmw_vsock/virtio_transport_common.c 
>> b/net/vmw_vsock/virtio_transport_common.c
>> index 5956939eebb78..871c84e0916b1 100644
>> --- a/net/vmw_vsock/virtio_transport_common.c
>> +++ b/net/vmw_vsock/virtio_transport_common.c
>> @@ -1062,6 +1062,14 @@ virtio_transport_recv_listen(struct sock *sk, 
>> struct virtio_vsock_pkt *pkt,
>>       vsock_addr_init(&vchild->remote_addr, 
>> le64_to_cpu(pkt->hdr.src_cid),
>>                       le32_to_cpu(pkt->hdr.src_port));
>>
>
> Maybe is better to create an helper function that other transports can
> use for the same purpose or we can put this code in the
> vsock_assign_transport() and set this flag only when the 'psk' argument
> is not NULL (this is the case when it's called by the transports when we
> receive a new connection request and 'psk' is the listener socket).
>
> The second way should allow us to support all the transports without
> touching them.

Ack, I was wondering about the other transports such as vmci or hyperv.

I can move the logic below in the codebase that assigns the transport, 
after checking 'psk'.

>
>> +      /* If the packet is coming with the source and destination 
>> CIDs higher
>> +       * than VMADDR_CID_HOST, then a vsock channel should be 
>> established for
>> +       * sibling VMs communication.
>> +       */
>> +      if (vchild->local_addr.svm_cid > VMADDR_CID_HOST &&
>> +          vchild->remote_addr.svm_cid > VMADDR_CID_HOST)
>> +              vchild->remote_addr.svm_flag = 
>> VMADDR_FLAG_SIBLING_VMS_COMMUNICATION;
>
> svm_flag is always initialized to 0 in vsock_addr_init(), so this
> assignment is the first one and it's okay, but to avoid future issues
> I'd use |= here to set the flag.

Fair point. I was thinking more towards exclusive flags values 
(purposes), but that's fine with the bitwise operator if we would get a 
set of flag values together. I will also update the field name to 
'svm_flags', let me know if we should keep the previous one or there is 
a better option.

Thanks,
Andra

>
>> +
>>       ret = vsock_assign_transport(vchild, vsk);
>>       /* Transport assigned (looking at remote_addr) must be the same
>>        * where we received the request.
>> -- 2.20.1 (Apple Git-117)
>>
>>
>>
>>
>> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. 
>> Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. 
>> Registered in Romania. Registration number J22/2621/2005.
>>
>




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag
  2020-12-01 16:23   ` Stefano Garzarella
@ 2020-12-01 19:06     ` Paraschiv, Andra-Irina
  0 siblings, 0 replies; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-01 19:06 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov



On 01/12/2020 18:23, Stefano Garzarella wrote:
>
> On Tue, Dec 01, 2020 at 05:25:05PM +0200, Andra Paraschiv wrote:
>> The vsock flag has been set in the connect and (listen) receive paths.
>>
>> When the vsock transport is assigned, the remote CID is used to
>> distinguish between types of connection.
>>
>> Use the vsock flag (in addition to the CID) from the remote address to
>> decide which vsock transport to assign. For the sibling VMs use case,
>> all the vsock packets need to be forwarded to the host, so always assign
>> the guest->host transport if the vsock flag is set. For the other use
>> cases, the vsock transport assignment logic is not changed.
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>> net/vmw_vsock/af_vsock.c | 15 +++++++++++----
>> 1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index d10916ab45267..bafc1cb20abd4 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -419,16 +419,21 @@ static void vsock_deassign_transport(struct 
>> vsock_sock *vsk)
>>  * (e.g. during the connect() or when a connection request on a listener
>>  * socket is received).
>>  * The vsk->remote_addr is used to decide which transport to use:
>> - *  - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or 
>> VMADDR_CID_HOST if
>> - *    g2h is not loaded, will use local transport;
>> - *  - remote CID <= VMADDR_CID_HOST will use guest->host transport;
>> - *  - remote CID > VMADDR_CID_HOST will use host->guest transport;
>> + *  - remote flag == VMADDR_FLAG_SIBLING_VMS_COMMUNICATION, will always
>> + *    forward the vsock packets to the host and use guest->host 
>> transport;
>> + *  - otherwise, going forward with the remote flag default value:
>> + *    - remote CID == VMADDR_CID_LOCAL or g2h->local_cid or 
>> VMADDR_CID_HOST
>> + *      if g2h is not loaded, will use local transport;
>> + *    - remote CID <= VMADDR_CID_HOST or h2g is not loaded, will use
>> + *      guest->host transport;
>> + *    - remote CID > VMADDR_CID_HOST will use host->guest transport;
>>  */
>> int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock 
>> *psk)
>> {
>>       const struct vsock_transport *new_transport;
>>       struct sock *sk = sk_vsock(vsk);
>>       unsigned int remote_cid = vsk->remote_addr.svm_cid;
>> +      unsigned short remote_flag = vsk->remote_addr.svm_flag;
>>       int ret;
>>
>>       switch (sk->sk_type) {
>> @@ -438,6 +443,8 @@ int vsock_assign_transport(struct vsock_sock 
>> *vsk, struct vsock_sock *psk)
>>       case SOCK_STREAM:
>>               if (vsock_use_local_transport(remote_cid))
>>                       new_transport = transport_local;
>> +              else if (remote_flag == 
>> VMADDR_FLAG_SIBLING_VMS_COMMUNICATION)
>
> Others flags can be added, so here we should use the bitwise AND
> operator to check if this flag is set.
>
> And what about merging with the next if clause?
>

Indeed, I'll update the codebase to use the bitwise operator. Then I can 
also merge all the checks corresponding to the g2h transport in a single 
if block.

Thanks,
Andra

>
>> +                      new_transport = transport_g2h;
>>               else if (remote_cid <= VMADDR_CID_HOST ||
>>               !transport_h2g)
>>                       new_transport = transport_g2h;
>>               else
>> -- 
>> 2.20.1 (Apple Git-117)
>>
>




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-01 18:15     ` Paraschiv, Andra-Irina
@ 2020-12-02  8:32       ` Stefano Garzarella
  0 siblings, 0 replies; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-02  8:32 UTC (permalink / raw)
  To: Paraschiv, Andra-Irina
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

On Tue, Dec 01, 2020 at 08:15:04PM +0200, Paraschiv, Andra-Irina wrote:
>
>
>On 01/12/2020 18:09, Stefano Garzarella wrote:
>>
>>On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
>>>vsock enables communication between virtual machines and the host they
>>>are running on. With the multi transport support (guest->host and
>>>host->guest), nested VMs can also use vsock channels for communication.
>>>
>>>In addition to this, by default, all the vsock packets are forwarded to
>>>the host, if no host->guest transport is loaded. This behavior can be
>>>implicitly used for enabling vsock communication between sibling VMs.
>>>
>>>Add a flag field in the vsock address data structure that can be used to
>>>explicitly mark the vsock connection as being targeted for a certain
>>>type of communication. This way, can distinguish between nested VMs and
>>>sibling VMs use cases and can also setup them at the same time. Till
>>>now, could either have nested VMs or sibling VMs at a time using the
>>>vsock communication stack.
>>>
>>>Use the already available "svm_reserved1" field and mark it as a flag
>>>field instead. This flag can be set when initializing the vsock address
>>>variable used for the connect() call.
>>
>>Maybe we can split this patch in 2 patches, one to rename the svm_flag
>>and one to add the new flags.
>
>Sure, I can split this in 2 patches, to have a bit more separation of 
>duties.
>
>>
>>>
>>>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>>>---
>>>include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
>>>1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/include/uapi/linux/vm_sockets.h 
>>>b/include/uapi/linux/vm_sockets.h
>>>index fd0ed7221645d..58da5a91413ac 100644
>>>--- a/include/uapi/linux/vm_sockets.h
>>>+++ b/include/uapi/linux/vm_sockets.h
>>>@@ -114,6 +114,22 @@
>>>
>>>#define VMADDR_CID_HOST 2
>>>
>>>+/* This sockaddr_vm flag value covers the current default use case:
>>>+ * local vsock communication between guest and host and nested 
>>>VMs setup.
>>>+ * In addition to this, implicitly, the vsock packets are 
>>>forwarded to the host
>>>+ * if no host->guest vsock transport is set.
>>>+ */
>>>+#define VMADDR_FLAG_DEFAULT_COMMUNICATION     0x0000
>>
>>I think we don't need this macro, since the next one can be used to
>>check if it a sibling communication (flag 0x1 set) or not (flag 0x1
>>not set).
>
>Right, that's not particularly the use of the flag value, as by 
>default comes as 0. It was more for sharing the cases this covers. But 
>I can remove the define and keep this kind of info, with regard to the 
>default case, in the commit message / comments.
>

Agree, you can add few lines in the comment block of VMADDR_FLAG_SIBLING 
describing the default case when it is not set.

>>
>>>+
>>>+/* Set this flag value in the sockaddr_vm corresponding field if 
>>>the vsock
>>>+ * channel needs to be setup between two sibling VMs running on 
>>>the same host.
>>>+ * This way can explicitly distinguish between vsock channels 
>>>created for nested
>>>+ * VMs (or local communication between guest and host) and the 
>>>ones created for
>>>+ * sibling VMs. And vsock channels for multiple use cases (nested 
>>>/ sibling VMs)
>>>+ * can be setup at the same time.
>>>+ */
>>>+#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION 0x0001
>>
>>What do you think if we shorten in VMADDR_FLAG_SIBLING?
>>
>
>Yup, this seems ok as well for me. I'll update the naming.
>

Thanks,
Stefano


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path
  2020-12-01 19:01     ` Paraschiv, Andra-Irina
@ 2020-12-02  8:53       ` Stefano Garzarella
  0 siblings, 0 replies; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-02  8:53 UTC (permalink / raw)
  To: Paraschiv, Andra-Irina
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

On Tue, Dec 01, 2020 at 09:01:05PM +0200, Paraschiv, Andra-Irina wrote:
>
>
>On 01/12/2020 18:22, Stefano Garzarella wrote:
>>
>>On Tue, Dec 01, 2020 at 05:25:04PM +0200, Andra Paraschiv wrote:
>>>The vsock flag can be set during the connect() setup logic, when
>>>initializing the vsock address data structure variable. Then the vsock
>>>transport is assigned, also considering this flag.
>>>
>>>The vsock transport is also assigned on the (listen) receive path. The
>>>flag needs to be set considering the use case.
>>>
>>>Set the vsock flag of the remote address to the one targeted for sibling
>>>VMs communication if the following conditions are met:
>>>
>>>* The source CID of the packet is higher than VMADDR_CID_HOST.
>>>* The destination CID of the packet is higher than VMADDR_CID_HOST.
>>>
>>>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>>>---
>>>net/vmw_vsock/virtio_transport_common.c | 8 ++++++++
>>>1 file changed, 8 insertions(+)
>>>
>>>diff --git a/net/vmw_vsock/virtio_transport_common.c 
>>>b/net/vmw_vsock/virtio_transport_common.c
>>>index 5956939eebb78..871c84e0916b1 100644
>>>--- a/net/vmw_vsock/virtio_transport_common.c
>>>+++ b/net/vmw_vsock/virtio_transport_common.c
>>>@@ -1062,6 +1062,14 @@ virtio_transport_recv_listen(struct sock 
>>>*sk, struct virtio_vsock_pkt *pkt,
>>>      vsock_addr_init(&vchild->remote_addr, 
>>>le64_to_cpu(pkt->hdr.src_cid),
>>>                      le32_to_cpu(pkt->hdr.src_port));
>>>
>>
>>Maybe is better to create an helper function that other transports can
>>use for the same purpose or we can put this code in the
>>vsock_assign_transport() and set this flag only when the 'psk' argument
>>is not NULL (this is the case when it's called by the transports when we
>>receive a new connection request and 'psk' is the listener socket).
>>
>>The second way should allow us to support all the transports without
>>touching them.
>
>Ack, I was wondering about the other transports such as vmci or hyperv.
>
>I can move the logic below in the codebase that assigns the transport, 
>after checking 'psk'.
>
>>
>>>+      /* If the packet is coming with the source and destination 
>>>CIDs higher
>>>+       * than VMADDR_CID_HOST, then a vsock channel should be 
>>>established for
>>>+       * sibling VMs communication.
>>>+       */
>>>+      if (vchild->local_addr.svm_cid > VMADDR_CID_HOST &&
>>>+          vchild->remote_addr.svm_cid > VMADDR_CID_HOST)
>>>+              vchild->remote_addr.svm_flag = 
>>>VMADDR_FLAG_SIBLING_VMS_COMMUNICATION;
>>
>>svm_flag is always initialized to 0 in vsock_addr_init(), so this
>>assignment is the first one and it's okay, but to avoid future issues
>>I'd use |= here to set the flag.
>
>Fair point. I was thinking more towards exclusive flags values 
>(purposes), but that's fine with the bitwise operator if we would get 
>a set of flag values together. I will also update the field name to 
>'svm_flags', let me know if we should keep the previous one or there 
>is a better option.

Yeah, maybe in the future we will add some new flags and we'll only need 
to add them without touching this code.

Agree with the new 'svm_flags' field name.

Thanks,
Stefano


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address
  2020-12-01 15:25 [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Andra Paraschiv
                   ` (3 preceding siblings ...)
  2020-12-01 16:27 ` [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Stefano Garzarella
@ 2020-12-02 13:37 ` Stefano Garzarella
  2020-12-02 16:18   ` Paraschiv, Andra-Irina
  4 siblings, 1 reply; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-02 13:37 UTC (permalink / raw)
  To: Andra Paraschiv
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

Hi Andra,

On Tue, Dec 01, 2020 at 05:25:02PM +0200, Andra Paraschiv wrote:
>vsock enables communication between virtual machines and the host they are
>running on. Nested VMs can be setup to use vsock channels, as the multi
>transport support has been available in the mainline since the v5.5 Linux kernel
>has been released.
>
>Implicitly, if no host->guest vsock transport is loaded, all the vsock packets
>are forwarded to the host. This behavior can be used to setup communication
>channels between sibling VMs that are running on the same host. One example can
>be the vsock channels that can be established within AWS Nitro Enclaves
>(see Documentation/virt/ne_overview.rst).
>
>To be able to explicitly mark a connection as being used for a certain use case,
>add a flag field in the vsock address data structure. The "svm_reserved1" field
>has been repurposed to be the flag field. The value of the flag will then be
>taken into consideration when the vsock transport is assigned.
>
>This way can distinguish between nested VMs / local communication and sibling
>VMs use cases. And can also setup one or more types of communication at the same
>time.
>

Another thing worth mentioning is that for now it is not supported in 
vhost-vsock, since we are discarding every packet not addressed to the 
host.

What we should do would be:
- add a new IOCTL to vhost-vsock to enable sibling communication, by 
   default I'd like to leave it disabled

- allow sibling forwarding only if both guests have sibling 
   communication enabled and we should implement some kind of filtering 
   or network namespace support to allow the communication only between a 
   subset of VMs


Do you have plans to work on it?

Otherwise I put it in my to-do list and hope I have time to do it (maybe 
next month).

Thanks,
Stefano


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address
  2020-12-02 13:37 ` Stefano Garzarella
@ 2020-12-02 16:18   ` Paraschiv, Andra-Irina
  2020-12-03  8:51     ` Stefano Garzarella
  0 siblings, 1 reply; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-02 16:18 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov



On 02/12/2020 15:37, Stefano Garzarella wrote:
>
> Hi Andra,
>
> On Tue, Dec 01, 2020 at 05:25:02PM +0200, Andra Paraschiv wrote:
>> vsock enables communication between virtual machines and the host 
>> they are
>> running on. Nested VMs can be setup to use vsock channels, as the multi
>> transport support has been available in the mainline since the v5.5 
>> Linux kernel
>> has been released.
>>
>> Implicitly, if no host->guest vsock transport is loaded, all the 
>> vsock packets
>> are forwarded to the host. This behavior can be used to setup 
>> communication
>> channels between sibling VMs that are running on the same host. One 
>> example can
>> be the vsock channels that can be established within AWS Nitro Enclaves
>> (see Documentation/virt/ne_overview.rst).
>>
>> To be able to explicitly mark a connection as being used for a 
>> certain use case,
>> add a flag field in the vsock address data structure. The 
>> "svm_reserved1" field
>> has been repurposed to be the flag field. The value of the flag will 
>> then be
>> taken into consideration when the vsock transport is assigned.
>>
>> This way can distinguish between nested VMs / local communication and 
>> sibling
>> VMs use cases. And can also setup one or more types of communication 
>> at the same
>> time.
>>
>
> Another thing worth mentioning is that for now it is not supported in
> vhost-vsock, since we are discarding every packet not addressed to the
> host.

Right, thanks for the follow-up.

>
> What we should do would be:
> - add a new IOCTL to vhost-vsock to enable sibling communication, by
>   default I'd like to leave it disabled
>
> - allow sibling forwarding only if both guests have sibling
>   communication enabled and we should implement some kind of filtering
>   or network namespace support to allow the communication only between a
>   subset of VMs
>
>
> Do you have plans to work on it?

Nope, not yet. But I can take some time in the second part of December / 
beginning of January for this. And we can catch up in the meantime if 
there is something blocking or more clarifications are needed to make it 
work.

Thanks,
Andra

>
>
> Otherwise I put it in my to-do list and hope I have time to do it (maybe
> next month).
>
> Thanks,
> Stefano
>




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address
  2020-12-02 16:18   ` Paraschiv, Andra-Irina
@ 2020-12-03  8:51     ` Stefano Garzarella
  0 siblings, 0 replies; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-03  8:51 UTC (permalink / raw)
  To: Paraschiv, Andra-Irina
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Stefan Hajnoczi,
	Vitaly Kuznetsov

On Wed, Dec 02, 2020 at 06:18:15PM +0200, Paraschiv, Andra-Irina wrote:
>
>
>On 02/12/2020 15:37, Stefano Garzarella wrote:
>>
>>Hi Andra,
>>
>>On Tue, Dec 01, 2020 at 05:25:02PM +0200, Andra Paraschiv wrote:
>>>vsock enables communication between virtual machines and the host 
>>>they are
>>>running on. Nested VMs can be setup to use vsock channels, as the multi
>>>transport support has been available in the mainline since the 
>>>v5.5 Linux kernel
>>>has been released.
>>>
>>>Implicitly, if no host->guest vsock transport is loaded, all the 
>>>vsock packets
>>>are forwarded to the host. This behavior can be used to setup 
>>>communication
>>>channels between sibling VMs that are running on the same host. 
>>>One example can
>>>be the vsock channels that can be established within AWS Nitro Enclaves
>>>(see Documentation/virt/ne_overview.rst).
>>>
>>>To be able to explicitly mark a connection as being used for a 
>>>certain use case,
>>>add a flag field in the vsock address data structure. The 
>>>"svm_reserved1" field
>>>has been repurposed to be the flag field. The value of the flag 
>>>will then be
>>>taken into consideration when the vsock transport is assigned.
>>>
>>>This way can distinguish between nested VMs / local communication 
>>>and sibling
>>>VMs use cases. And can also setup one or more types of 
>>>communication at the same
>>>time.
>>>
>>
>>Another thing worth mentioning is that for now it is not supported in
>>vhost-vsock, since we are discarding every packet not addressed to the
>>host.
>
>Right, thanks for the follow-up.
>
>>
>>What we should do would be:
>>- add a new IOCTL to vhost-vsock to enable sibling communication, by
>>  default I'd like to leave it disabled
>>
>>- allow sibling forwarding only if both guests have sibling
>>  communication enabled and we should implement some kind of filtering
>>  or network namespace support to allow the communication only between a
>>  subset of VMs
>>
>>
>>Do you have plans to work on it?
>
>Nope, not yet. But I can take some time in the second part of December 
>/ beginning of January for this. And we can catch up in the meantime 
>if there is something blocking or more clarifications are needed to 
>make it work.
>

Good, it will be great!

Thanks,
Stefano


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-01 15:25 ` [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure Andra Paraschiv
  2020-12-01 16:09   ` Stefano Garzarella
@ 2020-12-03  9:21   ` Stefan Hajnoczi
  2020-12-03 10:32     ` Paraschiv, Andra-Irina
  1 sibling, 1 reply; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-12-03  9:21 UTC (permalink / raw)
  To: Andra Paraschiv
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski,
	Stefano Garzarella, Vitaly Kuznetsov

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
> vsock enables communication between virtual machines and the host they
> are running on. With the multi transport support (guest->host and
> host->guest), nested VMs can also use vsock channels for communication.
> 
> In addition to this, by default, all the vsock packets are forwarded to
> the host, if no host->guest transport is loaded. This behavior can be
> implicitly used for enabling vsock communication between sibling VMs.
> 
> Add a flag field in the vsock address data structure that can be used to
> explicitly mark the vsock connection as being targeted for a certain
> type of communication. This way, can distinguish between nested VMs and
> sibling VMs use cases and can also setup them at the same time. Till
> now, could either have nested VMs or sibling VMs at a time using the
> vsock communication stack.
> 
> Use the already available "svm_reserved1" field and mark it as a flag
> field instead. This flag can be set when initializing the vsock address
> variable used for the connect() call.
> 
> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
> ---
>  include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
> index fd0ed7221645d..58da5a91413ac 100644
> --- a/include/uapi/linux/vm_sockets.h
> +++ b/include/uapi/linux/vm_sockets.h
> @@ -114,6 +114,22 @@
>  
>  #define VMADDR_CID_HOST 2
>  
> +/* This sockaddr_vm flag value covers the current default use case:
> + * local vsock communication between guest and host and nested VMs setup.
> + * In addition to this, implicitly, the vsock packets are forwarded to the host
> + * if no host->guest vsock transport is set.
> + */
> +#define VMADDR_FLAG_DEFAULT_COMMUNICATION	0x0000
> +
> +/* Set this flag value in the sockaddr_vm corresponding field if the vsock
> + * channel needs to be setup between two sibling VMs running on the same host.
> + * This way can explicitly distinguish between vsock channels created for nested
> + * VMs (or local communication between guest and host) and the ones created for
> + * sibling VMs. And vsock channels for multiple use cases (nested / sibling VMs)
> + * can be setup at the same time.
> + */
> +#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION	0x0001

vsock has the h2g and g2h concept. It would be more general to call this
flag VMADDR_FLAG_G2H or less cryptically VMADDR_FLAG_TO_HOST.

That way it just tells the driver in which direction to send packets
without implying that sibling communication is possible (it's not
allowed by default on any transport).

I don't have a strong opinion on this but wanted to suggest the idea.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-03  9:21   ` Stefan Hajnoczi
@ 2020-12-03 10:32     ` Paraschiv, Andra-Irina
  2020-12-03 13:38       ` Stefano Garzarella
  0 siblings, 1 reply; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-03 10:32 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski,
	Stefano Garzarella, Vitaly Kuznetsov



On 03/12/2020 11:21, Stefan Hajnoczi wrote:
> On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
>> vsock enables communication between virtual machines and the host they
>> are running on. With the multi transport support (guest->host and
>> host->guest), nested VMs can also use vsock channels for communication.
>>
>> In addition to this, by default, all the vsock packets are forwarded to
>> the host, if no host->guest transport is loaded. This behavior can be
>> implicitly used for enabling vsock communication between sibling VMs.
>>
>> Add a flag field in the vsock address data structure that can be used to
>> explicitly mark the vsock connection as being targeted for a certain
>> type of communication. This way, can distinguish between nested VMs and
>> sibling VMs use cases and can also setup them at the same time. Till
>> now, could either have nested VMs or sibling VMs at a time using the
>> vsock communication stack.
>>
>> Use the already available "svm_reserved1" field and mark it as a flag
>> field instead. This flag can be set when initializing the vsock address
>> variable used for the connect() call.
>>
>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>> ---
>>   include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
>> index fd0ed7221645d..58da5a91413ac 100644
>> --- a/include/uapi/linux/vm_sockets.h
>> +++ b/include/uapi/linux/vm_sockets.h
>> @@ -114,6 +114,22 @@
>>   
>>   #define VMADDR_CID_HOST 2
>>   
>> +/* This sockaddr_vm flag value covers the current default use case:
>> + * local vsock communication between guest and host and nested VMs setup.
>> + * In addition to this, implicitly, the vsock packets are forwarded to the host
>> + * if no host->guest vsock transport is set.
>> + */
>> +#define VMADDR_FLAG_DEFAULT_COMMUNICATION	0x0000
>> +
>> +/* Set this flag value in the sockaddr_vm corresponding field if the vsock
>> + * channel needs to be setup between two sibling VMs running on the same host.
>> + * This way can explicitly distinguish between vsock channels created for nested
>> + * VMs (or local communication between guest and host) and the ones created for
>> + * sibling VMs. And vsock channels for multiple use cases (nested / sibling VMs)
>> + * can be setup at the same time.
>> + */
>> +#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION	0x0001
> vsock has the h2g and g2h concept. It would be more general to call this
> flag VMADDR_FLAG_G2H or less cryptically VMADDR_FLAG_TO_HOST.

Thanks for the feedback, Stefan.

I can update the naming to be more general, such as  "_TO_HOST", and 
keep the use cases (e.g. guest <-> host / nested / sibling VMs 
communication) mention in the comments so that would relate more to the 
motivation behind it.

Andra

>
> That way it just tells the driver in which direction to send packets
> without implying that sibling communication is possible (it's not
> allowed by default on any transport).
>
> I don't have a strong opinion on this but wanted to suggest the idea.
>
> Stefan




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-03 10:32     ` Paraschiv, Andra-Irina
@ 2020-12-03 13:38       ` Stefano Garzarella
  2020-12-03 14:04         ` Paraschiv, Andra-Irina
  0 siblings, 1 reply; 21+ messages in thread
From: Stefano Garzarella @ 2020-12-03 13:38 UTC (permalink / raw)
  To: Paraschiv, Andra-Irina, Stefan Hajnoczi
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Vitaly Kuznetsov

On Thu, Dec 03, 2020 at 12:32:08PM +0200, Paraschiv, Andra-Irina wrote:
>
>
>On 03/12/2020 11:21, Stefan Hajnoczi wrote:
>>On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
>>>vsock enables communication between virtual machines and the host they
>>>are running on. With the multi transport support (guest->host and
>>>host->guest), nested VMs can also use vsock channels for communication.
>>>
>>>In addition to this, by default, all the vsock packets are forwarded to
>>>the host, if no host->guest transport is loaded. This behavior can be
>>>implicitly used for enabling vsock communication between sibling VMs.
>>>
>>>Add a flag field in the vsock address data structure that can be used to
>>>explicitly mark the vsock connection as being targeted for a certain
>>>type of communication. This way, can distinguish between nested VMs and
>>>sibling VMs use cases and can also setup them at the same time. Till
>>>now, could either have nested VMs or sibling VMs at a time using the
>>>vsock communication stack.
>>>
>>>Use the already available "svm_reserved1" field and mark it as a flag
>>>field instead. This flag can be set when initializing the vsock address
>>>variable used for the connect() call.
>>>
>>>Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>>>---
>>>  include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
>>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/include/uapi/linux/vm_sockets.h b/include/uapi/linux/vm_sockets.h
>>>index fd0ed7221645d..58da5a91413ac 100644
>>>--- a/include/uapi/linux/vm_sockets.h
>>>+++ b/include/uapi/linux/vm_sockets.h
>>>@@ -114,6 +114,22 @@
>>>  #define VMADDR_CID_HOST 2
>>>+/* This sockaddr_vm flag value covers the current default use case:
>>>+ * local vsock communication between guest and host and nested VMs setup.
>>>+ * In addition to this, implicitly, the vsock packets are forwarded to the host
>>>+ * if no host->guest vsock transport is set.
>>>+ */
>>>+#define VMADDR_FLAG_DEFAULT_COMMUNICATION	0x0000
>>>+
>>>+/* Set this flag value in the sockaddr_vm corresponding field if the vsock
>>>+ * channel needs to be setup between two sibling VMs running on the same host.
>>>+ * This way can explicitly distinguish between vsock channels created for nested
>>>+ * VMs (or local communication between guest and host) and the ones created for
>>>+ * sibling VMs. And vsock channels for multiple use cases (nested / sibling VMs)
>>>+ * can be setup at the same time.
>>>+ */
>>>+#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION	0x0001
>>vsock has the h2g and g2h concept. It would be more general to call this
>>flag VMADDR_FLAG_G2H or less cryptically VMADDR_FLAG_TO_HOST.

I agree, VMADDR_FLAG_TO_HOST is more general and it's clearer that is up 
to the host where to forward the packet (sibling if supported, or 
whatever).

Thanks,
Stefano

>
>Thanks for the feedback, Stefan.
>
>I can update the naming to be more general, such as  "_TO_HOST", and 
>keep the use cases (e.g. guest <-> host / nested / sibling VMs 
>communication) mention in the comments so that would relate more to 
>the motivation behind it.
>
>Andra
>
>>
>>That way it just tells the driver in which direction to send packets
>>without implying that sibling communication is possible (it's not
>>allowed by default on any transport).
>>
>>I don't have a strong opinion on this but wanted to suggest the idea.
>>
>>Stefan
>
>
>
>
>Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure
  2020-12-03 13:38       ` Stefano Garzarella
@ 2020-12-03 14:04         ` Paraschiv, Andra-Irina
  0 siblings, 0 replies; 21+ messages in thread
From: Paraschiv, Andra-Irina @ 2020-12-03 14:04 UTC (permalink / raw)
  To: Stefano Garzarella, Stefan Hajnoczi
  Cc: netdev, linux-kernel, David S . Miller, David Duncan, Dexuan Cui,
	Alexander Graf, Jorgen Hansen, Jakub Kicinski, Vitaly Kuznetsov



On 03/12/2020 15:38, Stefano Garzarella wrote:
>
> On Thu, Dec 03, 2020 at 12:32:08PM +0200, Paraschiv, Andra-Irina wrote:
>>
>>
>> On 03/12/2020 11:21, Stefan Hajnoczi wrote:
>>> On Tue, Dec 01, 2020 at 05:25:03PM +0200, Andra Paraschiv wrote:
>>>> vsock enables communication between virtual machines and the host they
>>>> are running on. With the multi transport support (guest->host and
>>>> host->guest), nested VMs can also use vsock channels for 
>>>> communication.
>>>>
>>>> In addition to this, by default, all the vsock packets are 
>>>> forwarded to
>>>> the host, if no host->guest transport is loaded. This behavior can be
>>>> implicitly used for enabling vsock communication between sibling VMs.
>>>>
>>>> Add a flag field in the vsock address data structure that can be 
>>>> used to
>>>> explicitly mark the vsock connection as being targeted for a certain
>>>> type of communication. This way, can distinguish between nested VMs 
>>>> and
>>>> sibling VMs use cases and can also setup them at the same time. Till
>>>> now, could either have nested VMs or sibling VMs at a time using the
>>>> vsock communication stack.
>>>>
>>>> Use the already available "svm_reserved1" field and mark it as a flag
>>>> field instead. This flag can be set when initializing the vsock 
>>>> address
>>>> variable used for the connect() call.
>>>>
>>>> Signed-off-by: Andra Paraschiv <andraprs@amazon.com>
>>>> ---
>>>>  include/uapi/linux/vm_sockets.h | 18 +++++++++++++++++-
>>>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/uapi/linux/vm_sockets.h 
>>>> b/include/uapi/linux/vm_sockets.h
>>>> index fd0ed7221645d..58da5a91413ac 100644
>>>> --- a/include/uapi/linux/vm_sockets.h
>>>> +++ b/include/uapi/linux/vm_sockets.h
>>>> @@ -114,6 +114,22 @@
>>>>  #define VMADDR_CID_HOST 2
>>>> +/* This sockaddr_vm flag value covers the current default use case:
>>>> + * local vsock communication between guest and host and nested VMs 
>>>> setup.
>>>> + * In addition to this, implicitly, the vsock packets are 
>>>> forwarded to the host
>>>> + * if no host->guest vsock transport is set.
>>>> + */
>>>> +#define VMADDR_FLAG_DEFAULT_COMMUNICATION   0x0000
>>>> +
>>>> +/* Set this flag value in the sockaddr_vm corresponding field if 
>>>> the vsock
>>>> + * channel needs to be setup between two sibling VMs running on 
>>>> the same host.
>>>> + * This way can explicitly distinguish between vsock channels 
>>>> created for nested
>>>> + * VMs (or local communication between guest and host) and the 
>>>> ones created for
>>>> + * sibling VMs. And vsock channels for multiple use cases (nested 
>>>> / sibling VMs)
>>>> + * can be setup at the same time.
>>>> + */
>>>> +#define VMADDR_FLAG_SIBLING_VMS_COMMUNICATION       0x0001
>>> vsock has the h2g and g2h concept. It would be more general to call 
>>> this
>>> flag VMADDR_FLAG_G2H or less cryptically VMADDR_FLAG_TO_HOST.
>
> I agree, VMADDR_FLAG_TO_HOST is more general and it's clearer that is up
> to the host where to forward the packet (sibling if supported, or
> whatever).

Ok, then VMADDR_FLAG_TO_HOST it is. :) I also updated the commit 
messages / comments to reflect this more general angle, with one of the 
current use cases being guest to guest communication.

Thanks,
Andra

>
>>
>> Thanks for the feedback, Stefan.
>>
>> I can update the naming to be more general, such as "_TO_HOST", and
>> keep the use cases (e.g. guest <-> host / nested / sibling VMs
>> communication) mention in the comments so that would relate more to
>> the motivation behind it.
>>
>> Andra
>>
>>>
>>> That way it just tells the driver in which direction to send packets
>>> without implying that sibling communication is possible (it's not
>>> allowed by default on any transport).
>>>
>>> I don't have a strong opinion on this but wanted to suggest the idea.
>>>
>>> Stefan
>>
>>
>>
>>
>> Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. 
>> Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. 
>> Registered in Romania. Registration number J22/2621/2005.
>>
>




Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-12-03 14:06 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 15:25 [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Andra Paraschiv
2020-12-01 15:25 ` [PATCH net-next v1 1/3] vm_sockets: Include flag field in the vsock address data structure Andra Paraschiv
2020-12-01 16:09   ` Stefano Garzarella
2020-12-01 18:15     ` Paraschiv, Andra-Irina
2020-12-02  8:32       ` Stefano Garzarella
2020-12-03  9:21   ` Stefan Hajnoczi
2020-12-03 10:32     ` Paraschiv, Andra-Irina
2020-12-03 13:38       ` Stefano Garzarella
2020-12-03 14:04         ` Paraschiv, Andra-Irina
2020-12-01 15:25 ` [PATCH net-next v1 2/3] virtio_transport_common: Set sibling VMs flag on the receive path Andra Paraschiv
2020-12-01 16:22   ` Stefano Garzarella
2020-12-01 19:01     ` Paraschiv, Andra-Irina
2020-12-02  8:53       ` Stefano Garzarella
2020-12-01 15:25 ` [PATCH net-next v1 3/3] af_vsock: Assign the vsock transport considering the vsock address flag Andra Paraschiv
2020-12-01 16:23   ` Stefano Garzarella
2020-12-01 19:06     ` Paraschiv, Andra-Irina
2020-12-01 16:27 ` [PATCH net-next v1 0/3] vsock: Add flag field in the vsock address Stefano Garzarella
2020-12-01 18:02   ` Paraschiv, Andra-Irina
2020-12-02 13:37 ` Stefano Garzarella
2020-12-02 16:18   ` Paraschiv, Andra-Irina
2020-12-03  8:51     ` Stefano Garzarella

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).