Re: [Qemu-devel] [PATCH v5 03/43] monitor: Don't access registers through CPUState

[Lluís Vilanova]

Andreas Färber writes:

> Am 15.03.2012 17:15, schrieb Lluís Vilanova:
>> Andreas Färber writes:
>> 
>>> Use CPUX86State etc. instead (hand-converted).
>> 
>> Is there any reason not to move this into CPU${arch}State and instead provide
>> virtual methods in CPUState?

> Yes, a very simple one: This is one of the prerequisite patches to
> having a QOM CPUState struct we can place such virtual methods in.

Sure, I meant besides that :)


> Originally, not even reset was in there, but there were comments that my
> series was getting too long, so I moved it out of the ARM series and
> squashed it into what is now patch 43.

>> For example (a CPUState argument is implicit on all methods):
>> 
>> int reg_get_number(const char *regname);
>> const char* reg_get_name(int regnum);
>> 
>> /* register validity for a specific instantiation must be chekced somehow */
>> bool reg_valid(int regnum);
>> 
>> size_t reg_get_size(int regnum);
>> 
>> /* multiple sizes must be handled somehow */
>> /* precondition: get_reg_validity(regnum) == true */
>> void reg_get_value(int regnum, void *buf);
>> void reg_set_value(int regnum, void *buf);
>> 
>> 
>> This way, target-specific code would be moved where it belongs, partially
>> merging (for example) code from both monitor.c, gdbstub.c and the target
>> itself.

> That is definitely the direction I want to go with QOM CPUs. I haven't
> dug into gdbstub or monitor too deeply yet, beyond looking for ways to
> make the CPUState -> CPUArchState conversion as small as possible.

Well, I just saw that when looking at your series.


> In this case, I am not sure why the original authors chose not to place
> the code into target-*. Maybe those reasons no longer apply.

> Anyway, once all CPUs are converted - or if you're adventurous based on
> my qom-cpu-wip branch - I'd be happy to review patches moving more stuff
> into CPUState. :) Moving properties from CPUArchState into CPUState
> seemed fairly mechanical, so I attacked the more challenging problem of
> statically calculated offsets for TCG's icount first.

> I'd also envision some of the #defines we have flying around in cpu.h to
> turn into read-only CPU properties, like page size or number of MMU
> modes, so that the CPU becomes more self-describing and doesn't pollute
> the global namespace.

>> The downside is that using pointers to set/get the value is pretty
>> uncomfortable.

> We can add cpu_...(CPUState, ...) wrapper functions, like cpu_reset().

What I meant is that it's much more straightforward to have

    type_t v = cpu_reg_get_value(REG_ARCH_REGID);

rather than

    type_t v;
    cpu_reg_get_value(REG_ARCH_REGID, &v);

The problem is that different registers can have different width. Even if you
decide for uint64_t (which seems reasonable), you can still have wider registers
(e.g., 128bit FP regs). But maybe those "uncommon" wider registers can be
handled separately.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth