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

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

[Max Filippov]

Hello.

I'm developing support for new qemu target architecture: xtensa [1],
primarily because AFAIK there's no free/open simulator for this
architecture.

Essential ISA parts (like core opcodes, special registers, windowed
registers, exceptions and interrupts) are implemented, other (like
TLB, MMU, caches, coprocessors, rare opcodes) are not, although I'm
planning to implement them if/when needed.

I'm wondering if this target could be eligible for inclusion into qemu mainline.
If it is, could anyone please review the code [2]?

There are several known issues which I'm planning to address:
- mixed coding style;
- no copyrights/license (it is BSD);
- no direct TB linking;
- dummy cpu_halted/cpu_has_work.

If you see more, please report, especially if you know how to fix them (:

[1] http://en.wikipedia.org/wiki/Tensilica
[2] http://jcmvbkbc.spb.ru/git/?p=dumb/qemu-xtensa.git;a=shortlog;h=refs/heads/xtensa

Thanks.
-- Max

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

[Blue Swirl]

On Sat, Apr 30, 2011 at 12:08 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
> Hello.
>
> I'm developing support for new qemu target architecture: xtensa [1],
> primarily because AFAIK there's no free/open simulator for this
> architecture.
>
> Essential ISA parts (like core opcodes, special registers, windowed
> registers, exceptions and interrupts) are implemented, other (like
> TLB, MMU, caches, coprocessors, rare opcodes) are not, although I'm
> planning to implement them if/when needed.

Nice work. What is the status, can the emulator boot Linux for example?

> I'm wondering if this target could be eligible for inclusion into qemu mainline.
> If it is, could anyone please review the code [2]?

Please send the patch set for easier reviewing to the list, use
scripts/checkpatch.pl to avoid some predictable issues.

> There are several known issues which I'm planning to address:
> - mixed coding style;
> - no copyrights/license (it is BSD);
> - no direct TB linking;
> - dummy cpu_halted/cpu_has_work.
>
> If you see more, please report, especially if you know how to fix them (:

- rebase to HEAD
- fix/rearrange patches should be merged to the original commits so
that they are bug free
- commit descriptions are short/nonexistent
- options field does not necessarily belong to CPUState, see for
example Sparc sparc_def_t how the different models are handled
- env->options or env->singlestep shouldn't be used in disas_insn (it
shouldn't take env parameter at all) but the fields should be copied
to DisasContext and used from there
- macros should be replaced by inline functions (or enums) when possible
- pointers to CPU and chipset docs should be added
- commenting out code is a no-no
- there is no disassembler for target, can you check if binutils
before GPLv3 switch contains one? It should be easy to add.
- if possible, simcall should become a linux-user target instead

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

[Max Filippov]

>> Essential ISA parts (like core opcodes, special registers, windowed
>> registers, exceptions and interrupts) are implemented, other (like
>> TLB, MMU, caches, coprocessors, rare opcodes) are not, although I'm
>> planning to implement them if/when needed.
>
> Nice work. What is the status, can the emulator boot Linux for example?

Rather complex applications based on ThreadX [1] realtime OS
successfully run on it, but Linux requires at least MMU, which I
haven't got to yet. But that's one of my goals.

>> I'm wondering if this target could be eligible for inclusion into qemu mainline.
>> If it is, could anyone please review the code [2]?
>
> Please send the patch set for easier reviewing to the list, use
> scripts/checkpatch.pl to avoid some predictable issues.

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?

[...]

> - options field does not necessarily belong to CPUState, see for
> example Sparc sparc_def_t how the different models are handled

My intent was to be able to support AMP configuration with cores that
has different options.

> - env->options or env->singlestep shouldn't be used in disas_insn (it
> shouldn't take env parameter at all) but the fields should be copied
> to DisasContext and used from there

What is the reason for that? DisasContext itself has a pointer to env.
Can it be used instead?

> - macros should be replaced by inline functions (or enums) when possible
> - pointers to CPU and chipset docs should be added

Is target-xtensa/README a good place for it?

> - commenting out code is a no-no
> - there is no disassembler for target, can you check if binutils
> before GPLv3 switch contains one? It should be easy to add.

You mean that there's no support for -d in_asm?

> - if possible, simcall should become a linux-user target instead

simcall is used in freestanding libc provided by Tensilica, I guess
primarily to ease OS-less development. I can make it an additional
option, but it seems impractical to remove it completely.

[1] http://en.wikipedia.org/wiki/ThreadX

Thanks.
-- Max

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

[Blue Swirl]

On Sat, Apr 30, 2011 at 11:24 AM, Max Filippov <jcmvbkbc@gmail.com> wrote:
>>> Essential ISA parts (like core opcodes, special registers, windowed
>>> registers, exceptions and interrupts) are implemented, other (like
>>> TLB, MMU, caches, coprocessors, rare opcodes) are not, although I'm
>>> planning to implement them if/when needed.
>>
>> Nice work. What is the status, can the emulator boot Linux for example?
>
> Rather complex applications based on ThreadX [1] realtime OS
> successfully run on it, but Linux requires at least MMU, which I
> haven't got to yet. But that's one of my goals.
>
>>> I'm wondering if this target could be eligible for inclusion into qemu mainline.
>>> If it is, could anyone please review the code [2]?
>>
>> Please send the patch set for easier reviewing to the list, use
>> scripts/checkpatch.pl to avoid some predictable issues.
>
> 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.

>> - options field does not necessarily belong to CPUState, see for
>> example Sparc sparc_def_t how the different models are handled
>
> My intent was to be able to support AMP configuration with cores that
> has different options.
>
>> - env->options or env->singlestep shouldn't be used in disas_insn (it
>> shouldn't take env parameter at all) but the fields should be copied
>> to DisasContext and used from there
>
> 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.

>> - macros should be replaced by inline functions (or enums) when possible
>> - pointers to CPU and chipset docs should be added
>
> Is target-xtensa/README a good place for it?

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

>> - commenting out code is a no-no
>> - there is no disassembler for target, can you check if binutils
>> before GPLv3 switch contains one? It should be easy to add.
>
> You mean that there's no support for -d in_asm?

Yes, that would be nice but it can be added later.

>> - if possible, simcall should become a linux-user target instead
>
> simcall is used in freestanding libc provided by Tensilica, I guess
> primarily to ease OS-less development. I can make it an additional
> option, but it seems impractical to remove it completely.

Does that still emulate devices? In that case it would be like
semihosting mode in ARM, so it would be OK to stay where you put it.

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

[Peter Maydell]

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

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

[Max Filippov]

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

That I understand. Ok, it seems that CPUState references may be
avoided quite easily.

>> 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

Updated that.

Thanks for the hints, guys. Will patchbomb you soon.

Thanks.
-- Max

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

[Max Filippov]

>>> - if possible, simcall should become a linux-user target instead
>>
>> simcall is used in freestanding libc provided by Tensilica, I guess
>> primarily to ease OS-less development. I can make it an additional
>> option, but it seems impractical to remove it completely.
>
> Does that still emulate devices? In that case it would be like
> semihosting mode in ARM, so it would be OK to stay where you put it.

It only provides very limited support for file I/O, argc/argv passing
and profiling/gdb related magic which I'm not going to implement.

Thanks.
-- Max