On Fri, Jan 25, 2013 at 10:14:43AM -0200, Luiz Capitulino wrote: > On Thu, 24 Jan 2013 13:12:08 -0500 > Peter Feiner <pe...@gridcentric.ca> 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