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

[Andreas Färber]

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.

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.

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

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg