| Message ID | 20220427224924.592546-17-gpiccoli@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | The panic notifiers refactor | expand |
From: Guilherme G. Piccoli <gpiccoli@igalia.com> Sent: Friday, April 29, 2022 3:35 PM > > Hi Michael, first of all thanks for the great review, much appreciated. > Some comments inline below: > > On 29/04/2022 14:16, Michael Kelley (LINUX) wrote: > > [...] > >> hypervisor I/O completion), so we postpone that to run late. But more > >> relevant: this *same* vmbus unloading happens in the crash_shutdown() > >> handler, so if kdump is set, we can safely skip this panic notifier and > >> defer such clean-up to the kexec crash handler. > > > > While the last sentence is true for Hyper-V on x86/x64, it's not true for > > Hyper-V on ARM64. x86/x64 has the 'machine_ops' data structure > > with the ability to provide a custom crash_shutdown() function, which > > Hyper-V does in the form of hv_machine_crash_shutdown(). But ARM64 > > has no mechanism to provide such a custom function that will eventually > > do the needed vmbus_initiate_unload() before running kdump. > > > > I'm not immediately sure what the best solution is for ARM64. At this > > point, I'm just pointing out the problem and will think about the tradeoffs > > for various possible solutions. Please do the same yourself. :-) > > > > Oh, you're totally right! I just assumed ARM64 would the the same, my > bad. Just to propose some alternatives, so you/others can also discuss > here and we can reach a consensus about the trade-offs: > > (a) We could forget about this change, and always do the clean-up here, > not relying in machine_crash_shutdown(). > Pro: really simple, behaves the same as it is doing currently. > Con: less elegant/concise, doesn't allow arm64 customization. > > (b) Add a way to allow ARM64 customization of shutdown crash handler. > Pro: matches x86, more customizable, improves arm64 arch code. > Con: A tad more complex. > > Also, a question that came-up: if ARM64 has no way of calling special > crash shutdown handler, how can you execute hv_stimer_cleanup() and > hv_synic_disable_regs() there? Or are they not required in ARM64? > My suggestion is to do (a) for now. I suspect (b) could be a more extended discussion and I wouldn't want your patch set to get held up on that discussion. I don't know what the sense of the ARM64 maintainers would be toward (b). They have tried to avoid picking up code warts like have accumulated on the x86/x64 side over the years, and I agree with that effort. But as more and varied hypervisors become available for ARM64, it seems like a framework for supporting a custom shutdown handler may become necessary. But that could take a little time. You are right about hv_stimer_cleanup() and hv_synic_disable_regs(). We are not running these when a panic occurs on ARM64, and we should be, though the risk is small. We will pursue (b) and add these additional cleanups as part of that. But again, I would suggest doing (a) for now, and we will switch back to your solution once (b) is in place. > > >> > >> (c) There is also a Hyper-V framebuffer panic notifier, which relies in > >> doing a vmbus operation that demands a valid connection. So, we must > >> order this notifier with the panic notifier from vmbus_drv.c, in order to > >> guarantee that the framebuffer code executes before the vmbus connection > >> is unloaded. > > > > Patch 21 of this set puts the Hyper-V FB panic notifier on the pre_reboot > > notifier list, which means it won't execute before the VMbus connection > > unload in the case of kdump. This notifier is making sure that Hyper-V > > is notified about the last updates made to the frame buffer before the > > panic, so maybe it needs to be put on the hypervisor notifier list. It > > sends a message to Hyper-V over its existing VMbus channel, but it > > does not wait for a reply. It does, however, obtain a spin lock on the > > ring buffer used to communicate with Hyper-V. Unless someone has > > a better suggestion, I'm inclined to take the risk of blocking on that > > spin lock. > > The logic behind that was: when kdump is set, we'd skip the vmbus > disconnect on notifiers, deferring that to crash_shutdown(), logic this > one refuted in the above discussion on ARM64 (one more Pro argument to > the idea of refactoring aarch64 code to allow a custom crash shutdown > handler heh). But you're right, for the default level 2, we skip the > pre_reboot notifiers on kdump, effectively skipping this notifier. > > Some ideas of what we can do here: > > I) we could change the framebuffer notifier to rely on trylocks, instead > of risking a lockup scenario, and with that, we can execute it before > the vmbus disconnect in the hypervisor list; I think we have to do this approach for now. > > II) we ignore the hypervisor notifier in case of kdump _by default_, and > if the users don't want that, they can always set the panic notifier > level to 4 and run all notifiers prior to kdump; would that be terrible > you think? Kdump users might don't care about the framebuffer... > > III) we go with approach (b) above and refactor arm64 code to allow the > custom crash handler on kdump time, then [with point (I) above] the > logic proposed in this series is still valid - seems more and more the > most correct/complete solution. But even when/if we get approach (b) implemented, having the framebuffer notifier on the pre_reboot list is still too late with the default of panic_notifier_level = 2. The kdump path will reset the VMbus connection and then the framebuffer notifier won't work. > > In any case, I guess we should avoid workarounds if possible and do the > things the best way we can, to encompass all (or almost all) the > possible scenarios and don't force things on users (like enforcing panic > notifier level 4 for Hyper-V or something like this...) > > More feedback from you / Hyper-V folks is pretty welcome about this. > > > > > >> [...] > > The "Fixes:" tags imply that these changes should be backported to older > > longterm kernel versions, which I don't think is the case. There is a > > dependency on Patch 14 of your series where PANIC_NOTIFIER is > > introduced. > > > > Oh, this was more related with archeology of the kernel. When I'm > investigating stuff, I really want to understand why code was added and > that usually require some time git blaming stuff, so having that pronto > in the commit message is a bonus. > > But of course we don't need to use the Fixes tag for that, easy to only > mention it in the text. A secondary benefit by using this tag is to > indicate this is a _real fix_ to some code, and not an improvement, but > as you say, I agree we shouldn't backport it to previous releases having > or not the Fixes tag (AFAIK it's not mandatory to backport stuff with > Fixes tag). > > > >> [...] > >> + * intrincated is the relation of this notifier with Hyper-V framebuffer > > > > s/intrincated/intricate/ > > Thanks, fixed in V2! > > > > > >> [...] > >> +static int hv_panic_vmbus_unload(struct notifier_block *nb, unsigned long val, > >> void *args) > >> +{ > >> + if (!kexec_crash_loaded()) > > > > I'm not clear on the purpose of this condition. I think it means > > we will skip the vmbus_initiate_unload() if a panic occurs in the > > kdump kernel. Is there a reason a panic in the kdump kernel > > should be treated differently? Or am I misunderstanding? > > This is really related with the point discussed in the top of this > response - I assumed both ARM64/x86_64 would behave the same and > disconnect the vmbus through the custom crash handler when kdump is set, > so worth skipping it here in the notifier. But that's not true for ARM64 > as you pointed, so this guard against kexec is really part of the > decision/discussion on what to do with ARM64 heh But note that vmbus_initiate_unload() already has a guard built-in. If the intent of this test is just as a guard against running twice, then it isn't needed. > > Cheers!
On 03/05/2022 15:13, Michael Kelley (LINUX) wrote: > [...] >> (a) We could forget about this change, and always do the clean-up here, >> not relying in machine_crash_shutdown(). >> Pro: really simple, behaves the same as it is doing currently. >> Con: less elegant/concise, doesn't allow arm64 customization. >> >> (b) Add a way to allow ARM64 customization of shutdown crash handler. >> Pro: matches x86, more customizable, improves arm64 arch code. >> Con: A tad more complex. >> >> Also, a question that came-up: if ARM64 has no way of calling special >> crash shutdown handler, how can you execute hv_stimer_cleanup() and >> hv_synic_disable_regs() there? Or are they not required in ARM64? >> > > My suggestion is to do (a) for now. I suspect (b) could be a more > extended discussion and I wouldn't want your patch set to get held > up on that discussion. I don't know what the sense of the ARM64 > maintainers would be toward (b). They have tried to avoid picking > up code warts like have accumulated on the x86/x64 side over the > years, and I agree with that effort. But as more and varied > hypervisors become available for ARM64, it seems like a framework > for supporting a custom shutdown handler may become necessary. > But that could take a little time. > > You are right about hv_stimer_cleanup() and hv_synic_disable_regs(). > We are not running these when a panic occurs on ARM64, and we > should be, though the risk is small. We will pursue (b) and add > these additional cleanups as part of that. But again, I would suggest > doing (a) for now, and we will switch back to your solution once > (b) is in place. > Thanks again Michael, I'll stick with (a) for now. I'll check with ARM64 community about that, and I might even try to implement something in parallel (if you are not already working on that - lemme know please), so we don't get stuck here. As you said, I feel that this is more and more relevant as the number of panic/crash/kexec scenarios tend to increase in ARM64. >> [...] >> Some ideas of what we can do here: >> >> I) we could change the framebuffer notifier to rely on trylocks, instead >> of risking a lockup scenario, and with that, we can execute it before >> the vmbus disconnect in the hypervisor list; > > I think we have to do this approach for now. > >> >> II) we ignore the hypervisor notifier in case of kdump _by default_, and >> if the users don't want that, they can always set the panic notifier >> level to 4 and run all notifiers prior to kdump; would that be terrible >> you think? Kdump users might don't care about the framebuffer... >> >> III) we go with approach (b) above and refactor arm64 code to allow the >> custom crash handler on kdump time, then [with point (I) above] the >> logic proposed in this series is still valid - seems more and more the >> most correct/complete solution. > > But even when/if we get approach (b) implemented, having the > framebuffer notifier on the pre_reboot list is still too late with the > default of panic_notifier_level = 2. The kdump path will reset the > VMbus connection and then the framebuffer notifier won't work. > OK, perfect! I'll work something along these lines in V2, allowing the FB notifier to always run in the hypervisor list before the vmbus unload mechanism. >> [...] >>>> +static int hv_panic_vmbus_unload(struct notifier_block *nb, unsigned long val, >>>> void *args) >>>> +{ >>>> + if (!kexec_crash_loaded()) >>> >>> I'm not clear on the purpose of this condition. I think it means >>> we will skip the vmbus_initiate_unload() if a panic occurs in the >>> kdump kernel. Is there a reason a panic in the kdump kernel >>> should be treated differently? Or am I misunderstanding? >> >> This is really related with the point discussed in the top of this >> response - I assumed both ARM64/x86_64 would behave the same and >> disconnect the vmbus through the custom crash handler when kdump is set, >> so worth skipping it here in the notifier. But that's not true for ARM64 >> as you pointed, so this guard against kexec is really part of the >> decision/discussion on what to do with ARM64 heh > > But note that vmbus_initiate_unload() already has a guard built-in. > If the intent of this test is just as a guard against running twice, > then it isn't needed. Since we're going to avoid relying in the custom crash_shutdown(), due to the lack of ARM64 support for now, this check will be removed in V2. Its purpose was to skip the notifier *proactively* in case kexec is set, given that...once kexec happens, the custom crash_shutdown() would run the same function (wrong assumption for ARM64, my bad). Postponing that slightly would maybe gain us some time while the hypervisor finish its work, so we'd delay less in the vmbus unload path - that was the rationale behind this check. Cheers!
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index 14de17087864..f37f12d48001 100644 --- a/drivers/hv/vmbus_drv.c +++ b/drivers/hv/vmbus_drv.c @@ -24,11 +24,11 @@ #include <linux/sched/task_stack.h> #include <linux/delay.h> -#include <linux/notifier.h> #include <linux/panic_notifier.h> #include <linux/ptrace.h> #include <linux/screen_info.h> #include <linux/kdebug.h> +#include <linux/kexec.h> #include <linux/efi.h> #include <linux/random.h> #include <linux/kernel.h> @@ -68,51 +68,75 @@ static int hyperv_report_reg(void) return !sysctl_record_panic_msg || !hv_panic_page; } -static int hyperv_panic_event(struct notifier_block *nb, unsigned long val, +/* + * The panic notifier below is responsible solely for unloading the + * vmbus connection, which is necessary in a panic event. But notice + * that this same unloading procedure is executed in the Hyper-V + * crash_shutdown() handler [see hv_crash_handler()], which basically + * means that we can postpone its execution if we have kdump set, + * since it will run the crash_shutdown() handler anyway. Even more + * intrincated is the relation of this notifier with Hyper-V framebuffer + * panic notifier - we need vmbus connection alive there in order to + * succeed, so we need to order both with each other [for reference see + * hvfb_on_panic()] - this is done using notifiers' priorities. + */ +static int hv_panic_vmbus_unload(struct notifier_block *nb, unsigned long val, void *args) +{ + if (!kexec_crash_loaded()) + vmbus_initiate_unload(true); + + return NOTIFY_DONE; +} +static struct notifier_block hyperv_panic_vmbus_unload_block = { + .notifier_call = hv_panic_vmbus_unload, + .priority = INT_MIN + 1, /* almost the latest one to execute */ +}; + +/* + * The following callback works both as die and panic notifier; its + * goal is to provide panic information to the hypervisor unless the + * kmsg dumper is gonna be used [see hv_kmsg_dump()], which provides + * more information but is not always available. + * + * Notice that both the panic/die report notifiers are registered only + * if we have the capability HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE set. + */ +static int hv_die_panic_notify_crash(struct notifier_block *nb, + unsigned long val, void *args) { struct pt_regs *regs; + bool is_die; - vmbus_initiate_unload(true); - - /* - * Hyper-V should be notified only once about a panic. If we will be - * doing hv_kmsg_dump() with kmsg data later, don't do the notification - * here. - */ - if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE - && hyperv_report_reg()) { + /* Don't notify Hyper-V unless we have a die oops event or panic. */ + switch (val) { + case DIE_OOPS: + is_die = true; + regs = ((struct die_args *)args)->regs; + break; + case PANIC_NOTIFIER: + is_die = false; regs = current_pt_regs(); - hyperv_report_panic(regs, val, false); - } - return NOTIFY_DONE; -} - -static int hyperv_die_event(struct notifier_block *nb, unsigned long val, - void *args) -{ - struct die_args *die = args; - struct pt_regs *regs = die->regs; - - /* Don't notify Hyper-V if the die event is other than oops */ - if (val != DIE_OOPS) + break; + default: return NOTIFY_DONE; + } /* - * Hyper-V should be notified only once about a panic. If we will be - * doing hv_kmsg_dump() with kmsg data later, don't do the notification - * here. + * Hyper-V should be notified only once about a panic/die. If we will + * be calling hv_kmsg_dump() later with kmsg data, don't do the + * notification here. */ if (hyperv_report_reg()) - hyperv_report_panic(regs, val, true); + hyperv_report_panic(regs, val, is_die); + return NOTIFY_DONE; } - -static struct notifier_block hyperv_die_block = { - .notifier_call = hyperv_die_event, +static struct notifier_block hyperv_die_report_block = { + .notifier_call = hv_die_panic_notify_crash, }; -static struct notifier_block hyperv_panic_block = { - .notifier_call = hyperv_panic_event, +static struct notifier_block hyperv_panic_report_block = { + .notifier_call = hv_die_panic_notify_crash, }; static const char *fb_mmio_name = "fb_range"; @@ -1589,16 +1613,17 @@ static int vmbus_bus_init(void) if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG) hv_kmsg_dump_register(); - register_die_notifier(&hyperv_die_block); + register_die_notifier(&hyperv_die_report_block); + atomic_notifier_chain_register(&panic_notifier_list, + &hyperv_panic_report_block); } /* - * Always register the panic notifier because we need to unload - * the VMbus channel connection to prevent any VMbus - * activity after the VM panics. + * Always register the vmbus unload panic notifier because we + * need to shut the VMbus channel connection on panic. */ atomic_notifier_chain_register(&panic_notifier_list, - &hyperv_panic_block); + &hyperv_panic_vmbus_unload_block); vmbus_request_offers(); @@ -2817,15 +2842,17 @@ static void __exit vmbus_exit(void) if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) { kmsg_dump_unregister(&hv_kmsg_dumper); - unregister_die_notifier(&hyperv_die_block); + unregister_die_notifier(&hyperv_die_report_block); + atomic_notifier_chain_unregister(&panic_notifier_list, + &hyperv_panic_report_block); } /* - * The panic notifier is always registered, hence we should + * The vmbus panic notifier is always registered, hence we should * also unconditionally unregister it here as well. */ atomic_notifier_chain_unregister(&panic_notifier_list, - &hyperv_panic_block); + &hyperv_panic_vmbus_unload_block); free_page((unsigned long)hv_panic_page); unregister_sysctl_table(hv_ctl_table_hdr); diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index c8e0ea27caf1..f3494b868a64 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -1244,7 +1244,15 @@ static int hvfb_probe(struct hv_device *hdev, par->fb_ready = true; par->synchronous_fb = false; + + /* + * We need to be sure this panic notifier runs _before_ the + * vmbus disconnect, so order it by priority. It must execute + * before the function hv_panic_vmbus_unload() [drivers/hv/vmbus_drv.c], + * which is almost at the end of list, with priority = INT_MIN + 1. + */ par->hvfb_panic_nb.notifier_call = hvfb_on_panic; + par->hvfb_panic_nb.priority = INT_MIN + 10, atomic_notifier_chain_register(&panic_notifier_list, &par->hvfb_panic_nb);
Currently Hyper-V guests are among the most relevant users of the panic infrastructure, like panic notifiers, kmsg dumpers, etc. The reasons rely both in cleaning-up procedures (closing a hypervisor <-> guest connection, disabling a paravirtualized timer) as well as to data collection (sending panic information to the hypervisor) and framebuffer management. The thing is: some notifiers are related to others, ordering matters, some functionalities are duplicated and there are lots of conditionals behind sending panic information to the hypervisor. This patch, as part of an effort to clean-up the panic notifiers mechanism and better document things, address some of the issues/complexities of Hyper-V panic handling through the following changes: (a) We have die and panic notifiers on vmbus_drv.c and both have goals of sending panic information to the hypervisor, though the panic notifier is also responsible for a cleaning-up procedure. This commit clears the code by splitting the panic notifier in two, one for closing the vmbus connection whereas the other is only for sending panic info to hypervisor. With that, it was possible to merge the die and panic notifiers in a single/well-documented function, and clear some conditional complexities on sending such information to the hypervisor. (b) The new panic notifier created after (a) is only doing a single thing: cleaning the vmbus connection. This procedure might cause a delay (due to hypervisor I/O completion), so we postpone that to run late. But more relevant: this *same* vmbus unloading happens in the crash_shutdown() handler, so if kdump is set, we can safely skip this panic notifier and defer such clean-up to the kexec crash handler. (c) There is also a Hyper-V framebuffer panic notifier, which relies in doing a vmbus operation that demands a valid connection. So, we must order this notifier with the panic notifier from vmbus_drv.c, in order to guarantee that the framebuffer code executes before the vmbus connection is unloaded. Also, this commit removes a useless header. Although there is code rework and re-ordering, we expect that this change has no functional regressions but instead optimize the path and increase panic reliability on Hyper-V. This was tested on Hyper-V with success. Fixes: 792f232d57ff ("Drivers: hv: vmbus: Fix potential crash on module unload") Fixes: 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic callback") Cc: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Cc: Dexuan Cui <decui@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: "K. Y. Srinivasan" <kys@microsoft.com> Cc: Michael Kelley <mikelley@microsoft.com> Cc: Stephen Hemminger <sthemmin@microsoft.com> Cc: Tianyu Lan <Tianyu.Lan@microsoft.com> Cc: Wei Liu <wei.liu@kernel.org> Tested-by: Fabio A M Martins <fabiomirmar@gmail.com> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- Special thanks to Michael Kelley for the good information about the Hyper-V panic path in email threads some months ago, and to Fabio for the testing performed. Michael and all Microsoft folks: a careful analysis to double-check our changes and assumptions here is really appreciated, this code is complex and intricate, it is possible some corner case might have been overlooked. Thanks in advance! drivers/hv/vmbus_drv.c | 109 ++++++++++++++++++++------------ drivers/video/fbdev/hyperv_fb.c | 8 +++ 2 files changed, 76 insertions(+), 41 deletions(-)