Patchwork [RFC,0/4] monitor device info infrastructure

login
register
mail settings
Submitter Blue Swirl
Date May 12, 2010, 8:56 p.m.
Message ID <AANLkTil-VFu1P4q_4athK2r1XkBgZh47NGjj2wtZw3G9@mail.gmail.com>
Download mbox | patch
Permalink /patch/52450/
State New
Headers show

Comments

Blue Swirl - May 12, 2010, 8:56 p.m.
Hi all,

I finally refreshed some of my monitor patches. PCI and HPET patches
(attached, they don't apply anymore) need more work because of the new
monitor design.

The patches provide a method for devices to register new monitor
commands. This fixes some design problems, like useless
pic_info/irq_info functions for most architectures.

I think automation via qdev field was requested the last time. I added
something like this, though it doesn't fit the cases where several
functions need to be registered.

Comments?

Blue Swirl (4):
  monitor: add device info infrastructure
  qdev: automatically register device info
  x86/Sparc32: use device info for pic and irq
  PPC: use device info for CPU statistics

 cpu-all.h               |    3 --
 hw/an5206.c             |    9 ------
 hw/arm_pic.c            |   10 ------
 hw/cris_pic_cpu.c       |    5 ---
 hw/i8259.c              |   50 ++++++++++++++++++++-----------
 hw/microblaze_pic_cpu.c |    5 ---
 hw/pc.h                 |    2 -
 hw/qdev.c               |    3 ++
 hw/qdev.h               |    3 ++
 hw/shix.c               |   10 ------
 hw/slavio_intctl.c      |   26 ++++++++++------
 hw/sun4c_intctl.c       |   16 +++++++++-
 hw/sun4m.c              |   15 +---------
 hw/sun4m.h              |    8 -----
 hw/sun4u.c              |    8 -----
 monitor.c               |   74 ++++++++++++++++++++++++++---------------------
 monitor.h               |   10 ++++++
 qemu-monitor.hx         |   19 ++++++++----
 target-ppc/cpu.h        |    2 +
 target-ppc/helper.c     |    4 ++
 target-ppc/translate.c  |   45 +++++++++++++++++++---------
 21 files changed, 169 insertions(+), 158 deletions(-)
Jan Kiszka - May 13, 2010, 7:40 a.m.
Blue Swirl wrote:
> Hi all,
> 
> I finally refreshed some of my monitor patches. PCI and HPET patches
> (attached, they don't apply anymore) need more work because of the new
> monitor design.
> 
> The patches provide a method for devices to register new monitor
> commands. This fixes some design problems, like useless
> pic_info/irq_info functions for most architectures.
> 
> I think automation via qdev field was requested the last time. I added
> something like this, though it doesn't fit the cases where several
> functions need to be registered.
> 
> Comments?

I'll soon send out a series that adds "device_show <qdev-path>" to
visualize the full state, something that will at least obsolete "info
pic" and "info hpet" sooner or later, maybe even more. Also, I would
like to qdev'ify CPUs in order to make them reachable for device_show
(which evaluates qdev->info.vmstate).

I'm not sure: How many use cases for a "dev_info" would remain?
Moreover, to dump statistics, we should rather add some "device_stats
<qdev-path>" command instead of mixing both usages under an info command.

Jan
Blue Swirl - May 13, 2010, 6:50 p.m.
On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
> Blue Swirl wrote:
>  > Hi all,
>  >
>  > I finally refreshed some of my monitor patches. PCI and HPET patches
>  > (attached, they don't apply anymore) need more work because of the new
>  > monitor design.
>  >
>  > The patches provide a method for devices to register new monitor
>  > commands. This fixes some design problems, like useless
>  > pic_info/irq_info functions for most architectures.
>  >
>  > I think automation via qdev field was requested the last time. I added
>  > something like this, though it doesn't fit the cases where several
>  > functions need to be registered.
>  >
>  > Comments?
>
>
> I'll soon send out a series that adds "device_show <qdev-path>" to
>  visualize the full state, something that will at least obsolete "info
>  pic" and "info hpet" sooner or later, maybe even more. Also, I would
>  like to qdev'ify CPUs in order to make them reachable for device_show
>  (which evaluates qdev->info.vmstate).

That sounds like much better design. But for example 'info pci' shows
different things compared to 'info qdev'. And what about 'info
usbhost', there is no qdev?

>  I'm not sure: How many use cases for a "dev_info" would remain?
>  Moreover, to dump statistics, we should rather add some "device_stats
>  <qdev-path>" command instead of mixing both usages under an info command.

Not too many. I think the VMState structure (with no connection to
savevm/loadvm/migration) would be useful to define and dump also
statistics. But because most statistics need to be enabled at compile
phase, they are probably not very widely used.
Jan Kiszka - May 13, 2010, 7:38 p.m.
Blue Swirl wrote:
> On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Blue Swirl wrote:
>>  > Hi all,
>>  >
>>  > I finally refreshed some of my monitor patches. PCI and HPET patches
>>  > (attached, they don't apply anymore) need more work because of the new
>>  > monitor design.
>>  >
>>  > The patches provide a method for devices to register new monitor
>>  > commands. This fixes some design problems, like useless
>>  > pic_info/irq_info functions for most architectures.
>>  >
>>  > I think automation via qdev field was requested the last time. I added
>>  > something like this, though it doesn't fit the cases where several
>>  > functions need to be registered.
>>  >
>>  > Comments?
>>
>>
>> I'll soon send out a series that adds "device_show <qdev-path>" to
>>  visualize the full state, something that will at least obsolete "info
>>  pic" and "info hpet" sooner or later, maybe even more. Also, I would
>>  like to qdev'ify CPUs in order to make them reachable for device_show
>>  (which evaluates qdev->info.vmstate).
> 
> That sounds like much better design. But for example 'info pci' shows
> different things compared to 'info qdev'. And what about 'info
> usbhost', there is no qdev?

That should be addressed differently (if there is a need to change it).
My point is basically about things that are (or will be) qdev related -
just as dev_info was suggesting.

> 
>>  I'm not sure: How many use cases for a "dev_info" would remain?
>>  Moreover, to dump statistics, we should rather add some "device_stats
>>  <qdev-path>" command instead of mixing both usages under an info command.
> 
> Not too many. I think the VMState structure (with no connection to
> savevm/loadvm/migration) would be useful to define and dump also
> statistics. But because most statistics need to be enabled at compile
> phase, they are probably not very widely used.

If they are special, then I would vote for a separate stats
infrastructure with its own data types where required. If certain stats
should rather be enabled by default, then there is actually the question
if they shouldn't be migrated as well, thus become part of the related
VMState definitions. Just thoughts, I haven't done any investigations on
the current situation.


However, I'm a fan of a plug-in interface for the existing info monitor
command. That would allow to register things dynamically that do not fit
in any regular structure without requiring stubs or tons of #ifdefs.
What about this as a first step?

Jan
Blue Swirl - May 13, 2010, 8:01 p.m.
On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
> Blue Swirl wrote:
>  > On 5/13/10, Jan Kiszka <jan.kiszka@web.de> wrote:
>  >> Blue Swirl wrote:
>  >>  > Hi all,
>  >>  >
>  >>  > I finally refreshed some of my monitor patches. PCI and HPET patches
>  >>  > (attached, they don't apply anymore) need more work because of the new
>  >>  > monitor design.
>  >>  >
>  >>  > The patches provide a method for devices to register new monitor
>  >>  > commands. This fixes some design problems, like useless
>  >>  > pic_info/irq_info functions for most architectures.
>  >>  >
>  >>  > I think automation via qdev field was requested the last time. I added
>  >>  > something like this, though it doesn't fit the cases where several
>  >>  > functions need to be registered.
>  >>  >
>  >>  > Comments?
>  >>
>  >>
>  >> I'll soon send out a series that adds "device_show <qdev-path>" to
>  >>  visualize the full state, something that will at least obsolete "info
>  >>  pic" and "info hpet" sooner or later, maybe even more. Also, I would
>  >>  like to qdev'ify CPUs in order to make them reachable for device_show
>  >>  (which evaluates qdev->info.vmstate).
>  >
>  > That sounds like much better design. But for example 'info pci' shows
>  > different things compared to 'info qdev'. And what about 'info
>  > usbhost', there is no qdev?
>
>
> That should be addressed differently (if there is a need to change it).
>  My point is basically about things that are (or will be) qdev related -
>  just as dev_info was suggesting.
>
>
>  >
>  >>  I'm not sure: How many use cases for a "dev_info" would remain?
>  >>  Moreover, to dump statistics, we should rather add some "device_stats
>  >>  <qdev-path>" command instead of mixing both usages under an info command.
>  >
>  > Not too many. I think the VMState structure (with no connection to
>  > savevm/loadvm/migration) would be useful to define and dump also
>  > statistics. But because most statistics need to be enabled at compile
>  > phase, they are probably not very widely used.
>
>
> If they are special, then I would vote for a separate stats
>  infrastructure with its own data types where required. If certain stats
>  should rather be enabled by default, then there is actually the question
>  if they shouldn't be migrated as well, thus become part of the related
>  VMState definitions. Just thoughts, I haven't done any investigations on
>  the current situation.

Currently for example PIC statistics (like i8259.c and
slavio_intctl.c) are not saved or migrated. Statistics generation need
a specially compiled QEMU, so I don't think statistics saving or
migration will be any normal use case.

>  However, I'm a fan of a plug-in interface for the existing info monitor
>  command. That would allow to register things dynamically that do not fit
>  in any regular structure without requiring stubs or tons of #ifdefs.
>  What about this as a first step?

I think qdev dump approach would be better for current and future qdev
objects. But I'll try if my version makes non-qdev stuff ('info
usbhost' or something) simpler.

The main benefit with the registration is IMHO that the caller can
specify a state variable when registering and this variable is also
available at callback time. This allows many cleanups.

Patch

From 13952817d5e4f01333c3d35d05da916719ed2322 Mon Sep 17 00:00:00 2001
From: Blue Swirl <blauwirbel@gmail.com>
Date: Wed, 9 Sep 2009 17:40:35 +0000
Subject: [PATCH] x86: use device info for hpet

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/pc.c         |   12 ++++++++++++
 monitor.c       |   10 ----------
 qemu-monitor.hx |    2 --
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 6292001..39a0970 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1108,6 +1108,17 @@  static CPUState *pc_new_cpu(const char *cpu_model)
     return env;
 }
 
+static void info_hpet(Monitor *mon, void *opaque)
+{
+    monitor_printf(mon, "HPET is %s by QEMU\n",
+                   (no_hpet) ? "disabled" : "enabled");
+}
+
+static const struct MonDevInfo hpet_info = {
+    .dev_name = "hpet",
+    .dev_info_cb = info_hpet,
+};
+
 /* PC hardware initialisation */
 static void pc_init1(ram_addr_t ram_size,
                      const char *boot_device,
@@ -1327,6 +1338,7 @@  static void pc_init1(ram_addr_t ram_size,
     if (!no_hpet) {
         hpet_init(isa_irq);
     }
+    monitor_register_device_info(&hpet_info, NULL);
 
     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
         if (serial_hds[i]) {
diff --git a/monitor.c b/monitor.c
index 3d18885..202c457 100644
--- a/monitor.c
+++ b/monitor.c
@@ -315,14 +315,6 @@  static void do_info_name(Monitor *mon)
         monitor_printf(mon, "%s\n", qemu_name);
 }
 
-#if defined(TARGET_I386)
-static void do_info_hpet(Monitor *mon)
-{
-    monitor_printf(mon, "HPET is %s by QEMU\n",
-                   (no_hpet) ? "disabled" : "enabled");
-}
-#endif
-
 static void do_info_uuid(Monitor *mon)
 {
     monitor_printf(mon, UUID_FMT "\n", qemu_uuid[0], qemu_uuid[1],
@@ -1851,8 +1843,6 @@  static const mon_cmd_t info_cmds[] = {
 #if defined(TARGET_I386)
     { "mem", "", mem_info,
       "", "show the active virtual memory mappings", },
-    { "hpet", "", do_info_hpet,
-      "", "show state of HPET", },
 #endif
     { "jit", "", do_info_jit,
       "", "show dynamic compiler info", },
diff --git a/qemu-monitor.hx b/qemu-monitor.hx
index 54078e2..26f9cf7 100644
--- a/qemu-monitor.hx
+++ b/qemu-monitor.hx
@@ -49,8 +49,6 @@  show the command line history
 show virtual to physical memory mappings (i386 only)
 @item info mem
 show the active virtual memory mappings (i386 only)
-@item info hpet
-show state of HPET (i386 only)
 @item info kvm
 show KVM information
 @item info usb
-- 
1.5.6.5