diff mbox

[11/11] qemu/kvm: mark in cpu state that hyper-v crash occured

Message ID 1434989108-20924-12-git-send-email-den@openvz.org (mailing list archive)
State New, archived
Headers show

Commit Message

Denis V. Lunev June 22, 2015, 4:05 p.m. UTC 
From: Andrey Smetanin <asmetanin@virtuozzo.com>

It's usually impossible to understand from Hyper-V
crash msr's that crash happened because ctl msr
always contains the same value HV_X64_MSR_CRASH_CTL_NOTIFY.
To solve it add a particalar value hv_crash_occurred
inside CPU state and migrate this value with crash msr's.

Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Andreas Färber <afaerber@suse.de>
---
 target-i386/cpu.h     | 1 +
 target-i386/kvm.c     | 1 +
 target-i386/machine.c | 1 +
 3 files changed, 3 insertions(+)

Comments

Andreas Färber June 22, 2015, 4:23 p.m. UTC | #1 
Am 22.06.2015 um 18:05 schrieb Denis V. Lunev:
> From: Andrey Smetanin <asmetanin@virtuozzo.com>
> 
> It's usually impossible to understand from Hyper-V
> crash msr's that crash happened because ctl msr
> always contains the same value HV_X64_MSR_CRASH_CTL_NOTIFY.
> To solve it add a particalar value hv_crash_occurred
> inside CPU state and migrate this value with crash msr's.
> 
> Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Andreas Färber <afaerber@suse.de>
> ---
[...]
> diff --git a/target-i386/machine.c b/target-i386/machine.c
> index 15b3f31..4f72ba8 100644
> --- a/target-i386/machine.c
> +++ b/target-i386/machine.c
> @@ -679,6 +679,7 @@ static const VMStateDescription vmstate_msr_hyperv_crash = {
>          VMSTATE_UINT64(env.msr_hv_crash_ctl, X86CPU),
>          VMSTATE_UINT64_ARRAY(env.msr_hv_crash_prm,
>                               X86CPU, HV_X64_MSR_CRASH_PARAMS),
> +        VMSTATE_UINT8(env.hv_crash_occurred, X86CPU),
>          VMSTATE_END_OF_LIST()
>      }
>  };

This looks like a migration format breakage. You probably need to squash
it with the preceding patch so that the "cpu/msr_hyperv_crash"
subsection does not change in size between commits. Just incrementing
the version is not an option for subsections, I think?

Regards,
Andreas
Paolo Bonzini June 22, 2015, 4:27 p.m. UTC | #2 
On 22/06/2015 18:23, Andreas Färber wrote:
>> > @@ -679,6 +679,7 @@ static const VMStateDescription vmstate_msr_hyperv_crash = {
>> >          VMSTATE_UINT64(env.msr_hv_crash_ctl, X86CPU),
>> >          VMSTATE_UINT64_ARRAY(env.msr_hv_crash_prm,
>> >                               X86CPU, HV_X64_MSR_CRASH_PARAMS),
>> > +        VMSTATE_UINT8(env.hv_crash_occurred, X86CPU),
>> >          VMSTATE_END_OF_LIST()
>> >      }
>> >  };
> This looks like a migration format breakage. You probably need to squash
> it with the preceding patch so that the "cpu/msr_hyperv_crash"
> subsection does not change in size between commits. Just incrementing
> the version is not an option for subsections, I think?

We don't usually care about migration format within the same upstream
release, but yes that would be better.

On the other hand, I wonder if current_cpu is available in
qemu_system_guest_panicked.  If so, you could add the field to the
generic CPUState struct and migrate it as a subsection of
vmstate_cpu_common.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
Andreas Färber June 22, 2015, 4:33 p.m. UTC | #3 
Am 22.06.2015 um 18:27 schrieb Paolo Bonzini:
> On the other hand, I wonder if current_cpu is available in
> qemu_system_guest_panicked.  If so, you could add the field to the
> generic CPUState struct and migrate it as a subsection of
> vmstate_cpu_common.

Hm, not sure whether it is.

Would that work with the two ways we use vmstate_cpu_common though?
I.e., can a nested VMState struct (VMSTATE_CPU()) have subsections?

Regards,
Andreas
Denis V. Lunev June 22, 2015, 4:35 p.m. UTC | #4 
On 22/06/15 19:33, Andreas Färber wrote:
> Am 22.06.2015 um 18:27 schrieb Paolo Bonzini:
>> On the other hand, I wonder if current_cpu is available in
>> qemu_system_guest_panicked.  If so, you could add the field to the
>> generic CPUState struct and migrate it as a subsection of
>> vmstate_cpu_common.
> Hm, not sure whether it is.
>
> Would that work with the two ways we use vmstate_cpu_common though?
> I.e., can a nested VMState struct (VMSTATE_CPU()) have subsections?
>
> Regards,
> Andreas
>
we'd better squash to avoid troubles. Any other issues?
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
Paolo Bonzini June 22, 2015, 4:36 p.m. UTC | #5 
On 22/06/2015 18:33, Andreas Färber wrote:
>> > On the other hand, I wonder if current_cpu is available in
>> > qemu_system_guest_panicked.  If so, you could add the field to the
>> > generic CPUState struct and migrate it as a subsection of
>> > vmstate_cpu_common.
> Hm, not sure whether it is.

It should be...

> Would that work with the two ways we use vmstate_cpu_common though?
> I.e., can a nested VMState struct (VMSTATE_CPU()) have subsections?

Yes, it can.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
Andreas Färber June 22, 2015, 5:46 p.m. UTC | #6 
Am 22.06.2015 um 18:36 schrieb Paolo Bonzini:
> On 22/06/2015 18:33, Andreas Färber wrote:
>>>> On the other hand, I wonder if current_cpu is available in
>>>> qemu_system_guest_panicked.  If so, you could add the field to the
>>>> generic CPUState struct and migrate it as a subsection of
>>>> vmstate_cpu_common.
>> Hm, not sure whether it is.
> 
> It should be...

Obviously depends on the call site. :) At some point in cpu-exec.c,
current_cpu gets set to NULL. So the function would at least deserve a
comment on when (not to) use it.

Cheers,
Andreas
Paolo Bonzini June 23, 2015, 9:53 a.m. UTC | #7 
On 22/06/2015 19:46, Andreas Färber wrote:
>>>>> >>>> On the other hand, I wonder if current_cpu is available in
>>>>> >>>> qemu_system_guest_panicked.  If so, you could add the field to the
>>>>> >>>> generic CPUState struct and migrate it as a subsection of
>>>>> >>>> vmstate_cpu_common.
>>> >> Hm, not sure whether it is.
>> > 
>> > It should be...
> Obviously depends on the call site. :) At some point in cpu-exec.c,
> current_cpu gets set to NULL. So the function would at least deserve a
> comment on when (not to) use it.

I think it could be used anyway even if current_cpu.  The difference
would be whether the crash is attached to a CPU or not.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 474a93e..2958cdc 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -907,6 +907,7 @@  typedef struct CPUX86State {
     uint64_t msr_hv_tsc;
     uint64_t msr_hv_crash_prm[HV_X64_MSR_CRASH_PARAMS];
     uint64_t msr_hv_crash_ctl;
+    uint8_t hv_crash_occurred;
 
     /* exception/interrupt handling */
     int error_code;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 690677b..2c8d00f 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2616,6 +2616,7 @@  int kvm_arch_handle_hv_crash(CPUState *cs)
             break;
         }
     }
+    env->hv_crash_occurred = 1;
 
     return 0;
 }
diff --git a/target-i386/machine.c b/target-i386/machine.c
index 15b3f31..4f72ba8 100644
--- a/target-i386/machine.c
+++ b/target-i386/machine.c
@@ -679,6 +679,7 @@  static const VMStateDescription vmstate_msr_hyperv_crash = {
         VMSTATE_UINT64(env.msr_hv_crash_ctl, X86CPU),
         VMSTATE_UINT64_ARRAY(env.msr_hv_crash_prm,
                              X86CPU, HV_X64_MSR_CRASH_PARAMS),
+        VMSTATE_UINT8(env.hv_crash_occurred, X86CPU),
         VMSTATE_END_OF_LIST()
     }
 };