[PATCH 0/2] Drivers: hv: vmbus: Fix a couple of bugs

[PATCH 0/2] Drivers: hv: vmbus: Fix a couple of bugs

[K. Y. Srinivasan]

Fix a couple of bugs.

Andrey Smetanin (1):
  drivers/hv: correct tsc page sequence invalid value

K. Y. Srinivasan (1):
  Drivers: hv: vmbus: Fix a Host signaling bug

 drivers/hv/channel.c   |   18 ++++++++++++++++++
 drivers/hv/hv.c        |    4 ++--
 include/linux/hyperv.h |   12 ++++++++++++
 3 files changed, 32 insertions(+), 2 deletions(-)

-- 
1.7.4.1

[PATCH 1/2] Drivers: hv: vmbus: Fix a Host signaling bug

[K. Y. Srinivasan]

Currently we have two policies for deciding when to signal the host:
One based on the ring buffer state and the other based on what the
VMBUS client driver wants to do. Consider the case when the client
wants to explicitly control when to signal the host. In this case,
if the client were to defer signaling, we will not be able to signal
the host subsequently when the client does want to signal since the
ring buffer state will prevent the signaling. Implement logic to
have only one signaling policy in force for a given channel.

Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Tested-by: Haiyang Zhang <haiyangz@microsoft.com>
Cc: <stable@vger.kernel.org> # v4.2+
---
 drivers/hv/channel.c   |   18 ++++++++++++++++++
 include/linux/hyperv.h |   12 ++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 77d2579..c6278c7 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -653,10 +653,19 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer,
 	 *    on the ring. We will not signal if more data is
 	 *    to be placed.
 	 *
+	 * Based on the channel signal state, we will decide
+	 * which signaling policy will be applied.
+	 *
 	 * If we cannot write to the ring-buffer; signal the host
 	 * even if we may not have written anything. This is a rare
 	 * enough condition that it should not matter.
 	 */
+
+	if (channel->signal_state)
+		signal = true;
+	else
+		kick_q = true;
+
 	if (((ret == 0) && kick_q && signal) || (ret))
 		vmbus_setevent(channel);
 
@@ -756,10 +765,19 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
 	 *    on the ring. We will not signal if more data is
 	 *    to be placed.
 	 *
+	 * Based on the channel signal state, we will decide
+	 * which signaling policy will be applied.
+	 *
 	 * If we cannot write to the ring-buffer; signal the host
 	 * even if we may not have written anything. This is a rare
 	 * enough condition that it should not matter.
 	 */
+
+	if (channel->signal_state)
+		signal = true;
+	else
+		kick_q = true;
+
 	if (((ret == 0) && kick_q && signal) || (ret))
 		vmbus_setevent(channel);
 
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 437c9c8..7b1af52 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -756,8 +756,20 @@ struct vmbus_channel {
 	 * link up channels based on their CPU affinity.
 	 */
 	struct list_head percpu_list;
+	/*
+	 * Host signaling policy: The default policy will be
+	 * based on the ring buffer state. We will also support
+	 * a policy where the client driver can have explicit
+	 * signaling control.
+	 */
+	bool signal_state;
 };
 
+static inline void set_channel_signal_state(struct vmbus_channel *c, bool state)
+{
+	c->signal_state = state;
+}
+
 static inline void set_channel_read_state(struct vmbus_channel *c, bool state)
 {
 	c->batched_reading = state;
-- 
1.7.4.1

[PATCH 2/2] drivers/hv: correct tsc page sequence invalid value

[K. Y. Srinivasan]

From: Andrey Smetanin <asmetanin@virtuozzo.com>

Hypervisor Top Level Functional Specification v3/4 says
that TSC page sequence value = -1(0xFFFFFFFF) is used to
indicate that TSC page no longer reliable source of reference
timer. Unfortunately, we found that Windows Hyper-V guest
side implementation uses sequence value = 0 to indicate
that Tsc page no longer valid. This is clearly visible
inside Windows 2012R2 ntoskrnl.exe HvlGetReferenceTime()
function dissassembly:

HvlGetReferenceTime proc near
                 xchg    ax, ax
loc_1401C3132:
                 mov     rax, cs:HvlpReferenceTscPage
                 mov     r9d, [rax]
                 test    r9d, r9d
                 jz      short loc_1401C3176
                 rdtsc
                 mov     rcx, cs:HvlpReferenceTscPage
                 shl     rdx, 20h
                 or      rdx, rax
                 mov     rax, [rcx+8]
                 mov     rcx, cs:HvlpReferenceTscPage
                 mov     r8, [rcx+10h]
                 mul     rdx
                 mov     rax, cs:HvlpReferenceTscPage
                 add     rdx, r8
                 mov     ecx, [rax]
                 cmp     ecx, r9d
                 jnz     short loc_1401C3132
                 jmp     short loc_1401C3184
loc_1401C3176:
                 mov     ecx, 40000020h
                 rdmsr
                 shl     rdx, 20h
                 or      rdx, rax
loc_1401C3184:
                 mov     rax, rdx
                 retn
HvlGetReferenceTime endp

This patch aligns Tsc page invalid sequence value with
Windows Hyper-V guest implementation which is more
compatible with both Hyper-V hypervisor and KVM hypervisor.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: "K. Y. Srinivasan" <kys@microsoft.com>
CC: Haiyang Zhang <haiyangz@microsoft.com>
CC: Vitaly Kuznetsov <vkuznets@redhat.com>

Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
---
 drivers/hv/hv.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 7a06933..1db9556 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -140,7 +140,7 @@ static cycle_t read_hv_clock_tsc(struct clocksource *arg)
 	cycle_t current_tick;
 	struct ms_hyperv_tsc_page *tsc_pg = hv_context.tsc_page;
 
-	if (tsc_pg->tsc_sequence != -1) {
+	if (tsc_pg->tsc_sequence != 0) {
 		/*
 		 * Use the tsc page to compute the value.
 		 */
@@ -162,7 +162,7 @@ static cycle_t read_hv_clock_tsc(struct clocksource *arg)
 			if (tsc_pg->tsc_sequence == sequence)
 				return current_tick;
 
-			if (tsc_pg->tsc_sequence != -1)
+			if (tsc_pg->tsc_sequence != 0)
 				continue;
 			/*
 			 * Fallback using MSR method.
-- 
1.7.4.1

Re: [PATCH 1/2] Drivers: hv: vmbus: Fix a Host signaling bug

[Vitaly Kuznetsov]

"K. Y. Srinivasan" <kys@microsoft.com> writes:

> Currently we have two policies for deciding when to signal the host:
> One based on the ring buffer state and the other based on what the
> VMBUS client driver wants to do. Consider the case when the client
> wants to explicitly control when to signal the host. In this case,
> if the client were to defer signaling, we will not be able to signal
> the host subsequently when the client does want to signal since the
> ring buffer state will prevent the signaling. Implement logic to
> have only one signaling policy in force for a given channel.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Tested-by: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: <stable@vger.kernel.org> # v4.2+
> ---
>  drivers/hv/channel.c   |   18 ++++++++++++++++++
>  include/linux/hyperv.h |   12 ++++++++++++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 77d2579..c6278c7 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -653,10 +653,19 @@ int vmbus_sendpacket_ctl(struct vmbus_channel *channel, void *buffer,
>  	 *    on the ring. We will not signal if more data is
>  	 *    to be placed.
>  	 *
> +	 * Based on the channel signal state, we will decide
> +	 * which signaling policy will be applied.
> +	 *
>  	 * If we cannot write to the ring-buffer; signal the host
>  	 * even if we may not have written anything. This is a rare
>  	 * enough condition that it should not matter.
>  	 */
> +
> +	if (channel->signal_state)
> +		signal = true;
> +	else
> +		kick_q = true;
> +
>  	if (((ret == 0) && kick_q && signal) || (ret))
>  		vmbus_setevent(channel);
>
> @@ -756,10 +765,19 @@ int vmbus_sendpacket_pagebuffer_ctl(struct vmbus_channel *channel,
>  	 *    on the ring. We will not signal if more data is
>  	 *    to be placed.
>  	 *
> +	 * Based on the channel signal state, we will decide
> +	 * which signaling policy will be applied.
> +	 *
>  	 * If we cannot write to the ring-buffer; signal the host
>  	 * even if we may not have written anything. This is a rare
>  	 * enough condition that it should not matter.
>  	 */
> +
> +	if (channel->signal_state)
> +		signal = true;
> +	else
> +		kick_q = true;
> +
>  	if (((ret == 0) && kick_q && signal) || (ret))
>  		vmbus_setevent(channel);
>
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 437c9c8..7b1af52 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -756,8 +756,20 @@ struct vmbus_channel {
>  	 * link up channels based on their CPU affinity.
>  	 */
>  	struct list_head percpu_list;
> +	/*
> +	 * Host signaling policy: The default policy will be
> +	 * based on the ring buffer state. We will also support
> +	 * a policy where the client driver can have explicit
> +	 * signaling control.
> +	 */
> +	bool signal_state;

Making policy selector 'bool' is counter-intuitive: I suggest we rename
this to somethink like 'signal_explicit' if we're sure there won't be
new policies or (even better in my opinion) introduce a new enum with
these policies, e.g:

       enum hv_channel_policy signal_policy;

where

enum hv_channel_policy {
      HV_CHANNELPOLICY_DEFAULT,
      HV_CHANNELPOLICY_EXPLICIT,
};

>  };
>
> +static inline void set_channel_signal_state(struct vmbus_channel *c, bool state)
> +{
> +	c->signal_state = state;
> +}
> +
>  static inline void set_channel_read_state(struct vmbus_channel *c, bool state)
>  {
>  	c->batched_reading = state;

-- 
  Vitaly