On 30 April 2011 10:06, Blue Swirl <blauwir...@gmail.com> wrote: > On Sat, Apr 30, 2011 at 11:24 AM, Max Filippov <jcmvb...@gmail.com> 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
