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

[Luiz Capitulino]

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

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