Re: [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set

[Tianyu Lan]

On 4/2/2020 2:53 AM, Wei Liu wrote:
> On Tue, Mar 31, 2020 at 10:26:06PM +0800, Tianyu Lan wrote:
> > On 3/31/2020 9:51 PM, Michael Kelley wrote:
> > > From: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx> Sent: Tuesday, March 31, 2020 12:39 AM
> > > > When oops happens with panic_on_oops unset, the oops
> > > > thread is killed by die() and system continues to run.
> > > > In such case, guest should not report crash register
> > > > data to host since system still runs. Fix it.
> > > >
> > > > Reviewed-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> > > > Signed-off-by: Tianyu Lan <Tianyu.Lan@xxxxxxxxxxxxx>
> > > > ---
> > > > Change since v3:
> > > > 	Fix compile error
> > > > ---
> > > >    drivers/hv/vmbus_drv.c | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> > > > index 172ceae69abb..4bc02aea2098 100644
> > > > --- a/drivers/hv/vmbus_drv.c
> > > > +++ b/drivers/hv/vmbus_drv.c
> > > > @@ -31,6 +31,7 @@
> > > >    #include <linux/kdebug.h>
> > > >    #include <linux/efi.h>
> > > >    #include <linux/random.h>
> > > > +#include <linux/kernel.h>
> > >
> > > Unfortunately, adding the #include doesn't solve the problem.  The error occurs when
> > > CONFIG_HYPERV=m, because panic_on_oops is not exported.  I haven't thought it
> > > through, but hopefully there's a solution where panic_on_oops can be tested in
> > > hyperv_report_panic() or some other Hyper-V specific function that's never in a
> > > module, so that we don't need to export panic_on_oops.
> >
> > Yes, I don't consider modules case. I think we may introduce a check
> > function of panic_on_oops in the mshyperv.c and expose it to module.
>
> Why expose something new? You can just test panic_on_oops in
> hyperv_report_panic and bail if it is false, right?
>
> Something like the following (not compiled) diff:
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index b0da5320bcff..0dc229a9142c 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -424,6 +424,9 @@ void hyperv_report_panic(struct pt_regs *regs, long err)
>          static bool panic_reported;
>          u64 guest_id;
>
> +       if (!panic_on_oops)
> +               return;
> +
>          /*
>           * We prefer to report panic on 'die' chain as we have proper
>           * registers to report, but if we miss it (e.g. on BUG()) we need
>
> I haven't checked all the error reporting paths and don't know if this
> works or not though.

Hi Wei:
    hyperv_report_panic() is also called in panic() which may not be
triggered by die(). In these cases, we still need to report crash
register data to host even when panic_on_oops is unset.
    Another approach is to add new parameter "in_die" for
hyperv_report_panic() and just check panic_on_oops when the function is
called in die notifier callback and "in_die" is set to true.