[Qemu-devel] [PATCH for 2.9 0/3] guest panic information follow-up

[Qemu-devel] [PATCH for 2.9 0/3] guest panic information follow-up

[Denis V. Lunev]

This is a set of followup patches requested by Eric Blake.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Eric Blake <eblake@redhat.com>

Anton Nefedov (3):
  i386/cpu: release GuestPanicInformation memory
  qapi: flatten GuestPanicInformation union
  qmp-events: fix GUEST_PANICKED description formatting

 kvm-all.c         |  5 ++++-
 qapi-schema.json  | 12 ++++++++++++
 qapi/event.json   |  4 ++--
 target/i386/cpu.c | 15 ++++++---------
 vl.c              | 22 +++++++++-------------
 5 files changed, 33 insertions(+), 25 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] i386/cpu: release GuestPanicInformation memory

[Denis V. Lunev]

From: Anton Nefedov <anton.nefedov@virtuozzo.com>

Do not make some foreign function do it.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 kvm-all.c |  5 ++++-
 vl.c      | 22 +++++++++-------------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 0c94637..ac07902 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1909,6 +1909,7 @@ int kvm_cpu_exec(CPUState *cpu)
 
     do {
         MemTxAttrs attrs;
+        GuestPanicInformation *crash_info;
 
         if (cpu->kvm_vcpu_dirty) {
             kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
@@ -2001,9 +2002,11 @@ int kvm_cpu_exec(CPUState *cpu)
                 break;
             case KVM_SYSTEM_EVENT_CRASH:
                 kvm_cpu_synchronize_state(cpu);
+                crash_info = cpu_get_crash_info(cpu);
                 qemu_mutex_lock_iothread();
-                qemu_system_guest_panicked(cpu_get_crash_info(cpu));
+                qemu_system_guest_panicked(crash_info);
                 qemu_mutex_unlock_iothread();
+                qapi_free_GuestPanicInformation(crash_info);
                 ret = 0;
                 break;
             default:
diff --git a/vl.c b/vl.c
index 27d9829..81382b6 100644
--- a/vl.c
+++ b/vl.c
@@ -1682,6 +1682,15 @@ void qemu_system_reset(bool report)
 void qemu_system_guest_panicked(GuestPanicInformation *info)
 {
     qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed\n");
+    if (info && info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
+        qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
+                      " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
+                      info->u.hyper_v.data->arg1,
+                      info->u.hyper_v.data->arg2,
+                      info->u.hyper_v.data->arg3,
+                      info->u.hyper_v.data->arg4,
+                      info->u.hyper_v.data->arg5);
+    }
 
     if (current_cpu) {
         current_cpu->crash_occurred = true;
@@ -1694,19 +1703,6 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
                                        !!info, info, &error_abort);
         qemu_system_shutdown_request();
     }
-
-    if (info) {
-        if (info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
-            qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
-                          " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
-                          info->u.hyper_v.data->arg1,
-                          info->u.hyper_v.data->arg2,
-                          info->u.hyper_v.data->arg3,
-                          info->u.hyper_v.data->arg4,
-                          info->u.hyper_v.data->arg5);
-        }
-        qapi_free_GuestPanicInformation(info);
-    }
 }
 
 void qemu_system_reset_request(void)
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] i386/cpu: release GuestPanicInformation memory

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]

On 02/20/2017 12:21 PM, Denis V. Lunev wrote:
> From: Anton Nefedov <anton.nefedov@virtuozzo.com>
> 
> Do not make some foreign function do it.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  kvm-all.c |  5 ++++-
>  vl.c      | 22 +++++++++-------------
>  2 files changed, 13 insertions(+), 14 deletions(-)
> 

> +++ b/vl.c
> @@ -1682,6 +1682,15 @@ void qemu_system_reset(bool report)
>  void qemu_system_guest_panicked(GuestPanicInformation *info)
>  {
>      qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed\n");
> +    if (info && info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
> +                      " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
> +                      info->u.hyper_v.data->arg1,
> +                      info->u.hyper_v.data->arg2,
> +                      info->u.hyper_v.data->arg3,
> +                      info->u.hyper_v.data->arg4,
> +                      info->u.hyper_v.data->arg5);
> +    }

Why are we hoisting this code earlier in the function (which changes the
log order, if I'm reading correctly)...

>  
>      if (current_cpu) {
>          current_cpu->crash_occurred = true;
> @@ -1694,19 +1703,6 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
>                                         !!info, info, &error_abort);
>          qemu_system_shutdown_request();
>      }
> -
> -    if (info) {
> -        if (info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
> -            qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
> -                          " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
> -                          info->u.hyper_v.data->arg1,
> -                          info->u.hyper_v.data->arg2,
> -                          info->u.hyper_v.data->arg3,
> -                          info->u.hyper_v.data->arg4,
> -                          info->u.hyper_v.data->arg5);
> -        }
> -        qapi_free_GuestPanicInformation(info);
> -    }

...instead of just cleaning up this code in place?

But the rest of the patch looks fine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 2/3] qapi: flatten GuestPanicInformation union

[Denis V. Lunev]

From: Anton Nefedov <anton.nefedov@virtuozzo.com>

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 qapi-schema.json  | 12 ++++++++++++
 target/i386/cpu.c | 15 ++++++---------
 vl.c              | 12 ++++++------
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index e9a6364..b142e15 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5872,6 +5872,16 @@
   'data': [ 'pause', 'poweroff' ] }
 
 ##
+# @GuestPanicInformationType:
+#
+# An enumeration of the guest panic information types
+#
+# Since: 2.9
+##
+{ 'enum': 'GuestPanicInformationType',
+  'data': [ 'hyper-v'] }
+
+##
 # @GuestPanicInformation:
 #
 # Information about a guest panic
@@ -5879,6 +5889,8 @@
 # Since: 2.9
 ##
 {'union': 'GuestPanicInformation',
+ 'base': {'type': 'GuestPanicInformationType'},
+ 'discriminator': 'type',
  'data': { 'hyper-v': 'GuestPanicInformationHyperV' } }
 
 ##
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index fd7add2..63be816 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3502,19 +3502,16 @@ static GuestPanicInformation *x86_cpu_get_crash_info(CPUState *cs)
     GuestPanicInformation *panic_info = NULL;
 
     if (env->features[FEAT_HYPERV_EDX] & HV_X64_GUEST_CRASH_MSR_AVAILABLE) {
-        GuestPanicInformationHyperV *panic_info_hv =
-            g_malloc0(sizeof(GuestPanicInformationHyperV));
         panic_info = g_malloc0(sizeof(GuestPanicInformation));
 
-        panic_info->type = GUEST_PANIC_INFORMATION_KIND_HYPER_V;
-        panic_info->u.hyper_v.data = panic_info_hv;
+        panic_info->type = GUEST_PANIC_INFORMATION_TYPE_HYPER_V;
 
         assert(HV_X64_MSR_CRASH_PARAMS >= 5);
-        panic_info_hv->arg1 = env->msr_hv_crash_params[0];
-        panic_info_hv->arg2 = env->msr_hv_crash_params[1];
-        panic_info_hv->arg3 = env->msr_hv_crash_params[2];
-        panic_info_hv->arg4 = env->msr_hv_crash_params[3];
-        panic_info_hv->arg5 = env->msr_hv_crash_params[4];
+        panic_info->u.hyper_v.arg1 = env->msr_hv_crash_params[0];
+        panic_info->u.hyper_v.arg2 = env->msr_hv_crash_params[1];
+        panic_info->u.hyper_v.arg3 = env->msr_hv_crash_params[2];
+        panic_info->u.hyper_v.arg4 = env->msr_hv_crash_params[3];
+        panic_info->u.hyper_v.arg5 = env->msr_hv_crash_params[4];
     }
 
     return panic_info;
diff --git a/vl.c b/vl.c
index 81382b6..2781716 100644
--- a/vl.c
+++ b/vl.c
@@ -1682,14 +1682,14 @@ void qemu_system_reset(bool report)
 void qemu_system_guest_panicked(GuestPanicInformation *info)
 {
     qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed\n");
-    if (info && info->type == GUEST_PANIC_INFORMATION_KIND_HYPER_V) {
+    if (info && info->type == GUEST_PANIC_INFORMATION_TYPE_HYPER_V) {
         qemu_log_mask(LOG_GUEST_ERROR, "HV crash parameters: (%#"PRIx64
                       " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
-                      info->u.hyper_v.data->arg1,
-                      info->u.hyper_v.data->arg2,
-                      info->u.hyper_v.data->arg3,
-                      info->u.hyper_v.data->arg4,
-                      info->u.hyper_v.data->arg5);
+                      info->u.hyper_v.arg1,
+                      info->u.hyper_v.arg2,
+                      info->u.hyper_v.arg3,
+                      info->u.hyper_v.arg4,
+                      info->u.hyper_v.arg5);
     }
 
     if (current_cpu) {
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/3] qapi: flatten GuestPanicInformation union

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

On 02/20/2017 12:21 PM, Denis V. Lunev wrote:
> From: Anton Nefedov <anton.nefedov@virtuozzo.com>
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  qapi-schema.json  | 12 ++++++++++++
>  target/i386/cpu.c | 15 ++++++---------
>  vl.c              | 12 ++++++------
>  3 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index e9a6364..b142e15 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -5872,6 +5872,16 @@
>    'data': [ 'pause', 'poweroff' ] }
>  
>  ##
> +# @GuestPanicInformationType:
> +#
> +# An enumeration of the guest panic information types
> +#
> +# Since: 2.9
> +##
> +{ 'enum': 'GuestPanicInformationType',
> +  'data': [ 'hyper-v'] }

Perhaps 'hyperv' is better? It's the difference between
GUEST_PANIC_INFORMATION_TYPE_HYPER_V and
GUEST_PANIC_INFORMATION_TYPE_HYPERV. But that's bikeshedding, so no need
to change it.

Must go into 2.9, so we aren't baking in bad API.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qapi: flatten GuestPanicInformation union

[Denis V. Lunev]

On 02/20/2017 08:48 PM, Eric Blake wrote:
> On 02/20/2017 12:21 PM, Denis V. Lunev wrote:
>> From: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> ---
>>  qapi-schema.json  | 12 ++++++++++++
>>  target/i386/cpu.c | 15 ++++++---------
>>  vl.c              | 12 ++++++------
>>  3 files changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index e9a6364..b142e15 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -5872,6 +5872,16 @@
>>    'data': [ 'pause', 'poweroff' ] }
>>  
>>  ##
>> +# @GuestPanicInformationType:
>> +#
>> +# An enumeration of the guest panic information types
>> +#
>> +# Since: 2.9
>> +##
>> +{ 'enum': 'GuestPanicInformationType',
>> +  'data': [ 'hyper-v'] }
> Perhaps 'hyperv' is better? It's the difference between
> GUEST_PANIC_INFORMATION_TYPE_HYPER_V and
> GUEST_PANIC_INFORMATION_TYPE_HYPERV. But that's bikeshedding, so no need
> to change it.
>
> Must go into 2.9, so we aren't baking in bad API.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
We can you plain 'hv', like all command line options controlling
HyperV features.

Roman, Evgeniy, do you have any opinion on that?

Den

Re: [Qemu-devel] [PATCH 2/3] qapi: flatten GuestPanicInformation union

[Evgeny Yakovlev]

On 20.02.2017 22:58, Denis V. Lunev wrote:
> On 02/20/2017 08:48 PM, Eric Blake wrote:
>> On 02/20/2017 12:21 PM, Denis V. Lunev wrote:
>>> From: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> ---
>>>   qapi-schema.json  | 12 ++++++++++++
>>>   target/i386/cpu.c | 15 ++++++---------
>>>   vl.c              | 12 ++++++------
>>>   3 files changed, 24 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index e9a6364..b142e15 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -5872,6 +5872,16 @@
>>>     'data': [ 'pause', 'poweroff' ] }
>>>   
>>>   ##
>>> +# @GuestPanicInformationType:
>>> +#
>>> +# An enumeration of the guest panic information types
>>> +#
>>> +# Since: 2.9
>>> +##
>>> +{ 'enum': 'GuestPanicInformationType',
>>> +  'data': [ 'hyper-v'] }
>> Perhaps 'hyperv' is better? It's the difference between
>> GUEST_PANIC_INFORMATION_TYPE_HYPER_V and
>> GUEST_PANIC_INFORMATION_TYPE_HYPERV. But that's bikeshedding, so no need
>> to change it.
>>
>> Must go into 2.9, so we aren't baking in bad API.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
> We can you plain 'hv', like all command line options controlling
> HyperV features.
>
> Roman, Evgeniy, do you have any opinion on that?

In vmbus we've settled on using "hyperv-" prefixes for stuff related to 
QAPI namespace and QOM objects' property names: "hyperv-synic", 
"hyperv-sint-route", etc. We might as well stick to it.

>
> Den

[Qemu-devel] [PATCH 3/3] qmp-events: fix GUEST_PANICKED description formatting

[Denis V. Lunev]

From: Anton Nefedov <anton.nefedov@virtuozzo.com>

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Eric Blake <eblake@redhat.com>
---
 qapi/event.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/event.json b/qapi/event.json
index 970ff02..e02852c 100644
--- a/qapi/event.json
+++ b/qapi/event.json
@@ -488,9 +488,9 @@
 #
 # @action: action that has been taken, currently always "pause"
 #
-# @info: optional information about a panic
+# @info: #optional information about a panic (since 2.9)
 #
-# Since: 1.5 (@info since 2.9)
+# Since: 1.5
 #
 # Example:
 #
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/3] qmp-events: fix GUEST_PANICKED description formatting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

On 02/20/2017 12:21 PM, Denis V. Lunev wrote:
> From: Anton Nefedov <anton.nefedov@virtuozzo.com>
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  qapi/event.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for 2.9 0/3] guest panic information follow-up

[Paolo Bonzini]

On 20/02/2017 19:21, Denis V. Lunev wrote:
> This is a set of followup patches requested by Eric Blake.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> 
> Anton Nefedov (3):
>   i386/cpu: release GuestPanicInformation memory
>   qapi: flatten GuestPanicInformation union
>   qmp-events: fix GUEST_PANICKED description formatting
> 
>  kvm-all.c         |  5 ++++-
>  qapi-schema.json  | 12 ++++++++++++
>  qapi/event.json   |  4 ++--
>  target/i386/cpu.c | 15 ++++++---------
>  vl.c              | 22 +++++++++-------------
>  5 files changed, 33 insertions(+), 25 deletions(-)
> 

I am queuing patches 2 and 3.  For patch 1, I've just sent an
alternative idea.

Paolo