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

[Gleb Natapov]

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.

> > >  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.