[Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Paul Brook]

CVSROOT:	/sources/qemu
Module name:	qemu
Changes by:	Paul Brook <pbrook>	07/11/23 17:33:13

Modified files:
	hw             : ppc_oldworld.c 
	target-ppc     : cpu.h helper.c op_helper.c 

Log message:
	Fix ppc32 register dumps on 64-bit hosts.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_oldworld.c?cvsroot=qemu&r1=1.9&r2=1.10
http://cvs.savannah.gnu.org/viewcvs/qemu/target-ppc/cpu.h?cvsroot=qemu&r1=1.105&r2=1.106
http://cvs.savannah.gnu.org/viewcvs/qemu/target-ppc/helper.c?cvsroot=qemu&r1=1.95&r2=1.96
http://cvs.savannah.gnu.org/viewcvs/qemu/target-ppc/op_helper.c?cvsroot=qemu&r1=1.70&r2=1.71

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Jocelyn Mayer]

On Fri, 2007-11-23 at 17:33 +0000, Paul Brook wrote:
> CVSROOT:	/sources/qemu
> Module name:	qemu
> Changes by:	Paul Brook <pbrook>	07/11/23 17:33:13
> 
> Modified files:
> 	hw             : ppc_oldworld.c 
> 	target-ppc     : cpu.h helper.c op_helper.c 
> 
> Log message:
> 	Fix ppc32 register dumps on 64-bit hosts.

HOW MANY WILL WE HAVE TO ASK PEOPLE TO RESPECT OTHER WORK ???
Furthermore this patch was made in a brainless way, it will be reverted
asap.
If you think there is a bug in someone else code, submit it a patch, if
it's cleaver and addresses a real bug (which is not the case here) it
will be accepted and merged.
DOT
DO NOT COMMIT AND BREAK OTHER PEOPLE'S WORK ? Is there another way to
say it in order to be understood by everyone here ?

-- 
Jocelyn Mayer <l_indien@magic.fr>

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Paul Brook]

> Furthermore this patch was made in a brainless way, it will be reverted
> asap.
> If you think there is a bug in someone else code, submit it a patch, if
> it's cleaver and addresses a real bug (which is not the case here) it
> will be accepted and merged.

The old code before the patch is obviously broken. It's mixing 64-bit 
(ppc_gpr_t) and 32-bit (target_ulong) values.

As implied by the comments aboce the definition of ppc_gpt_t, and now 
explicitly in the above the definition of REGX, printing a ppc_gpr_t is 
obviously not meaningful.

I don't claim that my patch is perfect, the code is still a bit of a mess. 
However, unlike the original code, it is at least self-consistent, and won't 
crash 64-bit hosts (The fact that it usually prints garbage rather than 
crashing is an accident of the x64-64 ABI).

Paul

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Jocelyn Mayer]

On Fri, 2007-11-23 at 18:22 +0000, Paul Brook wrote:
> > Furthermore this patch was made in a brainless way, it will be reverted
> > asap.
> > If you think there is a bug in someone else code, submit it a patch, if
> > it's cleaver and addresses a real bug (which is not the case here) it
> > will be accepted and merged.
> 
> The old code before the patch is obviously broken. It's mixing 64-bit 
> (ppc_gpr_t) and 32-bit (target_ulong) values.

It seems you do not understand that what was done was correct. It's not
mixing two different types. GPR are of ppc_gpr_t type and should be
displayed this way. The only case which is incorrect is not addressed by
your patch. But your patch breaks the general case which was OK.

> 
> As implied by the comments aboce the definition of ppc_gpt_t, and now 
> explicitly in the above the definition of REGX, printing a ppc_gpr_t is 
> obviously not meaningful.
> 
> I don't claim that my patch is perfect, the code is still a bit of a mess. 
> However, unlike the original code, it is at least self-consistent, and won't 
> crash 64-bit hosts (The fact that it usually prints garbage rather than 
> crashing is an accident of the x64-64 ABI).

It's not garbage. On 64 bits hosts, the 64 bits GPR dump is correct. GPR
_are 64 bits_ when compiling the ppcemb target and should be displayed
as 64 bits value. It's not correct on 32 bits targets, because the
highest 32 bits of the GPR should be printed and they are not. Here's
the real bug. Your patch break the first case, which was OK, and does
not fix any actual bug.

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Jocelyn Mayer]

On Fri, 2007-11-23 at 19:42 +0100, Jocelyn Mayer wrote:
> On Fri, 2007-11-23 at 18:22 +0000, Paul Brook wrote:
> > > Furthermore this patch was made in a brainless way, it will be reverted
> > > asap.
> > > If you think there is a bug in someone else code, submit it a patch, if
> > > it's cleaver and addresses a real bug (which is not the case here) it
> > > will be accepted and merged.
> > 
> > The old code before the patch is obviously broken. It's mixing 64-bit 
> > (ppc_gpr_t) and 32-bit (target_ulong) values.
> 
> It seems you do not understand that what was done was correct. It's not
> mixing two different types. GPR are of ppc_gpr_t type and should be
> displayed this way. The only case which is incorrect is not addressed by
> your patch. But your patch breaks the general case which was OK.
> 
> > 
> > As implied by the comments aboce the definition of ppc_gpt_t, and now 
> > explicitly in the above the definition of REGX, printing a ppc_gpr_t is 
> > obviously not meaningful.
> > 
> > I don't claim that my patch is perfect, the code is still a bit of a mess. 
> > However, unlike the original code, it is at least self-consistent, and won't 
> > crash 64-bit hosts (The fact that it usually prints garbage rather than 
> > crashing is an accident of the x64-64 ABI).
> 
> It's not garbage. On 64 bits hosts, the 64 bits GPR dump is correct. GPR
> _are 64 bits_ when compiling the ppcemb target and should be displayed
> as 64 bits value. It's not correct on 32 bits targets

Sorry, I meant 32 bits _hosts_ here...

> , because the
> highest 32 bits of the GPR should be printed and they are not. Here's
> the real bug. Your patch break the first case, which was OK, and does
> not fix any actual bug.

-- 
Jocelyn Mayer <l_indien@magic.fr>

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Paul Brook]

> > The old code before the patch is obviously broken. It's mixing 64-bit
> > (ppc_gpr_t) and 32-bit (target_ulong) values.
>
> It seems you do not understand that what was done was correct. It's not
> mixing two different types. GPR are of ppc_gpr_t type and should be
> displayed this way. 
> It's not garbage. On 64 bits hosts, the 64 bits GPR dump is correct. GPR
> _are 64 bits_ when compiling the ppcemb target and should be displayed
> as 64 bits value. 

Really? Where exactly is the code that uses a 64-bit ppc_gpr_t ?
I don't see any evidence that the high bits of the value is ever used.

I see the SPE stuff that uses T0_64 et al, however this still uses stores the 
value in the low 32 bits of the {gpr,gprth} pair.

Paul

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Jocelyn Mayer]

On Fri, 2007-11-23 at 19:10 +0000, Paul Brook wrote:
> > > The old code before the patch is obviously broken. It's mixing 64-bit
> > > (ppc_gpr_t) and 32-bit (target_ulong) values.
> >
> > It seems you do not understand that what was done was correct. It's not
> > mixing two different types. GPR are of ppc_gpr_t type and should be
> > displayed this way. 
> > It's not garbage. On 64 bits hosts, the 64 bits GPR dump is correct. GPR
> > _are 64 bits_ when compiling the ppcemb target and should be displayed
> > as 64 bits value. 
> 
> Really? Where exactly is the code that uses a 64-bit ppc_gpr_t ?
> I don't see any evidence that the high bits of the value is ever used.
> 
> I see the SPE stuff that uses T0_64 et al, however this still uses stores the 
> value in the low 32 bits of the {gpr,gprth} pair.

Whatever, your patch is more buggy that the code that previously exists
then it will be reverted. The real issues will be addressed, in a
cleaver way (I hope) by someone who really understands what the code is
supposed to do.
But the more important issue is: DO NOT COMMIT IN THE CODE YOU'RE NOT
SUPPOSED TO CHANGE WITHOUT REQUESTING AN AGREEMENT. This is not
negociable AT ALL.

-- 
Jocelyn Mayer <l_indien@magic.fr>

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Jocelyn Mayer]

On Fri, 2007-11-23 at 19:10 +0000, Paul Brook wrote:
> > > The old code before the patch is obviously broken. It's mixing 64-bit
> > > (ppc_gpr_t) and 32-bit (target_ulong) values.
> >
> > It seems you do not understand that what was done was correct. It's not
> > mixing two different types. GPR are of ppc_gpr_t type and should be
> > displayed this way. 
> > It's not garbage. On 64 bits hosts, the 64 bits GPR dump is correct. GPR
> > _are 64 bits_ when compiling the ppcemb target and should be displayed
> > as 64 bits value. 
> 
> Really? Where exactly is the code that uses a 64-bit ppc_gpr_t ?
> I don't see any evidence that the high bits of the value is ever used.

Then I took a closer look to the code, to ensure I was not wrong.
The PowerPC 32 on 64 bits hosts is implemented the same way that the
specification says a PowerPC in 32 bits mode should be. Then higher bits
are not garbage. They are what the PowerPC specification say they should
be (apart if they are some bugs in the implementation). The fact that
they are or not used by computations is another point. The fact is the
registers values are correct.
And the fact is that printing a uint64_t on any 64 bits host (x86_64 or
any other) using PRIx64 is exactly what is to be done, according to ISO
C. Then, pretending that it would crash on any host is completelly
pointless.

> I see the SPE stuff that uses T0_64 et al, however this still uses
> stores the value in the low 32 bits of the {gpr,gprth} pair.

SPE dump is the case that does not work properly. Your patch does not solve anything here, just breaks the main stream case.

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Paul Brook]

> Then I took a closer look to the code, to ensure I was not wrong.
> The PowerPC 32 on 64 bits hosts is implemented the same way that the
> specification says a PowerPC in 32 bits mode should be. Then higher bits
> are not garbage. They are what the PowerPC specification say they should
> be (apart if they are some bugs in the implementation). The fact that
> they are or not used by computations is another point. The fact is the
> registers values are correct.

AFAICS the high bits are never used by anything.

I think what you mean is that they work the way that ppc64 is defined, to 
remain compatible with ppc32.  IMHO this is entirely irrelevant as we're 
emulating a ppc32. You could replace the high bits with garbage and nothing 
would ever be able to tell the difference.

> And the fact is that printing a uint64_t on any 64 bits host (x86_64 or
> any other) using PRIx64 is exactly what is to be done, according to ISO
> C. Then, pretending that it would crash on any host is completelly
> pointless.

We weren't printing a 64-bit value. We were passing a 32-bit target_ulong with 
a PRIx64 format. Some concrete examples:
translate.c:6052:
    cpu_fprintf(f, "MSR " REGX FILL " HID0 " REGX FILL "  HF " REGX FILL
                env->msr, env->hflags, env->spr[SPR_HID0],

All these values are 32-bit tagret_ulong. Using a 64-bit format specifierfor 
ppc32 targets is just nonsense.

And at line 6069 we even have an explicit cast to a 32-bit type:

        cpu_fprintf(f, " " REGX, (target_ulong)env->gpr[i]);

> > I see the SPE stuff that uses T0_64 et al, however this still uses
> > stores the value in the low 32 bits of the {gpr,gprth} pair.
>
> SPE dump is the case that does not work properly. Your patch does not solve 
> anything here, just breaks the main stream case.

I agree that SPE register dumping does not work, however I also assert that it 
was never even close to working, and if REGX is supposed to be the solution 
then most of the uses of REGX are incorrect.

Please give a concrete example of something that worked before and does not 
now.

Paul

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[J. Mayer]

On Fri, 2007-11-23 at 21:36 +0000, Paul Brook wrote:
> > Then I took a closer look to the code, to ensure I was not wrong.
> > The PowerPC 32 on 64 bits hosts is implemented the same way that the
> > specification says a PowerPC in 32 bits mode should be. Then higher bits
> > are not garbage. They are what the PowerPC specification say they should
> > be (apart if they are some bugs in the implementation). The fact that
> > they are or not used by computations is another point. The fact is the
> > registers values are correct.
> 
> AFAICS the high bits are never used by anything.

They are used the way the specification tells it should be, ie "when
running in 32 bits mode, all computation are done in 64 bits".

> I think what you mean is that they work the way that ppc64 is defined, to 
> remain compatible with ppc32.  IMHO this is entirely irrelevant as we're 
> emulating a ppc32. You could replace the high bits with garbage and nothing 
> would ever be able to tell the difference.

PowerPC is a 64 bits architecture. PowerPC 32 on 32 bits host is
optimized not to compute the 32 highest bits, the same way it's allowed
to cut down the GPR when implementing a CPU that would not support the
64 bits mode (but this is a tolerance, this is not the architecture is
defined).
PowerPC 32 on 64 bits host is implemented as PowerPC 32 bits mode. This
is a choice that may be discussed but this is the way it's done. Then,
the 32 highest bits are to be computed properly, even if they do not
actually participate in any result seen from the 32 bits application.
Then, print the 64 bits GPR is relevant.
Running the PowerPC 32 emulation on a 64 bits hosts is strictly
equivalent to running the PowerPC 64 emulation in 32 bits mode, as the
architecture specifies it should be. One could then argue the PowerPC 32
targets are not relevant when running on a 64 bits host, which is true.

> > And the fact is that printing a uint64_t on any 64 bits host (x86_64 or
> > any other) using PRIx64 is exactly what is to be done, according to ISO
> > C. Then, pretending that it would crash on any host is completelly
> > pointless.
> 
> We weren't printing a 64-bit value. We were passing a 32-bit target_ulong with 
> a PRIx64 format. Some concrete examples:
> translate.c:6052:
>     cpu_fprintf(f, "MSR " REGX FILL " HID0 " REGX FILL "  HF " REGX FILL
>                 env->msr, env->hflags, env->spr[SPR_HID0],
> 
> All these values are 32-bit tagret_ulong. Using a 64-bit format specifierfor 
> ppc32 targets is just nonsense.

OK. Those are real bugs to be fixed. I'll take a look.... But I'll try
not to break the GPR dump. In fact, GPR should always dumped as 64 bits,
even when runnig on 32 bits hosts. This would be more consistent with
the specification.


> And at line 6069 we even have an explicit cast to a 32-bit type:
> 
>         cpu_fprintf(f, " " REGX, (target_ulong)env->gpr[i]);

OK, this is false. I'll remove this buggy cast.

> > > I see the SPE stuff that uses T0_64 et al, however this still uses
> > > stores the value in the low 32 bits of the {gpr,gprth} pair.
> >
> > SPE dump is the case that does not work properly. Your patch does not solve 
> > anything here, just breaks the main stream case.
> 
> I agree that SPE register dumping does not work, however I also assert that it 
> was never even close to working, and if REGX is supposed to be the solution 
> then most of the uses of REGX are incorrect.
> 
> Please give a concrete example of something that worked before and does not 
> now.

The fact that you cannot dump the full GPR is a bug. When GPR is stored
as 64 bits, they are to be dumped as 64 bits values. If you see bugs in
my code, please tell me I'll try to fix them (and I'll thank you for
this). But not doing weird things that are more buggy than the original
code !
But once again, the biggest problem is that you break my code without
any concertation and without even trying to understand why the code has
been written this way. So, tell me you think there's a bug, or propose a
patch. If I think the patch is OK, I'll tell you. If not, I'll try to
address the bug the way I think ithas to be done.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

[Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Jocelyn Mayer]

CVSROOT:	/sources/qemu
Module name:	qemu
Changes by:	Jocelyn Mayer <j_mayer>	07/11/23 22:16:59

Modified files:
	hw             : ppc_oldworld.c 
	target-ppc     : cpu.h helper.c op_helper.c 

Log message:
	Revert foolish patch.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/qemu/hw/ppc_oldworld.c?cvsroot=qemu&r1=1.10&r2=1.11
http://cvs.savannah.gnu.org/viewcvs/qemu/target-ppc/cpu.h?cvsroot=qemu&r1=1.106&r2=1.107
http://cvs.savannah.gnu.org/viewcvs/qemu/target-ppc/helper.c?cvsroot=qemu&r1=1.96&r2=1.97
http://cvs.savannah.gnu.org/viewcvs/qemu/target-ppc/op_helper.c?cvsroot=qemu&r1=1.71&r2=1.72

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Paul Brook]

> > I think what you mean is that they work the way that ppc64 is defined, to
> > remain compatible with ppc32.  IMHO this is entirely irrelevant as we're
> > emulating a ppc32. You could replace the high bits with garbage and
> > nothing would ever be able to tell the difference.
>
> PowerPC is a 64 bits architecture. PowerPC 32 on 32 bits host is
> optimized not to compute the 32 highest bits, the same way it's allowed
> to cut down the GPR when implementing a CPU that would not support the
> 64 bits mode (but this is a tolerance, this is not the architecture is
> defined).

No. PowerPC is defined as a 64-bit archirecure. However there is a subset of 
this architecture (aka ppc32) that is a complete 32-bit architecture in its 
own right.  By your own admission, we can get away with not calculating the 
high 32 bit of the register. If follows that the high bits are completely 
meaningless. 

The qemu ppc32 emulation is implemented in such a way that on 64-bit hosts it 
looks a lot like a ppc64 implementation. However this need not, and should 
not be exposed to the user.

> OK. Those are real bugs to be fixed. I'll take a look.... But I'll try
> not to break the GPR dump. In fact, GPR should always dumped as 64 bits,
> even when runnig on 32 bits hosts. This would be more consistent with
> the specification.

I disagree. qemu is implementing ppc32. Showing more than 32 bits of register 
is completely bogus. Any differences between a 32-bit host and a 64-bit host 
are a qemu bug. If you display 64 bits, then those 64 bits had better be the 
same when run on 32-bit hosts.

Paul

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Thiemo Seufer]

Paul Brook wrote:
> > > I think what you mean is that they work the way that ppc64 is defined, to
> > > remain compatible with ppc32.  IMHO this is entirely irrelevant as we're
> > > emulating a ppc32. You could replace the high bits with garbage and
> > > nothing would ever be able to tell the difference.
> >
> > PowerPC is a 64 bits architecture. PowerPC 32 on 32 bits host is
> > optimized not to compute the 32 highest bits, the same way it's allowed
> > to cut down the GPR when implementing a CPU that would not support the
> > 64 bits mode (but this is a tolerance, this is not the architecture is
> > defined).
> 
> No. PowerPC is defined as a 64-bit archirecure. However there is a subset of 
> this architecture (aka ppc32) that is a complete 32-bit architecture in its 
> own right.  By your own admission, we can get away with not calculating the 
> high 32 bit of the register. If follows that the high bits are completely 
> meaningless. 

This btw. also means that the ppc32 emulation on 32-bit hosts is needlessly
inefficient if the high bits are carried around.

> The qemu ppc32 emulation is implemented in such a way that on 64-bit hosts it 
> looks a lot like a ppc64 implementation. However this need not, and should 
> not be exposed to the user.
> 
> > OK. Those are real bugs to be fixed. I'll take a look.... But I'll try
> > not to break the GPR dump. In fact, GPR should always dumped as 64 bits,
> > even when runnig on 32 bits hosts. This would be more consistent with
> > the specification.
> 
> I disagree. qemu is implementing ppc32. Showing more than 32 bits of register 
> is completely bogus. Any differences between a 32-bit host and a 64-bit host 
> are a qemu bug. If you display 64 bits, then those 64 bits had better be the 
> same when run on 32-bit hosts.

I strongly agree with Paul.


Thiemo

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[J. Mayer]

On Fri, 2007-11-23 at 22:23 +0000, Paul Brook wrote:
> > > I think what you mean is that they work the way that ppc64 is defined, to
> > > remain compatible with ppc32.  IMHO this is entirely irrelevant as we're
> > > emulating a ppc32. You could replace the high bits with garbage and
> > > nothing would ever be able to tell the difference.
> >
> > PowerPC is a 64 bits architecture. PowerPC 32 on 32 bits host is
> > optimized not to compute the 32 highest bits, the same way it's allowed
> > to cut down the GPR when implementing a CPU that would not support the
> > 64 bits mode (but this is a tolerance, this is not the architecture is
> > defined).
> 
> No. PowerPC is defined as a 64-bit archirecure. However there is a subset of 
> this architecture (aka ppc32) that is a complete 32-bit architecture in its 
> own right. 

It used to be defined this way... years ago. The latest specifications
say that there is one 64 bits architecture with 2 computational modes.
They also say that an embedded implementation can avoid implementating
some parts or the whole 64 bits computation mode. To complicate the
situation, it's also required that "standard" implementation do all
computations on 64 bit values but that embedded implementations that
implement SPE extension do never modify the highest 32 bits of GPR if
they do not implement the 64 bits computation mode (but this restriction
do not exists if the implementation implements the 2 computation modes),
which explains why I added the gprh registers to be able to handle all
cases without ifdefs.

>  By your own admission, we can get away with not calculating the 
> high 32 bit of the register. If follows that the high bits are completely 
> meaningless. 

Not completelly. There are even some way to do 64 bits computations when
running in 32 bits mode... Some may see this as an architecture hack,
but this gives the only way to switch from 32 bits to 64 bits mode (as
the sixty-four MSR bits lies in the highest bits of the register).

> The qemu ppc32 emulation is implemented in such a way that on 64-bit hosts it 
> looks a lot like a ppc64 implementation. However this need not, and should 
> not be exposed to the user.
> 
> > OK. Those are real bugs to be fixed. I'll take a look.... But I'll try
> > not to break the GPR dump. In fact, GPR should always dumped as 64 bits,
> > even when runnig on 32 bits hosts. This would be more consistent with
> > the specification.
> 
> I disagree. qemu is implementing ppc32.

which does not exists.

>  Showing more than 32 bits of register 
> is completely bogus.

No. It's showing the full CPU state, which can be more than what the
application (or the OS, when running virtualized on a real CPU) could
see. The OS cannot see the whole CPU state, but Qemu must implement more
than the OS can see and is then able to dump it. 64 bits GPR is just a
specific case of a general behavior.

>  Any differences between a 32-bit host and a 64-bit host 
> are a qemu bug. If you display 64 bits, then those 64 bits had better be the 
> same when run on 32-bit hosts.

Why ? The idea is that it costs too much to keep the whole state when
running on a 32 bits host, then we act as a restricted embedded
implementation. When the host CPU allows it without any extra cost, we
act as the specification defines we should. This is a choice. Once
again, this choice can be discussed and may be changed if I get
convinced it would be better not to act this way. But this behavior is
sure not bugged, it exactly follows (or may say should exactly if well
implemented) the PowerPC specification.

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Thiemo Seufer]

J. Mayer wrote:
[snip]
> >  Showing more than 32 bits of register 
> > is completely bogus.
> 
> No. It's showing the full CPU state, which can be more than what the
> application (or the OS, when running virtualized on a real CPU) could
> see. The OS cannot see the whole CPU state, but Qemu must implement more
> than the OS can see and is then able to dump it. 64 bits GPR is just a
> specific case of a general behavior.
> 
> >  Any differences between a 32-bit host and a 64-bit host 
> > are a qemu bug. If you display 64 bits, then those 64 bits had better be the 
> > same when run on 32-bit hosts.
> 
> Why ? The idea is that it costs too much to keep the whole state when
> running on a 32 bits host, then we act as a restricted embedded
> implementation. When the host CPU allows it without any extra cost, we
> act as the specification defines we should. This is a choice. Once
> again, this choice can be discussed and may be changed if I get
> convinced it would be better not to act this way. But this behavior is
> sure not bugged, it exactly follows (or may say should exactly if well
> implemented) the PowerPC specification.

Degrading the emulation capabilities in dependence of the host capabilities
with disregard of the user's requests sounds like a tremendously bad idea
to me. So

- If the high bits state is discoverable from guest software then it
  should always be emulated, independent of the host.
- If the high bits state is software transparent, then it should never
  be emulated. Even on a 64-bit host it will reduce performance as it
  moves more state through the caches. Moreover, the emulation will stay
  architecturally correct WRT to software execution, which is, I believe,
  the part of the spec which actually counts for QEMU.


Thiemo

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Paul Brook]

> >  By your own admission, we can get away with not calculating the
> > high 32 bit of the register. If follows that the high bits are completely
> > meaningless.
>
> Not completelly. There are even some way to do 64 bits computations when
> running in 32 bits mode... Some may see this as an architecture hack,
> but this gives the only way to switch from 32 bits to 64 bits mode (as
> the sixty-four MSR bits lies in the highest bits of the register).

Anything that involves switching to 64-bit mode to see th results is irelevant 
because we don't implement that.

You can't have it both ways. Either you need to implement the full 64-bit gpr 
for correctness, in which case I guess we're most of the way to scrapping 
ppc-softmmu and using ppc64-softmmu all the time, or the high bits are not 
part of the interesting CPU state.

I can believe that on some hosts it's cheaper to use a 64-bit gpr_t, and the 
architecture/implementation is such that it gives the same results as a 
32-bit gpr_t. However this is an implementation detail, and should not be 
exposed to the user.

> To complicate the situation, it's also required that "standard"
> implementation do all computations on 64 bit values

Really? Are you sure? I can understand the architecture being defined in terms 
of 64-bit gprs. However if the high half of those registers is never visible 
to the application/OS then those aren't actually requirements, they're just a 
convenient shorthand for avoiding having to describe everything twice.

> > I disagree. qemu is implementing ppc32.
>
> which does not exists.

Well, I admit I've invented the term "ppc32", but there are dozens of 32-bit 
PowerPC chips. I'd be amazed if they do 64-bit computations or have 64-bit 
GPRs. SPE doesn't count as the high half is effectively a separate register 
file on 32-bit cores.

Paul

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Julian Seward]

> Well, I admit I've invented the term "ppc32", but there are dozens of
> 32-bit PowerPC chips. I'd be amazed if they do 64-bit computations or have
> 64-bit GPRs.

Indeed not.  Valgrind implements the 32-bit PPC user-space instruction set
quite adequately using 32-bit computations throughout.  No need for 64-bit
computations.

J

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[J. Mayer]

On Sat, 2007-11-24 at 00:52 +0000, Paul Brook wrote:
> > >  By your own admission, we can get away with not calculating the
> > > high 32 bit of the register. If follows that the high bits are completely
> > > meaningless.
> >
> > Not completelly. There are even some way to do 64 bits computations when
> > running in 32 bits mode... Some may see this as an architecture hack,
> > but this gives the only way to switch from 32 bits to 64 bits mode (as
> > the sixty-four MSR bits lies in the highest bits of the register).
> 
> Anything that involves switching to 64-bit mode to see th results is irelevant 
> because we don't implement that.
>
> You can't have it both ways. Either you need to implement the full 64-bit gpr 
> for correctness, in which case I guess we're most of the way to scrapping 
> ppc-softmmu and using ppc64-softmmu all the time, or the high bits are not 
> part of the interesting CPU state.

Yes, when running on a 64 bits host, we could avoid compiling
ppc-softmmu. It's still interresting to use it on 32 bits host, as an
optimisation, because it runs much faster than the ppc64-softmmu
version.

> I can believe that on some hosts it's cheaper to use a 64-bit gpr_t, and the 
> architecture/implementation is such that it gives the same results as a 
> 32-bit gpr_t. However this is an implementation detail, and should not be 
> exposed to the user.
> 
> > To complicate the situation, it's also required that "standard"
> > implementation do all computations on 64 bit values
> 
> Really? Are you sure? I can understand the architecture being defined in terms 
> of 64-bit gprs. However if the high half of those registers is never visible 
> to the application/OS then those aren't actually requirements, they're just a 
> convenient shorthand for avoiding having to describe everything twice.
> 
> > > I disagree. qemu is implementing ppc32.
> >
> > which does not exists.
> 
> Well, I admit I've invented the term "ppc32", but there are dozens of 32-bit 
> PowerPC chips. I'd be amazed if they do 64-bit computations or have 64-bit 
> GPRs. SPE doesn't count as the high half is effectively a separate register 
> file on 32-bit cores.

OK. Maybe I did not properly said the fact: the spec says that if an
implementation (said "embedded"...) does not implement any 64 bits
operation, it could also optionally avoid using 64 bits GPR. And of
course this can lead to avoid any 64 bits computation to be implemented
on the silicium. But this is not defined as the normal behavior of a
PowerPC CPU.
But as you said, this does not change nothing when seen from the
execution environment, even if it's not architecturally correct.
SPE is not using separate registers. The specification actually says the
GPR are 64 bits even on 32 bits implementations if SPR is implemented.
SPE operations affect all 64 bits of the register and this can then be
seen with "standard" PowerPC operations.

I did take care of your remarks about the buggy prints and made a
general pass in all the target-ppc code (it seems they were even more
issues). I also made the SPE part of GPR available in CPU dumps.
This leads me to say that if we ever want to change the behavior of the
32 bits PowerPC emulation, it will only need a one line patch. For now I
would really like to keep the current behavior which is architecturally
correct and helps me debugging the 64 bits part; this is one of the
reasons why I first decided to do this way, the other one being the fact
it seems to lead to better code on my x86_64 host. When the 64 bits
emulation will be fully usable, I could imagine come back to "strict 32
bits" for the ppc-xxx target, as those target would become "not so
useful"...

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[J. Mayer]

On Sat, 2007-11-24 at 02:32 +0100, J. Mayer wrote:
> On Sat, 2007-11-24 at 00:52 +0000, Paul Brook wrote:
> > > >  By your own admission, we can get away with not calculating the
> > > > high 32 bit of the register. If follows that the high bits are completely
> > > > meaningless.
> > >
> > > Not completelly. There are even some way to do 64 bits computations when
> > > running in 32 bits mode... Some may see this as an architecture hack,
> > > but this gives the only way to switch from 32 bits to 64 bits mode (as
> > > the sixty-four MSR bits lies in the highest bits of the register).
> > 
> > Anything that involves switching to 64-bit mode to see th results is irelevant 
> > because we don't implement that.
> >
> > You can't have it both ways. Either you need to implement the full 64-bit gpr 
> > for correctness, in which case I guess we're most of the way to scrapping 
> > ppc-softmmu and using ppc64-softmmu all the time, or the high bits are not 
> > part of the interesting CPU state.
> 
> Yes, when running on a 64 bits host, we could avoid compiling
> ppc-softmmu. It's still interresting to use it on 32 bits host, as an
> optimisation, because it runs much faster than the ppc64-softmmu
> version.
> 
> > I can believe that on some hosts it's cheaper to use a 64-bit gpr_t, and the 
> > architecture/implementation is such that it gives the same results as a 
> > 32-bit gpr_t. However this is an implementation detail, and should not be 
> > exposed to the user.

I did forget one important point: it's not exposed to the qemu user.
The qemu log is a qemu developper tool.Most of the trace involved has to
be explicitally activated with specific defines in the Qemu code. The
qemu log is not to be of any help for a end-user, it's a tool for Qemu
development and bug reports.
>From the end-user side, the fact that GPR are implemented as 64 bits
values is never visible, even from the gdb stub. Then...

[...]

-- 
J. Mayer <l_indien@magic.fr>
Never organized

Re: [Qemu-devel] qemu hw/ppc_oldworld.c target-ppc/cpu.h target-...

[Fabrice Bellard]

Thiemo Seufer wrote:
> Paul Brook wrote:
>>>> I think what you mean is that they work the way that ppc64 is defined, to
>>>> remain compatible with ppc32.  IMHO this is entirely irrelevant as we're
>>>> emulating a ppc32. You could replace the high bits with garbage and
>>>> nothing would ever be able to tell the difference.
>>> PowerPC is a 64 bits architecture. PowerPC 32 on 32 bits host is
>>> optimized not to compute the 32 highest bits, the same way it's allowed
>>> to cut down the GPR when implementing a CPU that would not support the
>>> 64 bits mode (but this is a tolerance, this is not the architecture is
>>> defined).
>> No. PowerPC is defined as a 64-bit archirecure. However there is a subset of 
>> this architecture (aka ppc32) that is a complete 32-bit architecture in its 
>> own right.  By your own admission, we can get away with not calculating the 
>> high 32 bit of the register. If follows that the high bits are completely 
>> meaningless. 
> 
> This btw. also means that the ppc32 emulation on 32-bit hosts is needlessly
> inefficient if the high bits are carried around.
> 
>> The qemu ppc32 emulation is implemented in such a way that on 64-bit hosts it 
>> looks a lot like a ppc64 implementation. However this need not, and should 
>> not be exposed to the user.
>>
>>> OK. Those are real bugs to be fixed. I'll take a look.... But I'll try
>>> not to break the GPR dump. In fact, GPR should always dumped as 64 bits,
>>> even when runnig on 32 bits hosts. This would be more consistent with
>>> the specification.
>> I disagree. qemu is implementing ppc32. Showing more than 32 bits of register 
>> is completely bogus. Any differences between a 32-bit host and a 64-bit host 
>> are a qemu bug. If you display 64 bits, then those 64 bits had better be the 
>> same when run on 32-bit hosts.
> 
> I strongly agree with Paul.

I strongly agree too and I suggest to remove the type ppc_gpr_t and to
replace it with target_ulong (and uint32_t for the high part of the SPE
extensions).

Regards,

Fabrice.