Re: [Qemu-devel] [PATCH 9/9] qemu/kvm: kvm hyper-v based guest crash event handling

[Andrey Smetanin]

On Wed, 2015-07-01 at 17:07 +0200, Paolo Bonzini wrote:
> 
> On 30/06/2015 13:33, Denis V. Lunev wrote:
> > 
> > +static int kvm_arch_handle_hv_crash(CPUState *cs)
> > +{
> > +    X86CPU *cpu = X86_CPU(cs);
> > +    CPUX86State *env = &cpu->env;
> > +
> > +    /* Mark that Hyper-v guest crash occurred */
> > +    env->hv_crash_occurred = 1;
> 
> This need not be a hv crash.  You can add crash_occurred to CPUState
> directly, and set it in qemu_system_guest_panicked:
> 
>       if (current_cpu) {
>           current_cpu->crash_occurred = true;
>         }
> 
> Then you would add two subsections: one for crash_occurred in exec.c
> (attached to vmstate_cpu_common), one for hyperv crash params in
> target-i386/machine.c.
> 
> This also gives an idea about splitting the patch: first the
> introduction of qemu_system_guest_panicked and crash_occurred, second
> the Hyper-V specific bits.
> 
> > +        if (cpu->hyperv_crash) {
> > +            c->edx |= HV_X64_GUEST_CRASH_MSR_AVAILABLE;
> > +            has_msr_hv_crash = true;
> 
> You can only set this to true if the kernel also supports the MSRs.
> 
> > +        }
> > +
> >          c = &cpuid_data.entries[cpuid_i++];
> >          c->function = HYPERV_CPUID_ENLIGHTMENT_INFO;
> >          if (cpu->hyperv_relaxed_timing) {
> > @@ -761,6 +767,10 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
> >      } else {
> >          env->mp_state = KVM_MP_STATE_RUNNABLE;
> >      }
> > +    if (has_msr_hv_crash) {
> > +            env->msr_hv_crash_ctl = HV_X64_MSR_CRASH_CTL_NOTIFY;
> 
> The value is always host-defined, so I think it doesn't need a field in
> CPUX86State.  On the other hand, this:
Kernel just works with that value, kernel doesn't setup it. 
The user space is allowed to setup this msr if qemu option "hv-crash" is
on. So the code "env->msr_hv_crash_ctl = HV_X64_MSR_CRASH_CTL_NOTIFY;"
setups msr in user space at cpu reset. When cpu setup it's registers
these msr's values are uploaded into kernel.

Anyway we need a code that initially set up crash ctl msr with value
HV_X64_MSR_CRASH_CTL_NOTIFY. And I think that code should be user space.
Any objections ?
> 
> 
> +static bool hyperv_crash_enable_needed(void *opaque)
> +{
> +    X86CPU *cpu = opaque;
> +    CPUX86State *env = &cpu->env;
> +
> +    return (env->msr_hv_crash_ctl & HV_X64_MSR_CRASH_CTL_CONTENTS) ?
> +            true : false;
> +}
> +
> 
> can just check if any of the params fields is nonzero.
> 
If we setup crash ctl msr by user space, we need it to migrate.

> Thanks,
> 
> Paolo
> 
> > +            env->hv_crash_occurred = 0;
> > +    }