Patchwork add "info ioapic" monitor command

login
register
mail settings
Submitter Gleb Natapov
Date Dec. 24, 2009, 3:02 p.m.
Message ID <20091224150242.GD5691@redhat.com>
Download mbox | patch
Permalink /patch/41780/
State New
Headers show

Comments

Gleb Natapov - Dec. 24, 2009, 3:02 p.m.
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.
Luiz Capitulino - Dec. 29, 2009, 4:37 p.m.
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..
Gleb Natapov - Dec. 29, 2009, 4:53 p.m.
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.
Luiz Capitulino - Dec. 29, 2009, 5:39 p.m.
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.
Michael S. Tsirkin - Dec. 29, 2009, 6:16 p.m.
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.
Gleb Natapov - Dec. 29, 2009, 6:49 p.m.
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.
Luiz Capitulino - Dec. 29, 2009, 8:06 p.m.
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.
Luiz Capitulino - Dec. 29, 2009, 8:49 p.m.
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.
Anthony Liguori - Dec. 30, 2009, 2 a.m.
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
Avi Kivity - Dec. 30, 2009, 6:53 a.m.
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.
Gleb Natapov - Dec. 30, 2009, 9:27 a.m.
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.
Gleb Natapov - Dec. 30, 2009, 10:45 a.m.
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.
Luiz Capitulino - Dec. 30, 2009, 1:05 p.m.
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...
Anthony Liguori - Dec. 30, 2009, 2:16 p.m.
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.
Anthony Liguori - Dec. 30, 2009, 2:17 p.m.
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
Avi Kivity - Dec. 30, 2009, 2:19 p.m.
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.
Gleb Natapov - Dec. 30, 2009, 2:26 p.m.
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.
Gleb Natapov - Dec. 30, 2009, 2:30 p.m.
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.
Anthony Liguori - Dec. 30, 2009, 10:46 p.m.
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
Anthony Liguori - Dec. 30, 2009, 10:47 p.m.
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.
Anthony Liguori - Dec. 30, 2009, 10:49 p.m.
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
Gleb Natapov - Dec. 31, 2009, 12:14 a.m.
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.
Gleb Natapov - Dec. 31, 2009, 12:16 a.m.
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.
Avi Kivity - Dec. 31, 2009, 6:38 a.m.
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.
Anthony Liguori - Dec. 31, 2009, 2:15 p.m.
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.
Anthony Liguori - Dec. 31, 2009, 2:15 p.m.
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.
Anthony Liguori - Dec. 31, 2009, 2:18 p.m.
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
Gleb Natapov - Dec. 31, 2009, 3:33 p.m.
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.
Gleb Natapov - Dec. 31, 2009, 3:39 p.m.
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.
Anthony Liguori - Dec. 31, 2009, 6:57 p.m.
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.
Anthony Liguori - Dec. 31, 2009, 7:01 p.m.
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.
Gleb Natapov - Jan. 1, 2010, 10:23 a.m.
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.
Gleb Natapov - Jan. 1, 2010, 10:37 a.m.
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.

Patch

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,
     },
 };