Re: [Qemu-devel] xtensa: new target architecture for qemu

[Peter Maydell]

On 30 April 2011 10:06, Blue Swirl <address@hidden> wrote:
> On Sat, Apr 30, 2011 at 11:24 AM, Max Filippov <address@hidden> wrote:
>> Is it worth keeping patches as small as they are now (as they only
>> reflect development chronology), or maybe it's better to consolidate
>> them on some other basis?
>
> Small patches are nicer for review.

A couple of other patch series structuring hints:
 * avoid having a patch which implements X and then a later patch
   which fixes a bug in X
 * make sure everything compiles at all points in the series, not
   just at the end; usually "enable target in configure" comes last

>> What is the reason for that? DisasContext itself has a pointer to env.
>> Can it be used instead?
>
> DisasContext shouldn't have that pointer. Some of the decisions during
> translation are made based on CPU model/features which don't change
> during execution. Other decisions should be made based on the current
> CPU state, however usually here we should not rely on env but on
> tb_flags. Not using env during disas_insn tries to enforce that.
> Please take a look at other targets how this should be handled.

Some targets are better choices to copy than others. target-arm
passes CPUState around way too much still (it only relies on things
it is OK to, but it would be nicer not to have CPUState quite so
conveniently available.) I think the ideal is that none of the
functions implementing decode logic (in your case disas_xtensa_insn())
are directly passed a CPUState pointer.

The reason for all this is that the CPUState you get during
translation may not be the state of the CPU at the start
of the code you are translating, so if you rely on bits of
it that can change you'll run into problems.

> I'd put CPU doc pointers to translate.c and chipset pointers to
> corresponding device file.

We also have a wiki page for CPU docs:
http://wiki.qemu.org/Documentation/ISAManuals

-- PMM