RE: [PATCH 1/1] Drivers: HV: Send one page worth of kmsg dump over Hyper-V during panic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <kys@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>; vkuznets@xxxxxxxxxx
> Cc: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> Subject: RE: [PATCH 1/1] Drivers: HV: Send one page worth of kmsg dump
> over Hyper-V during panic
> 
> From kys@xxxxxxxxxxxxxxxxx <kys@xxxxxxxxxxxxxxxxx>  Sent: Saturday,
> July 7, 2018 7:57 PM
> >
> > From: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> >
> > 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 <kys@xxxxxxxxxxxxx>
> > Cc: Stephen Hemminger <sthemmin@xxxxxxxxxxxxx>
> > Signed-off-by: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > ---
> 
> > +	/*
> > +	 * 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
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux