[Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Vladimir Prus]

This patch makes qemu recognize (and ignore), three instructions from SH4A.
First if prefetch (prefi). QEMU does not model cache, and the instruction is
documented to never cause TLB miss. Second is icbi, which invalidates cache
block, and again, no cache is simulated. Last is synco, which is memory barrier,
but qemu does not reorder memory operations anyway, I think.

- Volodya

	* target-sh4/translate.c (_decode_opc): Handle
	prefi, icbi, and synco.
---
 target-sh4/translate.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index 0eeb294..bf86329 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -1552,6 +1552,12 @@ void _decode_opc(DisasContext * ctx)
 	return;
     case 0x0083:		/* pref @Rn */
 	return;
+    case 0x00d3:		/* prefi @Rn */
+	return;
+    case 0x00e3:		/* icbi @Rn */
+	return;
+    case 0x00ab:		/* synco */
+	return;
     case 0x4024:		/* rotcl Rn */
 	{
 	    TCGv tmp = tcg_temp_new(TCG_TYPE_I32);
-- 
1.5.3.5

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Brook]

On Friday 17 October 2008, Vladimir Prus wrote:
> This patch makes qemu recognize (and ignore), three instructions from SH4A.

Shouldn't these generate an illegal instruction exception on SH4 cpus?

Paul

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Mundt]

On Fri, Oct 17, 2008 at 08:56:52PM +0100, Paul Brook wrote:
> On Friday 17 October 2008, Vladimir Prus wrote:
> > This patch makes qemu recognize (and ignore), three instructions from SH4A.
> 
> Shouldn't these generate an illegal instruction exception on SH4 cpus?
> 
Note that all of these are SH-5 instructions, which gradually found their
way in to the SH-4A ISA. There are also other SH-5 instructions that have
found their way in to the SH-2A ISA which are not as of yet reflected in
SH-4A. The dependency tracking gets to be pretty ugly, as is evident in
binutils. In order to get this right, it would be necessary to throw ISA
versioning in to the CPU definition and test to figure out what to
support, as per the table in binutils. While this would be a good idea in
general, it is something that should be done as a larger follow-up to
these patches.

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Brook]

On Monday 20 October 2008, Paul Mundt wrote:
> On Fri, Oct 17, 2008 at 08:56:52PM +0100, Paul Brook wrote:
> > On Friday 17 October 2008, Vladimir Prus wrote:
> > > This patch makes qemu recognize (and ignore), three instructions from
> > > SH4A.
> >
> > Shouldn't these generate an illegal instruction exception on SH4 cpus?
>
> Note that all of these are SH-5 instructions, which gradually found their
> way in to the SH-4A ISA. There are also other SH-5 instructions that have
> found their way in to the SH-2A ISA which are not as of yet reflected in
> SH-4A. The dependency tracking gets to be pretty ugly, as is evident in
> binutils. In order to get this right, it would be necessary to throw ISA
> versioning in to the CPU definition and test to figure out what to
> support, as per the table in binutils. While this would be a good idea in
> general, it is something that should be done as a larger follow-up to
> these patches.

I disagree. This is something that should be done right from the start. Trying 
to fix it up later is a real pain.  Doing fine grained features isn't that 
hard. MIPS, sparc, arm, ppc, m68k and sparc already do this. IIRC binutils is 
only complicated because it tried to create a strict hieracy of features, 
rather than using feature bits.

Paul

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Mundt]

On Mon, Oct 20, 2008 at 11:54:20AM +0100, Paul Brook wrote:
> On Monday 20 October 2008, Paul Mundt wrote:
> > On Fri, Oct 17, 2008 at 08:56:52PM +0100, Paul Brook wrote:
> > > On Friday 17 October 2008, Vladimir Prus wrote:
> > > > This patch makes qemu recognize (and ignore), three instructions from
> > > > SH4A.
> > >
> > > Shouldn't these generate an illegal instruction exception on SH4 cpus?
> >
> > Note that all of these are SH-5 instructions, which gradually found their
> > way in to the SH-4A ISA. There are also other SH-5 instructions that have
> > found their way in to the SH-2A ISA which are not as of yet reflected in
> > SH-4A. The dependency tracking gets to be pretty ugly, as is evident in
> > binutils. In order to get this right, it would be necessary to throw ISA
> > versioning in to the CPU definition and test to figure out what to
> > support, as per the table in binutils. While this would be a good idea in
> > general, it is something that should be done as a larger follow-up to
> > these patches.
> 
> I disagree. This is something that should be done right from the start. Trying 
> to fix it up later is a real pain.  Doing fine grained features isn't that 
> hard. MIPS, sparc, arm, ppc, m68k and sparc already do this. IIRC binutils is 
> only complicated because it tried to create a strict hieracy of features, 
> rather than using feature bits.
> 
When we do it does not matter, but it is completely unrelated from this
patch, in that there are already plenty of instructions that are specific
to a certain CPU family that we don't perform an illegal instruction
exception for. Trying to force the prefi/icbi/synco cases to rework all
of the existing instructions that aren't universal doesn't make a lot of
sense, as it is a clear incremental change of existing behaviour, rather
than a situation caused purely by the addition of these new instructions.

At a quick glance:

	movca.l - SH-4 and up
	movua.l - SH-4A and up
	ocbi/ocbp/ocbwb - SH-4 and up
	pref - SH-2A or SH-4 and up, excluding other legacy parts.
	shad/shld - SH-2A or SH-3 and up.

etc, etc.

The fact that the qemu target directory is 'target-sh4' instead of
'target-sh' already makes it clear that the original patch was only
concerned with SH-4, and so therefore did not have any ISA versioning or
feature differentiation in mind. Even things like the register layout
will be different for the other CPUs.

Given that, perhaps the best solution is something like:

	- Add synco/icbi/prefi
	- Add ISA versioning based on the binutils inheritance list
	- Rename to target-sh

These are all going to be prerequisites for fitting in other more exotic
CPUs, like the SH-2A and SH-5 also.

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Brook]

> > I disagree. This is something that should be done right from the start.
> > Trying to fix it up later is a real pain.  Doing fine grained features
> > isn't that hard. MIPS, sparc, arm, ppc, m68k and sparc already do this.
> > IIRC binutils is only complicated because it tried to create a strict
> > hieracy of features, rather than using feature bits.
>
> When we do it does not matter, but it is completely unrelated from this
> patch, in that there are already plenty of instructions that are specific
> to a certain CPU family that we don't perform an illegal instruction
> exception for. Trying to force the prefi/icbi/synco cases to rework all 
> of the existing instructions that aren't universal doesn't make a lot of
> sense, as it is a clear incremental change of existing behaviour, rather
> than a situation caused purely by the addition of these new instructions.

The only cpu we currently claim to support is SH4. When adding support for 
other cores these should be properly conditionalized.  Unconditionally 
implementing additional instructions is a regression. I don't consider "we'll 
fix this at some undefined point in the future" to be a good enough answer. 

Paul

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Vladimir Prus]

On Monday 20 October 2008 20:27:00 Paul Brook wrote:
> > > I disagree. This is something that should be done right from the start.
> > > Trying to fix it up later is a real pain.  Doing fine grained features
> > > isn't that hard. MIPS, sparc, arm, ppc, m68k and sparc already do this.
> > > IIRC binutils is only complicated because it tried to create a strict
> > > hieracy of features, rather than using feature bits.
> >
> > When we do it does not matter, but it is completely unrelated from this
> > patch, in that there are already plenty of instructions that are specific
> > to a certain CPU family that we don't perform an illegal instruction
> > exception for. Trying to force the prefi/icbi/synco cases to rework all 
> > of the existing instructions that aren't universal doesn't make a lot of
> > sense, as it is a clear incremental change of existing behaviour, rather
> > than a situation caused purely by the addition of these new instructions.
> 
> The only cpu we currently claim to support is SH4. When adding support for 
> other cores these should be properly conditionalized.  Unconditionally 
> implementing additional instructions is a regression. I don't consider "we'll 
> fix this at some undefined point in the future" to be a good enough answer. 

Can you outline what changes should I make to implement proper conditionalization?

Thanks,
Volodya

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Brook]

> > The only cpu we currently claim to support is SH4. When adding support
> > for other cores these should be properly conditionalized. 
> > Unconditionally implementing additional instructions is a regression. I
> > don't consider "we'll fix this at some undefined point in the future" to
> > be a good enough answer.
>
> Can you outline what changes should I make to implement proper
> conditionalization?

You probably want something like ARM arm_feature, MIPS check_insn or
SPARC CHECK_*_FEATURE.

Paul

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Mundt]

On Mon, Oct 20, 2008 at 08:31:19PM +0400, Vladimir Prus wrote:
> On Monday 20 October 2008 20:27:00 Paul Brook wrote:
> > > > I disagree. This is something that should be done right from the start.
> > > > Trying to fix it up later is a real pain.  Doing fine grained features
> > > > isn't that hard. MIPS, sparc, arm, ppc, m68k and sparc already do this.
> > > > IIRC binutils is only complicated because it tried to create a strict
> > > > hieracy of features, rather than using feature bits.
> > >
> > > When we do it does not matter, but it is completely unrelated from this
> > > patch, in that there are already plenty of instructions that are specific
> > > to a certain CPU family that we don't perform an illegal instruction
> > > exception for. Trying to force the prefi/icbi/synco cases to rework all 
> > > of the existing instructions that aren't universal doesn't make a lot of
> > > sense, as it is a clear incremental change of existing behaviour, rather
> > > than a situation caused purely by the addition of these new instructions.
> > 
> > The only cpu we currently claim to support is SH4. When adding support for 
> > other cores these should be properly conditionalized.  Unconditionally 
> > implementing additional instructions is a regression. I don't consider "we'll 
> > fix this at some undefined point in the future" to be a good enough answer. 
> 
> Can you outline what changes should I make to implement proper conditionalization?
> 
The issue comes with how the ISA is versioned. It changes across CPU
families, with different families borrowing off of each other but not
being strict supersets of previous generations. It is a complicated mess
in binutils, and if someone has a better idea of how to do that cleanly
in qemu, then that is certainly something that should be experimented
with.

Regardless, as Paul seemed to completely ignore, we already have non-SH4
instructions in the tree already, so the illegal instruction behaviour is
already bogus when we hit SH-4A instructions on SH-4. There are also
extra registers that are not handled under the SH-4 CPU definition
in-tree.

Going be a set of features is possible to some extent, but there are
fundamental architectural differences in SH-4A compared to SH-4, so the
feature thing is rather misleading. The existing target-sh4 thing really
ought to be renamed to target-sh and generalized. Adding in more SH-4A
logic is going to create more deviation, and it is already going to be
helpful to start isolating the SH-4 and up parts so that parts that
diverged from before that point can be integrated. Note that even though
SH-4 and up excluded SH-2 and SH-3 at the time, SH-2A borrows quite
heavily from SH-4 and SH-4A also, in which case the feature tests would
be helpful.

In addition to the CPU subtype, it is necessary to track the CPU family
that it falls under also, as this is the only way to know properly how
the instructions have been inherited. The feature bits will have to be
fairly fine grained, as there are CPUs that implement the same features
in different ways. SH-5 for example supports synco/prefi/icbi in two
different ways, depending on which instruction set mode it is in.

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Brook]

> The issue comes with how the ISA is versioned. It changes across CPU
> families, with different families borrowing off of each other but not
> being strict supersets of previous generations. It is a complicated mess
> in binutils, and if someone has a better idea of how to do that cleanly
> in qemu, then that is certainly something that should be experimented
> with.

> In addition to the CPU subtype, it is necessary to track the CPU family
> that it falls under also, as this is the only way to know properly how
> the instructions have been inherited.

Binutils ends up a mess because it tries to create a hierarchal tree of cpus, 
IIRC because of the way it represents this in the object file. Qemu is quite 
happy to pick and choose features. There's no need to complicate things by 
pretending there's some sort of logic behind the ISA revisions.

> Going be a set of features is possible to some extent, but there are
> fundamental architectural differences in SH-4A compared to SH-4, so the
> feature thing is rather misleading.

ARM and PPC already have support for multiple system models.
I'd guess most of these differences aren't actually part of the CPU core, 
they're part of the supporting devices/peripherals.

Paul

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Laurent Desnogues]

On Mon, Oct 20, 2008 at 12:54 PM, Paul Brook <paul@codesourcery.com> wrote:
> Doing fine grained features isn't that
> hard. MIPS, sparc, arm, ppc, m68k and sparc already do this.

So let's say someone finds some unprotected use of some ARM
instructions, should that be reported and is there any hope the
patch will ever be considered by the maintainer of the ARM target?
(Examples are ldrex/strex and ubfx/sbfx.)

Also what about undetected undefined instructions which I asked
about last May?

http://lists.gnu.org/archive/html/qemu-devel/2008-05/msg00844.html

Given the lack of answer at that time, I thought it didn't matter.  I now
see I was wrong.  It's a pity communication is almost impossible.


Laurent

Re: [Qemu-devel] [PATCH] SH: Add prefi, icbi, synco

[Paul Brook]

On Monday 20 October 2008, Laurent Desnogues wrote:
> On Mon, Oct 20, 2008 at 12:54 PM, Paul Brook <paul@codesourcery.com> wrote:
> > Doing fine grained features isn't that
> > hard. MIPS, sparc, arm, ppc, m68k and sparc already do this.
>
> So let's say someone finds some unprotected use of some ARM
> instructions, should that be reported and is there any hope the
> patch will ever be considered by the maintainer of the ARM target?
> (Examples are ldrex/strex and ubfx/sbfx.)

Yes.

> Also what about undetected undefined instructions which I asked
> about last May?
>
> http://lists.gnu.org/archive/html/qemu-devel/2008-05/msg00844.html
>
> Given the lack of answer at that time, I thought it didn't matter.  I now
> see I was wrong.  It's a pity communication is almost impossible.

Rejecting incorrect patches is always easier than verifying that patches are 
correct.

Paul