Thanks, Michael. In which branch should I fix these now that the changes have
been
merged with the char-misc-next branch?
Comments inline.
> -----Original Message-----
> From: Michael Kelley (EOSG)
> Sent: Tuesday, July 10, 2018 6:05 PM
> To: KY Srinivasan <k...@microsoft.com>; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
> a...@canonical.com; jasow...@redhat.com; Stephen Hemminger
> <sthem...@microsoft.com>; vkuzn...@redhat.com
> Cc: Sunil Muthuswamy <sunil...@microsoft.com>
> Subject: RE: [PATCH 1/1] Drivers: HV: Send one page worth of kmsg dump
> over Hyper-V during panic
>
> From k...@linuxonhyperv.com <k...@linuxonhyperv.com> Sent: Saturday,
> July 7, 2018 7:57 PM
> >
> > From: Sunil Muthuswamy <sunil...@microsoft.com>
> >
> > In the VM mode on Hyper-V, currently, when the kernel panics, an error
> > code and few register values are populated in an MSR and the Hypervisor
> > notified. This information is collected on the host. The amount of
> > information currently collected is found to be limited and not very
> > actionable. To gather more actionable data, such as stack trace, the
> > proposal is to write one page worth of kmsg data on an allocated page
> > and the Hypervisor notified of the page address through the MSR.
> >
> > - Sysctl option to control the behavior, with ON by default.
> >
> > Cc: K. Y. Srinivasan <k...@microsoft.com>
> > Cc: Stephen Hemminger <sthem...@microsoft.com>
> > Signed-off-by: Sunil Muthuswamy <sunil...@microsoft.com>
> > Signed-off-by: K. Y. Srinivasan <k...@microsoft.com>
> > ---
>
> > + /*
> > + * Write dump contents to the page. No need to synchronize; panic
> should
> > + * be single-threaded.
> > + */
> > + if (!kmsg_dump_get_buffer(dumper, true, hv_panic_page,
> > + PAGE_SIZE, &bytes_written)) {
> > + pr_err("Hyper-V: Unable to get kmsg data for panic\n");
> > + return;
>
> From what I can see, the return value from kmsg_dump_get_buffer()
> is not an indication of success or failure -- it's an indication of whether
> there is more data available. There's no reason to output an error
> message.
>
That seems correct. Will address this.
> > @@ -1065,6 +1136,32 @@ static int vmbus_bus_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;
> > + /*
> > + * Sysctl registration is not fatal, since by default
> > + * reporting is enabled.
> > + */
> > + 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.
> > + */
> > + rdmsrl(HV_X64_MSR_CRASH_CTL, hyperv_crash_ctl);
> > + if (hyperv_crash_ctl &
> HV_CRASH_CTL_CRASH_NOTIFY_MSG) {
>
> vmbus_drv.c is architecture independent code, and should not be
> referencing
> x86/x64 MSRs. Reading the MSR (and maybe the test as well?) should go
> in a separate function in an x86-specific source file.
>
I will move the code.
> And just to confirm, is this the right way to test for the feature? Usually,
> feature determination is based on one of the feature registers. The
> NOTIFY_MSG flag seems to have a dual meaning -- on read it indicates
> the feature is present. On write in hyperv_report_panic_msg(), it evidently
> means that the guest is sending a full page of data to Hyper-V.
>
As per my conversation with John, this seems to be correct and something
also he suggested. The host sets these bits depending on whether it supports
these features or not.
> > @@ -1081,6 +1178,11 @@ static int vmbus_bus_init(void)
> > bus_unregister(&hv_bus);
> > + free_page((unsigned long)hv_panic_page);
> > + if (!hv_ctl_table_hdr) {
>
> The above test is backwards. Remove the bang.
Good call, will do.
>
> > @@ -1785,10 +1887,18 @@ static void __exit vmbus_exit(void)
> > + free_page((unsigned long)hv_panic_page);
> > + if (!hv_ctl_table_hdr) {
>
> Same here. Test is backwards.
>
Will fix.
> Michael
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
