[RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

[RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

[Vit Kabele]

The Hyper-V crash reporting feature is initialized after a successful
vmbus setup. The reporting feature however does not require vmbus at all
and Windows guests can indeed use the reporting capabilities even with
the minimal Hyper-V implementation (as described in the Minimal
Requirements document).

Reorder the initialization callbacks so that the crash reporting
callbacks are registered before the vmbus initialization starts.

Nevertheless, I am not sure about following:

1/ The vmbus_initiate_unload function is called within the panic handler
even when the vmbus initialization does not finish (there might be no
vmbus at all). This should probably not be problem because the vmbus
unload function always checks for current connection state and does
nothing when this is "DISCONNECTED". For better readability, it might be
better to add separate panic notifier for vmbus and crash reporting.

2/ Wouldn't it be better to extract the whole reporting capability out
of the vmbus module, so that it stays present in the kernel even when
the vmbus module is possibly unloaded?

Signed-off-by: Vit Kabele <vit.kabele@sysgo.com>

---
 drivers/hv/vmbus_drv.c | 77 +++++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 35 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 714d549b7b46..97873f03aa7a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1509,41 +1509,6 @@ static int vmbus_bus_init(void)
 	if (hv_is_isolation_supported())
 		sysctl_record_panic_msg = 0;
 
-	/*
-	 * Only register if the crash MSRs are available
-	 */
-	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
-		u64 hyperv_crash_ctl;
-		/*
-		 * Panic message recording (sysctl_record_panic_msg)
-		 * is enabled by default in non-isolated guests and
-		 * disabled by default in isolated guests; the panic
-		 * message recording won't be available in isolated
-		 * guests should the following registration fail.
-		 */
-		hv_ctl_table_hdr = register_sysctl_table(hv_root_table);
-		if (!hv_ctl_table_hdr)
-			pr_err("Hyper-V: sysctl table register error");
-
-		/*
-		 * Register for panic kmsg callback only if the right
-		 * capability is supported by the hypervisor.
-		 */
-		hyperv_crash_ctl = hv_get_register(HV_REGISTER_CRASH_CTL);
-		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
-			hv_kmsg_dump_register();
-
-		register_die_notifier(&hyperv_die_block);
-	}
-
-	/*
-	 * Always register the panic notifier because we need to unload
-	 * the VMbus channel connection to prevent any VMbus
-	 * activity after the VM panics.
-	 */
-	atomic_notifier_chain_register(&panic_notifier_list,
-			       &hyperv_panic_block);
-
 	vmbus_request_offers();
 
 	return 0;
@@ -2675,6 +2640,46 @@ static struct syscore_ops hv_synic_syscore_ops = {
 	.resume = hv_synic_resume,
 };
 
+static void __init crash_reporting_init(void)
+{
+	/*
+	 * Only register if the crash MSRs are available
+	 */
+	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
+		u64 hyperv_crash_ctl;
+		/*
+		 * Panic message recording (sysctl_record_panic_msg)
+		 * is enabled by default in non-isolated guests and
+		 * disabled by default in isolated guests; the panic
+		 * message recording won't be available in isolated
+		 * guests should the following registration fail.
+		 */
+		hv_ctl_table_hdr = register_sysctl_table(hv_root_table);
+		if (!hv_ctl_table_hdr)
+			pr_err("Hyper-V: sysctl table register error");
+
+		/*
+		 * Register for panic kmsg callback only if the right
+		 * capability is supported by the hypervisor.
+		 */
+		hyperv_crash_ctl = hv_get_register(HV_REGISTER_CRASH_CTL);
+		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
+			hv_kmsg_dump_register();
+
+		register_die_notifier(&hyperv_die_block);
+	}
+
+	/*
+	 * Always register the panic notifier because we need to unload
+	 * the VMbus channel connection to prevent any VMbus
+	 * activity after the VM panics.
+	 */
+	atomic_notifier_chain_register(&panic_notifier_list,
+			       &hyperv_panic_block);
+
+
+}
+
 static int __init hv_acpi_init(void)
 {
 	int ret, t;
@@ -2687,6 +2692,8 @@ static int __init hv_acpi_init(void)
 
 	init_completion(&probe_event);
 
+	crash_reporting_init();
+
 	/*
 	 * Get ACPI resources first.
 	 */
-- 
2.30.2

Re: [RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

[Vitaly Kuznetsov]

Vit Kabele <vit.kabele@sysgo.com> writes:

> The Hyper-V crash reporting feature is initialized after a successful
> vmbus setup. The reporting feature however does not require vmbus at all
> and Windows guests can indeed use the reporting capabilities even with
> the minimal Hyper-V implementation (as described in the Minimal
> Requirements document).
>
> Reorder the initialization callbacks so that the crash reporting
> callbacks are registered before the vmbus initialization starts.
>
> Nevertheless, I am not sure about following:
>
> 1/ The vmbus_initiate_unload function is called within the panic handler
> even when the vmbus initialization does not finish (there might be no
> vmbus at all). This should probably not be problem because the vmbus
> unload function always checks for current connection state and does
> nothing when this is "DISCONNECTED". For better readability, it might be
> better to add separate panic notifier for vmbus and crash reporting.
>
> 2/ Wouldn't it be better to extract the whole reporting capability out
> of the vmbus module, so that it stays present in the kernel even when
> the vmbus module is possibly unloaded?

IMHO yes but as you mention hyperv_panic_event() currently does to
things:
1) Initiates VMBus unload
2) Reports panic to the hypervisor

I think untangling them moving the later to arch/x86/hyper-v (and
arch/arm64/hyperv/) makes sense.

>
> Signed-off-by: Vit Kabele <vit.kabele@sysgo.com>
>
> ---
>  drivers/hv/vmbus_drv.c | 77 +++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 714d549b7b46..97873f03aa7a 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1509,41 +1509,6 @@ static int vmbus_bus_init(void)
>  	if (hv_is_isolation_supported())
>  		sysctl_record_panic_msg = 0;
>  
> -	/*
> -	 * Only register if the crash MSRs are available
> -	 */
> -	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
> -		u64 hyperv_crash_ctl;
> -		/*
> -		 * Panic message recording (sysctl_record_panic_msg)
> -		 * is enabled by default in non-isolated guests and
> -		 * disabled by default in isolated guests; the panic
> -		 * message recording won't be available in isolated
> -		 * guests should the following registration fail.
> -		 */
> -		hv_ctl_table_hdr = register_sysctl_table(hv_root_table);
> -		if (!hv_ctl_table_hdr)
> -			pr_err("Hyper-V: sysctl table register error");
> -
> -		/*
> -		 * Register for panic kmsg callback only if the right
> -		 * capability is supported by the hypervisor.
> -		 */
> -		hyperv_crash_ctl = hv_get_register(HV_REGISTER_CRASH_CTL);
> -		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
> -			hv_kmsg_dump_register();
> -
> -		register_die_notifier(&hyperv_die_block);
> -	}
> -
> -	/*
> -	 * Always register the panic notifier because we need to unload
> -	 * the VMbus channel connection to prevent any VMbus
> -	 * activity after the VM panics.
> -	 */
> -	atomic_notifier_chain_register(&panic_notifier_list,
> -			       &hyperv_panic_block);
> -
>  	vmbus_request_offers();
>  
>  	return 0;
> @@ -2675,6 +2640,46 @@ static struct syscore_ops hv_synic_syscore_ops = {
>  	.resume = hv_synic_resume,
>  };
>  
> +static void __init crash_reporting_init(void)
> +{
> +	/*
> +	 * Only register if the crash MSRs are available
> +	 */
> +	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
> +		u64 hyperv_crash_ctl;
> +		/*
> +		 * Panic message recording (sysctl_record_panic_msg)
> +		 * is enabled by default in non-isolated guests and
> +		 * disabled by default in isolated guests; the panic
> +		 * message recording won't be available in isolated
> +		 * guests should the following registration fail.
> +		 */
> +		hv_ctl_table_hdr = register_sysctl_table(hv_root_table);
> +		if (!hv_ctl_table_hdr)
> +			pr_err("Hyper-V: sysctl table register error");
> +
> +		/*
> +		 * Register for panic kmsg callback only if the right
> +		 * capability is supported by the hypervisor.
> +		 */
> +		hyperv_crash_ctl = hv_get_register(HV_REGISTER_CRASH_CTL);
> +		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
> +			hv_kmsg_dump_register();
> +
> +		register_die_notifier(&hyperv_die_block);
> +	}
> +
> +	/*
> +	 * Always register the panic notifier because we need to unload
> +	 * the VMbus channel connection to prevent any VMbus
> +	 * activity after the VM panics.
> +	 */
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +			       &hyperv_panic_block);
> +
> +
> +}
> +
>  static int __init hv_acpi_init(void)
>  {
>  	int ret, t;
> @@ -2687,6 +2692,8 @@ static int __init hv_acpi_init(void)
>  
>  	init_completion(&probe_event);
>  
> +	crash_reporting_init();
> +
>  	/*
>  	 * Get ACPI resources first.
>  	 */

-- 
Vitaly

Re: [RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

[Vit Kabele]

On Thu, Jun 16, 2022 at 05:03:16PM +0200, Vitaly Kuznetsov wrote:
> Vit Kabele <vit.kabele@sysgo.com> writes:
> > Nevertheless, I am not sure about following:
> >
> > 1/ The vmbus_initiate_unload function is called within the panic handler
> > even when the vmbus initialization does not finish (there might be no
> > vmbus at all). This should probably not be problem because the vmbus
> > unload function always checks for current connection state and does
> > nothing when this is "DISCONNECTED". For better readability, it might be
> > better to add separate panic notifier for vmbus and crash reporting.
> >
> > 2/ Wouldn't it be better to extract the whole reporting capability out
> > of the vmbus module, so that it stays present in the kernel even when
> > the vmbus module is possibly unloaded?
> 
> IMHO yes but as you mention hyperv_panic_event() currently does to
> things:
> 1) Initiates VMBus unload
> 2) Reports panic to the hypervisor
> 
> I think untangling them moving the later to arch/x86/hyper-v (and
> arch/arm64/hyperv/) makes sense.
Ok, I will send the complete patch soon.

-- 
Best regards,
Vit Kabele

RE: [RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

[Michael Kelley (LINUX)]

From: Vit Kabele <vit.kabele@sysgo.com> Sent: Monday, June 20, 2022 6:56 AM
> 
> On Thu, Jun 16, 2022 at 05:03:16PM +0200, Vitaly Kuznetsov wrote:
> > Vit Kabele <vit.kabele@sysgo.com> writes:
> > > Nevertheless, I am not sure about following:
> > >
> > > 1/ The vmbus_initiate_unload function is called within the panic handler
> > > even when the vmbus initialization does not finish (there might be no
> > > vmbus at all). This should probably not be problem because the vmbus
> > > unload function always checks for current connection state and does
> > > nothing when this is "DISCONNECTED". For better readability, it might be
> > > better to add separate panic notifier for vmbus and crash reporting.
> > >
> > > 2/ Wouldn't it be better to extract the whole reporting capability out
> > > of the vmbus module, so that it stays present in the kernel even when
> > > the vmbus module is possibly unloaded?
> >
> > IMHO yes but as you mention hyperv_panic_event() currently does to
> > things:
> > 1) Initiates VMBus unload
> > 2) Reports panic to the hypervisor
> >
> > I think untangling them moving the later to arch/x86/hyper-v (and
> > arch/arm64/hyperv/) makes sense.
> Ok, I will send the complete patch soon.
> 

Vit --

FYI, there's a large patch series [1] that proposed some reorganization
of the panic notifiers across the Linux kernel.  Patch 16 of the series
splits the Hyper-V panic notifier into two along the lines that you
suggest.  In addition to the patch itself, the comments and follow-on
discussion are relevant to changes you propose.  See my responses
throughout the series.

The author of the series is planning a v2, but he's out for a few weeks
so there will be a delay. [2]

Michael

[1] https://lore.kernel.org/linux-hyperv/20220427224924.592546-1-gpiccoli@igalia.com/T/#t
[2] https://lore.kernel.org/linux-hyperv/20220427224924.592546-1-gpiccoli@igalia.com/T/#m3c190913bcb6f66e3ace792b4e6f2236839d4fa7

Re: [RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

[Vit Kabele]

On Mon, Jun 20, 2022 at 02:53:58PM +0000, Michael Kelley (LINUX) wrote:
> FYI, there's a large patch series [1] that proposed some reorganization
> of the panic notifiers across the Linux kernel.  Patch 16 of the series
> splits the Hyper-V panic notifier into two along the lines that you
> suggest.  In addition to the patch itself, the comments and follow-on
> discussion are relevant to changes you propose.  See my responses
> throughout the series.
> 
> The author of the series is planning a v2, but he's out for a few weeks
> so there will be a delay. [2]
Aha, great. Thank you for the information, I won't reinvent the wheel
then :)
 
> [1] https://lore.kernel.org/linux-hyperv/20220427224924.592546-1-gpiccoli@igalia.com/T/#t
> [2] https://lore.kernel.org/linux-hyperv/20220427224924.592546-1-gpiccoli@igalia.com/T/#m3c190913bcb6f66e3ace792b4e6f2236839d4fa7

-- 
Best regards,
Vit Kabele