Re: [Qemu-devel] [PATCH] qapi/x86: add control registers to query-cpus

[Eduardo Habkost]

On Fri, Jan 25, 2013 at 10:14:43AM -0200, Luiz Capitulino wrote:
> On Thu, 24 Jan 2013 13:12:08 -0500
> Peter Feiner <address@hidden> wrote:
> 
> > > What about converting 'info registers' to QMP (ie. having 
> > > query-cpu-registers)?
> > 
> > We had thought about it, but we decided to go with this lower hanging fruit
> > because it provides immediately useful functionality at a low implementation
> > cost. It's harder (for us) to think of why would anyone want to know XMM12 
> > or
> > r10 in the general case outside of gdb, which is already supported.
> 
> For the same reason you need the other registers now. Besides, it would be
> nice to allow GUIs to have more debug info like this.

Maybe a GUI that needs to show that debug info should use gdb instead?

> 
> Let me re-state the problem for the CC'ed people: you're adding x86 control
> registers to the query-cpus command. I think this has a few problems:
> 
>  1. Won't "scale", as query-cpus will become a huge mess if people start
>     doing this for other archs
> 
>  2. query-cpus is bad designed. I'd prefer adding new commands instead of
>     extending it (unless the information is general enough)
> 
>  3. It's very desirable to have registers info in QMP

Is it?


> 
> The obvious suggestion is to add query-cpu-registers. I understand this has a
> few problems (see questions below), but I think the following incremental
> approach could work:
> 
>  1. Add a CPURegisters union
>  2. Each CPU arch is added as a type to the union (eg. CPUX86Registers)
>  3. query-cpu-registers returns the union

We already have CPU QOM objects, we just need to add a
methods/properties so each per-architecture subclass will implement
what's necessary for the command.

We could go even further: just use the qom-* commands. Then if there's
some set of registers we really want to expose to the outside, just add
them as properties to the CPU object.


>  4. Move do_info_registers() to hmp.c as hmp_info_registers()
>  5. Change hmp_info_registers() to first call qmp_query_cpu_registers(), if
>     this returns the CPU arch it expects, then print it. Otherwise fallback
>     to cpu_dump_state()
> 
> You start by adding CPUX86Registers. Other CPUs are added as needed.
> 
> What do the CC'ed people think?

I still don't see why we need this. But if we need it, why aren't the
qom-* commands good enough to implement this, instead of adding new
commands?


> 
> > Additionally, porting over the entire functionality of 'info registers' has 
> > a
> > bunch of wrinkles:
> > 
> >     * I'm afraid that 'info registers' couldn't so much be converted from 
> > HMP to
> >       QMP as added. That is, most of the work done by each target's various
> >       'info registers' implementation (i.e., the target's cpu_dump_state
> >       function) is formatting the output nicely. So most of the existing
> >       'info registers' logic would remain and a qmp_query_cpu_registers 
> > would
> >       have to be added for each target.
> 
> Explained above how this could be solved.
> 
> >     * How do you represent 128bit registers (e.g., XMM)?  Perhaps add a
> >       'int128' type to QMP? The simplest solution is 64-bit components 
> > (e.g.,
> >       XMM0_low, XMM0_high).
> 
> Yes, that's where json is problematic. We could have _low and _high you
> suggest or represent 128bit registers as strings.

QOM properties would be problematic as well, as
object_property_{get,set}_int() and QInt support only 64-bit values. But
in the worst case we can work around it using strings or _high/_low
properties.


> 
> >     * Like query-cpus, does the schema make all registers optional because
> >       they're architecture specific? This would entail hundreds of data 
> > fields.
> >       Or should query-cpu-registers return a list of (name, value) pairs?
> 
> QAPI has union support, I think that's the best way to solve this. Search
> for 'union' in qapi-schema.json.

QOM is even more flexible: it has inheritance and allows introspection
of properties at runtime.

> 
> >     * Should register names change depending on processor mode (e.g., eax vs
> >       rax), or should they have canonical names (e.g., always use "a" or 
> > "rax").
> 
> I don't think they should change.

Agreed.

-- 
Eduardo