RE: [PATCH v1] 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, Greg.

My first patch to the Linux kernel. Still making mistakes, but, learning through the documented process.

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, May 9, 2018 11:51 PM
> To: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>;
> devel@xxxxxxxxxxxxxxxxxxxxxx; Stephen Hemminger
> <sthemmin@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v1] Drivers: HV: Send one page worth of kmsg dump
> over Hyper-V during panic
> 
> On Wed, May 09, 2018 at 07:19:24PM +0000, Sunil Muthuswamy wrote:
> >     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.
> 
> Odd indentation, what editor made you do that?  Please move it all to the
> left.
I inserted them. Will fix.
> 
> >
> > CC: kys@xxxxxxxxxxxxx
> > CC: sthemmin@xxxxxxxxxxxxx
> > Signed-off-by: Sunil Muthuswamy <sunilmut@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/hyperv/hv_init.c          | 28 +++++++++++++++++
> >  arch/x86/include/asm/hyperv-tlfs.h |  5 ++--
> >  arch/x86/include/asm/mshyperv.h    |  1 +
> >  drivers/hv/vmbus_drv.c             | 61
> ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 93 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index cfecc22..88ee90d 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -395,6 +395,34 @@ void hyperv_report_panic(struct pt_regs *regs,
> > long err)  }  EXPORT_SYMBOL_GPL(hyperv_report_panic);
> >
> > +void hyperv_report_panic_msg(phys_addr_t pa, size_t size) {
> > +	static bool panic_msg_reported;
> > +
> > +	if (panic_msg_reported)
> > +		return;
> > +	panic_msg_reported = true;
> 
> Why do you only care about the first message?
It is following the general direction from ' hyperv_report_panic', but, I don't think it needs to. Will change.
> 
> > +
> > +	/*
> > +	 * P3 to contain the physical address of the panic page & P4 to
> > +	 * contain the size of the panic data in that page. Rest of the
> > +	 * registers are no-op when the NOTIFY_MSG flag is set.
> > +	 */
> > +	wrmsrl(HV_X64_MSR_CRASH_P0, 0);
> > +	wrmsrl(HV_X64_MSR_CRASH_P1, 0);
> > +	wrmsrl(HV_X64_MSR_CRASH_P2, 0);
> > +	wrmsrl(HV_X64_MSR_CRASH_P3, pa);
> > +	wrmsrl(HV_X64_MSR_CRASH_P4, size);
> > +
> > +	/*
> > +	 * Let Hyper-V know there is crash data available along with
> > +	 * the panic message.
> > +	 */
> > +	wrmsrl(HV_X64_MSR_CRASH_CTL,
> > +	       (HV_CRASH_CTL_CRASH_NOTIFY |
> HV_CRASH_CTL_CRASH_NOTIFY_MSG));
> > +} EXPORT_SYMBOL_GPL(hyperv_report_panic_msg);
> > +
> >  bool hv_is_hyperv_initialized(void)
> >  {
> >  	union hv_x64_msr_hypercall_contents hypercall_msr; diff --git
> > a/arch/x86/include/asm/hyperv-tlfs.h
> > b/arch/x86/include/asm/hyperv-tlfs.h
> > index 416cb0e..fc2932c 100644
> > --- a/arch/x86/include/asm/hyperv-tlfs.h
> > +++ b/arch/x86/include/asm/hyperv-tlfs.h
> > @@ -171,9 +171,10 @@
> >  #define HV_X64_ENLIGHTENED_VMCS_RECOMMENDED    (1 << 14)
> >
> >  /*
> > - * Crash notification flag.
> > + * Crash notification flags.
> >   */
> > -#define HV_CRASH_CTL_CRASH_NOTIFY (1ULL << 63)
> > +#define HV_CRASH_CTL_CRASH_NOTIFY     (1ULL << 63)
> > +#define HV_CRASH_CTL_CRASH_NOTIFY_MSG (1ULL << 62)
> 
> Not in numerical order?
> 
> And can you use the BIT() macro here instead?  Not a requirement, just a
> general question.
Will change in the next version.
> 
> >
> >  /* MSR used to identify the guest OS. */
> >  #define HV_X64_MSR_GUEST_OS_ID			0x40000000
> > diff --git a/arch/x86/include/asm/mshyperv.h
> > b/arch/x86/include/asm/mshyperv.h index b90e796..ac83f2d 100644
> > --- a/arch/x86/include/asm/mshyperv.h
> > +++ b/arch/x86/include/asm/mshyperv.h
> > @@ -262,6 +262,7 @@ void hyperv_init(void);  void
> > hyperv_setup_mmu_ops(void);  void hyper_alloc_mmu(void);  void
> > hyperv_report_panic(struct pt_regs *regs, long err);
> > +void hyperv_report_panic_msg(phys_addr_t pa, size_t size);
> >  bool hv_is_hyperv_initialized(void);
> >  void hyperv_cleanup(void);
> >
> > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index
> > b10fe26..40d915c 100644
> > --- a/drivers/hv/vmbus_drv.c
> > +++ b/drivers/hv/vmbus_drv.c
> > @@ -56,6 +56,8 @@ static struct completion probe_event;
> >
> >  static int hyperv_cpuhp_online;
> >
> > +static void *hv_panic_page;
> > +
> >  static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
> >  			      void *args)
> >  {
> > @@ -1018,6 +1020,41 @@ static void vmbus_isr(void)
> >  	add_interrupt_randomness(HYPERVISOR_CALLBACK_VECTOR, 0);  }
> >
> > +/*
> > + * Callback from kmsg_dump. Grab as much as possible from the end of
> > +the kmsg
> > + * buffer and call into Hyper-V to transfer the data.
> > + */
> > +static void hv_kmsg_dump(struct kmsg_dumper *dumper,
> > +			 enum kmsg_dump_reason reason)
> > +{
> > +	size_t bytes_written;
> > +	phys_addr_t panic_pa;
> > +
> > +	/* We are only interested in panics. */
> > +	if (reason != KMSG_DUMP_PANIC)
> > +		return;
> > +
> > +	if (!hv_panic_page)
> > +		return;
> 
> How is this check every going to be possible?  You don't register the dumper
> unless you have a page of memory, so no need to check this.
True, will fix.
> 
> > +
> > +	panic_pa = virt_to_phys(hv_panic_page);
> > +
> > +	/*
> > +	 * 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;
> > +	}
> > +
> > +	hyperv_report_panic_msg(panic_pa, bytes_written); }
> > +
> > +static struct kmsg_dumper hv_kmsg_dumper = {
> > +	.dump = hv_kmsg_dump,
> > +};
> >
> >  /*
> >   * vmbus_bus_init -Main vmbus driver initialization routine.
> > @@ -1065,6 +1102,19 @@ 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)
> > {
> > +		hv_panic_page = (void *)get_zeroed_page(GFP_KERNEL);
> > +		if (!hv_panic_page) {
> > +			ret = -ENOMEM;
> > +			goto err_connect;
> > +		}
> > +
> > +		ret = kmsg_dump_register(&hv_kmsg_dumper);
> > +		if (ret) {
> > +			pr_err("Hyper-V - kmsg dump register failure
> 0x%x\n",
> > +				ret);
> > +			goto err_connect;
> > +		}
> > +
> >  		register_die_notifier(&hyperv_die_block);
> >  		atomic_notifier_chain_register(&panic_notifier_list,
> >  					       &hyperv_panic_block);
> > @@ -1081,6 +1131,10 @@ static int vmbus_bus_init(void)
> >  	hv_remove_vmbus_irq();
> >
> >  	bus_unregister(&hv_bus);
> > +	if (hv_panic_page) {
> > +		free_page((unsigned long)hv_panic_page);
> 
> No need to check, free_page() can always be called with a 0 value
> successfully.
> 
Will fix.
> > +		hv_panic_page = NULL;
> > +	}
> >
> >  	return ret;
> >  }
> > @@ -1785,10 +1839,17 @@ static void __exit vmbus_exit(void)
> >  	vmbus_free_channels();
> >
> >  	if (ms_hyperv.misc_features &
> HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE)
> > {
> > +		kmsg_dump_unregister(&hv_kmsg_dumper);
> >  		unregister_die_notifier(&hyperv_die_block);
> >  		atomic_notifier_chain_unregister(&panic_notifier_list,
> >  						 &hyperv_panic_block);
> >  	}
> > +
> > +	if (hv_panic_page) {
> > +		free_page((unsigned long)hv_panic_page);
> 
> Same here, no need to check.
Will fix.
> 
> thanks,
> 
> greg k-h
_______________________________________________
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