mbox series

[v6,00/16] tcg/ppc: Add vector opcodes

Message ID 20190629130017.2973-1-richard.henderson@linaro.org
Headers show
Series tcg/ppc: Add vector opcodes | expand

Message

Richard Henderson June 29, 2019, 1 p.m. UTC
Changes since v5:
  * Disable runtime altivec detection until all of the required
    opcodes are implemented.
    Because dup2 was last, that really means all of the pure altivec
    bits, so the initial patches are not bisectable in any meaningful
    sense.  I thought about reshuffling dup2 earlier, but that created
    too many conflicts and I was too lazy.
  * Rearranged the patches a little bit to make sure that each
    one actually builds, which was not the case before.
  * Folded in the fix to tcg_out_mem_long, as discussed in the
    followup within the v4 thread.

Changes since v4:
  * Patch 1, "tcg/ppc: Introduce Altivec registers", is divided into
    ten smaller patches.
  * The net result (code-wise) is not changed between former patch 1
    and ten new patches.
  * Remaining (2-7) patches from v4 are applied verbatim.
  * This means that code-wise v5 and v4 do not differ.
  * v5 is devised to help debugging, and to better organize the code.

Changes since v3:
  * Add support for bitsel, with the vsx xxsel insn.
  * Rely on the new relocation overflow handling, so
    we don't require 3 insns for a vector load.

Changes since v2:
  * Several generic tcg patches to improve dup vs dupi vs dupm.
    In particular, if a global temp (like guest r10) is not in
    a host register, we should duplicate from memory instead of
    loading to an integer register, spilling to stack, loading
    to a vector register, and then duplicating.
  * I have more confidence that 32-bit ppc host should work
    this time around.  No testing on that front yet, but I've
    unified some code sequences with 64-bit ppc host.
  * Base altivec now supports V128 only.  Moved V64 support to
    Power7 (v2.06), which has 64-bit load/store.
  * Dropped support for 64-bit vector multiply using Power8.
    The expansion was too large compared to using integer regs.

Richard Henderson (16):
  tcg/ppc: Introduce Altivec registers
  tcg/ppc: Introduce macro VX4()
  tcg/ppc: Introduce macros VRT(), VRA(), VRB(), VRC()
  tcg/ppc: Enable tcg backend vector compilation
  tcg/ppc: Add support for load/store/logic/comparison
  tcg/ppc: Add support for vector maximum/minimum
  tcg/ppc: Add support for vector add/subtract
  tcg/ppc: Add support for vector saturated add/subtract
  tcg/ppc: Prepare case for vector multiply
  tcg/ppc: Support vector shift by immediate
  tcg/ppc: Support vector multiply
  tcg/ppc: Support vector dup2
  tcg/ppc: Enable Altivec detection
  tcg/ppc: Update vector support to v2.06
  tcg/ppc: Update vector support to v2.07
  tcg/ppc: Update vector support to v3.00

 tcg/ppc/tcg-target.h     |   39 +-
 tcg/ppc/tcg-target.opc.h |   13 +
 tcg/ppc/tcg-target.inc.c | 1091 +++++++++++++++++++++++++++++++++++---
 3 files changed, 1076 insertions(+), 67 deletions(-)
 create mode 100644 tcg/ppc/tcg-target.opc.h

-- 
2.17.1

Comments

no-reply@patchew.org June 29, 2019, 1:37 p.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20190629130017.2973-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v6 00/16] tcg/ppc: Add vector opcodes
Message-id: 20190629130017.2973-1-richard.henderson@linaro.org
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
735e428 tcg/ppc: Update vector support to v3.00
d5df8ce tcg/ppc: Update vector support to v2.07
70bae8c tcg/ppc: Update vector support to v2.06
cdcb6fd tcg/ppc: Enable Altivec detection
5eca04a tcg/ppc: Support vector dup2
9a92a5b tcg/ppc: Support vector multiply
9dcbbb5 tcg/ppc: Support vector shift by immediate
5707cff tcg/ppc: Prepare case for vector multiply
4e8c856 tcg/ppc: Add support for vector saturated add/subtract
8542349 tcg/ppc: Add support for vector add/subtract
09dcca3 tcg/ppc: Add support for vector maximum/minimum
940d802 tcg/ppc: Add support for load/store/logic/comparison
1354b48 tcg/ppc: Enable tcg backend vector compilation
ce65dc7 tcg/ppc: Introduce macros VRT(), VRA(), VRB(), VRC()
c15e076 tcg/ppc: Introduce macro VX4()
a351796 tcg/ppc: Introduce Altivec registers

=== OUTPUT BEGIN ===
1/16 Checking commit a35179674cf2 (tcg/ppc: Introduce Altivec registers)
2/16 Checking commit c15e076c7d0f (tcg/ppc: Introduce macro VX4())
ERROR: spaces required around that '|' (ctx:VxV)
#21: FILE: tcg/ppc/tcg-target.inc.c:322:
+#define VX4(opc)  (OPCD(4)|(opc))
                           ^

total: 1 errors, 0 warnings, 7 lines checked

Patch 2/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/16 Checking commit ce65dc76f743 (tcg/ppc: Introduce macros VRT(), VRA(), VRB(), VRC())
4/16 Checking commit 1354b48a4dce (tcg/ppc: Enable tcg backend vector compilation)
WARNING: Block comments use a leading /* on a separate line
#155: FILE: tcg/ppc/tcg-target.inc.c:2842:
+    if (hwcap & /* PPC_FEATURE_HAS_ALTIVEC -- NOT YET */ 0) {

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#173: 
new file mode 100644

total: 0 errors, 2 warnings, 138 lines checked

Patch 4/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/16 Checking commit 940d8027994d (tcg/ppc: Add support for load/store/logic/comparison)
6/16 Checking commit 09dcca3c9f87 (tcg/ppc: Add support for vector maximum/minimum)
7/16 Checking commit 8542349a45f4 (tcg/ppc: Add support for vector add/subtract)
8/16 Checking commit 4e8c8565186d (tcg/ppc: Add support for vector saturated add/subtract)
9/16 Checking commit 5707cff60faf (tcg/ppc: Prepare case for vector multiply)
10/16 Checking commit 9dcbbb561046 (tcg/ppc: Support vector shift by immediate)
11/16 Checking commit 9a92a5bffebd (tcg/ppc: Support vector multiply)
ERROR: code indent should never use tabs
#133: FILE: tcg/ppc/tcg-target.inc.c:3220:
+^Ibreak;$

total: 1 errors, 0 warnings, 185 lines checked

Patch 11/16 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

12/16 Checking commit 5eca04a86aea (tcg/ppc: Support vector dup2)
13/16 Checking commit cdcb6fdbe190 (tcg/ppc: Enable Altivec detection)
14/16 Checking commit 70bae8c6b3df (tcg/ppc: Update vector support to v2.06)
15/16 Checking commit d5df8cec3718 (tcg/ppc: Update vector support to v2.07)
16/16 Checking commit 735e428f5f2a (tcg/ppc: Update vector support to v3.00)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190629130017.2973-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Mark Cave-Ayland June 30, 2019, 5:58 p.m. UTC | #2
On 29/06/2019 14:00, Richard Henderson wrote:

> Changes since v5:

>   * Disable runtime altivec detection until all of the required

>     opcodes are implemented.

>     Because dup2 was last, that really means all of the pure altivec

>     bits, so the initial patches are not bisectable in any meaningful

>     sense.  I thought about reshuffling dup2 earlier, but that created

>     too many conflicts and I was too lazy.

>   * Rearranged the patches a little bit to make sure that each

>     one actually builds, which was not the case before.

>   * Folded in the fix to tcg_out_mem_long, as discussed in the

>     followup within the v4 thread.

> 

> Changes since v4:

>   * Patch 1, "tcg/ppc: Introduce Altivec registers", is divided into

>     ten smaller patches.

>   * The net result (code-wise) is not changed between former patch 1

>     and ten new patches.

>   * Remaining (2-7) patches from v4 are applied verbatim.

>   * This means that code-wise v5 and v4 do not differ.

>   * v5 is devised to help debugging, and to better organize the code.

> 

> Changes since v3:

>   * Add support for bitsel, with the vsx xxsel insn.

>   * Rely on the new relocation overflow handling, so

>     we don't require 3 insns for a vector load.

> 

> Changes since v2:

>   * Several generic tcg patches to improve dup vs dupi vs dupm.

>     In particular, if a global temp (like guest r10) is not in

>     a host register, we should duplicate from memory instead of

>     loading to an integer register, spilling to stack, loading

>     to a vector register, and then duplicating.

>   * I have more confidence that 32-bit ppc host should work

>     this time around.  No testing on that front yet, but I've

>     unified some code sequences with 64-bit ppc host.

>   * Base altivec now supports V128 only.  Moved V64 support to

>     Power7 (v2.06), which has 64-bit load/store.

>   * Dropped support for 64-bit vector multiply using Power8.

>     The expansion was too large compared to using integer regs.

> 

> Richard Henderson (16):

>   tcg/ppc: Introduce Altivec registers

>   tcg/ppc: Introduce macro VX4()

>   tcg/ppc: Introduce macros VRT(), VRA(), VRB(), VRC()

>   tcg/ppc: Enable tcg backend vector compilation

>   tcg/ppc: Add support for load/store/logic/comparison

>   tcg/ppc: Add support for vector maximum/minimum

>   tcg/ppc: Add support for vector add/subtract

>   tcg/ppc: Add support for vector saturated add/subtract

>   tcg/ppc: Prepare case for vector multiply

>   tcg/ppc: Support vector shift by immediate

>   tcg/ppc: Support vector multiply

>   tcg/ppc: Support vector dup2

>   tcg/ppc: Enable Altivec detection

>   tcg/ppc: Update vector support to v2.06

>   tcg/ppc: Update vector support to v2.07

>   tcg/ppc: Update vector support to v3.00

> 

>  tcg/ppc/tcg-target.h     |   39 +-

>  tcg/ppc/tcg-target.opc.h |   13 +

>  tcg/ppc/tcg-target.inc.c | 1091 +++++++++++++++++++++++++++++++++++---

>  3 files changed, 1076 insertions(+), 67 deletions(-)

>  create mode 100644 tcg/ppc/tcg-target.opc.h


I don't have space for a full set of images on the G4, however I've tried boot tests
on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks good here.

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]



ATB,

Mark.
Richard Henderson July 1, 2019, 10:30 a.m. UTC | #3
On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:
> I don't have space for a full set of images on the G4, however I've tried boot tests

> on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks good here.

> 

> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]


Thanks!


r!
Howard Spoelstra July 1, 2019, 6:34 p.m. UTC | #4
On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:

> > I don't have space for a full set of images on the G4, however I've

> tried boot tests

> > on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks

> good here.

> >

> > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]

>

> Thanks!

>

> Hi


I just compiled the v6 set applied to current master on my G5, Ubuntu 16.
command line:
./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \
-netdev user,id=net1 -device sungem,netdev=net1 \
-drive file=10.3.img,format=raw,media=disk \

With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do not get
to the desktop, they just hang while still in the openbios window. They
need -cpu G4 on the command line to get to the desktop.

OSX 10.3 installed image boots to desktop.
OSX 10.3 iso boots to installer
OSX 10.4 installed image boots to desktop.
OSX 10.4 iso boot to installer
OSX 10.5 installed image boots to desktop.
OSX 10.5 iso boots to installer

So there seems to be a difference between hosts: If ran on a G4 host there
is no need to add -cpu G4 to run Mac OS 9.x, while there is when ran on a
G5 host.

Best,
Howard
Mark Cave-Ayland Sept. 3, 2019, 5:02 p.m. UTC | #5
On 01/07/2019 19:34, Howard Spoelstra wrote:

> On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson <

> richard.henderson@linaro.org> wrote:

> 

>> On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:

>>> I don't have space for a full set of images on the G4, however I've

>> tried boot tests

>>> on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks

>> good here.

>>>

>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]

>>

>> Thanks!

>>

>> Hi

> 

> I just compiled the v6 set applied to current master on my G5, Ubuntu 16.

> command line:

> ./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \

> -netdev user,id=net1 -device sungem,netdev=net1 \

> -drive file=10.3.img,format=raw,media=disk \

> 

> With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do not get

> to the desktop, they just hang while still in the openbios window. They

> need -cpu G4 on the command line to get to the desktop.

> 

> OSX 10.3 installed image boots to desktop.

> OSX 10.3 iso boots to installer

> OSX 10.4 installed image boots to desktop.

> OSX 10.4 iso boot to installer

> OSX 10.5 installed image boots to desktop.

> OSX 10.5 iso boots to installer

> 

> So there seems to be a difference between hosts: If ran on a G4 host there

> is no need to add -cpu G4 to run Mac OS 9.x, while there is when ran on a

> G5 host.


Are there any outstanding issues with this patchset now, or is it ready to be merged?
I'm really looking forward to seeing the improved performance when testing QEMU on my
Mac Mini :)


ATB,

Mark.
Aleksandar Markovic Sept. 3, 2019, 5:37 p.m. UTC | #6
On Tue, Sep 3, 2019 at 7:05 PM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> On 01/07/2019 19:34, Howard Spoelstra wrote:

>

> > On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson <

> > richard.henderson@linaro.org> wrote:

> >

> >> On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:

> >>> I don't have space for a full set of images on the G4, however I've

> >> tried boot tests

> >>> on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks

> >> good here.

> >>>

> >>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]

> >>

> >> Thanks!

> >>

> >> Hi

> >

> > I just compiled the v6 set applied to current master on my G5, Ubuntu 16.

> > command line:

> > ./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \

> > -netdev user,id=net1 -device sungem,netdev=net1 \

> > -drive file=10.3.img,format=raw,media=disk \

> >

> > With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do not

> get

> > to the desktop, they just hang while still in the openbios window. They

> > need -cpu G4 on the command line to get to the desktop.

> >

> > OSX 10.3 installed image boots to desktop.

> > OSX 10.3 iso boots to installer

> > OSX 10.4 installed image boots to desktop.

> > OSX 10.4 iso boot to installer

> > OSX 10.5 installed image boots to desktop.

> > OSX 10.5 iso boots to installer

> >

> > So there seems to be a difference between hosts: If ran on a G4 host

> there

> > is no need to add -cpu G4 to run Mac OS 9.x, while there is when ran on a

> > G5 host.

>

> Are there any outstanding issues with this patchset now, or is it ready to

> be merged?

> I'm really looking forward to seeing the improved performance when testing

> QEMU on my

> Mac Mini :)

>

>

Howard pointed to some illogical quirks of command line:

> If ran on a G4 host there is no need to add -cpu G4 to run Mac OS 9.x,

> while there is when ran on a G5 host.


I am not sure if Howard says that this is a consequence of this series
though.

Overall, I think this is a very good series - however, I had a number of
minor
objections to multiple patches, that don't affect (or affect in a minimal
way)
provided functionality - those objections are not addressed, nor properly
discussed - but I do think they should be addressed in order to get the
series
in a better shape before upstreaming.

Thanks,
Aleksandar


> ATB,

>

> Mark.

>

>
Mark Cave-Ayland Sept. 3, 2019, 6:32 p.m. UTC | #7
On 03/09/2019 18:37, Aleksandar Markovic wrote:

> On Tue, Sep 3, 2019 at 7:05 PM Mark Cave-Ayland <

> mark.cave-ayland@ilande.co.uk> wrote:

> 

>> On 01/07/2019 19:34, Howard Spoelstra wrote:

>>

>>> On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson <

>>> richard.henderson@linaro.org> wrote:

>>>

>>>> On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:

>>>>> I don't have space for a full set of images on the G4, however I've

>>>> tried boot tests

>>>>> on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks

>>>> good here.

>>>>>

>>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]

>>>>

>>>> Thanks!

>>>>

>>>> Hi

>>>

>>> I just compiled the v6 set applied to current master on my G5, Ubuntu 16.

>>> command line:

>>> ./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \

>>> -netdev user,id=net1 -device sungem,netdev=net1 \

>>> -drive file=10.3.img,format=raw,media=disk \

>>>

>>> With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do not

>> get

>>> to the desktop, they just hang while still in the openbios window. They

>>> need -cpu G4 on the command line to get to the desktop.

>>>

>>> OSX 10.3 installed image boots to desktop.

>>> OSX 10.3 iso boots to installer

>>> OSX 10.4 installed image boots to desktop.

>>> OSX 10.4 iso boot to installer

>>> OSX 10.5 installed image boots to desktop.

>>> OSX 10.5 iso boots to installer

>>>

>>> So there seems to be a difference between hosts: If ran on a G4 host

>> there

>>> is no need to add -cpu G4 to run Mac OS 9.x, while there is when ran on a

>>> G5 host.

>>

>> Are there any outstanding issues with this patchset now, or is it ready to

>> be merged?

>> I'm really looking forward to seeing the improved performance when testing

>> QEMU on my

>> Mac Mini :)

>>

>>

> Howard pointed to some illogical quirks of command line:

> 

>> If ran on a G4 host there is no need to add -cpu G4 to run Mac OS 9.x,

>> while there is when ran on a G5 host.

> 

> I am not sure if Howard says that this is a consequence of this series

> though.


No, that has been an existing issue for a long time :)

> Overall, I think this is a very good series - however, I had a number of

> minor

> objections to multiple patches, that don't affect (or affect in a minimal

> way)

> provided functionality - those objections are not addressed, nor properly

> discussed - but I do think they should be addressed in order to get the

> series

> in a better shape before upstreaming.


I've had a quick look at some of your review comments, and certainly I can see how
the earlier revisions have benefited from your feedback. There has been a lot of
positive discussion, and Richard has taken the time to respond and update the
patchset over several weeks to its latest revision.

AFAICT the only remaining issue is that related to the ISA flags, but to me this
isn't something that should prevent the patchset being merged. I can certainly see
how the current flags implementation may not be considered technically correct, but
then from your comments I don't see that it would be something that would be
particularly difficult to change at a later date either.

The things that are important to me are i) is the patchset functionally correct and
ii) is it understandable and maintainable. I would say that the first point is
clearly true (both myself and Howard have spent a lot of time testing it), and given
that I had to delve into these patches to fix the R2 register issue on 32-bit PPC
then I can confirm that the contents of the patches were a reasonably accurate
representation of the changes described within. And that's from someone like me who
is mostly still a TCG beginner :)

From a slightly more selfish position as the PPC Mac machine maintainer, these
patches make a significant difference to me in that they reduce the MacOS boot times
during everyday testing. Now for someone like myself who works on QEMU as a hobby
outside of family life and a full time job, those few minutes are really important to
me and soon add up really quickly during testing.

I would really like these patches to be merged soon, since the worst thing that can
happen is that the patchset ends up bit-rotting and then all the time and effort put
into writing, testing and reviewing the patches by Richard, Howard, David, myself and
indeed your review time will all end up going to waste.


ATB,

Mark.
Aleksandar Markovic Sept. 5, 2019, 11:43 a.m. UTC | #8
ping for Richard

03.09.2019. 20.34, "Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk> је
написао/ла:
>

> On 03/09/2019 18:37, Aleksandar Markovic wrote:

>

> > On Tue, Sep 3, 2019 at 7:05 PM Mark Cave-Ayland <

> > mark.cave-ayland@ilande.co.uk> wrote:

> >

> >> On 01/07/2019 19:34, Howard Spoelstra wrote:

> >>

> >>> On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson <

> >>> richard.henderson@linaro.org> wrote:

> >>>

> >>>> On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:

> >>>>> I don't have space for a full set of images on the G4, however I've

> >>>> tried boot tests

> >>>>> on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it

looks
> >>>> good here.

> >>>>>

> >>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> [PPC32]

> >>>>

> >>>> Thanks!

> >>>>

> >>>> Hi

> >>>

> >>> I just compiled the v6 set applied to current master on my G5, Ubuntu

16.
> >>> command line:

> >>> ./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \

> >>> -netdev user,id=net1 -device sungem,netdev=net1 \

> >>> -drive file=10.3.img,format=raw,media=disk \

> >>>

> >>> With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do

not
> >> get

> >>> to the desktop, they just hang while still in the openbios window.

They
> >>> need -cpu G4 on the command line to get to the desktop.

> >>>

> >>> OSX 10.3 installed image boots to desktop.

> >>> OSX 10.3 iso boots to installer

> >>> OSX 10.4 installed image boots to desktop.

> >>> OSX 10.4 iso boot to installer

> >>> OSX 10.5 installed image boots to desktop.

> >>> OSX 10.5 iso boots to installer

> >>>

> >>> So there seems to be a difference between hosts: If ran on a G4 host

> >> there

> >>> is no need to add -cpu G4 to run Mac OS 9.x, while there is when ran

on a
> >>> G5 host.

> >>

> >> Are there any outstanding issues with this patchset now, or is it

ready to
> >> be merged?

> >> I'm really looking forward to seeing the improved performance when

testing
> >> QEMU on my

> >> Mac Mini :)

> >>

> >>

> > Howard pointed to some illogical quirks of command line:

> >

> >> If ran on a G4 host there is no need to add -cpu G4 to run Mac OS 9.x,

> >> while there is when ran on a G5 host.

> >

> > I am not sure if Howard says that this is a consequence of this series

> > though.

>

> No, that has been an existing issue for a long time :)

>

> > Overall, I think this is a very good series - however, I had a number of

> > minor

> > objections to multiple patches, that don't affect (or affect in a

minimal
> > way)

> > provided functionality - those objections are not addressed, nor

properly
> > discussed - but I do think they should be addressed in order to get the

> > series

> > in a better shape before upstreaming.

>

> I've had a quick look at some of your review comments, and certainly I

can see how
> the earlier revisions have benefited from your feedback. There has been a

lot of
> positive discussion, and Richard has taken the time to respond and update

the
> patchset over several weeks to its latest revision.

>

> AFAICT the only remaining issue is that related to the ISA flags, but to

me this
> isn't something that should prevent the patchset being merged. I can

certainly see
> how the current flags implementation may not be considered technically

correct, but
> then from your comments I don't see that it would be something that would

be
> particularly difficult to change at a later date either.

>

> The things that are important to me are i) is the patchset functionally

correct and
> ii) is it understandable and maintainable. I would say that the first

point is
> clearly true (both myself and Howard have spent a lot of time testing

it), and given
> that I had to delve into these patches to fix the R2 register issue on

32-bit PPC
> then I can confirm that the contents of the patches were a reasonably

accurate
> representation of the changes described within. And that's from someone

like me who
> is mostly still a TCG beginner :)

>

> From a slightly more selfish position as the PPC Mac machine maintainer,

these
> patches make a significant difference to me in that they reduce the MacOS

boot times
> during everyday testing. Now for someone like myself who works on QEMU as

a hobby
> outside of family life and a full time job, those few minutes are really

important to
> me and soon add up really quickly during testing.

>

> I would really like these patches to be merged soon, since the worst

thing that can
> happen is that the patchset ends up bit-rotting and then all the time and

effort put
> into writing, testing and reviewing the patches by Richard, Howard,

David, myself and
> indeed your review time will all end up going to waste.

>

>

> ATB,

>

> Mark.

>
Aleksandar Markovic Sept. 27, 2019, 12:13 p.m. UTC | #9
ping

05.09.2019. 13.43, "Aleksandar Markovic" <aleksandar.m.mail@gmail.com> је
написао/ла:
>

>

> ping for Richard

>

> 03.09.2019. 20.34, "Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk> је

написао/ла:
> >

> > On 03/09/2019 18:37, Aleksandar Markovic wrote:

> >

> > > On Tue, Sep 3, 2019 at 7:05 PM Mark Cave-Ayland <

> > > mark.cave-ayland@ilande.co.uk> wrote:

> > >

> > >> On 01/07/2019 19:34, Howard Spoelstra wrote:

> > >>

> > >>> On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson <

> > >>> richard.henderson@linaro.org> wrote:

> > >>>

> > >>>> On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:

> > >>>>> I don't have space for a full set of images on the G4, however

I've
> > >>>> tried boot tests

> > >>>>> on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it

looks
> > >>>> good here.

> > >>>>>

> > >>>>> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

[PPC32]
> > >>>>

> > >>>> Thanks!

> > >>>>

> > >>>> Hi

> > >>>

> > >>> I just compiled the v6 set applied to current master on my G5,

Ubuntu 16.
> > >>> command line:

> > >>> ./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \

> > >>> -netdev user,id=net1 -device sungem,netdev=net1 \

> > >>> -drive file=10.3.img,format=raw,media=disk \

> > >>>

> > >>> With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do

not
> > >> get

> > >>> to the desktop, they just hang while still in the openbios window.

They
> > >>> need -cpu G4 on the command line to get to the desktop.

> > >>>

> > >>> OSX 10.3 installed image boots to desktop.

> > >>> OSX 10.3 iso boots to installer

> > >>> OSX 10.4 installed image boots to desktop.

> > >>> OSX 10.4 iso boot to installer

> > >>> OSX 10.5 installed image boots to desktop.

> > >>> OSX 10.5 iso boots to installer

> > >>>

> > >>> So there seems to be a difference between hosts: If ran on a G4 host

> > >> there

> > >>> is no need to add -cpu G4 to run Mac OS 9.x, while there is when

ran on a
> > >>> G5 host.

> > >>

> > >> Are there any outstanding issues with this patchset now, or is it

ready to
> > >> be merged?

> > >> I'm really looking forward to seeing the improved performance when

testing
> > >> QEMU on my

> > >> Mac Mini :)

> > >>

> > >>

> > > Howard pointed to some illogical quirks of command line:

> > >

> > >> If ran on a G4 host there is no need to add -cpu G4 to run Mac OS

9.x,
> > >> while there is when ran on a G5 host.

> > >

> > > I am not sure if Howard says that this is a consequence of this series

> > > though.

> >

> > No, that has been an existing issue for a long time :)

> >

> > > Overall, I think this is a very good series - however, I had a number

of
> > > minor

> > > objections to multiple patches, that don't affect (or affect in a

minimal
> > > way)

> > > provided functionality - those objections are not addressed, nor

properly
> > > discussed - but I do think they should be addressed in order to get

the
> > > series

> > > in a better shape before upstreaming.

> >

> > I've had a quick look at some of your review comments, and certainly I

can see how
> > the earlier revisions have benefited from your feedback. There has been

a lot of
> > positive discussion, and Richard has taken the time to respond and

update the
> > patchset over several weeks to its latest revision.

> >

> > AFAICT the only remaining issue is that related to the ISA flags, but

to me this
> > isn't something that should prevent the patchset being merged. I can

certainly see
> > how the current flags implementation may not be considered technically

correct, but
> > then from your comments I don't see that it would be something that

would be
> > particularly difficult to change at a later date either.

> >

> > The things that are important to me are i) is the patchset functionally

correct and
> > ii) is it understandable and maintainable. I would say that the first

point is
> > clearly true (both myself and Howard have spent a lot of time testing

it), and given
> > that I had to delve into these patches to fix the R2 register issue on

32-bit PPC
> > then I can confirm that the contents of the patches were a reasonably

accurate
> > representation of the changes described within. And that's from someone

like me who
> > is mostly still a TCG beginner :)

> >

> > From a slightly more selfish position as the PPC Mac machine

maintainer, these
> > patches make a significant difference to me in that they reduce the

MacOS boot times
> > during everyday testing. Now for someone like myself who works on QEMU

as a hobby
> > outside of family life and a full time job, those few minutes are

really important to
> > me and soon add up really quickly during testing.

> >

> > I would really like these patches to be merged soon, since the worst

thing that can
> > happen is that the patchset ends up bit-rotting and then all the time

and effort put
> > into writing, testing and reviewing the patches by Richard, Howard,

David, myself and
> > indeed your review time will all end up going to waste.

> >

> >

> > ATB,

> >

> > Mark.

> >
<p dir="ltr">ping</p>
<p dir="ltr">05.09.2019. 13.43, &quot;Aleksandar Markovic&quot; &lt;<a href="mailto:aleksandar.m.mail@gmail.com">aleksandar.m.mail@gmail.com</a>&gt; је написао/ла:<br>
&gt;<br>
&gt;<br>
&gt; ping for Richard<br>
&gt;<br>
&gt; 03.09.2019. 20.34, &quot;Mark Cave-Ayland&quot; &lt;<a href="mailto:mark.cave-ayland@ilande.co.uk">mark.cave-ayland@ilande.co.uk</a>&gt; је написао/ла:<br>
&gt; &gt;<br>
&gt; &gt; On 03/09/2019 18:37, Aleksandar Markovic wrote:<br>
&gt; &gt;<br>
&gt; &gt; &gt; On Tue, Sep 3, 2019 at 7:05 PM Mark Cave-Ayland &lt;<br>
&gt; &gt; &gt; <a href="mailto:mark.cave-ayland@ilande.co.uk">mark.cave-ayland@ilande.co.uk</a>&gt; wrote:<br>
&gt; &gt; &gt; <br>
&gt; &gt; &gt;&gt; On 01/07/2019 19:34, Howard Spoelstra wrote:<br>
&gt; &gt; &gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt; On Mon, Jul 1, 2019 at 12:30 PM Richard Henderson &lt;<br>
&gt; &gt; &gt;&gt;&gt; <a href="mailto:richard.henderson@linaro.org">richard.henderson@linaro.org</a>&gt; wrote:<br>
&gt; &gt; &gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt;&gt; On 6/30/19 7:58 PM, Mark Cave-Ayland wrote:<br>
&gt; &gt; &gt;&gt;&gt;&gt;&gt; I don&#39;t have space for a full set of images on the G4, however I&#39;ve<br>
&gt; &gt; &gt;&gt;&gt;&gt; tried boot tests<br>
&gt; &gt; &gt;&gt;&gt;&gt;&gt; on installer CDs for MacOS 9, OS X 10.2, Linux and HelenOS and it looks<br>
&gt; &gt; &gt;&gt;&gt;&gt; good here.<br>
&gt; &gt; &gt;&gt;&gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt;&gt;&gt; Tested-by: Mark Cave-Ayland &lt;<a href="mailto:mark.cave-ayland@ilande.co.uk">mark.cave-ayland@ilande.co.uk</a>&gt; [PPC32]<br>
&gt; &gt; &gt;&gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt;&gt; Thanks!<br>
&gt; &gt; &gt;&gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt;&gt; Hi<br>
&gt; &gt; &gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt; I just compiled the v6 set applied to current master on my G5, Ubuntu 16.<br>
&gt; &gt; &gt;&gt;&gt; command line:<br>
&gt; &gt; &gt;&gt;&gt; ./qemu-system-ppc -L pc-bios -boot c m 512 -M mac99,via=pmu \<br>
&gt; &gt; &gt;&gt;&gt; -netdev user,id=net1 -device sungem,netdev=net1 \<br>
&gt; &gt; &gt;&gt;&gt; -drive file=10.3.img,format=raw,media=disk \<br>
&gt; &gt; &gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt; With no specific cpu set, Mac OS 9.2 hard disk image and 9.2 iso do not<br>
&gt; &gt; &gt;&gt; get<br>
&gt; &gt; &gt;&gt;&gt; to the desktop, they just hang while still in the openbios window. They<br>
&gt; &gt; &gt;&gt;&gt; need -cpu G4 on the command line to get to the desktop.<br>
&gt; &gt; &gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt; OSX 10.3 installed image boots to desktop.<br>
&gt; &gt; &gt;&gt;&gt; OSX 10.3 iso boots to installer<br>
&gt; &gt; &gt;&gt;&gt; OSX 10.4 installed image boots to desktop.<br>
&gt; &gt; &gt;&gt;&gt; OSX 10.4 iso boot to installer<br>
&gt; &gt; &gt;&gt;&gt; OSX 10.5 installed image boots to desktop.<br>
&gt; &gt; &gt;&gt;&gt; OSX 10.5 iso boots to installer<br>
&gt; &gt; &gt;&gt;&gt;<br>
&gt; &gt; &gt;&gt;&gt; So there seems to be a difference between hosts: If ran on a G4 host<br>
&gt; &gt; &gt;&gt; there<br>
&gt; &gt; &gt;&gt;&gt; is no need to add -cpu G4 to run Mac OS 9.x, while there is when ran on a<br>
&gt; &gt; &gt;&gt;&gt; G5 host.<br>
&gt; &gt; &gt;&gt;<br>
&gt; &gt; &gt;&gt; Are there any outstanding issues with this patchset now, or is it ready to<br>
&gt; &gt; &gt;&gt; be merged?<br>
&gt; &gt; &gt;&gt; I&#39;m really looking forward to seeing the improved performance when testing<br>
&gt; &gt; &gt;&gt; QEMU on my<br>
&gt; &gt; &gt;&gt; Mac Mini :)<br>
&gt; &gt; &gt;&gt;<br>
&gt; &gt; &gt;&gt;<br>
&gt; &gt; &gt; Howard pointed to some illogical quirks of command line:<br>
&gt; &gt; &gt; <br>
&gt; &gt; &gt;&gt; If ran on a G4 host there is no need to add -cpu G4 to run Mac OS 9.x,<br>
&gt; &gt; &gt;&gt; while there is when ran on a G5 host.<br>
&gt; &gt; &gt; <br>
&gt; &gt; &gt; I am not sure if Howard says that this is a consequence of this series<br>
&gt; &gt; &gt; though.<br>
&gt; &gt;<br>
&gt; &gt; No, that has been an existing issue for a long time :)<br>
&gt; &gt;<br>
&gt; &gt; &gt; Overall, I think this is a very good series - however, I had a number of<br>
&gt; &gt; &gt; minor<br>
&gt; &gt; &gt; objections to multiple patches, that don&#39;t affect (or affect in a minimal<br>
&gt; &gt; &gt; way)<br>
&gt; &gt; &gt; provided functionality - those objections are not addressed, nor properly<br>
&gt; &gt; &gt; discussed - but I do think they should be addressed in order to get the<br>
&gt; &gt; &gt; series<br>
&gt; &gt; &gt; in a better shape before upstreaming.<br>
&gt; &gt;<br>
&gt; &gt; I&#39;ve had a quick look at some of your review comments, and certainly I can see how<br>
&gt; &gt; the earlier revisions have benefited from your feedback. There has been a lot of<br>
&gt; &gt; positive discussion, and Richard has taken the time to respond and update the<br>
&gt; &gt; patchset over several weeks to its latest revision.<br>
&gt; &gt;<br>
&gt; &gt; AFAICT the only remaining issue is that related to the ISA flags, but to me this<br>
&gt; &gt; isn&#39;t something that should prevent the patchset being merged. I can certainly see<br>
&gt; &gt; how the current flags implementation may not be considered technically correct, but<br>
&gt; &gt; then from your comments I don&#39;t see that it would be something that would be<br>
&gt; &gt; particularly difficult to change at a later date either.<br>
&gt; &gt;<br>
&gt; &gt; The things that are important to me are i) is the patchset functionally correct and<br>
&gt; &gt; ii) is it understandable and maintainable. I would say that the first point is<br>
&gt; &gt; clearly true (both myself and Howard have spent a lot of time testing it), and given<br>
&gt; &gt; that I had to delve into these patches to fix the R2 register issue on 32-bit PPC<br>
&gt; &gt; then I can confirm that the contents of the patches were a reasonably accurate<br>
&gt; &gt; representation of the changes described within. And that&#39;s from someone like me who<br>
&gt; &gt; is mostly still a TCG beginner :)<br>
&gt; &gt;<br>
&gt; &gt; From a slightly more selfish position as the PPC Mac machine maintainer, these<br>
&gt; &gt; patches make a significant difference to me in that they reduce the MacOS boot times<br>
&gt; &gt; during everyday testing. Now for someone like myself who works on QEMU as a hobby<br>
&gt; &gt; outside of family life and a full time job, those few minutes are really important to<br>
&gt; &gt; me and soon add up really quickly during testing.<br>
&gt; &gt;<br>
&gt; &gt; I would really like these patches to be merged soon, since the worst thing that can<br>
&gt; &gt; happen is that the patchset ends up bit-rotting and then all the time and effort put<br>
&gt; &gt; into writing, testing and reviewing the patches by Richard, Howard, David, myself and<br>
&gt; &gt; indeed your review time will all end up going to waste.<br>
&gt; &gt;<br>
&gt; &gt;<br>
&gt; &gt; ATB,<br>
&gt; &gt;<br>
&gt; &gt; Mark.<br>
&gt; &gt;<br>
</p>