| Message ID | 20091230115043.GD26899@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov <gleb@redhat.com> wrote: > Knowing ioapic configuration is very useful for the poor soles > how need to debug guest occasionally. > +static struct IOAPICState *ioapic; Ugly. I really think the monitor interface needs to be changed to take an opaque parameter. The parameter can passed to the caller with for example a registration function, like the patches I sent before QObject conversion. > +void monitor_print_ioapic(Monitor *mon, const QObject *ret_data) In this case the function would be void monitor_print_ioapic(Monitor *mon, const QObject *ret_data, void *opaque) where opaque replaces the static ioapic variable. > +void do_info_ioapic(Monitor *mon, QObject **ret_data) Same here.
On Wed, Dec 30, 2009 at 12:01:28PM +0000, Blue Swirl wrote: > On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov <gleb@redhat.com> wrote: > > Knowing ioapic configuration is very useful for the poor soles > > how need to debug guest occasionally. > > > +static struct IOAPICState *ioapic; > > Ugly. I really think the monitor interface needs to be changed to take > an opaque parameter. > Agree, but that's the reality now. Pic does the same. As long as there is only one IOAPIC (and there is no indication that there will be more) that is OK. > The parameter can passed to the caller with for example a registration > function, like the patches I sent before QObject conversion. > > > +void monitor_print_ioapic(Monitor *mon, const QObject *ret_data) > > In this case the function would be > void monitor_print_ioapic(Monitor *mon, const QObject *ret_data, void *opaque) > where opaque replaces the static ioapic variable. > > > +void do_info_ioapic(Monitor *mon, QObject **ret_data) > > Same here. > -- Gleb.
On Wed, 30 Dec 2009 14:05:10 +0200 Gleb Natapov <gleb@redhat.com> wrote: > On Wed, Dec 30, 2009 at 12:01:28PM +0000, Blue Swirl wrote: > > On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov <gleb@redhat.com> wrote: > > > Knowing ioapic configuration is very useful for the poor soles > > > how need to debug guest occasionally. > > > > > +static struct IOAPICState *ioapic; > > > > Ugly. > > > Agree And now it seems like even you dont agree with me!!!
On Wed, Dec 30, 2009 at 02:08:27PM +0200, Izik Eidus wrote: > On Wed, 30 Dec 2009 14:05:10 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > On Wed, Dec 30, 2009 at 12:01:28PM +0000, Blue Swirl wrote: > > > On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov <gleb@redhat.com> wrote: > > > > Knowing ioapic configuration is very useful for the poor soles > > > > how need to debug guest occasionally. > > > > > > > +static struct IOAPICState *ioapic; > > > > > > Ugly. > > > > > Agree > > And now it seems like even you dont agree with me!!! > I agree, but all other info functions in qemu do exactly same: referencing global data. This is ugly _and_ this nothing to do with my patch. -- Gleb.
On Wed, 30 Dec 2009 13:50:43 +0200 Gleb Natapov <gleb@redhat.com> wrote: > I am starring to learn this QObject kung-fu. Nice, you really got how to do it. Just two minor comments. > One question: > Why qlist_iter(..., func, ...) and not > FOREACH_QOBJ() { > do things > } Well, when I started working on the QObjects I was still getting familiar with QEMU internals and just ignored the FOREACH_ loops. Later, I realized that they could be better but then we were planning to have libqmp and now I'm wondering if it's ok to expose data structure members to the public. > diff --git a/hw/ioapic.c b/hw/ioapic.c > index b0ad78f..efb9744 100644 > --- a/hw/ioapic.c > +++ b/hw/ioapic.c > @@ -24,6 +24,12 @@ > #include "pc.h" > #include "qemu-timer.h" > #include "host-utils.h" > +#include "monitor.h" > +#include "qint.h" > +#include "qlist.h" > +#include "qdict.h" > +#include "qstring.h" > +#include "qjson.h" You can include qemu-objects.h. > +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': 1}", i); 'masked' should be a bool, using %i will do it, like: obj = qobject_from_jsonf("{'index': %d, 'masked': %i}", i);
On Wed, Dec 30, 2009 at 11:19:30AM -0200, Luiz Capitulino wrote: > On Wed, 30 Dec 2009 13:50:43 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > I am starring to learn this QObject kung-fu. > > Nice, you really got how to do it. Just two minor comments. > > > One question: > > Why qlist_iter(..., func, ...) and not > > FOREACH_QOBJ() { > > do things > > } > > Well, when I started working on the QObjects I was still getting > familiar with QEMU internals and just ignored the FOREACH_ loops. > > Later, I realized that they could be better but then we were > planning to have libqmp and now I'm wondering if it's ok to expose > data structure members to the public. > > > diff --git a/hw/ioapic.c b/hw/ioapic.c > > index b0ad78f..efb9744 100644 > > --- a/hw/ioapic.c > > +++ b/hw/ioapic.c > > @@ -24,6 +24,12 @@ > > #include "pc.h" > > #include "qemu-timer.h" > > #include "host-utils.h" > > +#include "monitor.h" > > +#include "qint.h" > > +#include "qlist.h" > > +#include "qdict.h" > > +#include "qstring.h" > > +#include "qjson.h" > > You can include qemu-objects.h. > Hm. Copied all this from monitor.c > > +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': 1}", i); > > 'masked' should be a bool, using %i will do it, like: > > > obj = qobject_from_jsonf("{'index': %d, 'masked': %i}", i); No need to put anything here? ------------------------------^? OR should it be obj = qobject_from_jsonf("{'index': %d, 'masked': %i}", i, true); -- Gleb.
On Wed, 30 Dec 2009 12:01:28 +0000 Blue Swirl <blauwirbel@gmail.com> wrote: > On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov <gleb@redhat.com> wrote: > > Knowing ioapic configuration is very useful for the poor soles > > how need to debug guest occasionally. > > > +static struct IOAPICState *ioapic; > > Ugly. I really think the monitor interface needs to be changed to take > an opaque parameter. > > The parameter can passed to the caller with for example a registration > function, like the patches I sent before QObject conversion. Do you mind updating your series and sending it again? You could make the opaque parameter part of info_new or cmd_new, provided the new handlers have global data so that we can see better the benefit in practice.
On Wed, 30 Dec 2009 15:22:19 +0200 Gleb Natapov <gleb@redhat.com> wrote: > > > +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': 1}", i); > > > > 'masked' should be a bool, using %i will do it, like: > > > > > > obj = qobject_from_jsonf("{'index': %d, 'masked': %i}", i); > No need to put anything here? ------------------------------^? > OR should it be > obj = qobject_from_jsonf("{'index': %d, 'masked': %i}", i, true); Oh yes, you right. Actually, you can do: obj = qobject_from_jsonf("{'index': %d, 'masked': true }", i); I would also include this key in the unmasked dict (the one you fill in the else clause), just for consistency in the protocol. And yes, having to worry about the protocol in the handler is a sign that we have to improve.
On 12/30/2009 01:50 PM, Gleb Natapov wrote: > Knowing ioapic configuration is very useful for the poor soles > how need to debug guest occasionally. > + > +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; > This assumes there is only one ioapic. While I don't think there's a good reason to add more (the one we have is causing sufficient trouble), I suggest returning an array of ioapics (or include gsibase: [{ ioapic: object, gsibase: 0 }, { ioapic: object, gsibase: 24 }].
On Wed, Dec 30, 2009 at 05:04:08PM +0200, Avi Kivity wrote: > On 12/30/2009 01:50 PM, Gleb Natapov wrote: > >Knowing ioapic configuration is very useful for the poor soles > >how need to debug guest occasionally. > >+ > >+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; > > > This assumes there is only one ioapic. While I don't think there's > a good reason to add more (the one we have is causing sufficient > trouble), I suggest returning an array of ioapics (or include > gsibase: [{ ioapic: object, gsibase: 0 }, { ioapic: object, gsibase: > 24 }]. > For the case of multiple ioapics I thought about extending the command to get ioapic id as a parameter when time comes. gsibase is not know to ioapic itself, so this info does not belong here. -- Gleb.
On 12/30/2009 05:53 PM, Gleb Natapov wrote: > >> This assumes there is only one ioapic. While I don't think there's >> a good reason to add more (the one we have is causing sufficient >> trouble), I suggest returning an array of ioapics (or include >> gsibase: [{ ioapic: object, gsibase: 0 }, { ioapic: object, gsibase: >> 24 }]. >> >> > For the case of multiple ioapics I thought about extending the command > to get ioapic id as a parameter when time comes. gsibase is not know to > ioapic itself, so this info does not belong here. > > Ok.
On 12/30/2009 09:58 AM, Avi Kivity wrote: > On 12/30/2009 05:53 PM, Gleb Natapov wrote: >> >>> This assumes there is only one ioapic. While I don't think there's >>> a good reason to add more (the one we have is causing sufficient >>> trouble), I suggest returning an array of ioapics (or include >>> gsibase: [{ ioapic: object, gsibase: 0 }, { ioapic: object, gsibase: >>> 24 }]. >>> >> For the case of multiple ioapics I thought about extending the command >> to get ioapic id as a parameter when time comes. gsibase is not know to >> ioapic itself, so this info does not belong here. >> > > Ok. Here's a good example of why attacking device-info is a better general approach. Right now, you only support one ioapic and you only display a portion of the device's state. In the future, it's likely that you'll want to support multiple ioapic (okay, maybe not likely, but possible), and it's very likely you'll want to include more state for the ioapic. By definition, VMState contains all of the state of a device that's guest visible. That means from a debugging perspective, you are guaranteed to have all of the information you need for the particular device. Also, because it's necessarily to save multiple instances of a device with VMState, it automatically supports the ability to view multiple instances of a device. Regards, Anthony Liguori
On 12/30/2009 06:13 AM, Gleb Natapov wrote: > On Wed, Dec 30, 2009 at 02:08:27PM +0200, Izik Eidus wrote: >> On Wed, 30 Dec 2009 14:05:10 +0200 >> Gleb Natapov<gleb@redhat.com> wrote: >> >>> On Wed, Dec 30, 2009 at 12:01:28PM +0000, Blue Swirl wrote: >>>> On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov<gleb@redhat.com> wrote: >>>>> Knowing ioapic configuration is very useful for the poor soles >>>>> how need to debug guest occasionally. >>>> >>>>> +static struct IOAPICState *ioapic; >>>> >>>> Ugly. >>>> >>> Agree >> >> And now it seems like even you dont agree with me!!! >> > I agree, but all other info functions in qemu do exactly same: > referencing global data. This is ugly _and_ this nothing to > do with my patch. This goes away if you use VMState since we already have a global table that contains all of the registered VMState instances. It also maps device names/instance ids to the actual field contents. Regards, Anthony Liguori > -- > Gleb. > >
On Wed, Dec 30, 2009 at 05:04:53PM -0600, Anthony Liguori wrote: > On 12/30/2009 09:58 AM, Avi Kivity wrote: > >On 12/30/2009 05:53 PM, Gleb Natapov wrote: > >> > >>>This assumes there is only one ioapic. While I don't think there's > >>>a good reason to add more (the one we have is causing sufficient > >>>trouble), I suggest returning an array of ioapics (or include > >>>gsibase: [{ ioapic: object, gsibase: 0 }, { ioapic: object, gsibase: > >>>24 }]. > >>> > >>For the case of multiple ioapics I thought about extending the command > >>to get ioapic id as a parameter when time comes. gsibase is not know to > >>ioapic itself, so this info does not belong here. > >> > > > >Ok. > > Here's a good example of why attacking device-info is a better > general approach. > > Right now, you only support one ioapic and you only display a > portion of the device's state. In the future, it's likely that > you'll want to support multiple ioapic (okay, maybe not likely, but > possible), and it's very likely you'll want to include more state > for the ioapic. I included only the state I need for debugging. I don't what to see complete state. It will just clatter important info. > > By definition, VMState contains all of the state of a device that's > guest visible. That means from a debugging perspective, you are > guaranteed to have all of the information you need for the > particular device. Also, because it's necessarily to save multiple > instances of a device with VMState, it automatically supports the > ability to view multiple instances of a device. > -- Gleb.
On Wed, Dec 30, 2009 at 05:07:14PM -0600, Anthony Liguori wrote: > On 12/30/2009 06:13 AM, Gleb Natapov wrote: > >On Wed, Dec 30, 2009 at 02:08:27PM +0200, Izik Eidus wrote: > >>On Wed, 30 Dec 2009 14:05:10 +0200 > >>Gleb Natapov<gleb@redhat.com> wrote: > >> > >>>On Wed, Dec 30, 2009 at 12:01:28PM +0000, Blue Swirl wrote: > >>>>On Wed, Dec 30, 2009 at 11:50 AM, Gleb Natapov<gleb@redhat.com> wrote: > >>>>>Knowing ioapic configuration is very useful for the poor soles > >>>>>how need to debug guest occasionally. > >>>> > >>>>>+static struct IOAPICState *ioapic; > >>>> > >>>>Ugly. > >>>> > >>>Agree > >> > >>And now it seems like even you dont agree with me!!! > >> > >I agree, but all other info functions in qemu do exactly same: > >referencing global data. This is ugly _and_ this nothing to > >do with my patch. > > This goes away if you use VMState since we already have a global > table that contains all of the registered VMState instances. It > also maps device names/instance ids to the actual field contents. > I've got you point. When VMState will have this cool powers you are talking about I'll use it. I don't see the point of bragging about something that doesn't exists. -- Gleb.
On 12/30/2009 06:23 PM, Gleb Natapov wrote: > I've got you point. When VMState will have this cool powers you are talking > about I'll use it. I don't see the point of bragging about something > that doesn't exists. It *does* exist. The ioapic has already been converted to VMState. Regards, Anthony Liguori > -- > Gleb.
On 12/30/2009 06:20 PM, Gleb Natapov wrote: > I included only the state I need for debugging. I don't what to see > complete state. It will just clatter important info. Someone may find that full state useful though. There's no good reason to not show the entire device's state. Regards, Anthony Liguori
On Thu, Dec 31, 2009 at 08:16:27AM -0600, Anthony Liguori wrote: > On 12/30/2009 06:23 PM, Gleb Natapov wrote: > >I've got you point. When VMState will have this cool powers you are talking > >about I'll use it. I don't see the point of bragging about something > >that doesn't exists. > > It *does* exist. The ioapic has already been converted to VMState. > It *does not*, VMstate has nothing to do with displaying device state as of today. And implementing of general "device state printing facility" that dumps binary data was possible event before VMstate: just call device's save function with fake QEMUFile and dump it in binary. This haven't stopped addition of info functions that understand device internals and print relevant information in human readable form. This has nothing to do with VMState. -- Gleb.
On Thu, Dec 31, 2009 at 08:20:06AM -0600, Anthony Liguori wrote: > On 12/30/2009 06:20 PM, Gleb Natapov wrote: > >I included only the state I need for debugging. I don't what to see > >complete state. It will just clatter important info. > > Someone may find that full state useful though. There's no good > reason to not show the entire device's state. > There is no good reason to not show the entire device's state in addition to nicely formated most useful part of it. Here I fixed it for you. -- Gleb.
On 12/31/2009 09:42 AM, Gleb Natapov wrote: > On Thu, Dec 31, 2009 at 08:20:06AM -0600, Anthony Liguori wrote: >> On 12/30/2009 06:20 PM, Gleb Natapov wrote: >>> I included only the state I need for debugging. I don't what to see >>> complete state. It will just clatter important info. >> >> Someone may find that full state useful though. There's no good >> reason to not show the entire device's state. >> > There is no good reason to not show the entire device's state in > addition to nicely formated most useful part of it. Here I fixed it for > you. Have you even attempted to look at what the generic implementation would be? ioapic has three vmstate fields. If you decode ioredtbl into six subfields, then you're only adding two addition fields to be printed. I can't believe that having those two extra fields is really going to make it any more difficult to debug. And being able to dump any device state is certainly going to be useful in the future. Regards, Anthony Liguori > -- > Gleb.
On Thu, Dec 31, 2009 at 01:05:18PM -0600, Anthony Liguori wrote: > On 12/31/2009 09:42 AM, Gleb Natapov wrote: > >On Thu, Dec 31, 2009 at 08:20:06AM -0600, Anthony Liguori wrote: > >>On 12/30/2009 06:20 PM, Gleb Natapov wrote: > >>>I included only the state I need for debugging. I don't what to see > >>>complete state. It will just clatter important info. > >> > >>Someone may find that full state useful though. There's no good > >>reason to not show the entire device's state. > >> > >There is no good reason to not show the entire device's state in > >addition to nicely formated most useful part of it. Here I fixed it for > >you. > > > Have you even attempted to look at what the generic implementation > would be? ioapic has three vmstate fields. If you decode ioredtbl > into six subfields, then you're only adding two addition fields to > be printed. > I you seriously suggesting that we should tie the way device state is migrated to the way device information is printed? > I can't believe that having those two extra fields is really going > to make it any more difficult to debug. > Can't parse that. > And being able to dump any device state is certainly going to be > useful in the future. > No arguing with that. -- Gleb.
diff --git a/hw/ioapic.c b/hw/ioapic.c index b0ad78f..efb9744 100644 --- a/hw/ioapic.c +++ b/hw/ioapic.c @@ -24,6 +24,12 @@ #include "pc.h" #include "qemu-timer.h" #include "host-utils.h" +#include "monitor.h" +#include "qint.h" +#include "qlist.h" +#include "qdict.h" +#include "qstring.h" +#include "qjson.h" //#define DEBUG_IOAPIC @@ -50,6 +56,8 @@ struct IOAPICState { uint64_t ioredtbl[IOAPIC_NUM_PINS]; }; +static struct IOAPICState *ioapic; + static void ioapic_service(IOAPICState *s) { uint8_t i; @@ -232,7 +240,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 +253,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_haskey(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': 1}", 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, '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> -- I am starring to learn this QObject kung-fu. One question: Why qlist_iter(..., func, ...) and not FOREACH_QOBJ() { do things } -- Gleb.