Re: [Qemu-devel] [PATCH] add "info ioapic" monitor command

[Luiz Capitulino]

On Tue, 29 Dec 2009 20:49:53 +0200
Gleb Natapov <address@hidden> 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 <address@hidden> 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 <address@hidden> 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.