| Message ID | 20100106163502.GQ4905@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/06/2010 10:35 AM, Gleb Natapov wrote: > Knowing ioapic configuration is very useful for the poor soles > how need to debug guest occasionally. > This needs to be implemented in terms of VMState. There is no reason for it not to be. I've just gone through the code in great detail. You just need to implement a vmstate_get_field that looks up a field given an instance name and instance id. You then need to add a to_str callback in VMStateInfo. The vmstate_get_field code will look very much like the vmstate_save code except it calls to_str(). The new monitor command should be 'device_info name [instance_id] [field]'. For the ioapic, you can introduce a new VMStateInfo type that duplicates the vmstate_info_uint64 type except it has a different to_str callback that decodes the apic fields. It's just then a matter of a few macros and updating the VMState description for the ioapic. It's the same or less code than your current patch and it lays the ground work for a consistent command to look up arbitrary device info. Regards, Anthony Liguori
On Wed, Jan 06, 2010 at 01:50:21PM -0600, Anthony Liguori wrote: > On 01/06/2010 10:35 AM, Gleb Natapov wrote: > >Knowing ioapic configuration is very useful for the poor soles > >how need to debug guest occasionally. > > This needs to be implemented in terms of VMState. There is no > reason for it not to be. Except that this has nothing to do with VMState. > > I've just gone through the code in great detail. You just need to > implement a vmstate_get_field that looks up a field given an > instance name and instance id. You then need to add a to_str > callback in VMStateInfo. The vmstate_get_field code will look very > much like the vmstate_save code except it calls to_str(). > > The new monitor command should be 'device_info name [instance_id] [field]'. > What is instance_id? And 'field' is implementation detail that should not be exposed to user at all. And what happens when VMState will start to support subsections? > For the ioapic, you can introduce a new VMStateInfo type that > duplicates the vmstate_info_uint64 type except it has a different > to_str callback that decodes the apic fields. It's just then a > matter of a few macros and updating the VMState description for the > ioapic. > You just overdesigning! What is the point in all those to_str() callbacks instead of only one that knows device internals? What if a meaning of one field depends on values of other fields? If you want to have a way to dump VMState then just dump it in a "field_name = value" form. This is great for migration debugging. But to present device state in human readable from is much simpler done by having only one function that gets device and prints nice info about its current state. If anything, the devise state print callback should be part of a device structure. > It's the same or less code than your current patch and it lays the > ground work for a consistent command to look up arbitrary device > info. > Your description is almost the size of my patch, and you can't seriously say that what you propose is "the same or less code than my current patch" since all of my patch will be a subset of what you propose, because all it does is parse ioapic state into readable form. And this is really what my patch is about: parsing ioapic state into readable form. Now you are saying that we can call this code from a different place (that doesn't exist currently), or we can spread it over multiple to_str functions, but it can't just disappear. -- Gleb.
diff --git a/hw/ioapic.c b/hw/ioapic.c index b0ad78f..3afb2c4 100644 --- a/hw/ioapic.c +++ b/hw/ioapic.c @@ -24,6 +24,8 @@ #include "pc.h" #include "qemu-timer.h" #include "host-utils.h" +#include "monitor.h" +#include "qemu-objects.h" //#define DEBUG_IOAPIC @@ -50,6 +52,8 @@ struct IOAPICState { uint64_t ioredtbl[IOAPIC_NUM_PINS]; }; +static struct IOAPICState *ioapic; + static void ioapic_service(IOAPICState *s) { uint8_t i; @@ -232,7 +236,7 @@ qemu_irq *ioapic_init(void) qemu_irq *irq; int io_memory; - s = qemu_mallocz(sizeof(IOAPICState)); + ioapic = s = qemu_mallocz(sizeof(IOAPICState)); ioapic_reset(s); io_memory = cpu_register_io_memory(ioapic_mem_read, @@ -245,3 +249,70 @@ qemu_irq *ioapic_init(void) return irq; } + +static void qemu_ioapic_qlist_iter(QObject *data, void *opaque) +{ + QDict *qdict = qobject_to_qdict(data); + Monitor *mon = opaque; + + monitor_printf(mon, "%2"PRId64": ", qdict_get_int(qdict, "index")); + if (qdict_get_bool(qdict, "masked")) { + monitor_printf(mon, "masked\n"); + } else { + monitor_printf(mon, "vec=%3"PRId64" %s %s acive-%s %s dest=%"PRId64"\n", + qdict_get_int(qdict, "vector"), + qdict_get_str(qdict, "delivery_mode"), + qdict_get_str(qdict, "dest_mode"), + qdict_get_str(qdict, "polarity"), + qdict_get_str(qdict, "trig_mode"), + qdict_get_int(qdict, "destination")); + } +} + +void monitor_print_ioapic(Monitor *mon, const QObject *ret_data) +{ + qlist_iter(qobject_to_qlist(ret_data), qemu_ioapic_qlist_iter, mon); +} + +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", + "nmi", "init", "res", "extint"}; + +void do_info_ioapic(Monitor *mon, QObject **ret_data) +{ + int i; + QList *list; + + *ret_data = NULL; + + if (!ioapic) + return; + + list = qlist_new(); + + for (i = 0; i < IOAPIC_NUM_PINS; i++) { + QObject *obj; + uint64 e = ioapic->ioredtbl[i]; + if (e & IOAPIC_LVT_MASKED) { + obj = qobject_from_jsonf("{'index': %d, 'masked': true}", i); + } else { + uint8_t vec = e & 0xff; + uint8_t trig_mode = ((e >> 15) & 1); + uint8_t dest = e >> 56; + uint8_t dest_mode = (e >> 11) & 1; + uint8_t delivery_mode = (e >> 8) & 7; + uint8_t polarity = (e >> 13) & 1; + obj = qobject_from_jsonf("{'index': %d, 'masked': false," + "'vector': %d, 'delivery_mode': %s," + "'dest_mode': %s, 'polarity': %s," + "'trig_mode': %s, 'destination': %d}", + i, vec, + delivery_mode_string[delivery_mode], + dest_mode ? "logical":"physical", + polarity ? "low" : "high", + trig_mode ? "level": "edge", + dest); + } + qlist_append_obj(list, obj); + } + *ret_data = QOBJECT(list); +} diff --git a/hw/pc.h b/hw/pc.h index 03ffc91..3e39444 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -2,6 +2,7 @@ #define HW_PC_H #include "qemu-common.h" +#include "qobject.h" /* PC-style peripherals (also used by other machines). */ @@ -45,6 +46,8 @@ int apic_accept_pic_intr(CPUState *env); void apic_deliver_pic_intr(CPUState *env, int level); int apic_get_interrupt(CPUState *env); qemu_irq *ioapic_init(void); +void do_info_ioapic(Monitor *mon, QObject **ret_data); +void monitor_print_ioapic(Monitor *mon, const QObject *data); void ioapic_set_irq(void *opaque, int vector, int level); void apic_reset_irq_delivered(void); int apic_get_irq_delivered(void); diff --git a/monitor.c b/monitor.c index c0dc48e..367e330 100644 --- a/monitor.c +++ b/monitor.c @@ -2625,6 +2625,14 @@ static const mon_cmd_t info_cmds[] = { .mhandler.info = do_info_roms, }, { + .name = "ioapic", + .args_type = "", + .params = "", + .help = "show ioapic config", + .user_print = monitor_print_ioapic, + .mhandler.info_new = do_info_ioapic, + }, + { .name = NULL, }, };
Knowing ioapic configuration is very useful for the poor soles how need to debug guest occasionally. Signed-off-by: Gleb Natapov <gleb@redhat.com> -- Gleb.