[Qemu-devel] [PATCH v14 0/4] pvevent device to deal with guest panic event

[Qemu-devel] [PATCH v14 0/4] pvevent device to deal with guest panic event

[Hu Tao]

This series introduces a new simulated device, pvevent, to notify
qemu when guest panic event happens.

Along with this series, there are two patches to add seabios ACPI
driver and kernel ACPI driver for the device, respectively.

TODO: make the io port used by pvevent device configurable.

Tested with:

  - qemu(kvm)
  - qemu(tcg)
  - piix
  - q35

Changes from v13:

  - communicate the io port with guest by ACPI, instead of hardcoding it (Marcelo)
  - panic event action is always 'pause' (Paolo)
  - not kvm-specific (Gleb)
  - make the device target-independent (Paolo)
  - no runstate save/load(will be a seperate patch) (Paolo)
  - support q35 (Gleb)
  - doc about qmp event (Eric)
  - doc about pvevent device (Paolo)
 
v13: http://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg05361.html

Hu Tao (4):
  add a new runstate: RUN_STATE_GUEST_PANICKED
  add a new qevent: QEVENT_GUEST_PANICKED
  introduce a new qom device to deal with panicked event
  pv event: add document to describe the usage

 QMP/qmp-events.txt        |  14 ++++++
 docs/pvevent.txt          |  14 ++++++
 hw/Makefile.objs          |   2 +
 hw/pvevent.c              | 116 ++++++++++++++++++++++++++++++++++++++++++++++
 include/monitor/monitor.h |   1 +
 include/sysemu/sysemu.h   |   1 +
 monitor.c                 |   1 +
 qapi-schema.json          |   5 +-
 qmp.c                     |   3 +-
 vl.c                      |  13 +++++-
 10 files changed, 165 insertions(+), 5 deletions(-)
 create mode 100644 docs/pvevent.txt
 create mode 100644 hw/pvevent.c

-- 
1.8.1.4

[Qemu-devel] [PATCH v14 1/4] add a new runstate: RUN_STATE_GUEST_PANICKED

[Hu Tao]

The guest will be in this state when it is panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 include/sysemu/sysemu.h |  1 +
 qapi-schema.json        |  5 ++++-
 qmp.c                   |  3 +--
 vl.c                    | 13 +++++++++++--
 4 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b19ec95..0412a8a 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -22,6 +22,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 int runstate_is_running(void);
+bool runstate_needs_reset(void);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 28b070f..003cbf2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -174,11 +174,14 @@
 # @suspended: guest is suspended (ACPI S3)
 #
 # @watchdog: the watchdog action is configured to pause and has been triggered
+#
+# @guest-panicked: guest has been panicked as a result of guest OS panic
 ##
 { 'enum': 'RunState',
   'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
             'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
-            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
+            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
+            'guest-panicked' ] }
 
 ##
 # @SnapshotInfo
diff --git a/qmp.c b/qmp.c
index 55b056b..a1ebb5d 100644
--- a/qmp.c
+++ b/qmp.c
@@ -149,8 +149,7 @@ void qmp_cont(Error **errp)
 {
     Error *local_err = NULL;
 
-    if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-               runstate_check(RUN_STATE_SHUTDOWN)) {
+    if (runstate_needs_reset()) {
         error_set(errp, QERR_RESET_REQUIRED);
         return;
     } else if (runstate_check(RUN_STATE_SUSPENDED)) {
diff --git a/vl.c b/vl.c
index c03edf1..926822b 100644
--- a/vl.c
+++ b/vl.c
@@ -566,6 +566,7 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_RUNNING, RUN_STATE_SAVE_VM },
     { RUN_STATE_RUNNING, RUN_STATE_SHUTDOWN },
     { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
+    { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
 
     { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
 
@@ -580,6 +581,8 @@ static const RunStateTransition runstate_transitions_def[] = {
     { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
     { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
 
+    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
+
     { RUN_STATE_MAX, RUN_STATE_MAX },
 };
 
@@ -621,6 +624,13 @@ int runstate_is_running(void)
     return runstate_check(RUN_STATE_RUNNING);
 }
 
+bool runstate_needs_reset(void)
+{
+    return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
+        runstate_check(RUN_STATE_SHUTDOWN) ||
+        runstate_check(RUN_STATE_GUEST_PANICKED);
+}
+
 StatusInfo *qmp_query_status(Error **errp)
 {
     StatusInfo *info = g_malloc0(sizeof(*info));
@@ -1966,8 +1976,7 @@ static bool main_loop_should_exit(void)
         cpu_synchronize_all_states();
         qemu_system_reset(VMRESET_REPORT);
         resume_all_vcpus();
-        if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-            runstate_check(RUN_STATE_SHUTDOWN)) {
+        if (runstate_needs_reset()) {
             runstate_set(RUN_STATE_PAUSED);
         }
     }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v14 1/4] add a new runstate: RUN_STATE_GUEST_PANICKED

[Markus Armbruster]

Hu Tao <hutao@cn.fujitsu.com> writes:

> The guest will be in this state when it is panicked.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  include/sysemu/sysemu.h |  1 +
>  qapi-schema.json        |  5 ++++-
>  qmp.c                   |  3 +--
>  vl.c                    | 13 +++++++++++--
>  4 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index b19ec95..0412a8a 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -22,6 +22,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
>  bool runstate_check(RunState state);
>  void runstate_set(RunState new_state);
>  int runstate_is_running(void);
> +bool runstate_needs_reset(void);
>  typedef struct vm_change_state_entry VMChangeStateEntry;
>  typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 28b070f..003cbf2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -174,11 +174,14 @@
>  # @suspended: guest is suspended (ACPI S3)
>  #
>  # @watchdog: the watchdog action is configured to pause and has been triggered
> +#
> +# @guest-panicked: guest has been panicked as a result of guest OS panic
>  ##
>  { 'enum': 'RunState',
>    'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> -            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
> +            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> +            'guest-panicked' ] }
>  
>  ##
>  # @SnapshotInfo

RUN_STATE_GUEST_PANICKED is similar to RUN_STATE_INTERNAL_ERROR and
RUN_STATE_SHUTDOWN: the only way for the guest to continue is reset.
Correct?

> diff --git a/qmp.c b/qmp.c
> index 55b056b..a1ebb5d 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -149,8 +149,7 @@ void qmp_cont(Error **errp)
>  {
>      Error *local_err = NULL;
>  
> -    if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -               runstate_check(RUN_STATE_SHUTDOWN)) {
> +    if (runstate_needs_reset()) {
>          error_set(errp, QERR_RESET_REQUIRED);
>          return;
>      } else if (runstate_check(RUN_STATE_SUSPENDED)) {
> diff --git a/vl.c b/vl.c
> index c03edf1..926822b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -566,6 +566,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_RUNNING, RUN_STATE_SAVE_VM },
>      { RUN_STATE_RUNNING, RUN_STATE_SHUTDOWN },
>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
> +    { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>  
>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>  

This permits runstate_set(RUN_STATE_GUEST_PANICKED) in
RUN_STATE_RUNNING.  Used in PATCH 3/4.  Good.

> @@ -580,6 +581,8 @@ static const RunStateTransition runstate_transitions_def[] = {
>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>  
> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
> +
>      { RUN_STATE_MAX, RUN_STATE_MAX },
>  };

This permits runstate_set(RUN_STATE_PAUSED) in RUN_STATE_GUEST_PANICKED.
An example for such a transition is in the last patch hunk: main loop
resetting the guest.

Let's examine the other transitions to RUN_STATE_PAUSED, and whether
they can now come from RUN_STATE_GUEST_PANICKED:

* gdb_read_byte()

  No, because vm_stop() is protected by runstate_is_running() here.

* gdb_chr_event() case CHR_EVENT_OPENED

  Can this happen in RUN_STATE_GUEST_PANICKED?  Jan?

  What about RUN_STATE_INTERNAL_ERROR and RUN_STATE_SHUTDOWN?
  
* gdb_sigterm_handler()

  No, because vm_stop() is protected by runstate_is_running() here.

  Aside: despite its name, this function handles SIGINT.  Ugh.

* process_incoming_migration_co()

  No, because we're in RUN_STATE_INMIGRATE here, aren't we?  Juan?

* qmp_stop()

  No, because vm_stop() calls do_vm_stop() to do the actual state
  transition, which protects it by runstate_is_running().

  We can ignore the conditional, it merely punts the vm_stop() to the
  main loop.

Next question: RUN_STATE_INTERNAL_ERROR and RUN_STATE_SHUTDOWN may go to
RUN_STATE_FINISH_MIGRATE, but RUN_STATE_GUEST_PANICKED may not.  Why?

>  
> @@ -621,6 +624,13 @@ int runstate_is_running(void)
>      return runstate_check(RUN_STATE_RUNNING);
>  }
>  
> +bool runstate_needs_reset(void)
> +{
> +    return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> +        runstate_check(RUN_STATE_SHUTDOWN) ||
> +        runstate_check(RUN_STATE_GUEST_PANICKED);
> +}
> +
>  StatusInfo *qmp_query_status(Error **errp)
>  {
>      StatusInfo *info = g_malloc0(sizeof(*info));
> @@ -1966,8 +1976,7 @@ static bool main_loop_should_exit(void)
>          cpu_synchronize_all_states();
>          qemu_system_reset(VMRESET_REPORT);
>          resume_all_vcpus();
> -        if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> -            runstate_check(RUN_STATE_SHUTDOWN)) {
> +        if (runstate_needs_reset()) {
>              runstate_set(RUN_STATE_PAUSED);
>          }
>      }

Re: [Qemu-devel] [PATCH v14 1/4] add a new runstate: RUN_STATE_GUEST_PANICKED

[Paolo Bonzini]

Il 20/03/2013 09:58, Markus Armbruster ha scritto:
> Let's examine the other transitions to RUN_STATE_PAUSED, and whether
> they can now come from RUN_STATE_GUEST_PANICKED:
> 
> * process_incoming_migration_co()
> 
>   No, because we're in RUN_STATE_INMIGRATE here, aren't we?  Juan?

Yes.

> * qmp_stop()
> 
>   No, because vm_stop() calls do_vm_stop() to do the actual state
>   transition, which protects it by runstate_is_running().
> 
>   We can ignore the conditional, it merely punts the vm_stop() to the
>   main loop.
> 
> Next question: RUN_STATE_INTERNAL_ERROR and RUN_STATE_SHUTDOWN may go to
> RUN_STATE_FINISH_MIGRATE, but RUN_STATE_GUEST_PANICKED may not.  Why?

RUN_STATE_FINISH_MIGRATE is reached with vm_stop_force_state, so every
state can go there.  Next question: why doesn't the switch to
RUN_STATE_SAVE_VM use vm_stop_force_state?

Next question: almost all states go to RUN_STATE_FINISH_MIGRATE, the
same would hold for RUN_STATE_SAVE_VM if it started using
vm_stop_force_state.  There are few exceptions, and I'm not even sure
all of them are correct (why can't RUN_STATE_DEBUG go to
RUN_STATE_FINISH_MIGRATE?).

Should vm_stop_force_state override the runstate check (either directly,
or by interposing a transition to RUN_STATE_PAUSED? The few outliers can
be handled with manually-placed assertions.

Paolo

Re: [Qemu-devel] [PATCH v14 1/4] add a new runstate: RUN_STATE_GUEST_PANICKED

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Hu Tao <hutao@cn.fujitsu.com> writes:
>
>> The guest will be in this state when it is panicked.
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> ---
>>  include/sysemu/sysemu.h |  1 +
>>  qapi-schema.json        |  5 ++++-
>>  qmp.c                   |  3 +--
>>  vl.c                    | 13 +++++++++++--
>>  4 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>> index b19ec95..0412a8a 100644
>> --- a/include/sysemu/sysemu.h
>> +++ b/include/sysemu/sysemu.h
>> @@ -22,6 +22,7 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid);
>>  bool runstate_check(RunState state);
>>  void runstate_set(RunState new_state);
>>  int runstate_is_running(void);
>> +bool runstate_needs_reset(void);
>>  typedef struct vm_change_state_entry VMChangeStateEntry;
>>  typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
>>  
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 28b070f..003cbf2 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -174,11 +174,14 @@
>>  # @suspended: guest is suspended (ACPI S3)
>>  #
>>  # @watchdog: the watchdog action is configured to pause and has been triggered
>> +#
>> +# @guest-panicked: guest has been panicked as a result of guest OS panic
>>  ##
>>  { 'enum': 'RunState',
>>    'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>>              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
>> -            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
>> +            'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
>> +            'guest-panicked' ] }
>>  
>>  ##
>>  # @SnapshotInfo
>
> RUN_STATE_GUEST_PANICKED is similar to RUN_STATE_INTERNAL_ERROR and
> RUN_STATE_SHUTDOWN: the only way for the guest to continue is reset.
> Correct?
>
>> diff --git a/qmp.c b/qmp.c
>> index 55b056b..a1ebb5d 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -149,8 +149,7 @@ void qmp_cont(Error **errp)
>>  {
>>      Error *local_err = NULL;
>>  
>> -    if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>> -               runstate_check(RUN_STATE_SHUTDOWN)) {
>> +    if (runstate_needs_reset()) {
>>          error_set(errp, QERR_RESET_REQUIRED);
>>          return;
>>      } else if (runstate_check(RUN_STATE_SUSPENDED)) {
>> diff --git a/vl.c b/vl.c
>> index c03edf1..926822b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -566,6 +566,7 @@ static const RunStateTransition runstate_transitions_def[] = {
>>      { RUN_STATE_RUNNING, RUN_STATE_SAVE_VM },
>>      { RUN_STATE_RUNNING, RUN_STATE_SHUTDOWN },
>>      { RUN_STATE_RUNNING, RUN_STATE_WATCHDOG },
>> +    { RUN_STATE_RUNNING, RUN_STATE_GUEST_PANICKED },
>>  
>>      { RUN_STATE_SAVE_VM, RUN_STATE_RUNNING },
>>  
>
> This permits runstate_set(RUN_STATE_GUEST_PANICKED) in
> RUN_STATE_RUNNING.  Used in PATCH 3/4.  Good.
>
>> @@ -580,6 +581,8 @@ static const RunStateTransition runstate_transitions_def[] = {
>>      { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING },
>>      { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE },
>>  
>> +    { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED },
>> +
>>      { RUN_STATE_MAX, RUN_STATE_MAX },
>>  };
>
> This permits runstate_set(RUN_STATE_PAUSED) in RUN_STATE_GUEST_PANICKED.
> An example for such a transition is in the last patch hunk: main loop
> resetting the guest.
>
> Let's examine the other transitions to RUN_STATE_PAUSED, and whether
> they can now come from RUN_STATE_GUEST_PANICKED:
>
> * gdb_read_byte()
>
>   No, because vm_stop() is protected by runstate_is_running() here.
>
> * gdb_chr_event() case CHR_EVENT_OPENED
>
>   Can this happen in RUN_STATE_GUEST_PANICKED?  Jan?
>
>   What about RUN_STATE_INTERNAL_ERROR and RUN_STATE_SHUTDOWN?

Nevermind this one.  Like for qmp_stop() below, the actual state
transition is protected by runstate_is_running().

> * gdb_sigterm_handler()
>
>   No, because vm_stop() is protected by runstate_is_running() here.
>
>   Aside: despite its name, this function handles SIGINT.  Ugh.
>
> * process_incoming_migration_co()
>
>   No, because we're in RUN_STATE_INMIGRATE here, aren't we?  Juan?
>
> * qmp_stop()
>
>   No, because vm_stop() calls do_vm_stop() to do the actual state
>   transition, which protects it by runstate_is_running().
>
>   We can ignore the conditional, it merely punts the vm_stop() to the
>   main loop.
[...]

[Qemu-devel] [PATCH v14 2/4] add a new qevent: QEVENT_GUEST_PANICKED

[Hu Tao]

This event will be emited when the guest is panicked.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 QMP/qmp-events.txt        | 14 ++++++++++++++
 include/monitor/monitor.h |  1 +
 monitor.c                 |  1 +
 3 files changed, 16 insertions(+)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index b2698e4..62fffad 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -428,3 +428,17 @@ Example:
 
 Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
 followed respectively by the RESET, SHUTDOWN, or STOP events.
+
+GUEST_PANICKED
+--------------
+
+Emitted when the guest OS is panicked.
+
+Data:
+
+- "action": Action that has been taken. Currently it's always "pause".
+
+Example:
+
+{ "event": "GUEST_PANICKED",
+     "data": { "action": "pause" } }
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 87fb49c..4006905 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -45,6 +45,7 @@ typedef enum MonitorEvent {
     QEVENT_WAKEUP,
     QEVENT_BALLOON_CHANGE,
     QEVENT_SPICE_MIGRATE_COMPLETED,
+    QEVENT_GUEST_PANICKED,
 
     /* Add to 'monitor_event_names' array in monitor.c when
      * defining new events here */
diff --git a/monitor.c b/monitor.c
index 32a6e74..5b7d7f9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -463,6 +463,7 @@ static const char *monitor_event_names[] = {
     [QEVENT_WAKEUP] = "WAKEUP",
     [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
     [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
+    [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED",
 };
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v14 2/4] add a new qevent: QEVENT_GUEST_PANICKED

[Markus Armbruster]

Hu Tao <hutao@cn.fujitsu.com> writes:

> This event will be emited when the guest is panicked.
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  QMP/qmp-events.txt        | 14 ++++++++++++++
>  include/monitor/monitor.h |  1 +
>  monitor.c                 |  1 +
>  3 files changed, 16 insertions(+)
>
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index b2698e4..62fffad 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -428,3 +428,17 @@ Example:
>  
>  Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
>  followed respectively by the RESET, SHUTDOWN, or STOP events.
> +
> +GUEST_PANICKED
> +--------------
> +
> +Emitted when the guest OS is panicked.

This suggests we reliably emit the event on guest panic.  That's not
accurate; we emit it only when the guest manages to tell us.  Better
clarify.  Perhaps "Emitted when we detect a guest OS panic" suffices.

> +
> +Data:
> +
> +- "action": Action that has been taken. Currently it's always "pause".

Please make that

- "action": Action that has been taken (json-string, currently always "pause")

> +
> +Example:
> +
> +{ "event": "GUEST_PANICKED",
> +     "data": { "action": "pause" } }
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 87fb49c..4006905 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -45,6 +45,7 @@ typedef enum MonitorEvent {
>      QEVENT_WAKEUP,
>      QEVENT_BALLOON_CHANGE,
>      QEVENT_SPICE_MIGRATE_COMPLETED,
> +    QEVENT_GUEST_PANICKED,
>  
>      /* Add to 'monitor_event_names' array in monitor.c when
>       * defining new events here */
> diff --git a/monitor.c b/monitor.c
> index 32a6e74..5b7d7f9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -463,6 +463,7 @@ static const char *monitor_event_names[] = {
>      [QEVENT_WAKEUP] = "WAKEUP",
>      [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
>      [QEVENT_SPICE_MIGRATE_COMPLETED] = "SPICE_MIGRATE_COMPLETED",
> +    [QEVENT_GUEST_PANICKED] = "GUEST_PANICKED",
>  };
>  QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)

[Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Hu Tao]

pvevent device is used to send guest panic event from guest to qemu.

When guest panic happens, pvevent device driver will write a event
number to IO port 0x505(which is the IO port occupied by pvevent device,
by default). On receiving the event, pvevent device will pause guest
cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  

TODO: make the IO port configurable

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 hw/Makefile.objs |   2 +
 hw/pvevent.c     | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 hw/pvevent.c

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 40ebe46..edf499e 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -218,5 +218,7 @@ obj-$(CONFIG_KVM) += ivshmem.o
 obj-$(CONFIG_LINUX) += vfio_pci.o
 endif
 
+common-obj-y += pvevent.o
+
 $(obj)/baum.o: QEMU_CFLAGS += $(SDL_CFLAGS) 
 endif
diff --git a/hw/pvevent.c b/hw/pvevent.c
new file mode 100644
index 0000000..c7df77b
--- /dev/null
+++ b/hw/pvevent.c
@@ -0,0 +1,116 @@
+/*
+ * QEMU simulated pvevent device.
+ *
+ * Copyright Fujitsu, Corp. 2013
+ *
+ * Authors:
+ *     Wen Congyang <wency@cn.fujitsu.com>
+ *     Hu Tao <hutao@cn.fujitsu.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include <qapi/qmp/qobject.h>
+#include <qapi/qmp/qjson.h>
+#include <monitor/monitor.h>
+#include <sysemu/sysemu.h>
+#include <sysemu/kvm.h>
+
+/* The bit of supported pv event */
+#define PVEVENT_F_PANICKED      0
+
+/* The pv event value */
+#define PVEVENT_PANICKED        (1 << PVEVENT_F_PANICKED)
+
+#define TYPE_ISA_PVEVENT_DEVICE    "pvevent"
+#define ISA_PVEVENT_DEVICE(obj)    \
+    OBJECT_CHECK(PVEventState, (obj), TYPE_ISA_PVEVENT_DEVICE)
+
+static void panicked_mon_event(const char *action)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'action': %s }", action);
+    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
+    qobject_decref(data);
+}
+
+static void handle_event(int event)
+{
+    if (event == PVEVENT_PANICKED) {
+        panicked_mon_event("pause");
+        vm_stop(RUN_STATE_GUEST_PANICKED);
+        return;
+    }
+}
+
+#include "hw/isa.h"
+
+typedef struct PVEventState {
+    ISADevice parent_obj;
+
+    MemoryRegion io;
+    uint16_t ioport;
+} PVEventState;
+
+/* return supported events on read */
+static uint64_t pvevent_ioport_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return PVEVENT_PANICKED;
+}
+
+static void pvevent_ioport_write(void *opaque, hwaddr addr, uint64_t val,
+                                 unsigned size)
+{
+    handle_event(val);
+}
+
+static const MemoryRegionOps pvevent_ops = {
+    .read = pvevent_ioport_read,
+    .write = pvevent_ioport_write,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static int pvevent_isa_initfn(ISADevice *dev)
+{
+    PVEventState *s = ISA_PVEVENT_DEVICE(dev);
+
+    memory_region_init_io(&s->io, &pvevent_ops, s, "pvevent", 1);
+    isa_register_ioport(dev, &s->io, s->ioport);
+
+    return 0;
+}
+
+static Property pvevent_isa_properties[] = {
+    DEFINE_PROP_UINT16("ioport", PVEventState, ioport, 0x505),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pvevent_isa_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISADeviceClass *ic = ISA_DEVICE_CLASS(klass);
+
+    ic->init = pvevent_isa_initfn;
+    dc->no_user = 1;
+    dc->props = pvevent_isa_properties;
+}
+
+static TypeInfo pvevent_isa_info = {
+    .name          = TYPE_ISA_PVEVENT_DEVICE,
+    .parent        = TYPE_ISA_DEVICE,
+    .instance_size = sizeof(PVEventState),
+    .class_init    = pvevent_isa_class_init,
+};
+
+static void pvevent_register_types(void)
+{
+    type_register_static(&pvevent_isa_info);
+}
+
+type_init(pvevent_register_types)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 09:15, Hu Tao ha scritto:
> pvevent device is used to send guest panic event from guest to qemu.
> 
> When guest panic happens, pvevent device driver will write a event
> number to IO port 0x505(which is the IO port occupied by pvevent device,
> by default). On receiving the event, pvevent device will pause guest
> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> 
> TODO: make the IO port configurable

The port is already configurable as far as the device is concerned; when
you add the port to the PC boards you will have to wind up fw-cfg.

So these patches can go in already, with the sole change that the HID
must be in the QEMU namespace rather than MSFT.

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 09:15, Hu Tao ha scritto:
> > pvevent device is used to send guest panic event from guest to qemu.
> > 
> > When guest panic happens, pvevent device driver will write a event
> > number to IO port 0x505(which is the IO port occupied by pvevent device,
> > by default). On receiving the event, pvevent device will pause guest
> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> > 
> > TODO: make the IO port configurable
> 
> The port is already configurable as far as the device is concerned; when
> you add the port to the PC boards you will have to wind up fw-cfg.
> 
Why not add fw-cfg when device is created (with -device for instance)?

> So these patches can go in already, with the sole change that the HID
> must be in the QEMU namespace rather than MSFT.
> 
We should request one for QEMU.

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 10:19, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>> pvevent device is used to send guest panic event from guest to qemu.
>>>
>>> When guest panic happens, pvevent device driver will write a event
>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>> by default). On receiving the event, pvevent device will pause guest
>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>
>>> TODO: make the IO port configurable
>>
>> The port is already configurable as far as the device is concerned; when
>> you add the port to the PC boards you will have to wind up fw-cfg.
>
> Why not add fw-cfg when device is created (with -device for instance)?

It depends on what we decide is the supported interface for the device:

* it can be an ISA device; the interface is the I/O port and ACPI
support is provided just for convenience of the OSPM.  In this case,
"-device pvevent" should just add handlers for the port.  The ACPI
support is similar to what we do for other on-board ISA devices, for
example serial ports (the serial ports use PIIX PCI configuration
instead of fw-cfg, but that's a minor detail).  It only needs to work
for port 0x505, so the fw-cfg data can be a single yes/no value and only
the _STA method needs patching.  See piix4_pm_machine_ready in
hw/acpi_piix4.c.

* ACPI support is a first-class part of the device.  Each instance of
the device should be there in the ACPI tables.  In this case the fw-cfg
data needs to be a list of ports, and it is probably simpler to combine
all the definitions in an SSDT that is dynamically-built (similar to
what we do for PCI hotplug slots).  Or even provide a separate SSDT for
each instance of the device.


I prefer the first, the second seems to be over-engineered.

>> So these patches can go in already, with the sole change that the HID
>> must be in the QEMU namespace rather than MSFT.
>>
> We should request one for QEMU.

Yes.  BTW, the QEMU patches can go in only in the first of the two cases
above.

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Alexander Graf]

On 14.03.2013, at 10:43, Paolo Bonzini wrote:

> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>> 
>>>> When guest panic happens, pvevent device driver will write a event
>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>> by default). On receiving the event, pvevent device will pause guest
>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>> 
>>>> TODO: make the IO port configurable
>>> 
>>> The port is already configurable as far as the device is concerned; when
>>> you add the port to the PC boards you will have to wind up fw-cfg.
>> 
>> Why not add fw-cfg when device is created (with -device for instance)?
> 
> It depends on what we decide is the supported interface for the device:
> 
> * it can be an ISA device; the interface is the I/O port and ACPI

Is there any particular reason it's an ISA device with a PIO port, rather than a platform / sysbus device with MMIO access? With the latter, we could easily reuse the device on other platforms (ppc, arm) and even the guest driver code for platforms that do ACPI (arm?).

Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?


Alex

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 12:00, Alexander Graf ha scritto:
> 
> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
> 
>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>>>
>>>>> When guest panic happens, pvevent device driver will write a event
>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>>> by default). On receiving the event, pvevent device will pause guest
>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>>>
>>>>> TODO: make the IO port configurable
>>>>
>>>> The port is already configurable as far as the device is concerned; when
>>>> you add the port to the PC boards you will have to wind up fw-cfg.
>>>
>>> Why not add fw-cfg when device is created (with -device for instance)?
>>
>> It depends on what we decide is the supported interface for the device:
>>
>> * it can be an ISA device; the interface is the I/O port and ACPI
> 
> Is there any particular reason it's an ISA device with a PIO port,
> rather than a platform / sysbus device with MMIO access? With the
> latter, we could easily reuse the device on other platforms (ppc, arm)
> and even the guest driver code for platforms that do ACPI (arm?).

Where would you place the MMIO area on x86?  But anyway you can easily
define an MMIO variant, the guest driver code will be shared (the ACPI
in the firmware no, of course).

> Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?

I think Xen uses xenstore for this, or a hypercall I don't remember.
But not something that can be shared unfortunately.

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Alexander Graf]

On 14.03.2013, at 12:03, Paolo Bonzini wrote:

> Il 14/03/2013 12:00, Alexander Graf ha scritto:
>> 
>> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
>> 
>>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>>>> 
>>>>>> When guest panic happens, pvevent device driver will write a event
>>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>>>> by default). On receiving the event, pvevent device will pause guest
>>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>>>> 
>>>>>> TODO: make the IO port configurable
>>>>> 
>>>>> The port is already configurable as far as the device is concerned; when
>>>>> you add the port to the PC boards you will have to wind up fw-cfg.
>>>> 
>>>> Why not add fw-cfg when device is created (with -device for instance)?
>>> 
>>> It depends on what we decide is the supported interface for the device:
>>> 
>>> * it can be an ISA device; the interface is the I/O port and ACPI
>> 
>> Is there any particular reason it's an ISA device with a PIO port,
>> rather than a platform / sysbus device with MMIO access? With the
>> latter, we could easily reuse the device on other platforms (ppc, arm)
>> and even the guest driver code for platforms that do ACPI (arm?).
> 
> Where would you place the MMIO area on x86?  

Wherever the board thinks it makes sense.

> But anyway you can easily
> define an MMIO variant, the guest driver code will be shared (the ACPI
> in the firmware no, of course).

Yes, at which point we have 2 variants where we could have had 1. I don't know if it's worth caring about it, just wanted to bring it up.

> 
>> Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?
> 
> I think Xen uses xenstore for this, or a hypercall I don't remember.
> But not something that can be shared unfortunately.

At least the guest kernel hook could be shared. In fact, how does that one work with this device? I've only seen an ACPI patch so far. Does ACPI already support panic hooks?


Alex

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 12:23:01PM +0100, Alexander Graf wrote:
> 
> On 14.03.2013, at 12:03, Paolo Bonzini wrote:
> 
> > Il 14/03/2013 12:00, Alexander Graf ha scritto:
> >> 
> >> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
> >> 
> >>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
> >>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> >>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
> >>>>>> pvevent device is used to send guest panic event from guest to qemu.
> >>>>>> 
> >>>>>> When guest panic happens, pvevent device driver will write a event
> >>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
> >>>>>> by default). On receiving the event, pvevent device will pause guest
> >>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> >>>>>> 
> >>>>>> TODO: make the IO port configurable
> >>>>> 
> >>>>> The port is already configurable as far as the device is concerned; when
> >>>>> you add the port to the PC boards you will have to wind up fw-cfg.
> >>>> 
> >>>> Why not add fw-cfg when device is created (with -device for instance)?
> >>> 
> >>> It depends on what we decide is the supported interface for the device:
> >>> 
> >>> * it can be an ISA device; the interface is the I/O port and ACPI
> >> 
> >> Is there any particular reason it's an ISA device with a PIO port,
> >> rather than a platform / sysbus device with MMIO access? With the
> >> latter, we could easily reuse the device on other platforms (ppc, arm)
> >> and even the guest driver code for platforms that do ACPI (arm?).
> > 
> > Where would you place the MMIO area on x86?  
> 
> Wherever the board thinks it makes sense.
> 
On x86 it makes sense to put it in IO space :)

> > But anyway you can easily
> > define an MMIO variant, the guest driver code will be shared (the ACPI
> > in the firmware no, of course).
> 
> Yes, at which point we have 2 variants where we could have had 1. I don't know if it's worth caring about it, just wanted to bring it up.
> 
We can have the same device and control via properties what address
space to use for the device.

> > 
> >> Also, don't the Xen guys already have a similar interface? Could we at least share the guest side implementation maybe?
> > 
> > I think Xen uses xenstore for this, or a hypercall I don't remember.
> > But not something that can be shared unfortunately.
> 
> At least the guest kernel hook could be shared. In fact, how does that one work with this device? I've only seen an ACPI patch so far. Does ACPI already support panic hooks?
> 
> 
No need for any special hook to use the device. There was a separate
patch with guest platform driver that uses the device. IN fact it should
work on XEN too.

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Markus Armbruster]

Alexander Graf <agraf@suse.de> writes:

> On 14.03.2013, at 10:43, Paolo Bonzini wrote:
>
>> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 09:15, Hu Tao ha scritto:
>>>>> pvevent device is used to send guest panic event from guest to qemu.
>>>>> 
>>>>> When guest panic happens, pvevent device driver will write a event
>>>>> number to IO port 0x505(which is the IO port occupied by pvevent device,
>>>>> by default). On receiving the event, pvevent device will pause guest
>>>>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>>>>> 
>>>>> TODO: make the IO port configurable
>>>> 
>>>> The port is already configurable as far as the device is concerned; when
>>>> you add the port to the PC boards you will have to wind up fw-cfg.
>>> 
>>> Why not add fw-cfg when device is created (with -device for instance)?
>> 
>> It depends on what we decide is the supported interface for the device:
>> 
>> * it can be an ISA device; the interface is the I/O port and ACPI
>
> Is there any particular reason it's an ISA device with a PIO port,
> rather than a platform / sysbus device with MMIO access? With the
> latter, we could easily reuse the device on other platforms (ppc, arm)
> and even the guest driver code for platforms that do ACPI (arm?).

As Paolo and Gleb observed, this is the most natural way to do such a
device on x86.  Other systems may want different connectors,
e.g. sysbus/MMIO.

We already have devices coming in variants with different connectors,
e.g. serial and fdc.  They share core code just fine.  Some boilerplate
gets duplicated, but it's not horrible.

There has been talk about modelling such things more nicely in QOM.
Opportunity for a QOM expert to tell us more about it :)

[...]

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 10:43:43AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 10:19, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 09:15, Hu Tao ha scritto:
> >>> pvevent device is used to send guest panic event from guest to qemu.
> >>>
> >>> When guest panic happens, pvevent device driver will write a event
> >>> number to IO port 0x505(which is the IO port occupied by pvevent device,
> >>> by default). On receiving the event, pvevent device will pause guest
> >>> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> >>>
> >>> TODO: make the IO port configurable
> >>
> >> The port is already configurable as far as the device is concerned; when
> >> you add the port to the PC boards you will have to wind up fw-cfg.
> >
> > Why not add fw-cfg when device is created (with -device for instance)?
> 
> It depends on what we decide is the supported interface for the device:
> 
> * it can be an ISA device; the interface is the I/O port and ACPI
> support is provided just for convenience of the OSPM.  In this case,
> "-device pvevent" should just add handlers for the port.  The ACPI
> support is similar to what we do for other on-board ISA devices, for
> example serial ports (the serial ports use PIIX PCI configuration
> instead of fw-cfg, but that's a minor detail).  It only needs to work
> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> the _STA method needs patching.  See piix4_pm_machine_ready in
> hw/acpi_piix4.c.
> 
Again I think there is a big difference between well knows device and
PV devices that we add at random location. And if we make the later
configurable i.e it may or may not be present and location where it is
present can be changed then we better not make a guest to do guesses.

> * ACPI support is a first-class part of the device.  Each instance of
> the device should be there in the ACPI tables.  In this case the fw-cfg
> data needs to be a list of ports, and it is probably simpler to combine
> all the definitions in an SSDT that is dynamically-built (similar to
> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> each instance of the device.
> 
> 
> I prefer the first, the second seems to be over-engineered.
> 
Second is over-engineering indeed. The device should be singleton and
fail if second instance is created. Do we have such capability in qdev?

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>> * it can be an ISA device; the interface is the I/O port and ACPI
>> support is provided just for convenience of the OSPM.  In this case,
>> "-device pvevent" should just add handlers for the port.  The ACPI
>> support is similar to what we do for other on-board ISA devices, for
>> example serial ports (the serial ports use PIIX PCI configuration
>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>> the _STA method needs patching.  See piix4_pm_machine_ready in
>> hw/acpi_piix4.c.
>
> Again I think there is a big difference between well knows device and
> PV devices that we add at random location. And if we make the later
> configurable i.e it may or may not be present and location where it is
> present can be changed then we better not make a guest to do guesses.

No guesses here on part of the guest, and no probing in the firmware
two.  The same number is hard-coded in QEMU and the DSDT, which go in
pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
trick with the methods).

I think it's a nice compromise.

>> * ACPI support is a first-class part of the device.  Each instance of
>> the device should be there in the ACPI tables.  In this case the fw-cfg
>> data needs to be a list of ports, and it is probably simpler to combine
>> all the definitions in an SSDT that is dynamically-built (similar to
>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>> each instance of the device.
>>
>> I prefer the first, the second seems to be over-engineered.
>>
> Second is over-engineering indeed. The device should be singleton and
> fail if second instance is created. Do we have such capability in qdev?

No, but why should it fail?

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >> * it can be an ISA device; the interface is the I/O port and ACPI
> >> support is provided just for convenience of the OSPM.  In this case,
> >> "-device pvevent" should just add handlers for the port.  The ACPI
> >> support is similar to what we do for other on-board ISA devices, for
> >> example serial ports (the serial ports use PIIX PCI configuration
> >> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >> the _STA method needs patching.  See piix4_pm_machine_ready in
> >> hw/acpi_piix4.c.
> >
> > Again I think there is a big difference between well knows device and
> > PV devices that we add at random location. And if we make the later
> > configurable i.e it may or may not be present and location where it is
> > present can be changed then we better not make a guest to do guesses.
> 
> No guesses here on part of the guest, and no probing in the firmware
> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> trick with the methods).
> 
That's the problem. The number is not hard coded in QEMU only DSDT. If
you hard code it in QEMU (make it non configurable) and make device mandatory
static DSDT make sense if provided by QEMU.

> I think it's a nice compromise.
> 
> >> * ACPI support is a first-class part of the device.  Each instance of
> >> the device should be there in the ACPI tables.  In this case the fw-cfg
> >> data needs to be a list of ports, and it is probably simpler to combine
> >> all the definitions in an SSDT that is dynamically-built (similar to
> >> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> >> each instance of the device.
> >>
> >> I prefer the first, the second seems to be over-engineered.
> >>
> > Second is over-engineering indeed. The device should be singleton and
> > fail if second instance is created. Do we have such capability in qdev?
> 
> No, but why should it fail?
> 
Why should it not? Guest cannot use more than on of them, why allow to
create insane configs?

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>> support is provided just for convenience of the OSPM.  In this case,
>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>> support is similar to what we do for other on-board ISA devices, for
>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>> hw/acpi_piix4.c.
>>>
>>> Again I think there is a big difference between well knows device and
>>> PV devices that we add at random location. And if we make the later
>>> configurable i.e it may or may not be present and location where it is
>>> present can be changed then we better not make a guest to do guesses.
>>
>> No guesses here on part of the guest, and no probing in the firmware
>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>> trick with the methods).
>
> That's the problem. The number is not hard coded in QEMU only DSDT.

It is hard-coded where the board creates it, or at least as the default
value of the qdev property.

> If you hard code it in QEMU (make it non configurable) and make device mandatory
> static DSDT make sense if provided by QEMU.

You cannot make it mandatory due to versioned machine types, but my plan
would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
makes sense to have a static DSDT.  Sorry if it was unclear.

>> I think it's a nice compromise.
>>
>>>> * ACPI support is a first-class part of the device.  Each instance of
>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
>>>> data needs to be a list of ports, and it is probably simpler to combine
>>>> all the definitions in an SSDT that is dynamically-built (similar to
>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>>>> each instance of the device.
>>>>
>>>> I prefer the first, the second seems to be over-engineered.
>>>>
>>> Second is over-engineering indeed. The device should be singleton and
>>> fail if second instance is created. Do we have such capability in qdev?
>>
>> No, but why should it fail?
>>
> Why should it not? Guest cannot use more than on of them, why allow to
> create insane configs?

Who cares?  Insane ISA device configs anyway are not discoverable by
guests, you need to teach the guest about the device manually.

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>> support is provided just for convenience of the OSPM.  In this case,
> >>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>> support is similar to what we do for other on-board ISA devices, for
> >>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>> hw/acpi_piix4.c.
> >>>
> >>> Again I think there is a big difference between well knows device and
> >>> PV devices that we add at random location. And if we make the later
> >>> configurable i.e it may or may not be present and location where it is
> >>> present can be changed then we better not make a guest to do guesses.
> >>
> >> No guesses here on part of the guest, and no probing in the firmware
> >> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >> trick with the methods).
> >
> > That's the problem. The number is not hard coded in QEMU only DSDT.
> 
> It is hard-coded where the board creates it, or at least as the default
> value of the qdev property.
> 
Default value that can be changes is not hard coded. Why do you allow
change in one place, but not the other?

> > If you hard code it in QEMU (make it non configurable) and make device mandatory
> > static DSDT make sense if provided by QEMU.
> 
> You cannot make it mandatory due to versioned machine types, but my plan
> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> makes sense to have a static DSDT.  Sorry if it was unclear.
And then you will have to have different DSDT for pre pc-1.5. Dynamic
patching solves exactly that problem.

> 
> >> I think it's a nice compromise.
> >>
> >>>> * ACPI support is a first-class part of the device.  Each instance of
> >>>> the device should be there in the ACPI tables.  In this case the fw-cfg
> >>>> data needs to be a list of ports, and it is probably simpler to combine
> >>>> all the definitions in an SSDT that is dynamically-built (similar to
> >>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> >>>> each instance of the device.
> >>>>
> >>>> I prefer the first, the second seems to be over-engineered.
> >>>>
> >>> Second is over-engineering indeed. The device should be singleton and
> >>> fail if second instance is created. Do we have such capability in qdev?
> >>
> >> No, but why should it fail?
> >>
> > Why should it not? Guest cannot use more than on of them, why allow to
> > create insane configs?
> 
> Who cares?  Insane ISA device configs anyway are not discoverable by
> guests, you need to teach the guest about the device manually.
> 
With proper ACPI they are discoverable. Since writing ACPI support for
multiple pvpanic devices is clear case of over-engineering it is a
courtesy to QEMU users to fail machine creation that cannot be properly
described by ACPI.

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>>>> support is provided just for convenience of the OSPM.  In this case,
>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>>>> support is similar to what we do for other on-board ISA devices, for
>>>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>>>> hw/acpi_piix4.c.
>>>>>
>>>>> Again I think there is a big difference between well knows device and
>>>>> PV devices that we add at random location. And if we make the later
>>>>> configurable i.e it may or may not be present and location where it is
>>>>> present can be changed then we better not make a guest to do guesses.
>>>>
>>>> No guesses here on part of the guest, and no probing in the firmware
>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>>>> trick with the methods).
>>>
>>> That's the problem. The number is not hard coded in QEMU only DSDT.
>>
>> It is hard-coded where the board creates it, or at least as the default
>> value of the qdev property.
>
> Default value that can be changes is not hard coded.
> Why do you allow change in one place, but not the other?

I'm just following the model of other ISA devices, I don't think there's
any difference in this respect between well-known and pv devices (also
because in the end all modern guests will use ACPI to discover even
well-known devices).

The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
serial ports.

>>> If you hard code it in QEMU (make it non configurable) and make device mandatory
>>> static DSDT make sense if provided by QEMU.
>>
>> You cannot make it mandatory due to versioned machine types, but my plan
>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
>> makes sense to have a static DSDT.  Sorry if it was unclear.
> 
> And then you will have to have different DSDT for pre pc-1.5. Dynamic
> patching solves exactly that problem.

Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.

>>>> I think it's a nice compromise.

^^^ This still holds. :)

>>>>>> * ACPI support is a first-class part of the device.  Each instance of
>>>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
>>>>>> data needs to be a list of ports, and it is probably simpler to combine
>>>>>> all the definitions in an SSDT that is dynamically-built (similar to
>>>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
>>>>>> each instance of the device.
>>>>>>
>>>>>> I prefer the first, the second seems to be over-engineered.
>>>>>>
>>>>> Second is over-engineering indeed. The device should be singleton and
>>>>> fail if second instance is created. Do we have such capability in qdev?
>>>>
>>>> No, but why should it fail?
>>>>
>>> Why should it not? Guest cannot use more than on of them, why allow to
>>> create insane configs?
>>
>> Who cares?  Insane ISA device configs anyway are not discoverable by
>> guests, you need to teach the guest about the device manually.
>>
> With proper ACPI they are discoverable. Since writing ACPI support for
> multiple pvpanic devices is clear case of over-engineering it is a
> courtesy to QEMU users to fail machine creation that cannot be properly
> described by ACPI.

We don't fail machine creation if someone wants to place a serial port
at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
a reason to make pvpanic special in this respect.

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> >>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>>>> support is provided just for convenience of the OSPM.  In this case,
> >>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>>>> support is similar to what we do for other on-board ISA devices, for
> >>>>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>>>> hw/acpi_piix4.c.
> >>>>>
> >>>>> Again I think there is a big difference between well knows device and
> >>>>> PV devices that we add at random location. And if we make the later
> >>>>> configurable i.e it may or may not be present and location where it is
> >>>>> present can be changed then we better not make a guest to do guesses.
> >>>>
> >>>> No guesses here on part of the guest, and no probing in the firmware
> >>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >>>> trick with the methods).
> >>>
> >>> That's the problem. The number is not hard coded in QEMU only DSDT.
> >>
> >> It is hard-coded where the board creates it, or at least as the default
> >> value of the qdev property.
> >
> > Default value that can be changes is not hard coded.
> > Why do you allow change in one place, but not the other?
> 
> I'm just following the model of other ISA devices, I don't think there's
> any difference in this respect between well-known and pv devices (also
> because in the end all modern guests will use ACPI to discover even
> well-known devices).
> 
We are not there yet :)

> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
> serial ports.
> 
> >>> If you hard code it in QEMU (make it non configurable) and make device mandatory
> >>> static DSDT make sense if provided by QEMU.
> >>
> >> You cannot make it mandatory due to versioned machine types, but my plan
> >> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> >> makes sense to have a static DSDT.  Sorry if it was unclear.
> > 
> > And then you will have to have different DSDT for pre pc-1.5. Dynamic
> > patching solves exactly that problem.
> 
> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
> 
Yes, if you do not allow changing IO port patching _STA is enough, but
if you already patching it is easy to patch both.

> >>>> I think it's a nice compromise.
> 
> ^^^ This still holds. :)
If we would have found a reasonable way to go without patching at all
then it would have been worthwhile to consider compromises, but if
patching is inevitable I honestly do not see big difference between
patching one place or two.

> 
> >>>>>> * ACPI support is a first-class part of the device.  Each instance of
> >>>>>> the device should be there in the ACPI tables.  In this case the fw-cfg
> >>>>>> data needs to be a list of ports, and it is probably simpler to combine
> >>>>>> all the definitions in an SSDT that is dynamically-built (similar to
> >>>>>> what we do for PCI hotplug slots).  Or even provide a separate SSDT for
> >>>>>> each instance of the device.
> >>>>>>
> >>>>>> I prefer the first, the second seems to be over-engineered.
> >>>>>>
> >>>>> Second is over-engineering indeed. The device should be singleton and
> >>>>> fail if second instance is created. Do we have such capability in qdev?
> >>>>
> >>>> No, but why should it fail?
> >>>>
> >>> Why should it not? Guest cannot use more than on of them, why allow to
> >>> create insane configs?
> >>
> >> Who cares?  Insane ISA device configs anyway are not discoverable by
> >> guests, you need to teach the guest about the device manually.
> >>
> > With proper ACPI they are discoverable. Since writing ACPI support for
> > multiple pvpanic devices is clear case of over-engineering it is a
> > courtesy to QEMU users to fail machine creation that cannot be properly
> > described by ACPI.
> 
> We don't fail machine creation if someone wants to place a serial port
> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
> a reason to make pvpanic special in this respect.
> 
Fine with me. That was just a suggestion. I thought we had singleton
qdev flag.

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Paolo Bonzini]

Il 14/03/2013 16:59, Gleb Natapov ha scritto:
> On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
>> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
>>> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
>>>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
>>>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
>>>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
>>>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
>>>>>>>> support is provided just for convenience of the OSPM.  In this case,
>>>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
>>>>>>>> support is similar to what we do for other on-board ISA devices, for
>>>>>>>> example serial ports (the serial ports use PIIX PCI configuration
>>>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
>>>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
>>>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
>>>>>>>> hw/acpi_piix4.c.
>>>>>>>
>>>>>>> Again I think there is a big difference between well knows device and
>>>>>>> PV devices that we add at random location. And if we make the later
>>>>>>> configurable i.e it may or may not be present and location where it is
>>>>>>> present can be changed then we better not make a guest to do guesses.
>>>>>>
>>>>>> No guesses here on part of the guest, and no probing in the firmware
>>>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
>>>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
>>>>>> trick with the methods).
>>>>>
>>>>> That's the problem. The number is not hard coded in QEMU only DSDT.
>>>>
>>>> It is hard-coded where the board creates it, or at least as the default
>>>> value of the qdev property.
>>>
>>> Default value that can be changes is not hard coded.
>>> Why do you allow change in one place, but not the other?
>>
>> I'm just following the model of other ISA devices, I don't think there's
>> any difference in this respect between well-known and pv devices (also
>> because in the end all modern guests will use ACPI to discover even
>> well-known devices).
>>
> We are not there yet :)

Kind of... Windows will hide serial ports that return not-present for
_STA, for example.  Linux will just hide the PNPxxxx path and present it
under /sys/bus/platform instead.

>> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
>> serial ports.
>>
>>>>> If you hard code it in QEMU (make it non configurable) and make device mandatory
>>>>> static DSDT make sense if provided by QEMU.
>>>>
>>>> You cannot make it mandatory due to versioned machine types, but my plan
>>>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
>>>> makes sense to have a static DSDT.  Sorry if it was unclear.
>>>
>>> And then you will have to have different DSDT for pre pc-1.5. Dynamic
>>> patching solves exactly that problem.
>>
>> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
>>
> Yes, if you do not allow changing IO port patching _STA is enough, but
> if you already patching it is easy to patch both.
> 
>>>>>> I think it's a nice compromise.
>>
>> ^^^ This still holds. :)
> If we would have found a reasonable way to go without patching at all
> then it would have been worthwhile to consider compromises, but if
> patching is inevitable I honestly do not see big difference between
> patching one place or two.

Hmm... can you do something like

        Name(PORT, 0xAAAA)
        OperationRegion(PEOR, SystemIO, PORT, 0x01)
        Field(PEOR, ByteAcc, NoLock, Preserve) {
            PEPT,   8,
        }

? i.e. use a Name inside an OperationRegion?

If so, then we can patch 0xAAAA to zero for not-present and the port for
present and indeed patch a single place.

If we have to patch 0x505 all over the place there's an advantage in
patching _STA only.  But if we can do the above it's a bit cleaner to
use the port for the patched value, indeed.

>> We don't fail machine creation if someone wants to place a serial port
>> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
>> a reason to make pvpanic special in this respect.
>>
> Fine with me. That was just a suggestion. I thought we had singleton
> qdev flag.

We have no_user, but it's broken and not exactly a match for what you
want here.

Paolo

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 05:13:54PM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 16:59, Gleb Natapov ha scritto:
> > On Thu, Mar 14, 2013 at 04:50:40PM +0100, Paolo Bonzini wrote:
> >> Il 14/03/2013 15:23, Gleb Natapov ha scritto:
> >>> On Thu, Mar 14, 2013 at 03:05:22PM +0100, Paolo Bonzini wrote:
> >>>> Il 14/03/2013 14:56, Gleb Natapov ha scritto:
> >>>>> On Thu, Mar 14, 2013 at 02:49:48PM +0100, Paolo Bonzini wrote:
> >>>>>> Il 14/03/2013 13:34, Gleb Natapov ha scritto:
> >>>>>>>> * it can be an ISA device; the interface is the I/O port and ACPI
> >>>>>>>> support is provided just for convenience of the OSPM.  In this case,
> >>>>>>>> "-device pvevent" should just add handlers for the port.  The ACPI
> >>>>>>>> support is similar to what we do for other on-board ISA devices, for
> >>>>>>>> example serial ports (the serial ports use PIIX PCI configuration
> >>>>>>>> instead of fw-cfg, but that's a minor detail).  It only needs to work
> >>>>>>>> for port 0x505, so the fw-cfg data can be a single yes/no value and only
> >>>>>>>> the _STA method needs patching.  See piix4_pm_machine_ready in
> >>>>>>>> hw/acpi_piix4.c.
> >>>>>>>
> >>>>>>> Again I think there is a big difference between well knows device and
> >>>>>>> PV devices that we add at random location. And if we make the later
> >>>>>>> configurable i.e it may or may not be present and location where it is
> >>>>>>> present can be changed then we better not make a guest to do guesses.
> >>>>>>
> >>>>>> No guesses here on part of the guest, and no probing in the firmware
> >>>>>> two.  The same number is hard-coded in QEMU and the DSDT, which go in
> >>>>>> pairs anyway, but _not_ in the guest kernel (also thanks to Hu's nice
> >>>>>> trick with the methods).
> >>>>>
> >>>>> That's the problem. The number is not hard coded in QEMU only DSDT.
> >>>>
> >>>> It is hard-coded where the board creates it, or at least as the default
> >>>> value of the qdev property.
> >>>
> >>> Default value that can be changes is not hard coded.
> >>> Why do you allow change in one place, but not the other?
> >>
> >> I'm just following the model of other ISA devices, I don't think there's
> >> any difference in this respect between well-known and pv devices (also
> >> because in the end all modern guests will use ACPI to discover even
> >> well-known devices).
> >>
> > We are not there yet :)
> 
> Kind of... Windows will hide serial ports that return not-present for
> _STA, for example.  Linux will just hide the PNPxxxx path and present it
> under /sys/bus/platform instead.
> 
> >> The board hardcodes 0x505 for pvpanic just like it hardcodes 0x3f8 for
> >> serial ports.
> >>
> >>>>> If you hard code it in QEMU (make it non configurable) and make device mandatory
> >>>>> static DSDT make sense if provided by QEMU.
> >>>>
> >>>> You cannot make it mandatory due to versioned machine types, but my plan
> >>>> would be to make it mandatory on "pc" and "pc-1.5".  For that plan it
> >>>> makes sense to have a static DSDT.  Sorry if it was unclear.
> >>>
> >>> And then you will have to have different DSDT for pre pc-1.5. Dynamic
> >>> patching solves exactly that problem.
> >>
> >> Yes, but it's enough to patch _STA.  Easier in both QEMU and the BIOS.
> >>
> > Yes, if you do not allow changing IO port patching _STA is enough, but
> > if you already patching it is easy to patch both.
> > 
> >>>>>> I think it's a nice compromise.
> >>
> >> ^^^ This still holds. :)
> > If we would have found a reasonable way to go without patching at all
> > then it would have been worthwhile to consider compromises, but if
> > patching is inevitable I honestly do not see big difference between
> > patching one place or two.
> 
> Hmm... can you do something like
> 
>         Name(PORT, 0xAAAA)
>         OperationRegion(PEOR, SystemIO, PORT, 0x01)
>         Field(PEOR, ByteAcc, NoLock, Preserve) {
>             PEPT,   8,
>         }
> 
> ? i.e. use a Name inside an OperationRegion?
> 
> If so, then we can patch 0xAAAA to zero for not-present and the port for
> present and indeed patch a single place.
> 
> If we have to patch 0x505 all over the place there's an advantage in
> patching _STA only.  But if we can do the above it's a bit cleaner to
> use the port for the patched value, indeed.
> 
And patching Name() is as simple as putting
ACPI_EXTRACT_NAME_WORD_CONST above it.

> >> We don't fail machine creation if someone wants to place a serial port
> >> at 0x5678.  With ISA it's basically garbage-in, garbage-out, I don't see
> >> a reason to make pvpanic special in this respect.
> >>
> > Fine with me. That was just a suggestion. I thought we had singleton
> > qdev flag.
> 
> We have no_user, but it's broken and not exactly a match for what you
> want here.
> 
> Paolo

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 14/03/2013 16:59, Gleb Natapov ha scritto:
[...]
>> Fine with me. That was just a suggestion. I thought we had singleton
>> qdev flag.
>
> We have no_user, but it's broken and not exactly a match for what you
> want here.

For what it's worth, no_user is set for this device.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Hu Tao]

On Thu, Mar 14, 2013 at 10:14:12AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 09:15, Hu Tao ha scritto:
> > pvevent device is used to send guest panic event from guest to qemu.
> > 
> > When guest panic happens, pvevent device driver will write a event
> > number to IO port 0x505(which is the IO port occupied by pvevent device,
> > by default). On receiving the event, pvevent device will pause guest
> > cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
> > 
> > TODO: make the IO port configurable
> 
> The port is already configurable as far as the device is concerned; when
> you add the port to the PC boards you will have to wind up fw-cfg.

Yes, by configurable, I mean to pass io port through fw-cfg to seabios
and patch it.

Re: [Qemu-devel] [PATCH v14 3/4] introduce pvevent device to deal with panicked event

[Markus Armbruster]

Hu Tao <hutao@cn.fujitsu.com> writes:

> pvevent device is used to send guest panic event from guest to qemu.
>
> When guest panic happens, pvevent device driver will write a event
> number to IO port 0x505(which is the IO port occupied by pvevent device,
> by default). On receiving the event, pvevent device will pause guest
> cpu(s), and send a qmp event QEVENT_GUEST_PANICKED.  
>
> TODO: make the IO port configurable
>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  hw/Makefile.objs |   2 +
>  hw/pvevent.c     | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 118 insertions(+)
>  create mode 100644 hw/pvevent.c
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 40ebe46..edf499e 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -218,5 +218,7 @@ obj-$(CONFIG_KVM) += ivshmem.o
>  obj-$(CONFIG_LINUX) += vfio_pci.o
>  endif
>  
> +common-obj-y += pvevent.o
> +
>  $(obj)/baum.o: QEMU_CFLAGS += $(SDL_CFLAGS) 
>  endif
> diff --git a/hw/pvevent.c b/hw/pvevent.c
> new file mode 100644
> index 0000000..c7df77b
> --- /dev/null
> +++ b/hw/pvevent.c
> @@ -0,0 +1,116 @@
> +/*
> + * QEMU simulated pvevent device.
> + *
> + * Copyright Fujitsu, Corp. 2013
> + *
> + * Authors:
> + *     Wen Congyang <wency@cn.fujitsu.com>
> + *     Hu Tao <hutao@cn.fujitsu.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include <qapi/qmp/qobject.h>
> +#include <qapi/qmp/qjson.h>
> +#include <monitor/monitor.h>
> +#include <sysemu/sysemu.h>
> +#include <sysemu/kvm.h>
> +
> +/* The bit of supported pv event */
> +#define PVEVENT_F_PANICKED      0
> +
> +/* The pv event value */
> +#define PVEVENT_PANICKED        (1 << PVEVENT_F_PANICKED)

This looks like events are encoded as bits, which can be combined into a
set, but...

> +
> +#define TYPE_ISA_PVEVENT_DEVICE    "pvevent"
> +#define ISA_PVEVENT_DEVICE(obj)    \
> +    OBJECT_CHECK(PVEventState, (obj), TYPE_ISA_PVEVENT_DEVICE)
> +
> +static void panicked_mon_event(const char *action)
> +{
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'action': %s }", action);
> +    monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
> +    qobject_decref(data);
> +}
> +
> +static void handle_event(int event)
> +{
> +    if (event == PVEVENT_PANICKED) {

... here you don't test bits.  Weird.  Intentional?

> +        panicked_mon_event("pause");
> +        vm_stop(RUN_STATE_GUEST_PANICKED);
> +        return;
> +    }
> +}

Should we log events we don't understand?

> +
> +#include "hw/isa.h"
> +
> +typedef struct PVEventState {
> +    ISADevice parent_obj;
> +
> +    MemoryRegion io;
> +    uint16_t ioport;
> +} PVEventState;
> +
> +/* return supported events on read */
> +static uint64_t pvevent_ioport_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    return PVEVENT_PANICKED;
> +}
> +
> +static void pvevent_ioport_write(void *opaque, hwaddr addr, uint64_t val,
> +                                 unsigned size)
> +{
> +    handle_event(val);
> +}
[...]

[Qemu-devel] [PATCH v14 4/4] pvevent: add document to describe the usage

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 docs/pvevent.txt | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 docs/pvevent.txt

diff --git a/docs/pvevent.txt b/docs/pvevent.txt
new file mode 100644
index 0000000..749b87c
--- /dev/null
+++ b/docs/pvevent.txt
@@ -0,0 +1,16 @@
+PVEVENT DEVICE
+==============
+
+pvevent device is a simulated ISA device, through which guest panic
+event is sent to qemu, and an qmp event is generated. This allows
+management apps(e.g. libvirt) to be notified and respond to the
+event.
+
+pvevent device is defined with HID "MSFT0001".
+
+To use it, one will have to:
+
+1. add the device by specifying `-device pv-event' in the qemu command
+   line.
+
+2. load pvevent device driver in guest OS.
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v14 4/4] pvevent: add document to describe the usage

[Paolo Bonzini]

Il 14/03/2013 09:15, Hu Tao ha scritto:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  docs/pvevent.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 docs/pvevent.txt
> 
> diff --git a/docs/pvevent.txt b/docs/pvevent.txt
> new file mode 100644
> index 0000000..749b87c
> --- /dev/null
> +++ b/docs/pvevent.txt
> @@ -0,0 +1,16 @@
> +PVEVENT DEVICE
> +==============
> +
> +pvevent device is a simulated ISA device, through which guest panic
> +event is sent to qemu, and an qmp event is generated. This allows
> +management apps(e.g. libvirt) to be notified and respond to the
> +event.
> +
> +pvevent device is defined with HID "MSFT0001".

Why MSFT0001?  Otherwise the series looks good.

Paolo

> +To use it, one will have to:
> +
> +1. add the device by specifying `-device pv-event' in the qemu command
> +   line.
> +
> +2. load pvevent device driver in guest OS.
> 

Re: [Qemu-devel] [PATCH v14 4/4] pvevent: add document to describe the usage

[Hu Tao]

On Thu, Mar 14, 2013 at 09:45:30AM +0100, Paolo Bonzini wrote:
> Il 14/03/2013 09:15, Hu Tao ha scritto:
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  docs/pvevent.txt | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >  create mode 100644 docs/pvevent.txt
> > 
> > diff --git a/docs/pvevent.txt b/docs/pvevent.txt
> > new file mode 100644
> > index 0000000..749b87c
> > --- /dev/null
> > +++ b/docs/pvevent.txt
> > @@ -0,0 +1,16 @@
> > +PVEVENT DEVICE
> > +==============
> > +
> > +pvevent device is a simulated ISA device, through which guest panic
> > +event is sent to qemu, and an qmp event is generated. This allows
> > +management apps(e.g. libvirt) to be notified and respond to the
> > +event.
> > +
> > +pvevent device is defined with HID "MSFT0001".
> 
> Why MSFT0001?  Otherwise the series looks good.

Will change to QEMU0001.

> 
> Paolo
> 
> > +To use it, one will have to:
> > +
> > +1. add the device by specifying `-device pv-event' in the qemu command
> > +   line.
> > +
> > +2. load pvevent device driver in guest OS.
> > 

Re: [Qemu-devel] [PATCH v14 4/4] pvevent: add document to describe the usage

[Eric Blake]

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

On 03/14/2013 02:15 AM, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  docs/pvevent.txt | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>  create mode 100644 docs/pvevent.txt
> 
> diff --git a/docs/pvevent.txt b/docs/pvevent.txt
> new file mode 100644
> index 0000000..749b87c
> --- /dev/null
> +++ b/docs/pvevent.txt
> @@ -0,0 +1,16 @@
> +PVEVENT DEVICE
> +==============
> +
> +pvevent device is a simulated ISA device, through which guest panic
> +event is sent to qemu, and an qmp event is generated. This allows

s/an qmp/a QMP/ - although mentioning _which_ event might be niceer.

> +management apps(e.g. libvirt) to be notified and respond to the

s/apps(/apps (/

> +event.
> +
> +pvevent device is defined with HID "MSFT0001".
> +
> +To use it, one will have to:
> +
> +1. add the device by specifying `-device pv-event' in the qemu command
> +   line.
> +
> +2. load pvevent device driver in guest OS.

Missing is a sentence stating that the management app then has the
option of waiting for GUEST_PANICKED events, and/or polling for
guest-panicked RunState, to learn when the pvevent device has fired a
panic event.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH v14 0/4] pvevent device to deal with guest panic event

[Gleb Natapov]

On Thu, Mar 14, 2013 at 04:15:49PM +0800, Hu Tao wrote:
> This series introduces a new simulated device, pvevent, to notify
> qemu when guest panic event happens.
> 
Call it something less generic. pvpanic for instance.

> Along with this series, there are two patches to add seabios ACPI
> driver and kernel ACPI driver for the device, respectively.
> 
> TODO: make the io port used by pvevent device configurable.
> 
> Tested with:
> 
>   - qemu(kvm)
>   - qemu(tcg)
>   - piix
>   - q35
> 
> Changes from v13:
> 
>   - communicate the io port with guest by ACPI, instead of hardcoding it (Marcelo)
>   - panic event action is always 'pause' (Paolo)
>   - not kvm-specific (Gleb)
>   - make the device target-independent (Paolo)
>   - no runstate save/load(will be a seperate patch) (Paolo)
>   - support q35 (Gleb)
>   - doc about qmp event (Eric)
>   - doc about pvevent device (Paolo)
>  
> v13: http://lists.nongnu.org/archive/html/qemu-devel/2013-02/msg05361.html
> 
> Hu Tao (4):
>   add a new runstate: RUN_STATE_GUEST_PANICKED
>   add a new qevent: QEVENT_GUEST_PANICKED
>   introduce a new qom device to deal with panicked event
>   pv event: add document to describe the usage
> 
>  QMP/qmp-events.txt        |  14 ++++++
>  docs/pvevent.txt          |  14 ++++++
>  hw/Makefile.objs          |   2 +
>  hw/pvevent.c              | 116 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/monitor/monitor.h |   1 +
>  include/sysemu/sysemu.h   |   1 +
>  monitor.c                 |   1 +
>  qapi-schema.json          |   5 +-
>  qmp.c                     |   3 +-
>  vl.c                      |  13 +++++-
>  10 files changed, 165 insertions(+), 5 deletions(-)
>  create mode 100644 docs/pvevent.txt
>  create mode 100644 hw/pvevent.c
> 
> -- 
> 1.8.1.4

--
			Gleb.

Re: [Qemu-devel] [PATCH v14 0/4] pvevent device to deal with guest panic event

[Hu Tao]

On Thu, Mar 14, 2013 at 10:58:45AM +0200, Gleb Natapov wrote:
> On Thu, Mar 14, 2013 at 04:15:49PM +0800, Hu Tao wrote:
> > This series introduces a new simulated device, pvevent, to notify
> > qemu when guest panic event happens.
> > 
> Call it something less generic. pvpanic for instance.

OK.

Re: [Qemu-devel] [PATCH v14 0/4] pvevent device to deal with guest panic event

[Markus Armbruster]

Gleb Natapov <gleb@redhat.com> writes:

> On Thu, Mar 14, 2013 at 04:15:49PM +0800, Hu Tao wrote:
>> This series introduces a new simulated device, pvevent, to notify
>> qemu when guest panic event happens.
>> 
> Call it something less generic. pvpanic for instance.

No objection, just observing that the device can indeed transport
arbitrary events[*].  It happens to transport just one at the moment,
PVEVENT_F_PANICKED.


[*] To be precise: a set of 8 event types carrying no additional
information.