Vit Kabele <vit.kabele@xxxxxxxxx> 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@xxxxxxxxx>
>
> ---
> 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
