[PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[mhkelley58]

From: Michael Kelley <mikelley@microsoft.com>

The synic_initialized flag is part of the global hv_context
structure.  But the Hyper-V synthetic interrupt controller is
fundamentally a per-cpu device, and other synic related
fields are in hv_per_cpu_context.  In a multi-CPU system,
synic_initialized gets set multiple times, making the test in
hv_synic_cleanup() invalid.  Fix this by moving the flag to
hv_per_cpu_context and adjusting the references.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/hv/hv.c           | 16 +++++++---------
 drivers/hv/hyperv_vmbus.h |  4 ++--
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 312fe5e..8d4fe0e 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -33,9 +33,7 @@
 #include "hyperv_vmbus.h"
 
 /* The one and only */
-struct hv_context hv_context = {
-	.synic_initialized	= false,
-};
+struct hv_context hv_context;
 
 /*
  * If false, we're using the old mechanism for stimer0 interrupts
@@ -315,7 +313,7 @@ int hv_synic_init(unsigned int cpu)
 
 	hv_set_synic_state(sctrl.as_uint64);
 
-	hv_context.synic_initialized = true;
+	hv_cpu->synic_initialized = true;
 
 	/*
 	 * Register the per-cpu clockevent source.
@@ -354,6 +352,8 @@ void hv_synic_clockevents_cleanup(void)
  */
 int hv_synic_cleanup(unsigned int cpu)
 {
+	struct hv_per_cpu_context *hv_cpu
+		= per_cpu_ptr(hv_context.cpu_context, cpu);
 	union hv_synic_sint shared_sint;
 	union hv_synic_simp simp;
 	union hv_synic_siefp siefp;
@@ -362,7 +362,7 @@ int hv_synic_cleanup(unsigned int cpu)
 	bool channel_found = false;
 	unsigned long flags;
 
-	if (!hv_context.synic_initialized)
+	if (!hv_cpu->synic_initialized)
 		return -EFAULT;
 
 	/*
@@ -395,12 +395,8 @@ int hv_synic_cleanup(unsigned int cpu)
 
 	/* Turn off clockevent device */
 	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
-		struct hv_per_cpu_context *hv_cpu
-			= this_cpu_ptr(hv_context.cpu_context);
-
 		clockevents_unbind_device(hv_cpu->clk_evt, cpu);
 		hv_ce_shutdown(hv_cpu->clk_evt);
-		put_cpu_ptr(hv_cpu);
 	}
 
 	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
@@ -428,5 +424,7 @@ int hv_synic_cleanup(unsigned int cpu)
 	sctrl.enable = 0;
 	hv_set_synic_state(sctrl.as_uint64);
 
+	hv_cpu->synic_initialized = false;
+
 	return 0;
 }
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 72eaba3..eadd3df 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -202,6 +202,8 @@ enum {
 struct hv_per_cpu_context {
 	void *synic_message_page;
 	void *synic_event_page;
+	bool synic_initialized;
+
 	/*
 	 * buffer to post messages to the host.
 	 */
@@ -230,8 +232,6 @@ struct hv_context {
 
 	void *tsc_page;
 
-	bool synic_initialized;
-
 	struct hv_per_cpu_context __percpu *cpu_context;
 
 	/*
-- 
1.8.3.1

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[Vitaly Kuznetsov]

mhkelley58@gmail.com writes:

> From: Michael Kelley <mikelley@microsoft.com>
>
> The synic_initialized flag is part of the global hv_context
> structure.  But the Hyper-V synthetic interrupt controller is
> fundamentally a per-cpu device, and other synic related
> fields are in hv_per_cpu_context.  In a multi-CPU system,
> synic_initialized gets set multiple times, making the test in
> hv_synic_cleanup() invalid.  Fix this by moving the flag to
> hv_per_cpu_context and adjusting the references.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/hv/hv.c           | 16 +++++++---------
>  drivers/hv/hyperv_vmbus.h |  4 ++--
>  2 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 312fe5e..8d4fe0e 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -33,9 +33,7 @@
>  #include "hyperv_vmbus.h"
>
>  /* The one and only */
> -struct hv_context hv_context = {
> -	.synic_initialized	= false,
> -};
> +struct hv_context hv_context;
>
>  /*
>   * If false, we're using the old mechanism for stimer0 interrupts
> @@ -315,7 +313,7 @@ int hv_synic_init(unsigned int cpu)
>
>  	hv_set_synic_state(sctrl.as_uint64);
>
> -	hv_context.synic_initialized = true;
> +	hv_cpu->synic_initialized = true;
>
>  	/*
>  	 * Register the per-cpu clockevent source.
> @@ -354,6 +352,8 @@ void hv_synic_clockevents_cleanup(void)
>   */
>  int hv_synic_cleanup(unsigned int cpu)
>  {
> +	struct hv_per_cpu_context *hv_cpu
> +		= per_cpu_ptr(hv_context.cpu_context, cpu);
>  	union hv_synic_sint shared_sint;
>  	union hv_synic_simp simp;
>  	union hv_synic_siefp siefp;
> @@ -362,7 +362,7 @@ int hv_synic_cleanup(unsigned int cpu)
>  	bool channel_found = false;
>  	unsigned long flags;
>
> -	if (!hv_context.synic_initialized)
> +	if (!hv_cpu->synic_initialized)
>  		return -EFAULT;
>
>  	/*
> @@ -395,12 +395,8 @@ int hv_synic_cleanup(unsigned int cpu)
>
>  	/* Turn off clockevent device */
>  	if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> -		struct hv_per_cpu_context *hv_cpu
> -			= this_cpu_ptr(hv_context.cpu_context);
> -
>  		clockevents_unbind_device(hv_cpu->clk_evt, cpu);
>  		hv_ce_shutdown(hv_cpu->clk_evt);
> -		put_cpu_ptr(hv_cpu);
>  	}
>
>  	hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
> @@ -428,5 +424,7 @@ int hv_synic_cleanup(unsigned int cpu)
>  	sctrl.enable = 0;
>  	hv_set_synic_state(sctrl.as_uint64);
>
> +	hv_cpu->synic_initialized = false;
> +
>  	return 0;
>  }
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 72eaba3..eadd3df 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -202,6 +202,8 @@ enum {
>  struct hv_per_cpu_context {
>  	void *synic_message_page;
>  	void *synic_event_page;
> +	bool synic_initialized;
> +
>  	/*
>  	 * buffer to post messages to the host.
>  	 */
> @@ -230,8 +232,6 @@ struct hv_context {
>
>  	void *tsc_page;
>
> -	bool synic_initialized;
> -
>  	struct hv_per_cpu_context __percpu *cpu_context;
>
>  	/*

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Alternatively, we can get rid of synic_initialized flag altogether:
hv_synic_init() never fails in the first place but we can always
implement something like:

int hv_synic_is_initialized(void) {
	union hv_synic_scontrol sctrl;

	hv_get_synic_state(sctrl.as_uint64);

	return sctrl.enable;
}

as it doesn't seem that we need to check synic state on _other_ CPUs.

-- 
  Vitaly

RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[Michael Kelley (EOSG)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, July 31, 2018 4:20 AM
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Thanks for the review

> 
> Alternatively, we can get rid of synic_initialized flag altogether:
> hv_synic_init() never fails in the first place but we can always
> implement something like:
> 
> int hv_synic_is_initialized(void) {
> 	union hv_synic_scontrol sctrl;
> 
> 	hv_get_synic_state(sctrl.as_uint64);
> 
> 	return sctrl.enable;
> }
> 
> as it doesn't seem that we need to check synic state on _other_ CPUs.
> 
> --
>   Vitaly

I was trying to decide if there are any arguments in favor of one
approach vs. the other:  a per-cpu flag in memory or checking
the synic_control "enable" bit.   Seems like a wash to me, in which
case I have a slight preference for the per-cpu flag in memory vs.
creating another function to return sctrl.enable.  But I'm completely
open to reasons why checking sctrl.enable is better.

Michael

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[Vitaly Kuznetsov]

"Michael Kelley (EOSG)" <Michael.H.Kelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, July 31, 2018 4:20 AM
>> 
>> Alternatively, we can get rid of synic_initialized flag altogether:
>> hv_synic_init() never fails in the first place but we can always
>> implement something like:
>> 
>> int hv_synic_is_initialized(void) {
>> 	union hv_synic_scontrol sctrl;
>> 
>> 	hv_get_synic_state(sctrl.as_uint64);
>> 
>> 	return sctrl.enable;
>> }
>> 
>> as it doesn't seem that we need to check synic state on _other_ CPUs.
>> 
>
> I was trying to decide if there are any arguments in favor of one
> approach vs. the other:  a per-cpu flag in memory or checking
> the synic_control "enable" bit.   Seems like a wash to me, in which
> case I have a slight preference for the per-cpu flag in memory vs.
> creating another function to return sctrl.enable.  But I'm completely
> open to reasons why checking sctrl.enable is better.

Just a few thoughts: reading MSR is definitely slower but we avoid
'shadowing' the state, the reading is always correct. In case there's a
chance the SynIC will get disabled from host side we can only find this
out by doing MSR read. This is a purely theoretical possibility, I
believe, we can go ahead with this patch.

-- 
  Vitaly

RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[Michael Kelley (EOSG)]

From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, August 1, 2018 2:26 AM

> > I was trying to decide if there are any arguments in favor of one
> > approach vs. the other:  a per-cpu flag in memory or checking
> > the synic_control "enable" bit.   Seems like a wash to me, in which
> > case I have a slight preference for the per-cpu flag in memory vs.
> > creating another function to return sctrl.enable.  But I'm completely
> > open to reasons why checking sctrl.enable is better.
> 
> Just a few thoughts: reading MSR is definitely slower but we avoid
> 'shadowing' the state, the reading is always correct. In case there's a
> chance the SynIC will get disabled from host side we can only find this
> out by doing MSR read. This is a purely theoretical possibility, I
> believe, we can go ahead with this patch.

Vitaly -- just to confirm:  you are OK with the patch as is?  (I'll
check, but I may need to rebase on the latest code.)

Michael

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Make synic_initialized flag per-cpu

[Vitaly Kuznetsov]

"Michael Kelley (EOSG)" <Michael.H.Kelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>  Sent: Wednesday, August 1, 2018 2:26 AM
>
>> > I was trying to decide if there are any arguments in favor of one
>> > approach vs. the other:  a per-cpu flag in memory or checking
>> > the synic_control "enable" bit.   Seems like a wash to me, in which
>> > case I have a slight preference for the per-cpu flag in memory vs.
>> > creating another function to return sctrl.enable.  But I'm completely
>> > open to reasons why checking sctrl.enable is better.
>> 
>> Just a few thoughts: reading MSR is definitely slower but we avoid
>> 'shadowing' the state, the reading is always correct. In case there's a
>> chance the SynIC will get disabled from host side we can only find this
>> out by doing MSR read. This is a purely theoretical possibility, I
>> believe, we can go ahead with this patch.
>
> Vitaly -- just to confirm:  you are OK with the patch as is?  (I'll
> check, but I may need to rebase on the latest code.)

Yes, feel free to use my R-b tag.

-- 
  Vitaly