| Message ID | 20091224150242.GD5691@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 24 Dec 2009 17:02:42 +0200 Gleb Natapov <gleb@redhat.com> wrote: > + > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > + "nmi", "init", "res", "extint"}; > + > +void do_info_ioapic(Monitor *mon) > +{ > + int i; > + > + if (!ioapic) > + return; > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > + uint64 e = ioapic->ioredtbl[i]; > + monitor_printf(mon, "%2d: ", i); > + if (e & IOAPIC_LVT_MASKED) { > + monitor_printf(mon, "masked\n"); > + } 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; > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > + vec, > + delivery_mode_string[delivery_mode], > + dest_mode ? "logical":"physical", > + polarity ? "low" : "high", > + trig_mode ? "level": "edge", > + dest); > + } > + } > +} New Monitor handlers should return QObjects, monitor_printf() can only be used in the print handler. You can take a look at qemu_chr_info() for an example, as it does what I think you should do here: build a qdict for each pin and return a qlist or return another qdict if it makes sense (or will make in the future) to have more the one ioapic. I'm still thinking in ways to make the work of writing the new Monitor handlers easier..
On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > On Thu, 24 Dec 2009 17:02:42 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > + > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > + "nmi", "init", "res", "extint"}; > > + > > +void do_info_ioapic(Monitor *mon) > > +{ > > + int i; > > + > > + if (!ioapic) > > + return; > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > + uint64 e = ioapic->ioredtbl[i]; > > + monitor_printf(mon, "%2d: ", i); > > + if (e & IOAPIC_LVT_MASKED) { > > + monitor_printf(mon, "masked\n"); > > + } 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; > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > + vec, > > + delivery_mode_string[delivery_mode], > > + dest_mode ? "logical":"physical", > > + polarity ? "low" : "high", > > + trig_mode ? "level": "edge", > > + dest); > > + } > > + } > > +} > > New Monitor handlers should return QObjects, monitor_printf() > can only be used in the print handler. > So I want to add only print handler. This is debug info only, no management should ever care about it. > You can take a look at qemu_chr_info() for an example, as it does > what I think you should do here: build a qdict for each pin and > return a qlist or return another qdict if it makes sense (or will > make in the future) to have more the one ioapic. I don't understand a single word from what you are saying :(, but qemu_chr_info() looks scary. Actually I don't understand what it does at all. > > I'm still thinking in ways to make the work of writing the new > Monitor handlers easier.. Something more simple is definitely needed. If I need to pars the data structure to some intermediate language and then parse it back again just to add debug command I will not event consider adding it. One more tracepoint in the kernel will take 10min to add and will accomplish the same goal to me albeit in less elegant way. -- Gleb.
On Tue, 29 Dec 2009 18:53:44 +0200 Gleb Natapov <gleb@redhat.com> wrote: > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > On Thu, 24 Dec 2009 17:02:42 +0200 > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > + > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > + "nmi", "init", "res", "extint"}; > > > + > > > +void do_info_ioapic(Monitor *mon) > > > +{ > > > + int i; > > > + > > > + if (!ioapic) > > > + return; > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > + uint64 e = ioapic->ioredtbl[i]; > > > + monitor_printf(mon, "%2d: ", i); > > > + if (e & IOAPIC_LVT_MASKED) { > > > + monitor_printf(mon, "masked\n"); > > > + } 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; > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > + vec, > > > + delivery_mode_string[delivery_mode], > > > + dest_mode ? "logical":"physical", > > > + polarity ? "low" : "high", > > > + trig_mode ? "level": "edge", > > > + dest); > > > + } > > > + } > > > +} > > > > New Monitor handlers should return QObjects, monitor_printf() > > can only be used in the print handler. > > > So I want to add only print handler. This is debug info only, no > management should ever care about it. Well, this is possible (at least for now) but there are plans to have an external shell. Also, I don't want people to avoid writing handlers because they feel it's difficult. > > You can take a look at qemu_chr_info() for an example, as it does > > what I think you should do here: build a qdict for each pin and > > return a qlist or return another qdict if it makes sense (or will > > make in the future) to have more the one ioapic. > I don't understand a single word from what you are saying :(, but > qemu_chr_info() looks scary. Actually I don't understand what it does at > all. Ouch. :(( > > I'm still thinking in ways to make the work of writing the new > > Monitor handlers easier.. > Something more simple is definitely needed. If I need to pars the data > structure to some intermediate language and then parse it back again just to add > debug command I will not event consider adding it. One more tracepoint > in the kernel will take 10min to add and will accomplish the same goal > to me albeit in less elegant way. Actually, you're building objects and iterating over them to get them printed, it's not parsing and it's common to do that in high level languages. But I agree that it's not as easy as calling monitor_printf(). Something that is on my TODO list is to add a default printing function. This will make things easier a bit, but you'll have to build the objects and the user output won't be pretty. We could consider allowing monitor_printf() in certain handlers, but as I said above this has a number of problems and doing this because writing handlers became harder only hides the real problem (which will come up again, soon or later). So, we'll have to wait for people to come back from vacations to discuss this issue.
On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote: > On Tue, 29 Dec 2009 18:53:44 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > > On Thu, 24 Dec 2009 17:02:42 +0200 > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > + > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > > + "nmi", "init", "res", "extint"}; > > > > + > > > > +void do_info_ioapic(Monitor *mon) > > > > +{ > > > > + int i; > > > > + > > > > + if (!ioapic) > > > > + return; > > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > > + uint64 e = ioapic->ioredtbl[i]; > > > > + monitor_printf(mon, "%2d: ", i); > > > > + if (e & IOAPIC_LVT_MASKED) { > > > > + monitor_printf(mon, "masked\n"); > > > > + } 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; > > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > > + vec, > > > > + delivery_mode_string[delivery_mode], > > > > + dest_mode ? "logical":"physical", > > > > + polarity ? "low" : "high", > > > > + trig_mode ? "level": "edge", > > > > + dest); > > > > + } > > > > + } > > > > +} > > > > > > New Monitor handlers should return QObjects, monitor_printf() > > > can only be used in the print handler. > > > > > So I want to add only print handler. This is debug info only, no > > management should ever care about it. > > Well, this is possible (at least for now) but there are plans to > have an external shell. > > Also, I don't want people to avoid writing handlers because > they feel it's difficult. > > > > You can take a look at qemu_chr_info() for an example, as it does > > > what I think you should do here: build a qdict for each pin and > > > return a qlist or return another qdict if it makes sense (or will > > > make in the future) to have more the one ioapic. > > I don't understand a single word from what you are saying :(, but > > qemu_chr_info() looks scary. Actually I don't understand what it does at > > all. > > Ouch. :(( > > > > I'm still thinking in ways to make the work of writing the new > > > Monitor handlers easier.. > > Something more simple is definitely needed. If I need to pars the data > > structure to some intermediate language and then parse it back again just to add > > debug command I will not event consider adding it. One more tracepoint > > in the kernel will take 10min to add and will accomplish the same goal > > to me albeit in less elegant way. > > Actually, you're building objects and iterating over them to get them > printed, it's not parsing and it's common to do that in high level > languages. > > But I agree that it's not as easy as calling monitor_printf(). > > Something that is on my TODO list is to add a default printing > function. This will make things easier a bit, but you'll have to > build the objects and the user output won't be pretty. > > We could consider allowing monitor_printf() in certain handlers, > but as I said above this has a number of problems and doing this > because writing handlers became harder only hides the real > problem (which will come up again, soon or later). > > So, we'll have to wait for people to come back from vacations > to discuss this issue. Maybe we need a "debug" monitor command. That would just print a string, no objects, and no guarantees for management about format.
On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote: > On Tue, 29 Dec 2009 18:53:44 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > > On Thu, 24 Dec 2009 17:02:42 +0200 > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > + > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > > + "nmi", "init", "res", "extint"}; > > > > + > > > > +void do_info_ioapic(Monitor *mon) > > > > +{ > > > > + int i; > > > > + > > > > + if (!ioapic) > > > > + return; > > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > > + uint64 e = ioapic->ioredtbl[i]; > > > > + monitor_printf(mon, "%2d: ", i); > > > > + if (e & IOAPIC_LVT_MASKED) { > > > > + monitor_printf(mon, "masked\n"); > > > > + } 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; > > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > > + vec, > > > > + delivery_mode_string[delivery_mode], > > > > + dest_mode ? "logical":"physical", > > > > + polarity ? "low" : "high", > > > > + trig_mode ? "level": "edge", > > > > + dest); > > > > + } > > > > + } > > > > +} > > > > > > New Monitor handlers should return QObjects, monitor_printf() > > > can only be used in the print handler. > > > > > So I want to add only print handler. This is debug info only, no > > management should ever care about it. > > Well, this is possible (at least for now) but there are plans to > have an external shell. > > Also, I don't want people to avoid writing handlers because > they feel it's difficult. > > > > You can take a look at qemu_chr_info() for an example, as it does > > > what I think you should do here: build a qdict for each pin and > > > return a qlist or return another qdict if it makes sense (or will > > > make in the future) to have more the one ioapic. > > I don't understand a single word from what you are saying :(, but > > qemu_chr_info() looks scary. Actually I don't understand what it does at > > all. > > Ouch. :(( > Well, after looking at it for 10 more minutes I can tell that it build some kind of object hierarchy, but it is too low level and requires from casual command writer too deep knowledge of the infrastructure. > > > I'm still thinking in ways to make the work of writing the new > > > Monitor handlers easier.. > > Something more simple is definitely needed. If I need to pars the data > > structure to some intermediate language and then parse it back again just to add > > debug command I will not event consider adding it. One more tracepoint > > in the kernel will take 10min to add and will accomplish the same goal > > to me albeit in less elegant way. > > Actually, you're building objects and iterating over them to get them > printed, it's not parsing and it's common to do that in high level > languages. I high level language the syntax hides all the complexity. > > But I agree that it's not as easy as calling monitor_printf(). > > Something that is on my TODO list is to add a default printing > function. This will make things easier a bit, but you'll have to > build the objects and the user output won't be pretty. > Why not have helper function that builds objects for common data structures? For ioapic each entry is an array of strings and integers. Why not have printf like function that will build the object for me? > We could consider allowing monitor_printf() in certain handlers, > but as I said above this has a number of problems and doing this > because writing handlers became harder only hides the real > problem (which will come up again, soon or later). > > So, we'll have to wait for people to come back from vacations > to discuss this issue. -- Gleb.
On Tue, 29 Dec 2009 20:16:10 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote: > > On Tue, 29 Dec 2009 18:53:44 +0200 > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > > > On Thu, 24 Dec 2009 17:02:42 +0200 > > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > > > + > > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > > > + "nmi", "init", "res", "extint"}; > > > > > + > > > > > +void do_info_ioapic(Monitor *mon) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + if (!ioapic) > > > > > + return; > > > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > > > + uint64 e = ioapic->ioredtbl[i]; > > > > > + monitor_printf(mon, "%2d: ", i); > > > > > + if (e & IOAPIC_LVT_MASKED) { > > > > > + monitor_printf(mon, "masked\n"); > > > > > + } 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; > > > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > > > + vec, > > > > > + delivery_mode_string[delivery_mode], > > > > > + dest_mode ? "logical":"physical", > > > > > + polarity ? "low" : "high", > > > > > + trig_mode ? "level": "edge", > > > > > + dest); > > > > > + } > > > > > + } > > > > > +} > > > > > > > > New Monitor handlers should return QObjects, monitor_printf() > > > > can only be used in the print handler. > > > > > > > So I want to add only print handler. This is debug info only, no > > > management should ever care about it. > > > > Well, this is possible (at least for now) but there are plans to > > have an external shell. > > > > Also, I don't want people to avoid writing handlers because > > they feel it's difficult. > > > > > > You can take a look at qemu_chr_info() for an example, as it does > > > > what I think you should do here: build a qdict for each pin and > > > > return a qlist or return another qdict if it makes sense (or will > > > > make in the future) to have more the one ioapic. > > > I don't understand a single word from what you are saying :(, but > > > qemu_chr_info() looks scary. Actually I don't understand what it does at > > > all. > > > > Ouch. :(( > > > > > > I'm still thinking in ways to make the work of writing the new > > > > Monitor handlers easier.. > > > Something more simple is definitely needed. If I need to pars the data > > > structure to some intermediate language and then parse it back again just to add > > > debug command I will not event consider adding it. One more tracepoint > > > in the kernel will take 10min to add and will accomplish the same goal > > > to me albeit in less elegant way. > > > > Actually, you're building objects and iterating over them to get them > > printed, it's not parsing and it's common to do that in high level > > languages. > > > > But I agree that it's not as easy as calling monitor_printf(). > > > > Something that is on my TODO list is to add a default printing > > function. This will make things easier a bit, but you'll have to > > build the objects and the user output won't be pretty. > > > > We could consider allowing monitor_printf() in certain handlers, > > but as I said above this has a number of problems and doing this > > because writing handlers became harder only hides the real > > problem (which will come up again, soon or later). > > > > So, we'll have to wait for people to come back from vacations > > to discuss this issue. > > Maybe we need a "debug" monitor command. > That would just print a string, no objects, > and no guarantees for management about format. I don't think a single string will make it, Gleb's new command is an example of that.
On Tue, 29 Dec 2009 20:49:53 +0200 Gleb Natapov <gleb@redhat.com> wrote: > On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote: > > On Tue, 29 Dec 2009 18:53:44 +0200 > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > > > On Thu, 24 Dec 2009 17:02:42 +0200 > > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > > > + > > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > > > + "nmi", "init", "res", "extint"}; > > > > > + > > > > > +void do_info_ioapic(Monitor *mon) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + if (!ioapic) > > > > > + return; > > > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > > > + uint64 e = ioapic->ioredtbl[i]; > > > > > + monitor_printf(mon, "%2d: ", i); > > > > > + if (e & IOAPIC_LVT_MASKED) { > > > > > + monitor_printf(mon, "masked\n"); > > > > > + } 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; > > > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > > > + vec, > > > > > + delivery_mode_string[delivery_mode], > > > > > + dest_mode ? "logical":"physical", > > > > > + polarity ? "low" : "high", > > > > > + trig_mode ? "level": "edge", > > > > > + dest); > > > > > + } > > > > > + } > > > > > +} > > > > > > > > New Monitor handlers should return QObjects, monitor_printf() > > > > can only be used in the print handler. > > > > > > > So I want to add only print handler. This is debug info only, no > > > management should ever care about it. > > > > Well, this is possible (at least for now) but there are plans to > > have an external shell. > > > > Also, I don't want people to avoid writing handlers because > > they feel it's difficult. > > > > > > You can take a look at qemu_chr_info() for an example, as it does > > > > what I think you should do here: build a qdict for each pin and > > > > return a qlist or return another qdict if it makes sense (or will > > > > make in the future) to have more the one ioapic. > > > I don't understand a single word from what you are saying :(, but > > > qemu_chr_info() looks scary. Actually I don't understand what it does at > > > all. > > > > Ouch. :(( > > > Well, after looking at it for 10 more minutes I can tell that it build > some kind of object hierarchy, but it is too low level and requires from > casual command writer too deep knowledge of the infrastructure. "too deep" is too strong :) a simple text explaining how to use the API would be enough, imho. > > > > I'm still thinking in ways to make the work of writing the new > > > > Monitor handlers easier.. > > > Something more simple is definitely needed. If I need to pars the data > > > structure to some intermediate language and then parse it back again just to add > > > debug command I will not event consider adding it. One more tracepoint > > > in the kernel will take 10min to add and will accomplish the same goal > > > to me albeit in less elegant way. > > > > Actually, you're building objects and iterating over them to get them > > printed, it's not parsing and it's common to do that in high level > > languages. > I high level language the syntax hides all the complexity. Right, but the handling of objects is still done by the programmer. > > But I agree that it's not as easy as calling monitor_printf(). > > > > Something that is on my TODO list is to add a default printing > > function. This will make things easier a bit, but you'll have to > > build the objects and the user output won't be pretty. > > > Why not have helper function that builds objects for common data > structures? For ioapic each entry is an array of strings and integers. > Why not have printf like function that will build the object for me? We have qobject_from_jsonf(), it does exactly that. But I think your point is that the complexity lays in the fact that the programmer has to come up with an object hierarchy and keep track of it, right? I'm looking forward in ways of making the API simple, but delegating this work to some mechanism seems magic to me. I mean, having an API which accepts an arbitrary data structure and converts it to an object hierarchy. I'm open to suggestions, though.
On 12/24/2009 09:02 AM, Gleb Natapov wrote: > Knowing ioapic configuration is very useful for the poor soles > how need to debug guest occasionally. Can this be implemented in terms of VMState? Regards, Anthony Liguori
On 12/30/2009 04:00 AM, Anthony Liguori wrote: > On 12/24/2009 09:02 AM, Gleb Natapov wrote: >> Knowing ioapic configuration is very useful for the poor soles >> how need to debug guest occasionally. > > Can this be implemented in terms of VMState? > That's a good idea, but it would mean that we'd need into ioapic and info ioapic-kvm if we go for a split implementation.
On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: > On 12/24/2009 09:02 AM, Gleb Natapov wrote: > >Knowing ioapic configuration is very useful for the poor soles > >how need to debug guest occasionally. > > Can this be implemented in terms of VMState? > What do you mean and why it whould be any better? -- Gleb.
On Tue, Dec 29, 2009 at 06:49:14PM -0200, Luiz Capitulino wrote: > On Tue, 29 Dec 2009 20:49:53 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote: > > > On Tue, 29 Dec 2009 18:53:44 +0200 > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > > > > On Thu, 24 Dec 2009 17:02:42 +0200 > > > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > > > > > + > > > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > > > > + "nmi", "init", "res", "extint"}; > > > > > > + > > > > > > +void do_info_ioapic(Monitor *mon) > > > > > > +{ > > > > > > + int i; > > > > > > + > > > > > > + if (!ioapic) > > > > > > + return; > > > > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > > > > + uint64 e = ioapic->ioredtbl[i]; > > > > > > + monitor_printf(mon, "%2d: ", i); > > > > > > + if (e & IOAPIC_LVT_MASKED) { > > > > > > + monitor_printf(mon, "masked\n"); > > > > > > + } 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; > > > > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > > > > + vec, > > > > > > + delivery_mode_string[delivery_mode], > > > > > > + dest_mode ? "logical":"physical", > > > > > > + polarity ? "low" : "high", > > > > > > + trig_mode ? "level": "edge", > > > > > > + dest); > > > > > > + } > > > > > > + } > > > > > > +} > > > > > > > > > > New Monitor handlers should return QObjects, monitor_printf() > > > > > can only be used in the print handler. > > > > > > > > > So I want to add only print handler. This is debug info only, no > > > > management should ever care about it. > > > > > > Well, this is possible (at least for now) but there are plans to > > > have an external shell. > > > > > > Also, I don't want people to avoid writing handlers because > > > they feel it's difficult. > > > > > > > > You can take a look at qemu_chr_info() for an example, as it does > > > > > what I think you should do here: build a qdict for each pin and > > > > > return a qlist or return another qdict if it makes sense (or will > > > > > make in the future) to have more the one ioapic. > > > > I don't understand a single word from what you are saying :(, but > > > > qemu_chr_info() looks scary. Actually I don't understand what it does at > > > > all. > > > > > > Ouch. :(( > > > > > Well, after looking at it for 10 more minutes I can tell that it build > > some kind of object hierarchy, but it is too low level and requires from > > casual command writer too deep knowledge of the infrastructure. > > "too deep" is too strong :) a simple text explaining how to use the > API would be enough, imho. > May be. May be adding helpers functions to handle common cases would be helpful too. For instance iterating over some data structure while creating qdict object for each one of them and collecting all of them in one list is used all over the palace as far as I can see. > > > > > I'm still thinking in ways to make the work of writing the new > > > > > Monitor handlers easier.. > > > > Something more simple is definitely needed. If I need to pars the data > > > > structure to some intermediate language and then parse it back again just to add > > > > debug command I will not event consider adding it. One more tracepoint > > > > in the kernel will take 10min to add and will accomplish the same goal > > > > to me albeit in less elegant way. > > > > > > Actually, you're building objects and iterating over them to get them > > > printed, it's not parsing and it's common to do that in high level > > > languages. > > I high level language the syntax hides all the complexity. > > Right, but the handling of objects is still done by the programmer. > The handling of objects is very different when it is not part of the language. There is a difference between this: static void qemu_chr_qlist_iter(QObject *obj, void *opaque) { QDict *chr_dict; Monitor *mon = opaque; chr_dict = qobject_to_qdict(obj); monitor_printf(mon, "%s: filename=%s\n", qdict_get_str(chr_dict, "label"), qdict_get_str(chr_dict, "filename")); } void qemu_chr_info_print(Monitor *mon, const QObject *ret_data) { qlist_iter(qobject_to_qlist(ret_data), qemu_chr_qlist_iter, mon); } And this: for i in chardev: print "%(label)s: filename=%(filename)s" % i > > > But I agree that it's not as easy as calling monitor_printf(). > > > > > > Something that is on my TODO list is to add a default printing > > > function. This will make things easier a bit, but you'll have to > > > build the objects and the user output won't be pretty. > > > > > Why not have helper function that builds objects for common data > > structures? For ioapic each entry is an array of strings and integers. > > Why not have printf like function that will build the object for me? > > We have qobject_from_jsonf(), it does exactly that. BTW that is the other question I wanted to ask. What json has to do with it? As far as I underhand (and I haven't followed QMP discussion closely) json is used by QMP protocol for outside communication. Why it is used internally for object creation? Other then that it definitely looks like the function I want to use. Will try. > > But I think your point is that the complexity lays in the fact that the > programmer has to come up with an object hierarchy and keep track of it, > right? Keeping track of it is fine. Creating it looks a bit tedious. What if we had a way to describe what properties the chrdev has and how to extract them and then common function would iterate over all chrdevs creating qobject for each one of them and collecting result in a list? > > I'm looking forward in ways of making the API simple, but delegating this > work to some mechanism seems magic to me. I mean, having an API which accepts > an arbitrary data structure and converts it to an object hierarchy. > > I'm open to suggestions, though. -- Gleb.
On Wed, 30 Dec 2009 12:45:08 +0200 Gleb Natapov <gleb@redhat.com> wrote: > On Tue, Dec 29, 2009 at 06:49:14PM -0200, Luiz Capitulino wrote: > > On Tue, 29 Dec 2009 20:49:53 +0200 > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > On Tue, Dec 29, 2009 at 03:39:29PM -0200, Luiz Capitulino wrote: > > > > On Tue, 29 Dec 2009 18:53:44 +0200 > > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > > > On Tue, Dec 29, 2009 at 02:37:20PM -0200, Luiz Capitulino wrote: > > > > > > On Thu, 24 Dec 2009 17:02:42 +0200 > > > > > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > > > > > > > > + > > > > > > > +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", > > > > > > > + "nmi", "init", "res", "extint"}; > > > > > > > + > > > > > > > +void do_info_ioapic(Monitor *mon) > > > > > > > +{ > > > > > > > + int i; > > > > > > > + > > > > > > > + if (!ioapic) > > > > > > > + return; > > > > > > > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > > > > > > > + uint64 e = ioapic->ioredtbl[i]; > > > > > > > + monitor_printf(mon, "%2d: ", i); > > > > > > > + if (e & IOAPIC_LVT_MASKED) { > > > > > > > + monitor_printf(mon, "masked\n"); > > > > > > > + } 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; > > > > > > > + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", > > > > > > > + vec, > > > > > > > + delivery_mode_string[delivery_mode], > > > > > > > + dest_mode ? "logical":"physical", > > > > > > > + polarity ? "low" : "high", > > > > > > > + trig_mode ? "level": "edge", > > > > > > > + dest); > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > > > > > > New Monitor handlers should return QObjects, monitor_printf() > > > > > > can only be used in the print handler. > > > > > > > > > > > So I want to add only print handler. This is debug info only, no > > > > > management should ever care about it. > > > > > > > > Well, this is possible (at least for now) but there are plans to > > > > have an external shell. > > > > > > > > Also, I don't want people to avoid writing handlers because > > > > they feel it's difficult. > > > > > > > > > > You can take a look at qemu_chr_info() for an example, as it does > > > > > > what I think you should do here: build a qdict for each pin and > > > > > > return a qlist or return another qdict if it makes sense (or will > > > > > > make in the future) to have more the one ioapic. > > > > > I don't understand a single word from what you are saying :(, but > > > > > qemu_chr_info() looks scary. Actually I don't understand what it does at > > > > > all. > > > > > > > > Ouch. :(( > > > > > > > Well, after looking at it for 10 more minutes I can tell that it build > > > some kind of object hierarchy, but it is too low level and requires from > > > casual command writer too deep knowledge of the infrastructure. > > > > "too deep" is too strong :) a simple text explaining how to use the > > API would be enough, imho. > > > May be. May be adding helpers functions to handle common cases would be > helpful too. Sure. > For instance iterating over some data structure while creating > qdict object for each one of them and collecting all of them in one list > is used all over the palace as far as I can see. The problem I have in devising a helper like that is how to do the mapping between data structure member and dict key, given that they have to be accessed at run time. Also, I think that even some simple data structures may need human intervention, when nested dicts are used for example. But maybe, the helper could accept an array with that information, that could be filled by the user. But I wonder if that really makes things simple or we could make this as part of the structure itself someway. Ideas? > > > > > > I'm still thinking in ways to make the work of writing the new > > > > > > Monitor handlers easier.. > > > > > Something more simple is definitely needed. If I need to pars the data > > > > > structure to some intermediate language and then parse it back again just to add > > > > > debug command I will not event consider adding it. One more tracepoint > > > > > in the kernel will take 10min to add and will accomplish the same goal > > > > > to me albeit in less elegant way. > > > > > > > > Actually, you're building objects and iterating over them to get them > > > > printed, it's not parsing and it's common to do that in high level > > > > languages. > > > I high level language the syntax hides all the complexity. > > > > Right, but the handling of objects is still done by the programmer. > > > The handling of objects is very different when it is not part of the > language. > > There is a difference between this: > > static void qemu_chr_qlist_iter(QObject *obj, void *opaque) > { > QDict *chr_dict; > Monitor *mon = opaque; > > chr_dict = qobject_to_qdict(obj); > monitor_printf(mon, "%s: filename=%s\n", qdict_get_str(chr_dict, "label"), > qdict_get_str(chr_dict, "filename")); > } > > void qemu_chr_info_print(Monitor *mon, const QObject *ret_data) > { > qlist_iter(qobject_to_qlist(ret_data), qemu_chr_qlist_iter, mon); > } > > And this: > for i in chardev: > print "%(label)s: filename=%(filename)s" % i Ah, sure thing! And I'm all for porting QEMU to a new language!! > > > > But I agree that it's not as easy as calling monitor_printf(). > > > > > > > > Something that is on my TODO list is to add a default printing > > > > function. This will make things easier a bit, but you'll have to > > > > build the objects and the user output won't be pretty. > > > > > > > Why not have helper function that builds objects for common data > > > structures? For ioapic each entry is an array of strings and integers. > > > Why not have printf like function that will build the object for me? > > > > We have qobject_from_jsonf(), it does exactly that. > BTW that is the other question I wanted to ask. What json has to do with > it? As far as I underhand (and I haven't followed QMP discussion > closely) json is used by QMP protocol for outside communication. Why it > is used internally for object creation? Other then that it definitely looks > like the function I want to use. Will try. The printf like function for creating objects required a language format, we could come up with our own format but json was good choice, first because it's a simple Python-like format, but also because the encoder/decoder would work with the same syntax. > > But I think your point is that the complexity lays in the fact that the > > programmer has to come up with an object hierarchy and keep track of it, > > right? > Keeping track of it is fine. Creating it looks a bit tedious. What if we > had a way to describe what properties the chrdev has and how to extract > them and then common function would iterate over all chrdevs creating > qobject for each one of them and collecting result in a list? Yes, this looks a good approach and I hope that what I described above matches this. :) I just have to find the time to work on this now...
On 12/30/2009 03:27 AM, Gleb Natapov wrote: > On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: >> On 12/24/2009 09:02 AM, Gleb Natapov wrote: >>> Knowing ioapic configuration is very useful for the poor soles >>> how need to debug guest occasionally. >> >> Can this be implemented in terms of VMState? >> > What do you mean and why it whould be any better? We've been discussing having a command that's something like 'info-device ioapic' that would dump the device state based on what's stored in VMState for the device. The advantage of a command like this is that it works for any qdev device making it much more useful. The disadvantage is that at the moment, we store ioredtbl as an array of uint64s. To get the level of info you're looking for we would need to teach VMState how to decode this structure. Regards, Anthony Liguori > -- > Gleb.
On 12/30/2009 12:53 AM, Avi Kivity wrote: > On 12/30/2009 04:00 AM, Anthony Liguori wrote: >> On 12/24/2009 09:02 AM, Gleb Natapov wrote: >>> Knowing ioapic configuration is very useful for the poor soles >>> how need to debug guest occasionally. >> >> Can this be implemented in terms of VMState? >> > > That's a good idea, but it would mean that we'd need into ioapic and > info ioapic-kvm if we go for a split implementation. I don't think that's a problem. A developer will be smart enough to know that they need to do that (and will get an error if they don't). Regards, Anthony Liguori
On 12/30/2009 04:17 PM, Anthony Liguori wrote: >> That's a good idea, but it would mean that we'd need into ioapic and >> info ioapic-kvm if we go for a split implementation. > > > I don't think that's a problem. A developer will be smart enough to > know that they need to do that (and will get an error if they don't). > I wasn't worried about that, only the increased code duplication.
On Wed, Dec 30, 2009 at 08:16:30AM -0600, Anthony Liguori wrote: > On 12/30/2009 03:27 AM, Gleb Natapov wrote: > >On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: > >>On 12/24/2009 09:02 AM, Gleb Natapov wrote: > >>>Knowing ioapic configuration is very useful for the poor soles > >>>how need to debug guest occasionally. > >> > >>Can this be implemented in terms of VMState? > >> > >What do you mean and why it whould be any better? > > We've been discussing having a command that's something like > 'info-device ioapic' that would dump the device state based on > what's stored in VMState for the device. > > The advantage of a command like this is that it works for any qdev > device making it much more useful. > > The disadvantage is that at the moment, we store ioredtbl as an > array of uint64s. To get the level of info you're looking for we > would need to teach VMState how to decode this structure. > So this is completely orthogonal to my patch. There are devices already that have info command. No need to hold the patch just because sometime in the feature VMState may be extended. Other then that adding print function to VMState sounds like a good idea. -- Gleb.
On Wed, Dec 30, 2009 at 04:26:11PM +0200, Gleb Natapov wrote: > On Wed, Dec 30, 2009 at 08:16:30AM -0600, Anthony Liguori wrote: > > On 12/30/2009 03:27 AM, Gleb Natapov wrote: > > >On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: > > >>On 12/24/2009 09:02 AM, Gleb Natapov wrote: > > >>>Knowing ioapic configuration is very useful for the poor soles > > >>>how need to debug guest occasionally. > > >> > > >>Can this be implemented in terms of VMState? > > >> > > >What do you mean and why it whould be any better? > > > > We've been discussing having a command that's something like > > 'info-device ioapic' that would dump the device state based on > > what's stored in VMState for the device. > > > > The advantage of a command like this is that it works for any qdev > > device making it much more useful. > > > > The disadvantage is that at the moment, we store ioredtbl as an > > array of uint64s. To get the level of info you're looking for we > > would need to teach VMState how to decode this structure. > > > > So this is completely orthogonal to my patch. There are devices already > that have info command. No need to hold the patch just because sometime > in the feature VMState may be extended. Other then that adding print > function to VMState sounds like a good idea. BTW I'd like this to be backported to 0.12 too. -- Gleb.
On 12/30/2009 08:19 AM, Avi Kivity wrote: > On 12/30/2009 04:17 PM, Anthony Liguori wrote: >>> That's a good idea, but it would mean that we'd need into ioapic and >>> info ioapic-kvm if we go for a split implementation. >> >> >> I don't think that's a problem. A developer will be smart enough to >> know that they need to do that (and will get an error if they don't). >> > > I wasn't worried about that, only the increased code duplication. I wasn't thinking there would be an info ioapic command but rather a generic device-info command that would work with any qdev device. No code duplication (in fact, much, much less in the long term). Regards, Anthony Liguori
On 12/30/2009 08:30 AM, Gleb Natapov wrote: > On Wed, Dec 30, 2009 at 04:26:11PM +0200, Gleb Natapov wrote: >> On Wed, Dec 30, 2009 at 08:16:30AM -0600, Anthony Liguori wrote: >>> On 12/30/2009 03:27 AM, Gleb Natapov wrote: >>>> On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: >>>>> On 12/24/2009 09:02 AM, Gleb Natapov wrote: >>>>>> Knowing ioapic configuration is very useful for the poor soles >>>>>> how need to debug guest occasionally. >>>>> >>>>> Can this be implemented in terms of VMState? >>>>> >>>> What do you mean and why it whould be any better? >>> >>> We've been discussing having a command that's something like >>> 'info-device ioapic' that would dump the device state based on >>> what's stored in VMState for the device. >>> >>> The advantage of a command like this is that it works for any qdev >>> device making it much more useful. >>> >>> The disadvantage is that at the moment, we store ioredtbl as an >>> array of uint64s. To get the level of info you're looking for we >>> would need to teach VMState how to decode this structure. >>> >> >> So this is completely orthogonal to my patch. There are devices already >> that have info command. No need to hold the patch just because sometime >> in the feature VMState may be extended. Other then that adding print >> function to VMState sounds like a good idea. > BTW I'd like this to be backported to 0.12 too. It's not a bug fix so that doesn't make sense. Regards, Anthony Liguori > -- > Gleb.
On 12/30/2009 08:26 AM, Gleb Natapov wrote: > On Wed, Dec 30, 2009 at 08:16:30AM -0600, Anthony Liguori wrote: >> On 12/30/2009 03:27 AM, Gleb Natapov wrote: >>> On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: >>>> On 12/24/2009 09:02 AM, Gleb Natapov wrote: >>>>> Knowing ioapic configuration is very useful for the poor soles >>>>> how need to debug guest occasionally. >>>> >>>> Can this be implemented in terms of VMState? >>>> >>> What do you mean and why it whould be any better? >> >> We've been discussing having a command that's something like >> 'info-device ioapic' that would dump the device state based on >> what's stored in VMState for the device. >> >> The advantage of a command like this is that it works for any qdev >> device making it much more useful. >> >> The disadvantage is that at the moment, we store ioredtbl as an >> array of uint64s. To get the level of info you're looking for we >> would need to teach VMState how to decode this structure. >> > > So this is completely orthogonal to my patch. There are devices already > that have info command. The other commands were added before we had the ability to do it right. Now we can do it right and there's really no reason not to. Regards, Anthony Liguori
On Wed, Dec 30, 2009 at 04:49:01PM -0600, Anthony Liguori wrote: > On 12/30/2009 08:26 AM, Gleb Natapov wrote: > >On Wed, Dec 30, 2009 at 08:16:30AM -0600, Anthony Liguori wrote: > >>On 12/30/2009 03:27 AM, Gleb Natapov wrote: > >>>On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: > >>>>On 12/24/2009 09:02 AM, Gleb Natapov wrote: > >>>>>Knowing ioapic configuration is very useful for the poor soles > >>>>>how need to debug guest occasionally. > >>>> > >>>>Can this be implemented in terms of VMState? > >>>> > >>>What do you mean and why it whould be any better? > >> > >>We've been discussing having a command that's something like > >>'info-device ioapic' that would dump the device state based on > >>what's stored in VMState for the device. > >> > >>The advantage of a command like this is that it works for any qdev > >>device making it much more useful. > >> > >>The disadvantage is that at the moment, we store ioredtbl as an > >>array of uint64s. To get the level of info you're looking for we > >>would need to teach VMState how to decode this structure. > >> > > > >So this is completely orthogonal to my patch. There are devices already > >that have info command. > > The other commands were added before we had the ability to do it > right. Now we can do it right and there's really no reason not to. > No we don't. You want to add the infrastructure to do it right and I have no problem with that go add it. But till then we do it old way. -- Gleb.
On Wed, Dec 30, 2009 at 04:47:45PM -0600, Anthony Liguori wrote: > On 12/30/2009 08:30 AM, Gleb Natapov wrote: > >On Wed, Dec 30, 2009 at 04:26:11PM +0200, Gleb Natapov wrote: > >>On Wed, Dec 30, 2009 at 08:16:30AM -0600, Anthony Liguori wrote: > >>>On 12/30/2009 03:27 AM, Gleb Natapov wrote: > >>>>On Tue, Dec 29, 2009 at 08:00:09PM -0600, Anthony Liguori wrote: > >>>>>On 12/24/2009 09:02 AM, Gleb Natapov wrote: > >>>>>>Knowing ioapic configuration is very useful for the poor soles > >>>>>>how need to debug guest occasionally. > >>>>> > >>>>>Can this be implemented in terms of VMState? > >>>>> > >>>>What do you mean and why it whould be any better? > >>> > >>>We've been discussing having a command that's something like > >>>'info-device ioapic' that would dump the device state based on > >>>what's stored in VMState for the device. > >>> > >>>The advantage of a command like this is that it works for any qdev > >>>device making it much more useful. > >>> > >>>The disadvantage is that at the moment, we store ioredtbl as an > >>>array of uint64s. To get the level of info you're looking for we > >>>would need to teach VMState how to decode this structure. > >>> > >> > >>So this is completely orthogonal to my patch. There are devices already > >>that have info command. No need to hold the patch just because sometime > >>in the feature VMState may be extended. Other then that adding print > >>function to VMState sounds like a good idea. > >BTW I'd like this to be backported to 0.12 too. > > It's not a bug fix so that doesn't make sense. > It helps debug problems and it is not intrusive. Since 0.12 will be used for a long time I want to add it there for convenience. -- Gleb.
On 12/31/2009 12:46 AM, Anthony Liguori wrote: >> I wasn't worried about that, only the increased code duplication. > > > I wasn't thinking there would be an info ioapic command but rather a > generic device-info command that would work with any qdev device. No > code duplication (in fact, much, much less in the long term). Machine printing isn't going to work. The information needs to be formatted to be legible and bit fields decoded. Some field's interpretation may depend on the value of others. In some case machine printing of vmstate will show you the history of that devices live migration bug fixes in addition to its state.
On 12/30/2009 06:14 PM, Gleb Natapov wrote: > No we don't. You want to add the infrastructure to do it right and I > have no problem with that go add it. But till then we do it old way. We have the infrastructure today to do it the right way. Regards, Anthony Liguori > -- > Gleb.
On 12/30/2009 06:16 PM, Gleb Natapov wrote: > It helps debug problems and it is not intrusive. Since 0.12 will be used > for a long time I want to add it there for convenience. stable is for bug fixes only. Regards, Anthony Liguori > -- > Gleb.
On 12/31/2009 12:38 AM, Avi Kivity wrote: > On 12/31/2009 12:46 AM, Anthony Liguori wrote: >>> I wasn't worried about that, only the increased code duplication. >> >> >> I wasn't thinking there would be an info ioapic command but rather a >> generic device-info command that would work with any qdev device. No >> code duplication (in fact, much, much less in the long term). > > Machine printing isn't going to work. The information needs to be > formatted to be legible and bit fields decoded. Some field's > interpretation may depend on the value of others. The cases where you want to have complex display of a device's state is the exception and it should be treated as such. The common case is where you just want to see the state of the device. We support hundreds of devices. We can have one code path that displays 95% of them with a couple devices that have exceptions or we could have hundreds of these commands that aren't consistent because they're all open-coded. In some case machine > printing of vmstate will show you the history of that devices live > migration bug fixes in addition to its state. I don't see this as a bad thing. Regards, Anthony Liguori
On Thu, Dec 31, 2009 at 08:15:29AM -0600, Anthony Liguori wrote: > On 12/30/2009 06:16 PM, Gleb Natapov wrote: > >It helps debug problems and it is not intrusive. Since 0.12 will be used > >for a long time I want to add it there for convenience. > > stable is for bug fixes only. > This is to close minded. Not able to debug hard to reproduce problems when they appear at the environment you can't replicate is a bug. -- Gleb.
On Thu, Dec 31, 2009 at 08:18:40AM -0600, Anthony Liguori wrote: > On 12/31/2009 12:38 AM, Avi Kivity wrote: > >On 12/31/2009 12:46 AM, Anthony Liguori wrote: > >>>I wasn't worried about that, only the increased code duplication. > >> > >> > >>I wasn't thinking there would be an info ioapic command but rather a > >>generic device-info command that would work with any qdev device. No > >>code duplication (in fact, much, much less in the long term). > > > >Machine printing isn't going to work. The information needs to be > >formatted to be legible and bit fields decoded. Some field's > >interpretation may depend on the value of others. > > The cases where you want to have complex display of a device's state > is the exception and it should be treated as such. > This is the other way around. We always want to have complex display of a device's state, but because we have many devices and not many resources it is impossible to achieve. > The common case is where you just want to see the state of the > device. We support hundreds of devices. We can have one code path > that displays 95% of them with a couple devices that have exceptions > or we could have hundreds of these commands that aren't consistent > because they're all open-coded. I am not against having common code path that prints out state of all devices. It just different from what I am trying to achieve with my patch. My patch adds parsing and pretty-printing of device state and that's the part that is not addressed with your common code scenario. > > In some case machine > >printing of vmstate will show you the history of that devices live > >migration bug fixes in addition to its state. > > I don't see this as a bad thing. > > Regards, > > Anthony Liguori -- Gleb.
On 12/31/2009 09:33 AM, Gleb Natapov wrote: > On Thu, Dec 31, 2009 at 08:15:29AM -0600, Anthony Liguori wrote: >> On 12/30/2009 06:16 PM, Gleb Natapov wrote: >>> It helps debug problems and it is not intrusive. Since 0.12 will be used >>> for a long time I want to add it there for convenience. >> >> stable is for bug fixes only. >> > This is to close minded. Not able to debug hard to reproduce problems when they appear > at the environment you can't replicate is a bug. Every feature is important to someone and there is at least someone who wants just about every feature backported to stable. Looking at this patch, it adds a new monitor command which potentially means that the QMP protocol is changing (as there is now a new command). This is definitely not something we want to do in stable. Regards, Anthony Liguori > -- > Gleb.
On 12/31/2009 09:39 AM, Gleb Natapov wrote: >> The common case is where you just want to see the state of the >> device. We support hundreds of devices. We can have one code path >> that displays 95% of them with a couple devices that have exceptions >> or we could have hundreds of these commands that aren't consistent >> because they're all open-coded. > I am not against having common code path that prints out state of all > devices. It just different from what I am trying to achieve with my patch. > My patch adds parsing and pretty-printing of device state and that's the > part that is not addressed with your common code scenario. A common device printing mechanism is probably no more code than your current patch. My position is that if that generic mechanism is not good enough for you, then we should look at fixing that problem instead of introducing a completely redundant mechanism. Heck, even if you had a generic mechanism and then a big if in the middle of it that treated ioapic better than other devices, it would be 100x better than introducing a 1-off command. Regards, Anthony Liguori >> >> In some case machine >>> printing of vmstate will show you the history of that devices live >>> migration bug fixes in addition to its state. >> >> I don't see this as a bad thing. >> >> Regards, >> >> Anthony Liguori > > -- > Gleb.
On Thu, Dec 31, 2009 at 12:57:44PM -0600, Anthony Liguori wrote: > On 12/31/2009 09:33 AM, Gleb Natapov wrote: > >On Thu, Dec 31, 2009 at 08:15:29AM -0600, Anthony Liguori wrote: > >>On 12/30/2009 06:16 PM, Gleb Natapov wrote: > >>>It helps debug problems and it is not intrusive. Since 0.12 will be used > >>>for a long time I want to add it there for convenience. > >> > >>stable is for bug fixes only. > >> > >This is to close minded. Not able to debug hard to reproduce problems when they appear > >at the environment you can't replicate is a bug. > > > Every feature is important to someone and there is at least someone > who wants just about every feature backported to stable. > Agree and that is why practical judgment is needed. Humans, not binary algorithm decides what to port and what not too. > Looking at this patch, it adds a new monitor command which > potentially means that the QMP protocol is changing (as there is now > a new command). This is definitely not something we want to do in > stable. > Does QMP expose list of available command through QMP protocol? Because if it is not, there is no such issues as you describe at all, but event if it is isn't the goal of QMP to make handling of such cases transparent for management software. There will be one more element returned in command list that is all. -- Gleb.
On Thu, Dec 31, 2009 at 01:01:50PM -0600, Anthony Liguori wrote: > On 12/31/2009 09:39 AM, Gleb Natapov wrote: > >>The common case is where you just want to see the state of the > >>device. We support hundreds of devices. We can have one code path > >>that displays 95% of them with a couple devices that have exceptions > >>or we could have hundreds of these commands that aren't consistent > >>because they're all open-coded. > >I am not against having common code path that prints out state of all > >devices. It just different from what I am trying to achieve with my patch. > >My patch adds parsing and pretty-printing of device state and that's the > >part that is not addressed with your common code scenario. > > A common device printing mechanism is probably no more code than > your current patch. > Have you looked at my patch at all? What part of it can be made generic in your opinion? All it does is transforms ioapic state to human readable way (and that requires device internals kwoulage) and pot it into QObject form. > My position is that if that generic mechanism is not good enough for > you, then we should look at fixing that problem instead of > introducing a completely redundant mechanism. > Generic mechanism is by definition cannot decode device state with knowledge of device specific details. > Heck, even if you had a generic mechanism and then a big if in the > middle of it that treated ioapic better than other devices, it would > be 100x better than introducing a 1-off command. > The 'if' you are talking about (which is crazy by the way 'if' for each device in a common code?) will call the function my patch adds. -- Gleb.
diff --git a/hw/ioapic.c b/hw/ioapic.c index b0ad78f..ffbe631 100644 --- a/hw/ioapic.c +++ b/hw/ioapic.c @@ -24,6 +24,7 @@ #include "pc.h" #include "qemu-timer.h" #include "host-utils.h" +#include "monitor.h" //#define DEBUG_IOAPIC @@ -50,6 +51,8 @@ struct IOAPICState { uint64_t ioredtbl[IOAPIC_NUM_PINS]; }; +static struct IOAPICState *ioapic; + static void ioapic_service(IOAPICState *s) { uint8_t i; @@ -232,7 +235,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 +248,35 @@ qemu_irq *ioapic_init(void) return irq; } + +static const char *delivery_mode_string[] = {"fixed", "lowprio", "smi", "res", + "nmi", "init", "res", "extint"}; + +void do_info_ioapic(Monitor *mon) +{ + int i; + + if (!ioapic) + return; + for (i = 0; i < IOAPIC_NUM_PINS; i++) { + uint64 e = ioapic->ioredtbl[i]; + monitor_printf(mon, "%2d: ", i); + if (e & IOAPIC_LVT_MASKED) { + monitor_printf(mon, "masked\n"); + } 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; + monitor_printf(mon, "vec=%3d %s %s acive-%s %s dest=%d\n", + vec, + delivery_mode_string[delivery_mode], + dest_mode ? "logical":"physical", + polarity ? "low" : "high", + trig_mode ? "level": "edge", + dest); + } + } +} diff --git a/hw/pc.h b/hw/pc.h index 03ffc91..6efb3e8 100644 --- a/hw/pc.h +++ b/hw/pc.h @@ -45,6 +45,7 @@ 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); 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..7848965 100644 --- a/monitor.c +++ b/monitor.c @@ -2625,6 +2625,13 @@ static const mon_cmd_t info_cmds[] = { .mhandler.info = do_info_roms, }, { + .name = "ioapic", + .args_type = "", + .params = "", + .help = "show ioapic config", + .mhandler.info = 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.