Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements
@ 2019-10-07 16:31 Andrea Parri
  2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-07 16:31 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Michael Kelley, Vitaly Kuznetsov, Andrea Parri

Hi all,

The patchset:

- simplifies/refactors the VMBus negotiation code by introducing
  the table of VMBus protocol versions (patch 1/2),

- enables VMBus protocol versions 5.1 and 5.2 (patch 2/2).

Thanks,
  Andrea

Andrea Parri (2):
  Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2

 drivers/hv/connection.c | 63 +++++++++++++++++------------------------
 include/linux/hyperv.h  |  6 ++--
 2 files changed, 30 insertions(+), 39 deletions(-)

-- 
2.23.0


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

* [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-07 16:31 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Andrea Parri
@ 2019-10-07 16:31 ` Andrea Parri
  2019-10-07 17:14   ` Vitaly Kuznetsov
  2019-10-07 17:25   ` Dexuan Cui
  2019-10-07 16:31 ` [PATCH 2/2] Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2 Andrea Parri
  2019-10-07 17:41 ` [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Dexuan Cui
  2 siblings, 2 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-07 16:31 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Michael Kelley, Vitaly Kuznetsov, Andrea Parri

The technique used to get the next VMBus version seems increasisly
clumsy as the number of VMBus versions increases.  Performance is
not a concern since this is only done once during system boot; it's
just that we'll end up with more lines of code than is really needed.

As an alternative, introduce a table with the version numbers listed
in order (from the most recent to the oldest).  vmbus_connect() loops
through the versions listed in the table until it gets an accepted
connection or gets to the end of the table (invalid version).

Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
 drivers/hv/connection.c | 51 +++++++++++++++--------------------------
 include/linux/hyperv.h  |  2 --
 2 files changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 6e4c015783ffc..90a32c9d79403 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -40,29 +40,19 @@ EXPORT_SYMBOL_GPL(vmbus_connection);
 __u32 vmbus_proto_version;
 EXPORT_SYMBOL_GPL(vmbus_proto_version);
 
-static __u32 vmbus_get_next_version(__u32 current_version)
-{
-	switch (current_version) {
-	case (VERSION_WIN7):
-		return VERSION_WS2008;
-
-	case (VERSION_WIN8):
-		return VERSION_WIN7;
-
-	case (VERSION_WIN8_1):
-		return VERSION_WIN8;
-
-	case (VERSION_WIN10):
-		return VERSION_WIN8_1;
-
-	case (VERSION_WIN10_V5):
-		return VERSION_WIN10;
-
-	case (VERSION_WS2008):
-	default:
-		return VERSION_INVAL;
-	}
-}
+/*
+ * Table of VMBus versions listed from newest to oldest; the table
+ * must terminate with VERSION_INVAL.
+ */
+__u32 vmbus_versions[] = {
+	VERSION_WIN10_V5,
+	VERSION_WIN10,
+	VERSION_WIN8_1,
+	VERSION_WIN8,
+	VERSION_WIN7,
+	VERSION_WS2008,
+	VERSION_INVAL
+};
 
 int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
 {
@@ -169,8 +159,8 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
  */
 int vmbus_connect(void)
 {
-	int ret = 0;
 	struct vmbus_channel_msginfo *msginfo = NULL;
+	int i, ret = 0;
 	__u32 version;
 
 	/* Initialize the vmbus connection */
@@ -244,21 +234,18 @@ int vmbus_connect(void)
 	 * version.
 	 */
 
-	version = VERSION_CURRENT;
+	for (i = 0; ; i++) {
+		version = vmbus_versions[i];
+		if (version == VERSION_INVAL)
+			goto cleanup;
 
-	do {
 		ret = vmbus_negotiate_version(msginfo, version);
 		if (ret == -ETIMEDOUT)
 			goto cleanup;
 
 		if (vmbus_connection.conn_state == CONNECTED)
 			break;
-
-		version = vmbus_get_next_version(version);
-	} while (version != VERSION_INVAL);
-
-	if (version == VERSION_INVAL)
-		goto cleanup;
+	}
 
 	vmbus_proto_version = version;
 	pr_info("Vmbus version:%d.%d\n",
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index b4a017093b697..7073f1eb3618c 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -194,8 +194,6 @@ static inline u32 hv_get_avail_to_write_percent(
 
 #define VERSION_INVAL -1
 
-#define VERSION_CURRENT VERSION_WIN10_V5
-
 /* Make maximum size of pipe payload of 16K */
 #define MAX_PIPE_DATA_PAYLOAD		(sizeof(u8) * 16384)
 
-- 
2.23.0


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

* [PATCH 2/2] Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2
  2019-10-07 16:31 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Andrea Parri
  2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
@ 2019-10-07 16:31 ` Andrea Parri
  2019-10-07 17:41 ` [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Dexuan Cui
  2 siblings, 0 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-07 16:31 UTC (permalink / raw)
  To: linux-kernel, linux-hyperv
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Michael Kelley, Vitaly Kuznetsov, Andrea Parri

Hyper-V has added VMBus protocol versions 5.1 and 5.2 in recent release
versions.  Allow Linux guests to negotiate these new protocol versions
on versions of Hyper-V that support them.

Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
---
 drivers/hv/connection.c | 12 +++++++-----
 include/linux/hyperv.h  |  4 ++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 90a32c9d79403..d05fef3e09080 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -45,6 +45,8 @@ EXPORT_SYMBOL_GPL(vmbus_proto_version);
  * must terminate with VERSION_INVAL.
  */
 __u32 vmbus_versions[] = {
+	VERSION_WIN10_V5_2,
+	VERSION_WIN10_V5_1,
 	VERSION_WIN10_V5,
 	VERSION_WIN10,
 	VERSION_WIN8_1,
@@ -70,12 +72,12 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
 	msg->vmbus_version_requested = version;
 
 	/*
-	 * VMBus protocol 5.0 (VERSION_WIN10_V5) requires that we must use
-	 * VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate Contact Message,
+	 * VMBus protocol 5.0 (VERSION_WIN10_V5) and higher require that we must
+	 * use VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate Contact Message,
 	 * and for subsequent messages, we must use the Message Connection ID
 	 * field in the host-returned Version Response Message. And, with
-	 * VERSION_WIN10_V5, we don't use msg->interrupt_page, but we tell
-	 * the host explicitly that we still use VMBUS_MESSAGE_SINT(2) for
+	 * VERSION_WIN10_V5 and higher, we don't use msg->interrupt_page, but we
+	 * tell the host explicitly that we still use VMBUS_MESSAGE_SINT(2) for
 	 * compatibility.
 	 *
 	 * On old hosts, we should always use VMBUS_MESSAGE_CONNECTION_ID (1).
@@ -400,7 +402,7 @@ int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep)
 		case HV_STATUS_INVALID_CONNECTION_ID:
 			/*
 			 * See vmbus_negotiate_version(): VMBus protocol 5.0
-			 * requires that we must use
+			 * and higher require that we must use
 			 * VMBUS_MESSAGE_CONNECTION_ID_4 for the Initiate
 			 * Contact message, but on old hosts that only
 			 * support VMBus protocol 4.0 or lower, here we get
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 7073f1eb3618c..5ecb2ff7cc25d 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -183,6 +183,8 @@ static inline u32 hv_get_avail_to_write_percent(
  * 3 . 0  (Windows 8 R2)
  * 4 . 0  (Windows 10)
  * 5 . 0  (Newer Windows 10)
+ * 5 . 1  (Windows 10 RS4)
+ * 5 . 2  (Windows Server 2019, RS5)
  */
 
 #define VERSION_WS2008  ((0 << 16) | (13))
@@ -191,6 +193,8 @@ static inline u32 hv_get_avail_to_write_percent(
 #define VERSION_WIN8_1    ((3 << 16) | (0))
 #define VERSION_WIN10	((4 << 16) | (0))
 #define VERSION_WIN10_V5 ((5 << 16) | (0))
+#define VERSION_WIN10_V5_1 ((5 << 16) | (1))
+#define VERSION_WIN10_V5_2 ((5 << 16) | (2))
 
 #define VERSION_INVAL -1
 
-- 
2.23.0


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

* Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
@ 2019-10-07 17:14   ` Vitaly Kuznetsov
  2019-10-08 12:41     ` Andrea Parri
  2019-10-07 17:25   ` Dexuan Cui
  1 sibling, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2019-10-07 17:14 UTC (permalink / raw)
  To: Andrea Parri, linux-kernel, linux-hyperv
  Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Sasha Levin, Michael Kelley, Andrea Parri

Andrea Parri <parri.andrea@gmail.com> writes:

> The technique used to get the next VMBus version seems increasisly
> clumsy as the number of VMBus versions increases.  Performance is
> not a concern since this is only done once during system boot; it's
> just that we'll end up with more lines of code than is really needed.
>
> As an alternative, introduce a table with the version numbers listed
> in order (from the most recent to the oldest).  vmbus_connect() loops
> through the versions listed in the table until it gets an accepted
> connection or gets to the end of the table (invalid version).
>
> Suggested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Andrea Parri <parri.andrea@gmail.com>
> ---
>  drivers/hv/connection.c | 51 +++++++++++++++--------------------------
>  include/linux/hyperv.h  |  2 --
>  2 files changed, 19 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 6e4c015783ffc..90a32c9d79403 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -40,29 +40,19 @@ EXPORT_SYMBOL_GPL(vmbus_connection);
>  __u32 vmbus_proto_version;
>  EXPORT_SYMBOL_GPL(vmbus_proto_version);
>  
> -static __u32 vmbus_get_next_version(__u32 current_version)
> -{
> -	switch (current_version) {
> -	case (VERSION_WIN7):
> -		return VERSION_WS2008;
> -
> -	case (VERSION_WIN8):
> -		return VERSION_WIN7;
> -
> -	case (VERSION_WIN8_1):
> -		return VERSION_WIN8;
> -
> -	case (VERSION_WIN10):
> -		return VERSION_WIN8_1;
> -
> -	case (VERSION_WIN10_V5):
> -		return VERSION_WIN10;
> -
> -	case (VERSION_WS2008):
> -	default:
> -		return VERSION_INVAL;
> -	}
> -}
> +/*
> + * Table of VMBus versions listed from newest to oldest; the table
> + * must terminate with VERSION_INVAL.
> + */
> +__u32 vmbus_versions[] = {
> +	VERSION_WIN10_V5,
> +	VERSION_WIN10,
> +	VERSION_WIN8_1,
> +	VERSION_WIN8,
> +	VERSION_WIN7,
> +	VERSION_WS2008,
> +	VERSION_INVAL
> +};
>  
>  int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
>  {
> @@ -169,8 +159,8 @@ int vmbus_negotiate_version(struct vmbus_channel_msginfo *msginfo, u32 version)
>   */
>  int vmbus_connect(void)
>  {
> -	int ret = 0;
>  	struct vmbus_channel_msginfo *msginfo = NULL;
> +	int i, ret = 0;
>  	__u32 version;
>  
>  	/* Initialize the vmbus connection */
> @@ -244,21 +234,18 @@ int vmbus_connect(void)
>  	 * version.
>  	 */
>  
> -	version = VERSION_CURRENT;
> +	for (i = 0; ; i++) {
> +		version = vmbus_versions[i];
> +		if (version == VERSION_INVAL)
> +			goto cleanup;

If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
this code look more natural.
>  
> -	do {
>  		ret = vmbus_negotiate_version(msginfo, version);
>  		if (ret == -ETIMEDOUT)
>  			goto cleanup;
>  
>  		if (vmbus_connection.conn_state == CONNECTED)
>  			break;
> -
> -		version = vmbus_get_next_version(version);
> -	} while (version != VERSION_INVAL);
> -
> -	if (version == VERSION_INVAL)
> -		goto cleanup;
> +	}
>  
>  	vmbus_proto_version = version;
>  	pr_info("Vmbus version:%d.%d\n",
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index b4a017093b697..7073f1eb3618c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -194,8 +194,6 @@ static inline u32 hv_get_avail_to_write_percent(
>  
>  #define VERSION_INVAL -1
>  
> -#define VERSION_CURRENT VERSION_WIN10_V5
> -
>  /* Make maximum size of pipe payload of 16K */
>  #define MAX_PIPE_DATA_PAYLOAD		(sizeof(u8) * 16384)

-- 
Vitaly


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

* RE: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
  2019-10-07 17:14   ` Vitaly Kuznetsov
@ 2019-10-07 17:25   ` Dexuan Cui
  2019-10-08 12:42     ` Andrea Parri
  1 sibling, 1 reply; 14+ messages in thread
From: Dexuan Cui @ 2019-10-07 17:25 UTC (permalink / raw)
  To: Andrea Parri, linux-kernel, linux-hyperv
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	Michael Kelley, vkuznets

> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andrea Parri
> Sent: Monday, October 7, 2019 9:31 AM
> ....
> +/*
> + * Table of VMBus versions listed from newest to oldest; the table
> + * must terminate with VERSION_INVAL.
> + */
> +__u32 vmbus_versions[] = {
> +	VERSION_WIN10_V5,

This should be "static"?
 
Thanks,
Dexuan

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

* RE: [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements
  2019-10-07 16:31 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Andrea Parri
  2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
  2019-10-07 16:31 ` [PATCH 2/2] Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2 Andrea Parri
@ 2019-10-07 17:41 ` Dexuan Cui
  2019-10-08 15:08   ` Andrea Parri
  2 siblings, 1 reply; 14+ messages in thread
From: Dexuan Cui @ 2019-10-07 17:41 UTC (permalink / raw)
  To: Andrea Parri, linux-kernel, linux-hyperv
  Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	Michael Kelley, vkuznets

> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andrea Parri
> Sent: Monday, October 7, 2019 9:31 AM
> 
> Hi all,
> 
> The patchset:
> 
> - simplifies/refactors the VMBus negotiation code by introducing
>   the table of VMBus protocol versions (patch 1/2),
> 
> - enables VMBus protocol versions 5.1 and 5.2 (patch 2/2).
> 
> Thanks,
>   Andrea
> 
> Andrea Parri (2):
>   Drivers: hv: vmbus: Introduce table of VMBus protocol versions
>   Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2

Should we add a module parameter to allow the user to specify a lower
protocol version, when the VM runs on the latest host? 

This can be useful for testing and debugging purpose: the variable
"vmbus_proto_version" is referenced by the vmbus driver itself and
some VSC drivers: if we always use the latest available proto version,
some code paths can not be tested on the latest hosts. 

Thanks,
-- Dexuan

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

* Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-07 17:14   ` Vitaly Kuznetsov
@ 2019-10-08 12:41     ` Andrea Parri
  2019-10-08 12:44       ` Andrea Parri
  2019-10-08 13:00       ` Vitaly Kuznetsov
  0 siblings, 2 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-08 12:41 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: linux-kernel, linux-hyperv, K . Y . Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Michael Kelley, Dexuan Cui

> > @@ -244,21 +234,18 @@ int vmbus_connect(void)
> >  	 * version.
> >  	 */
> >  
> > -	version = VERSION_CURRENT;
> > +	for (i = 0; ; i++) {
> > +		version = vmbus_versions[i];
> > +		if (version == VERSION_INVAL)
> > +			goto cleanup;
> 
> If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
> this code look more natural.

Thank you for pointing this out, Vitaly.

IIUC, you're suggesting that I do:

	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
		version = vmbus_versions[i];

		ret = vmbus_negotiate_version(msginfo, version);
		if (ret == -ETIMEDOUT)
			goto cleanup;

		if (vmbus_connection.conn_state == CONNECTED)
			break;
	}

	if (vmbus_connection.conn_state != CONNECTED)
		break;

and that I remove VERSION_INVAL from vmbus_versions, yes?

Looking at the uses of VERSION_INVAL, I find one remaining occurrence
of this macro in vmbus_bus_resume(), which does:

	if (vmbus_proto_version == VERSION_INVAL ||
	    vmbus_proto_version == 0) {
		...
	}

TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
first time and I'm not sure about how to change such check yet... any
suggestions?

Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
vmbus_connection.conn_state: can such accesses race with a concurrent
vmbus_connect()?

Thanks,
  Andrea


> >  
> > -	do {
> >  		ret = vmbus_negotiate_version(msginfo, version);
> >  		if (ret == -ETIMEDOUT)
> >  			goto cleanup;
> >  
> >  		if (vmbus_connection.conn_state == CONNECTED)
> >  			break;
> > -
> > -		version = vmbus_get_next_version(version);
> > -	} while (version != VERSION_INVAL);
> > -
> > -	if (version == VERSION_INVAL)
> > -		goto cleanup;
> > +	}
> >  
> >  	vmbus_proto_version = version;
> >  	pr_info("Vmbus version:%d.%d\n",

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

* Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-07 17:25   ` Dexuan Cui
@ 2019-10-08 12:42     ` Andrea Parri
  0 siblings, 0 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-08 12:42 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: linux-kernel, linux-hyperv, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Michael Kelley, vkuznets

On Mon, Oct 07, 2019 at 05:25:18PM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andrea Parri
> > Sent: Monday, October 7, 2019 9:31 AM
> > ....
> > +/*
> > + * Table of VMBus versions listed from newest to oldest; the table
> > + * must terminate with VERSION_INVAL.
> > + */
> > +__u32 vmbus_versions[] = {
> > +	VERSION_WIN10_V5,
> 
> This should be "static"?

I think so, will add in v2.  Thank you for pointing this out, Dexuan.

  Andrea

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

* Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-08 12:41     ` Andrea Parri
@ 2019-10-08 12:44       ` Andrea Parri
  2019-10-08 13:00       ` Vitaly Kuznetsov
  1 sibling, 0 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-08 12:44 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: linux-kernel, linux-hyperv, K . Y . Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Michael Kelley, Dexuan Cui

> IIUC, you're suggesting that I do:
> 
> 	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> 		version = vmbus_versions[i];
> 
> 		ret = vmbus_negotiate_version(msginfo, version);
> 		if (ret == -ETIMEDOUT)
> 			goto cleanup;
> 
> 		if (vmbus_connection.conn_state == CONNECTED)
> 			break;
> 	}
> 
> 	if (vmbus_connection.conn_state != CONNECTED)
> 		break;

This "break" should have been "goto cleanup".  Sorry.

  Andrea

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

* Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-08 12:41     ` Andrea Parri
  2019-10-08 12:44       ` Andrea Parri
@ 2019-10-08 13:00       ` Vitaly Kuznetsov
  2019-10-08 22:41         ` Dexuan Cui
  1 sibling, 1 reply; 14+ messages in thread
From: Vitaly Kuznetsov @ 2019-10-08 13:00 UTC (permalink / raw)
  To: Andrea Parri
  Cc: linux-kernel, linux-hyperv, K . Y . Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Michael Kelley, Dexuan Cui

Andrea Parri <parri.andrea@gmail.com> writes:

>> > @@ -244,21 +234,18 @@ int vmbus_connect(void)
>> >  	 * version.
>> >  	 */
>> >  
>> > -	version = VERSION_CURRENT;
>> > +	for (i = 0; ; i++) {
>> > +		version = vmbus_versions[i];
>> > +		if (version == VERSION_INVAL)
>> > +			goto cleanup;
>> 
>> If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
>> this code look more natural.
>
> Thank you for pointing this out, Vitaly.
>
> IIUC, you're suggesting that I do:
>
> 	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> 		version = vmbus_versions[i];
>
> 		ret = vmbus_negotiate_version(msginfo, version);
> 		if (ret == -ETIMEDOUT)
> 			goto cleanup;
>
> 		if (vmbus_connection.conn_state == CONNECTED)
> 			break;
> 	}
>
> 	if (vmbus_connection.conn_state != CONNECTED)
> 		break;
>
> and that I remove VERSION_INVAL from vmbus_versions, yes?
>

Yes, something like that.

> Looking at the uses of VERSION_INVAL, I find one remaining occurrence
> of this macro in vmbus_bus_resume(), which does:
>
> 	if (vmbus_proto_version == VERSION_INVAL ||
> 	    vmbus_proto_version == 0) {
> 		...
> 	}
>
> TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
> first time and I'm not sure about how to change such check yet... any
> suggestions?

Hm, I don't think vmbus_proto_version can ever become == VERSION_INVAL
if we rewrite the code the way you suggest, right? So you'll reduce this
to 'if (!vmbus_proto_version)' meaning we haven't negotiated any version
(yet).

>
> Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
> vmbus_connection.conn_state: can such accesses race with a concurrent
> vmbus_connect()?

Let's summon Dexuan who's the author! :-) 

>
> Thanks,
>   Andrea
>
>
>> >  
>> > -	do {
>> >  		ret = vmbus_negotiate_version(msginfo, version);
>> >  		if (ret == -ETIMEDOUT)
>> >  			goto cleanup;
>> >  
>> >  		if (vmbus_connection.conn_state == CONNECTED)
>> >  			break;
>> > -
>> > -		version = vmbus_get_next_version(version);
>> > -	} while (version != VERSION_INVAL);
>> > -
>> > -	if (version == VERSION_INVAL)
>> > -		goto cleanup;
>> > +	}
>> >  
>> >  	vmbus_proto_version = version;
>> >  	pr_info("Vmbus version:%d.%d\n",

-- 
Vitaly


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

* Re: [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements
  2019-10-07 17:41 ` [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Dexuan Cui
@ 2019-10-08 15:08   ` Andrea Parri
  2019-10-08 19:47     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 14+ messages in thread
From: Andrea Parri @ 2019-10-08 15:08 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: linux-kernel, linux-hyperv, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Michael Kelley, vkuznets

On Mon, Oct 07, 2019 at 05:41:10PM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andrea Parri
> > Sent: Monday, October 7, 2019 9:31 AM
> > 
> > Hi all,
> > 
> > The patchset:
> > 
> > - simplifies/refactors the VMBus negotiation code by introducing
> >   the table of VMBus protocol versions (patch 1/2),
> > 
> > - enables VMBus protocol versions 5.1 and 5.2 (patch 2/2).
> > 
> > Thanks,
> >   Andrea
> > 
> > Andrea Parri (2):
> >   Drivers: hv: vmbus: Introduce table of VMBus protocol versions
> >   Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2
> 
> Should we add a module parameter to allow the user to specify a lower
> protocol version, when the VM runs on the latest host? 
> 
> This can be useful for testing and debugging purpose: the variable
> "vmbus_proto_version" is referenced by the vmbus driver itself and
> some VSC drivers: if we always use the latest available proto version,
> some code paths can not be tested on the latest hosts. 

The idea is appealing to me (altough I made no attempt to implement/test
it yet).  What do others think about this?  Maybe can be considered as a
follow-up patch/work to this series?

Thanks,
  Andrea

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

* Re: [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements
  2019-10-08 15:08   ` Andrea Parri
@ 2019-10-08 19:47     ` Vitaly Kuznetsov
  0 siblings, 0 replies; 14+ messages in thread
From: Vitaly Kuznetsov @ 2019-10-08 19:47 UTC (permalink / raw)
  To: Andrea Parri, Dexuan Cui
  Cc: linux-kernel\, linux-hyperv\,
	KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
	Michael Kelley

Andrea Parri <parri.andrea@gmail.com> writes:

> On Mon, Oct 07, 2019 at 05:41:10PM +0000, Dexuan Cui wrote:
>> > From: linux-hyperv-owner@vger.kernel.org
>> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Andrea Parri
>> > Sent: Monday, October 7, 2019 9:31 AM
>> > 
>> > Hi all,
>> > 
>> > The patchset:
>> > 
>> > - simplifies/refactors the VMBus negotiation code by introducing
>> >   the table of VMBus protocol versions (patch 1/2),
>> > 
>> > - enables VMBus protocol versions 5.1 and 5.2 (patch 2/2).
>> > 
>> > Thanks,
>> >   Andrea
>> > 
>> > Andrea Parri (2):
>> >   Drivers: hv: vmbus: Introduce table of VMBus protocol versions
>> >   Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2
>> 
>> Should we add a module parameter to allow the user to specify a lower
>> protocol version, when the VM runs on the latest host? 
>> 
>> This can be useful for testing and debugging purpose: the variable
>> "vmbus_proto_version" is referenced by the vmbus driver itself and
>> some VSC drivers: if we always use the latest available proto version,
>> some code paths can not be tested on the latest hosts. 
>
> The idea is appealing to me (altough I made no attempt to implement/test
> it yet).  What do others think about this?  Maybe can be considered as a
> follow-up patch/work to this series?

IMO such debug option makes sense, it shouldn be a simple patch so you
may want to squeeze it in this series as it will definitely have code
dependencies.

-- 
Vitaly


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

* RE: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-08 13:00       ` Vitaly Kuznetsov
@ 2019-10-08 22:41         ` Dexuan Cui
  2019-10-09  9:54           ` Andrea Parri
  0 siblings, 1 reply; 14+ messages in thread
From: Dexuan Cui @ 2019-10-08 22:41 UTC (permalink / raw)
  To: vkuznets, Andrea Parri
  Cc: linux-kernel, linux-hyperv, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Sasha Levin, Michael Kelley

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> Sent: Tuesday, October 8, 2019 6:00 AM
>  ...
> > Looking at the uses of VERSION_INVAL, I find one remaining occurrence
> > of this macro in vmbus_bus_resume(), which does:
> >
> > 	if (vmbus_proto_version == VERSION_INVAL ||
> > 	    vmbus_proto_version == 0) {
> > 		...
> > 	}
> >
> > TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
> > first time and I'm not sure about how to change such check yet... any
> > suggestions?
> 
> Hm, I don't think vmbus_proto_version can ever become == VERSION_INVAL
> if we rewrite the code the way you suggest, right? So you'll reduce this
> to 'if (!vmbus_proto_version)' meaning we haven't negotiated any version
> (yet).

Yeah, Vitaly is correct. The check may be a little paranoid as I believe 
"vmbus_proto_version" must be a negotiated value in vmbus_bus_resume()
and vmbus_bus_suspend().  I added the check just in case.

> > Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
> > vmbus_connection.conn_state: can such accesses race with a concurrent
> > vmbus_connect()?
> 
> Let's summon Dexuan who's the author! :-)

There should not be an issue:

vmbus_connect() is called in the early subsys_initcall(hv_acpi_init).

vmbus_bus_suspend() is called late in the PM code after the kernel boots up, e.g.
in the hibernation function hibernation_snapshot() -> dpm_suspend(). 

vmbus_bus_resume() is also called later in late_initcall_sync(software_resume).

In the hibernatin process, vmbus_bus_suspend()/resume() can also be called a
few times, and vmbus_bus_resume() calls vmbus_negotiate_version(). As I
checked, there is no issue, either.

Thanks,
Dexuan

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

* Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions
  2019-10-08 22:41         ` Dexuan Cui
@ 2019-10-09  9:54           ` Andrea Parri
  0 siblings, 0 replies; 14+ messages in thread
From: Andrea Parri @ 2019-10-09  9:54 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: vkuznets, linux-kernel, linux-hyperv, KY Srinivasan,
	Haiyang Zhang, Stephen Hemminger, Sasha Levin, Michael Kelley

On Tue, Oct 08, 2019 at 10:41:42PM +0000, Dexuan Cui wrote:
> > From: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Sent: Tuesday, October 8, 2019 6:00 AM
> >  ...
> > > Looking at the uses of VERSION_INVAL, I find one remaining occurrence
> > > of this macro in vmbus_bus_resume(), which does:
> > >
> > > 	if (vmbus_proto_version == VERSION_INVAL ||
> > > 	    vmbus_proto_version == 0) {
> > > 		...
> > > 	}
> > >
> > > TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
> > > first time and I'm not sure about how to change such check yet... any
> > > suggestions?
> > 
> > Hm, I don't think vmbus_proto_version can ever become == VERSION_INVAL
> > if we rewrite the code the way you suggest, right? So you'll reduce this
> > to 'if (!vmbus_proto_version)' meaning we haven't negotiated any version
> > (yet).
> 
> Yeah, Vitaly is correct. The check may be a little paranoid as I believe 
> "vmbus_proto_version" must be a negotiated value in vmbus_bus_resume()
> and vmbus_bus_suspend().  I added the check just in case.
> 
> > > Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
> > > vmbus_connection.conn_state: can such accesses race with a concurrent
> > > vmbus_connect()?
> > 
> > Let's summon Dexuan who's the author! :-)
> 
> There should not be an issue:
> 
> vmbus_connect() is called in the early subsys_initcall(hv_acpi_init).
> 
> vmbus_bus_suspend() is called late in the PM code after the kernel boots up, e.g.
> in the hibernation function hibernation_snapshot() -> dpm_suspend(). 
> 
> vmbus_bus_resume() is also called later in late_initcall_sync(software_resume).
> 
> In the hibernatin process, vmbus_bus_suspend()/resume() can also be called a
> few times, and vmbus_bus_resume() calls vmbus_negotiate_version(). As I
> checked, there is no issue, either.

Thank you both for these remarks.

So, I'll proceed with the removal of VERSION_INVAL in v2 of this series.

Thanks,
  Andrea

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

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-07 16:31 [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Andrea Parri
2019-10-07 16:31 ` [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions Andrea Parri
2019-10-07 17:14   ` Vitaly Kuznetsov
2019-10-08 12:41     ` Andrea Parri
2019-10-08 12:44       ` Andrea Parri
2019-10-08 13:00       ` Vitaly Kuznetsov
2019-10-08 22:41         ` Dexuan Cui
2019-10-09  9:54           ` Andrea Parri
2019-10-07 17:25   ` Dexuan Cui
2019-10-08 12:42     ` Andrea Parri
2019-10-07 16:31 ` [PATCH 2/2] Drivers: hv: vmbus: Enable VMBus protocol versions 5.1 and 5.2 Andrea Parri
2019-10-07 17:41 ` [PATCH 0/2] Drivers: hv: vmbus: Miscellaneous improvements Dexuan Cui
2019-10-08 15:08   ` Andrea Parri
2019-10-08 19:47     ` Vitaly Kuznetsov

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org linux-hyperv@archiver.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/ public-inbox