[Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[Mark Cave-Ayland]

This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
QEMU, the original version of which was posted to the qemu-devel list at the
end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).

The patchset consisted of some simple patches from Alex and then a large set of
CUDA changes supplied as a single patch which were the result of Cormac analysing
MOL with Alex's help to try and further the boot process.

In their previous form, the patches were unsuitable for applying upstream since
while they furthered MacOS 9 boot, they also caused a couple of major regressions
such as breaking the mouse and causing Darwin/OS X boot to panic on startup.

This reworked patchset fixes these regressions, includes some other clean-ups 
and more importantly now passes all of my OpenBIOS image boot tests with an 
OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
I've uploaded a pre-compiled binary to 
https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
new MacOS 9 functionality.

Apologies for the delay in sending this out on-list, however due to recent
circumstances I've been without a reliable broadband connection for a couple
of weeks. However given that this is mostly a rework of the previous patchset 
and looks good in testing here, I'd definitely like it to be considered for
application during soft freeze.

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

Alexander Graf (3):
  PPC: Allow Rc bit to be set on mtspr
  PPC: Fix lsxw bounds checks
  PPC: mac99: Always add USB controller

Mark Cave-Ayland (10):
  cuda.c: fix CUDA ADB error packet format
  cuda.c: fix CUDA_PACKET response packet format
  cuda.c: implement simple CUDA_GET_6805_ADDR command
  cuda.c: implement dummy IIC access commands
  cuda.c: fix CUDA SR interrupt clearing
  cuda.c: add defines for CUDA registers
  cuda.c: refactor get_tb() so that the time can be passed in
  cuda.c: rename get_counter() state variable from s to ti for
    consistency
  cuda.c: fix T2 timer and enable its interrupt
  cuda.c: add delay to setting of SR_INT bit

 hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
 hw/ppc/mac.h            |    3 +
 hw/ppc/mac_newworld.c   |    3 +-
 target-ppc/mem_helper.c |    5 +-
 target-ppc/translate.c  |    2 +-
 5 files changed, 163 insertions(+), 93 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH 01/13] PPC: Allow Rc bit to be set on mtspr

[Mark Cave-Ayland]

From: Alexander Graf <agraf@suse.de>

According to the ISA setting the Rc bit on mtspr is undefined behavior.
Real 750 hardware simply ignores the bit and doesn't touch cr0 though.

Unfortunately, Mac OS 9 relies on this fact and executes a few mtspr
instructions (to set XER for example) with Rc set.

So let's handle the bit the same way hardware does and ignore it.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-ppc/translate.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index c2bc1a7..d1f0f13 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -9884,7 +9884,7 @@ GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC),
 GEN_HANDLER(mtmsrd, 0x1F, 0x12, 0x05, 0x001EF801, PPC_64B),
 #endif
 GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001FF801, PPC_MISC),
-GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000001, PPC_MISC),
+GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000000, PPC_MISC),
 GEN_HANDLER(dcbf, 0x1F, 0x16, 0x02, 0x03C00001, PPC_CACHE),
 GEN_HANDLER(dcbi, 0x1F, 0x16, 0x0E, 0x03E00001, PPC_CACHE),
 GEN_HANDLER(dcbst, 0x1F, 0x16, 0x01, 0x03E00001, PPC_CACHE),
-- 
1.7.10.4

[Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Mark Cave-Ayland]

From: Alexander Graf <agraf@suse.de>

The lsxw instruction checks whether the desired string actually fits
into all defined registers. Unfortunately it does the calculation wrong,
resulting in illegal instruction traps for loads that really should fit.

Fix it up, making Mac OS happier.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target-ppc/mem_helper.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
index 6d37dae..7e1f234 100644
--- a/target-ppc/mem_helper.c
+++ b/target-ppc/mem_helper.c
@@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
                  uint32_t ra, uint32_t rb)
 {
     if (likely(xer_bc != 0)) {
-        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
-                     (reg < rb && (reg + xer_bc) > rb))) {
+        int num_used_regs = (xer_bc + 3) / 4;
+        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
+                     (reg < rb && (reg + num_used_regs) > rb))) {
             helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
                                        POWERPC_EXCP_INVAL |
                                        POWERPC_EXCP_INVAL_LSWX);
-- 
1.7.10.4

[Qemu-devel] [PATCH 03/13] PPC: mac99: Always add USB controller

[Mark Cave-Ayland]

From: Alexander Graf <agraf@suse.de>

The mac99 machines always have a USB controller. Usually not having one around
doesn't hurt quite as much, but Mac OS 9 really really wants one or it crashes
on bootup.

So always add OHCI to make it happy.

Signed-off-by: Alexander Graf <agraf@suse.de>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/mac_newworld.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 66d016c..1b9a573 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -371,12 +371,13 @@ static void ppc_core99_init(MachineState *machine)
         /* 970 gets a U3 bus */
         pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io());
         machine_arch = ARCH_MAC99_U3;
-        machine->usb |= defaults_enabled() && !machine->usb_disabled;
     } else {
         pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io());
         machine_arch = ARCH_MAC99;
     }
 
+    machine->usb |= defaults_enabled() && !machine->usb_disabled;
+
     /* Timebase Frequency */
     if (kvm_enabled()) {
         tbfreq = kvmppc_get_tbfreq();
-- 
1.7.10.4

[Qemu-devel] [PATCH 04/13] cuda.c: fix CUDA ADB error packet format

[Mark Cave-Ayland]

ADB error packets should be of the form (type, status, cmd) rather than just
(type, status). This fixes ADB device detection under MacOS 9.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 5d7043e..9ec19af 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -560,19 +560,21 @@ static void cuda_receive_packet_from_host(CUDAState *s,
     switch(data[0]) {
     case ADB_PACKET:
         {
-            uint8_t obuf[ADB_MAX_OUT_LEN + 2];
+            uint8_t obuf[ADB_MAX_OUT_LEN + 3];
             int olen;
             olen = adb_request(&s->adb_bus, obuf + 2, data + 1, len - 1);
             if (olen > 0) {
                 obuf[0] = ADB_PACKET;
                 obuf[1] = 0x00;
+                cuda_send_packet_to_host(s, obuf, olen + 2);
             } else {
                 /* error */
                 obuf[0] = ADB_PACKET;
                 obuf[1] = -olen;
+                obuf[2] = data[1];
                 olen = 0;
+                cuda_send_packet_to_host(s, obuf, olen + 3);
             }
-            cuda_send_packet_to_host(s, obuf, olen + 2);
         }
         break;
     case CUDA_PACKET:
-- 
1.7.10.4

[Qemu-devel] [PATCH 05/13] cuda.c: fix CUDA_PACKET response packet format

[Mark Cave-Ayland]

According to comments in MOL, the response to a CUDA_PACKET should be one of
the following:

Reply: CUDA_PACKET, status, cmd
Error: ERROR_PACKET, status, CUDA_PACKET, cmd

Update cuda_receive_packet() accordingly to reflect this.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 9ec19af..88a0999 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -480,7 +480,7 @@ static void cuda_adb_poll(void *opaque)
 static void cuda_receive_packet(CUDAState *s,
                                 const uint8_t *data, int len)
 {
-    uint8_t obuf[16];
+    uint8_t obuf[16] = { CUDA_PACKET, 0, data[0] };
     int autopoll;
     uint32_t ti;
 
@@ -497,23 +497,15 @@ static void cuda_receive_packet(CUDAState *s,
                 timer_del(s->adb_poll_timer);
             }
         }
-        obuf[0] = CUDA_PACKET;
-        obuf[1] = data[1];
-        cuda_send_packet_to_host(s, obuf, 2);
+        cuda_send_packet_to_host(s, obuf, 3);
         break;
     case CUDA_SET_TIME:
         ti = (((uint32_t)data[1]) << 24) + (((uint32_t)data[2]) << 16) + (((uint32_t)data[3]) << 8) + data[4];
         s->tick_offset = ti - (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
-        obuf[0] = CUDA_PACKET;
-        obuf[1] = 0;
-        obuf[2] = 0;
         cuda_send_packet_to_host(s, obuf, 3);
         break;
     case CUDA_GET_TIME:
         ti = s->tick_offset + (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
-        obuf[0] = CUDA_PACKET;
-        obuf[1] = 0;
-        obuf[2] = 0;
         obuf[3] = ti >> 24;
         obuf[4] = ti >> 16;
         obuf[5] = ti >> 8;
@@ -524,20 +516,14 @@ static void cuda_receive_packet(CUDAState *s,
     case CUDA_SET_DEVICE_LIST:
     case CUDA_SET_AUTO_RATE:
     case CUDA_SET_POWER_MESSAGES:
-        obuf[0] = CUDA_PACKET;
-        obuf[1] = 0;
-        cuda_send_packet_to_host(s, obuf, 2);
+        cuda_send_packet_to_host(s, obuf, 3);
         break;
     case CUDA_POWERDOWN:
-        obuf[0] = CUDA_PACKET;
-        obuf[1] = 0;
-        cuda_send_packet_to_host(s, obuf, 2);
+        cuda_send_packet_to_host(s, obuf, 3);
         qemu_system_shutdown_request();
         break;
     case CUDA_RESET_SYSTEM:
-        obuf[0] = CUDA_PACKET;
-        obuf[1] = 0;
-        cuda_send_packet_to_host(s, obuf, 2);
+        cuda_send_packet_to_host(s, obuf, 3);
         qemu_system_reset_request();
         break;
     default:
-- 
1.7.10.4

[Qemu-devel] [PATCH 06/13] cuda.c: implement simple CUDA_GET_6805_ADDR command

[Mark Cave-Ayland]

This simply returns an empty response with no error status.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 88a0999..4fe901b 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -499,6 +499,9 @@ static void cuda_receive_packet(CUDAState *s,
         }
         cuda_send_packet_to_host(s, obuf, 3);
         break;
+    case CUDA_GET_6805_ADDR:
+        cuda_send_packet_to_host(s, obuf, 3);
+        break;
     case CUDA_SET_TIME:
         ti = (((uint32_t)data[1]) << 24) + (((uint32_t)data[2]) << 16) + (((uint32_t)data[3]) << 8) + data[4];
         s->tick_offset = ti - (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
-- 
1.7.10.4

[Qemu-devel] [PATCH 07/13] cuda.c: implement dummy IIC access commands

[Mark Cave-Ayland]

These are used by MacOS 9 on boot. Here we return an error except for 4-byte
commands which write to the IIC bus.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 4fe901b..d3ec58a 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -529,6 +529,24 @@ static void cuda_receive_packet(CUDAState *s,
         cuda_send_packet_to_host(s, obuf, 3);
         qemu_system_reset_request();
         break;
+    case CUDA_COMBINED_FORMAT_IIC:
+        obuf[0] = ERROR_PACKET;
+        obuf[1] = 0x5;
+        obuf[2] = CUDA_PACKET;
+        obuf[3] = data[0];
+        cuda_send_packet_to_host(s, obuf, 4);
+        break;
+    case CUDA_GET_SET_IIC:
+        if (len == 4) {
+            cuda_send_packet_to_host(s, obuf, 3);
+        } else {
+            obuf[0] = ERROR_PACKET;
+            obuf[1] = 0x2;
+            obuf[2] = CUDA_PACKET;
+            obuf[3] = data[0];
+            cuda_send_packet_to_host(s, obuf, 4);
+        }
+        break;
     default:
         break;
     }
-- 
1.7.10.4

[Qemu-devel] [PATCH 08/13] cuda.c: fix CUDA SR interrupt clearing

[Mark Cave-Ayland]

Make sure that we also clear the data and clock interrupts at the same time.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index d3ec58a..4027713 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -57,6 +57,8 @@
 #define IER_SET		0x80		/* set bits in IER */
 #define IER_CLR		0		/* clear bits in IER */
 #define SR_INT		0x04		/* Shift register full/empty */
+#define SR_DATA_INT	0x08
+#define SR_CLOCK_INT	0x10
 #define T1_INT          0x40            /* Timer 1 interrupt */
 #define T2_INT          0x20            /* Timer 2 interrupt */
 
@@ -261,7 +263,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
         break;
     case 10:
         val = s->sr;
-        s->ifr &= ~SR_INT;
+        s->ifr &= ~(SR_INT | SR_CLOCK_INT | SR_DATA_INT);
         cuda_update_irq(s);
         break;
     case 11:
-- 
1.7.10.4

[Qemu-devel] [PATCH 09/13] cuda.c: add defines for CUDA registers

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   87 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 34 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 4027713..d32afc6 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -110,6 +110,24 @@
 /* CUDA returns time_t's offset from Jan 1, 1904, not 1970 */
 #define RTC_OFFSET                      2082844800
 
+/* CUDA registers */
+#define CUDA_REG_B       0x00
+#define CUDA_REG_A       0x01
+#define CUDA_REG_DIRB    0x02
+#define CUDA_REG_DIRA    0x03
+#define CUDA_REG_T1CL    0x04
+#define CUDA_REG_T1CH    0x05
+#define CUDA_REG_T1LL    0x06
+#define CUDA_REG_T1LH    0x07
+#define CUDA_REG_T2CL    0x08
+#define CUDA_REG_T2CH    0x09
+#define CUDA_REG_SR      0x0a
+#define CUDA_REG_ACR     0x0b
+#define CUDA_REG_PCR     0x0c
+#define CUDA_REG_IFR     0x0d
+#define CUDA_REG_IER     0x0e
+#define CUDA_REG_ANH     0x0f
+
 static void cuda_update(CUDAState *s);
 static void cuda_receive_packet_from_host(CUDAState *s,
                                           const uint8_t *data, int len);
@@ -226,66 +244,67 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
 
     addr = (addr >> 9) & 0xf;
     switch(addr) {
-    case 0:
+    case CUDA_REG_B:
         val = s->b;
         break;
-    case 1:
+    case CUDA_REG_A:
         val = s->a;
         break;
-    case 2:
+    case CUDA_REG_DIRB:
         val = s->dirb;
         break;
-    case 3:
+    case CUDA_REG_DIRA:
         val = s->dira;
         break;
-    case 4:
+    case CUDA_REG_T1CL:
         val = get_counter(&s->timers[0]) & 0xff;
         s->ifr &= ~T1_INT;
         cuda_update_irq(s);
         break;
-    case 5:
+    case CUDA_REG_T1CH:
         val = get_counter(&s->timers[0]) >> 8;
         cuda_update_irq(s);
         break;
-    case 6:
+    case CUDA_REG_T1LL:
         val = s->timers[0].latch & 0xff;
         break;
-    case 7:
+    case CUDA_REG_T1LH:
         /* XXX: check this */
         val = (s->timers[0].latch >> 8) & 0xff;
         break;
-    case 8:
+    case CUDA_REG_T2CL:
         val = get_counter(&s->timers[1]) & 0xff;
         s->ifr &= ~T2_INT;
         break;
-    case 9:
+    case CUDA_REG_T2CH:
         val = get_counter(&s->timers[1]) >> 8;
         break;
-    case 10:
+    case CUDA_REG_SR:
         val = s->sr;
         s->ifr &= ~(SR_INT | SR_CLOCK_INT | SR_DATA_INT);
         cuda_update_irq(s);
         break;
-    case 11:
+    case CUDA_REG_ACR:
         val = s->acr;
         break;
-    case 12:
+    case CUDA_REG_PCR:
         val = s->pcr;
         break;
-    case 13:
+    case CUDA_REG_IFR:
         val = s->ifr;
-        if (s->ifr & s->ier)
+        if (s->ifr & s->ier) {
             val |= 0x80;
+        }
         break;
-    case 14:
+    case CUDA_REG_IER:
         val = s->ier | 0x80;
         break;
     default:
-    case 15:
+    case CUDA_REG_ANH:
         val = s->anh;
         break;
     }
-    if (addr != 13 || val != 0) {
+    if (addr != CUDA_REG_IFR || val != 0) {
         CUDA_DPRINTF("read: reg=0x%x val=%02x\n", (int)addr, val);
     }
 
@@ -300,61 +319,61 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
     CUDA_DPRINTF("write: reg=0x%x val=%02x\n", (int)addr, val);
 
     switch(addr) {
-    case 0:
+    case CUDA_REG_B:
         s->b = val;
         cuda_update(s);
         break;
-    case 1:
+    case CUDA_REG_A:
         s->a = val;
         break;
-    case 2:
+    case CUDA_REG_DIRB:
         s->dirb = val;
         break;
-    case 3:
+    case CUDA_REG_DIRA:
         s->dira = val;
         break;
-    case 4:
+    case CUDA_REG_T1CL:
         s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
         cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         break;
-    case 5:
+    case CUDA_REG_T1CH:
         s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
         s->ifr &= ~T1_INT;
         set_counter(s, &s->timers[0], s->timers[0].latch);
         break;
-    case 6:
+    case CUDA_REG_T1LL:
         s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
         cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         break;
-    case 7:
+    case CUDA_REG_T1LH:
         s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
         s->ifr &= ~T1_INT;
         cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         break;
-    case 8:
+    case CUDA_REG_T2CL:
         s->timers[1].latch = val;
         set_counter(s, &s->timers[1], val);
         break;
-    case 9:
+    case CUDA_REG_T2CH:
         set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
         break;
-    case 10:
+    case CUDA_REG_SR:
         s->sr = val;
         break;
-    case 11:
+    case CUDA_REG_ACR:
         s->acr = val;
         cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         cuda_update(s);
         break;
-    case 12:
+    case CUDA_REG_PCR:
         s->pcr = val;
         break;
-    case 13:
+    case CUDA_REG_IFR:
         /* reset bits */
         s->ifr &= ~val;
         cuda_update_irq(s);
         break;
-    case 14:
+    case CUDA_REG_IER:
         if (val & IER_SET) {
             /* set bits */
             s->ier |= val & 0x7f;
@@ -365,7 +384,7 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
         cuda_update_irq(s);
         break;
     default:
-    case 15:
+    case CUDA_REG_ANH:
         s->anh = val;
         break;
     }
-- 
1.7.10.4

[Qemu-devel] [PATCH 10/13] cuda.c: refactor get_tb() so that the time can be passed in

[Mark Cave-Ayland]

This is in preparation for sharing the code between timers.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index d32afc6..7a1b10b 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -143,10 +143,9 @@ static void cuda_update_irq(CUDAState *s)
     }
 }
 
-static uint64_t get_tb(uint64_t freq)
+static uint64_t get_tb(uint64_t time, uint64_t freq)
 {
-    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                    freq, get_ticks_per_sec());
+    return muldiv64(time, freq, get_ticks_per_sec());
 }
 
 static unsigned int get_counter(CUDATimer *s)
@@ -154,9 +153,10 @@ static unsigned int get_counter(CUDATimer *s)
     int64_t d;
     unsigned int counter;
     uint64_t tb_diff;
+    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup. */
-    tb_diff = get_tb(s->frequency) - s->load_time;
+    tb_diff = get_tb(current_time, s->frequency) - s->load_time;
     d = (tb_diff * 0xBF401675E5DULL) / (s->frequency << 24);
 
     if (s->index == 0) {
@@ -176,7 +176,8 @@ static unsigned int get_counter(CUDATimer *s)
 static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
 {
     CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
-    ti->load_time = get_tb(s->frequency);
+    ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                           s->frequency);
     ti->counter_value = val;
     cuda_timer_update(s, ti, ti->load_time);
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 11/13] cuda.c: rename get_counter() state variable from s to ti for consistency

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 7a1b10b..687cb54 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -148,7 +148,7 @@ static uint64_t get_tb(uint64_t time, uint64_t freq)
     return muldiv64(time, freq, get_ticks_per_sec());
 }
 
-static unsigned int get_counter(CUDATimer *s)
+static unsigned int get_counter(CUDATimer *ti)
 {
     int64_t d;
     unsigned int counter;
@@ -156,19 +156,19 @@ static unsigned int get_counter(CUDATimer *s)
     uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
 
     /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup. */
-    tb_diff = get_tb(current_time, s->frequency) - s->load_time;
-    d = (tb_diff * 0xBF401675E5DULL) / (s->frequency << 24);
+    tb_diff = get_tb(current_time, ti->frequency) - ti->load_time;
+    d = (tb_diff * 0xBF401675E5DULL) / (ti->frequency << 24);
 
-    if (s->index == 0) {
+    if (ti->index == 0) {
         /* the timer goes down from latch to -1 (period of latch + 2) */
-        if (d <= (s->counter_value + 1)) {
-            counter = (s->counter_value - d) & 0xffff;
+        if (d <= (ti->counter_value + 1)) {
+            counter = (ti->counter_value - d) & 0xffff;
         } else {
-            counter = (d - (s->counter_value + 1)) % (s->latch + 2);
-            counter = (s->latch - counter) & 0xffff;
+            counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
+            counter = (ti->latch - counter) & 0xffff;
         }
     } else {
-        counter = (s->counter_value - d) & 0xffff;
+        counter = (ti->counter_value - d) & 0xffff;
     }
     return counter;
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 12/13] cuda.c: fix T2 timer and enable its interrupt

[Mark Cave-Ayland]

Fix the counter loading logic and enable the T2 interrupt when the timer
expires.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 687cb54..d864b24 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -136,7 +136,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
 
 static void cuda_update_irq(CUDAState *s)
 {
-    if (s->ifr & s->ier & (SR_INT | T1_INT)) {
+    if (s->ifr & s->ier & (SR_INT | T1_INT | T2_INT)) {
         qemu_irq_raise(s->irq);
     } else {
         qemu_irq_lower(s->irq);
@@ -175,7 +175,7 @@ static unsigned int get_counter(CUDATimer *ti)
 
 static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
 {
-    CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
+    CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
     ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
                            s->frequency);
     ti->counter_value = val;
@@ -220,7 +220,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
 {
     if (!ti->timer)
         return;
-    if ((s->acr & T1MODE) != T1MODE_CONT) {
+    if (ti->index == 0 && (s->acr & T1MODE) != T1MODE_CONT) {
         timer_del(ti->timer);
     } else {
         ti->next_irq_time = get_next_irq_time(ti, current_time);
@@ -238,6 +238,16 @@ static void cuda_timer1(void *opaque)
     cuda_update_irq(s);
 }
 
+static void cuda_timer2(void *opaque)
+{
+    CUDAState *s = opaque;
+    CUDATimer *ti = &s->timers[1];
+
+    cuda_timer_update(s, ti, ti->next_irq_time);
+    s->ifr |= T2_INT;
+    cuda_update_irq(s);
+}
+
 static uint32_t cuda_readb(void *opaque, hwaddr addr)
 {
     CUDAState *s = opaque;
@@ -276,6 +286,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
     case CUDA_REG_T2CL:
         val = get_counter(&s->timers[1]) & 0xff;
         s->ifr &= ~T2_INT;
+        cuda_update_irq(s);
         break;
     case CUDA_REG_T2CH:
         val = get_counter(&s->timers[1]) >> 8;
@@ -352,11 +363,12 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
         cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
         break;
     case CUDA_REG_T2CL:
-        s->timers[1].latch = val;
-        set_counter(s, &s->timers[1], val);
+        s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
         break;
     case CUDA_REG_T2CH:
-        set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
+        s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
+        s->ifr &= ~T2_INT;
+        set_counter(s, &s->timers[1], s->timers[1].latch);
         break;
     case CUDA_REG_SR:
         s->sr = val;
@@ -719,8 +731,7 @@ static void cuda_reset(DeviceState *dev)
     s->timers[0].latch = 0xffff;
     set_counter(s, &s->timers[0], 0xffff);
 
-    s->timers[1].latch = 0;
-    set_counter(s, &s->timers[1], 0xffff);
+    s->timers[1].latch = 0xffff;
 }
 
 static void cuda_realizefn(DeviceState *dev, Error **errp)
@@ -730,7 +741,8 @@ static void cuda_realizefn(DeviceState *dev, Error **errp)
 
     s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer1, s);
     s->timers[0].frequency = s->frequency;
-    s->timers[1].frequency = s->frequency;
+    s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer2, s);
+    s->timers[1].frequency = (SCALE_US * 6000) / 4700;
 
     qemu_get_timedate(&tm, 0);
     s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
-- 
1.7.10.4

[Qemu-devel] [PATCH 13/13] cuda.c: add delay to setting of SR_INT bit

[Mark Cave-Ayland]

MacOS 9 is racy when it comes to accessing the shift register. Fix this by
introducing a small delay between data accesses and raising the SR_INT
interrupt bit.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/cuda.c |   44 +++++++++++++++++++++++++++++++++-----------
 hw/ppc/mac.h         |    3 +++
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index d864b24..5156c72 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -248,6 +248,31 @@ static void cuda_timer2(void *opaque)
     cuda_update_irq(s);
 }
 
+static void cuda_set_sr_int(void *opaque)
+{
+    CUDAState *s = opaque;
+
+    CUDA_DPRINTF("CUDA: %s:%d\n", __func__, __LINE__);
+    s->ifr |= SR_INT;
+    cuda_update_irq(s);
+}
+
+static void cuda_delay_set_sr_int(CUDAState *s)
+{
+    int64_t expire;
+
+    if (s->dirb == 0xff) {
+        /* Not in Mac OS, fire the IRQ directly */
+        cuda_set_sr_int(s);
+        return;
+    }
+
+    CUDA_DPRINTF("CUDA: %s:%d\n", __func__, __LINE__);
+
+    expire = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 300 * SCALE_US;
+    timer_mod(s->sr_delay_timer, expire);
+}
+
 static uint32_t cuda_readb(void *opaque, hwaddr addr)
 {
     CUDAState *s = opaque;
@@ -418,8 +443,7 @@ static void cuda_update(CUDAState *s)
                 if (s->data_out_index < sizeof(s->data_out)) {
                     CUDA_DPRINTF("send: %02x\n", s->sr);
                     s->data_out[s->data_out_index++] = s->sr;
-                    s->ifr |= SR_INT;
-                    cuda_update_irq(s);
+                    cuda_delay_set_sr_int(s);
                 }
             }
         } else {
@@ -432,8 +456,7 @@ static void cuda_update(CUDAState *s)
                     if (s->data_in_index >= s->data_in_size) {
                         s->b = (s->b | TREQ);
                     }
-                    s->ifr |= SR_INT;
-                    cuda_update_irq(s);
+                    cuda_delay_set_sr_int(s);
                 }
             }
         }
@@ -445,15 +468,13 @@ static void cuda_update(CUDAState *s)
                 s->b = (s->b | TREQ);
             else
                 s->b = (s->b & ~TREQ);
-            s->ifr |= SR_INT;
-            cuda_update_irq(s);
+            cuda_delay_set_sr_int(s);
         } else {
             if (!(s->last_b & TIP)) {
                 /* handle end of host to cuda transfer */
                 packet_received = (s->data_out_index > 0);
                 /* always an IRQ at the end of transfer */
-                s->ifr |= SR_INT;
-                cuda_update_irq(s);
+                cuda_delay_set_sr_int(s);
             }
             /* signal if there is data to read */
             if (s->data_in_index < s->data_in_size) {
@@ -490,8 +511,7 @@ static void cuda_send_packet_to_host(CUDAState *s,
     s->data_in_size = len;
     s->data_in_index = 0;
     cuda_update(s);
-    s->ifr |= SR_INT;
-    cuda_update_irq(s);
+    cuda_delay_set_sr_int(s);
 }
 
 static void cuda_adb_poll(void *opaque)
@@ -714,7 +734,7 @@ static void cuda_reset(DeviceState *dev)
 
     s->b = 0;
     s->a = 0;
-    s->dirb = 0;
+    s->dirb = 0xff;
     s->dira = 0;
     s->sr = 0;
     s->acr = 0;
@@ -732,6 +752,8 @@ static void cuda_reset(DeviceState *dev)
     set_counter(s, &s->timers[0], 0xffff);
 
     s->timers[1].latch = 0xffff;
+
+    s->sr_delay_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_set_sr_int, s);
 }
 
 static void cuda_realizefn(DeviceState *dev, Error **errp)
diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index 8bdba30..e375ed2 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -103,6 +103,9 @@ typedef struct CUDAState {
     uint8_t last_b;
     uint8_t last_acr;
 
+    /* MacOS 9 is racy and requires a delay upon setting the SR_INT bit */
+    QEMUTimer *sr_delay_timer;
+
     int data_in_size;
     int data_in_index;
     int data_out_index;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[Mark Cave-Ayland]

On 23/10/15 14:56, Mark Cave-Ayland wrote:

> This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
> QEMU, the original version of which was posted to the qemu-devel list at the
> end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
> 
> The patchset consisted of some simple patches from Alex and then a large set of
> CUDA changes supplied as a single patch which were the result of Cormac analysing
> MOL with Alex's help to try and further the boot process.
> 
> In their previous form, the patches were unsuitable for applying upstream since
> while they furthered MacOS 9 boot, they also caused a couple of major regressions
> such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
> 
> This reworked patchset fixes these regressions, includes some other clean-ups 
> and more importantly now passes all of my OpenBIOS image boot tests with an 
> OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
> Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
> I've uploaded a pre-compiled binary to 
> https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
> new MacOS 9 functionality.
> 
> Apologies for the delay in sending this out on-list, however due to recent
> circumstances I've been without a reliable broadband connection for a couple
> of weeks. However given that this is mostly a rework of the previous patchset 
> and looks good in testing here, I'd definitely like it to be considered for
> application during soft freeze.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Alexander Graf (3):
>   PPC: Allow Rc bit to be set on mtspr
>   PPC: Fix lsxw bounds checks
>   PPC: mac99: Always add USB controller
> 
> Mark Cave-Ayland (10):
>   cuda.c: fix CUDA ADB error packet format
>   cuda.c: fix CUDA_PACKET response packet format
>   cuda.c: implement simple CUDA_GET_6805_ADDR command
>   cuda.c: implement dummy IIC access commands
>   cuda.c: fix CUDA SR interrupt clearing
>   cuda.c: add defines for CUDA registers
>   cuda.c: refactor get_tb() so that the time can be passed in
>   cuda.c: rename get_counter() state variable from s to ti for
>     consistency
>   cuda.c: fix T2 timer and enable its interrupt
>   cuda.c: add delay to setting of SR_INT bit
> 
>  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
>  hw/ppc/mac.h            |    3 +
>  hw/ppc/mac_newworld.c   |    3 +-
>  target-ppc/mem_helper.c |    5 +-
>  target-ppc/translate.c  |    2 +-
>  5 files changed, 163 insertions(+), 93 deletions(-)

Ping? Can anyone review this in Alex's absence? In the meantime I've
added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
good to get the GSoC work upstream for 2.5.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 01/13] PPC: Allow Rc bit to be set on mtspr

[Thomas Huth]

On 23/10/15 15:56, Mark Cave-Ayland wrote:
> From: Alexander Graf <agraf@suse.de>
> 
> According to the ISA setting the Rc bit on mtspr is undefined behavior.
> Real 750 hardware simply ignores the bit and doesn't touch cr0 though.

According to PowerISA 2.07, chapter 1.1.3:

"Reserved fields in instructions are ignored by the pro-
cessor."

And the lowest bit of the mtspr opcode is marked as reserved. So I think
this is not just a hack, but even a proper fix.

> Unfortunately, Mac OS 9 relies on this fact and executes a few mtspr
> instructions (to set XER for example) with Rc set.
> 
> So let's handle the bit the same way hardware does and ignore it.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target-ppc/translate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index c2bc1a7..d1f0f13 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -9884,7 +9884,7 @@ GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC),
>  GEN_HANDLER(mtmsrd, 0x1F, 0x12, 0x05, 0x001EF801, PPC_64B),
>  #endif
>  GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001FF801, PPC_MISC),
> -GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000001, PPC_MISC),
> +GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000000, PPC_MISC),
>  GEN_HANDLER(dcbf, 0x1F, 0x16, 0x02, 0x03C00001, PPC_CACHE),
>  GEN_HANDLER(dcbi, 0x1F, 0x16, 0x0E, 0x03E00001, PPC_CACHE),
>  GEN_HANDLER(dcbst, 0x1F, 0x16, 0x01, 0x03E00001, PPC_CACHE),

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Thomas Huth]

On 23/10/15 15:56, Mark Cave-Ayland wrote:
> From: Alexander Graf <agraf@suse.de>
> 
> The lsxw instruction checks whether the desired string actually fits
> into all defined registers. Unfortunately it does the calculation wrong,
> resulting in illegal instruction traps for loads that really should fit.

s/lsxw/lswx/ in the above text and in the title ... but I guess this
could also be done when this patch gets picked up.

> Fix it up, making Mac OS happier.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target-ppc/mem_helper.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
> index 6d37dae..7e1f234 100644
> --- a/target-ppc/mem_helper.c
> +++ b/target-ppc/mem_helper.c
> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
>                   uint32_t ra, uint32_t rb)
>  {
>      if (likely(xer_bc != 0)) {
> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
> -                     (reg < rb && (reg + xer_bc) > rb))) {
> +        int num_used_regs = (xer_bc + 3) / 4;
> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
> +                     (reg < rb && (reg + num_used_regs) > rb))) {
>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>                                         POWERPC_EXCP_INVAL |
>                                         POWERPC_EXCP_INVAL_LSWX);

The calculation of num_used_regs looks fine to me ... but is the
remaining part of the condition really right already?

According to the PowerISA:

 If RA or RB is in the range of registers to be loaded,
 including the case in which RA=0, the instruction is
 treated as if the instruction form were invalid. If RT=RA
 or RT=RB, the instruction form is invalid.

So I wonder whether the check for "ra != 0" is really necessary here?
Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
ra"? And "reg <= rb", too, of course?

Also this code seems to completely ignore the case of the register
wrap-around, where the sequence of registers jumps back to r0 ...

So I'm basically fine with the num_used_regs fix for now, but I think
this needs a big "FIXME" comment so that the remaining issues get
cleaned up later?

 Thomas

Re: [Qemu-devel] [PATCH 03/13] PPC: mac99: Always add USB controller

[Thomas Huth]

On 23/10/15 15:56, Mark Cave-Ayland wrote:
> From: Alexander Graf <agraf@suse.de>
> 
> The mac99 machines always have a USB controller. Usually not having one around
> doesn't hurt quite as much, but Mac OS 9 really really wants one or it crashes
> on bootup.
> 
> So always add OHCI to make it happy.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/ppc/mac_newworld.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 66d016c..1b9a573 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -371,12 +371,13 @@ static void ppc_core99_init(MachineState *machine)
>          /* 970 gets a U3 bus */
>          pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io());
>          machine_arch = ARCH_MAC99_U3;
> -        machine->usb |= defaults_enabled() && !machine->usb_disabled;
>      } else {
>          pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io());
>          machine_arch = ARCH_MAC99;
>      }
>  
> +    machine->usb |= defaults_enabled() && !machine->usb_disabled;
> +
>      /* Timebase Frequency */
>      if (kvm_enabled()) {
>          tbfreq = kvmppc_get_tbfreq();
> 

According to https://en.wikipedia.org/wiki/New_World_ROM :

"The simplest way to distinguish a New World ROM Mac is that it will
have a factory built-in USB port."

... so it seems to me that this is right.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Mark Cave-Ayland]

On 03/11/15 15:23, Thomas Huth wrote:

> On 23/10/15 15:56, Mark Cave-Ayland wrote:
>> From: Alexander Graf <agraf@suse.de>
>>
>> The lsxw instruction checks whether the desired string actually fits
>> into all defined registers. Unfortunately it does the calculation wrong,
>> resulting in illegal instruction traps for loads that really should fit.
> 
> s/lsxw/lswx/ in the above text and in the title ... but I guess this
> could also be done when this patch gets picked up.
> 
>> Fix it up, making Mac OS happier.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  target-ppc/mem_helper.c |    5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>> index 6d37dae..7e1f234 100644
>> --- a/target-ppc/mem_helper.c
>> +++ b/target-ppc/mem_helper.c
>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
>>                   uint32_t ra, uint32_t rb)
>>  {
>>      if (likely(xer_bc != 0)) {
>> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
>> -                     (reg < rb && (reg + xer_bc) > rb))) {
>> +        int num_used_regs = (xer_bc + 3) / 4;
>> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
>> +                     (reg < rb && (reg + num_used_regs) > rb))) {
>>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>>                                         POWERPC_EXCP_INVAL |
>>                                         POWERPC_EXCP_INVAL_LSWX);
> 
> The calculation of num_used_regs looks fine to me ... but is the
> remaining part of the condition really right already?
> 
> According to the PowerISA:
> 
>  If RA or RB is in the range of registers to be loaded,
>  including the case in which RA=0, the instruction is
>  treated as if the instruction form were invalid. If RT=RA
>  or RT=RB, the instruction form is invalid.
> 
> So I wonder whether the check for "ra != 0" is really necessary here?
> Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
> ra"? And "reg <= rb", too, of course?
> 
> Also this code seems to completely ignore the case of the register
> wrap-around, where the sequence of registers jumps back to r0 ...
> 
> So I'm basically fine with the num_used_regs fix for now, but I think
> this needs a big "FIXME" comment so that the remaining issues get
> cleaned up later?

This was one of Alex's patches that was originally queued for ppc-next
before being dropped for missing the SoB, so I was expecting review to
find issues with other patches in the set rather than this one...

Having said that, I'm not sure whether this was deliberate for
compatibility reasons or just an oversight. Unless David has any ideas
it might be that we have to wait for Alex to return before this series
can be included, but thanks for the review anyhow :)


ATB,

Mark.

Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Thomas Huth]

On 03/11/15 20:21, Mark Cave-Ayland wrote:
> On 03/11/15 15:23, Thomas Huth wrote:
> 
>> On 23/10/15 15:56, Mark Cave-Ayland wrote:
>>> From: Alexander Graf <agraf@suse.de>
>>>
>>> The lsxw instruction checks whether the desired string actually fits
>>> into all defined registers. Unfortunately it does the calculation wrong,
>>> resulting in illegal instruction traps for loads that really should fit.
>>
>> s/lsxw/lswx/ in the above text and in the title ... but I guess this
>> could also be done when this patch gets picked up.
>>
>>> Fix it up, making Mac OS happier.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>  target-ppc/mem_helper.c |    5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>>> index 6d37dae..7e1f234 100644
>>> --- a/target-ppc/mem_helper.c
>>> +++ b/target-ppc/mem_helper.c
>>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
>>>                   uint32_t ra, uint32_t rb)
>>>  {
>>>      if (likely(xer_bc != 0)) {
>>> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
>>> -                     (reg < rb && (reg + xer_bc) > rb))) {
>>> +        int num_used_regs = (xer_bc + 3) / 4;
>>> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
>>> +                     (reg < rb && (reg + num_used_regs) > rb))) {
>>>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>>>                                         POWERPC_EXCP_INVAL |
>>>                                         POWERPC_EXCP_INVAL_LSWX);
>>
>> The calculation of num_used_regs looks fine to me ... but is the
>> remaining part of the condition really right already?
>>
>> According to the PowerISA:
>>
>>  If RA or RB is in the range of registers to be loaded,
>>  including the case in which RA=0, the instruction is
>>  treated as if the instruction form were invalid. If RT=RA
>>  or RT=RB, the instruction form is invalid.
>>
>> So I wonder whether the check for "ra != 0" is really necessary here?
>> Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
>> ra"? And "reg <= rb", too, of course?
>>
>> Also this code seems to completely ignore the case of the register
>> wrap-around, where the sequence of registers jumps back to r0 ...
>>
>> So I'm basically fine with the num_used_regs fix for now, but I think
>> this needs a big "FIXME" comment so that the remaining issues get
>> cleaned up later?
> 
> This was one of Alex's patches that was originally queued for ppc-next
> before being dropped for missing the SoB, so I was expecting review to
> find issues with other patches in the set rather than this one...
> 
> Having said that, I'm not sure whether this was deliberate for
> compatibility reasons or just an oversight. Unless David has any ideas
> it might be that we have to wait for Alex to return before this series
> can be included, but thanks for the review anyhow :)

Well, as I said, I'm basically fine with the patch, since it fixes one
bug and the fix itself also looks fine. It's just that the surrounding
code looks like it contains some more bugs... but these could also be
fixed with a later patch, I guess.

 Thomas

Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[Mark Cave-Ayland]

On 03/11/15 21:03, Thomas Huth wrote:

> On 03/11/15 20:21, Mark Cave-Ayland wrote:
>> On 03/11/15 15:23, Thomas Huth wrote:
>>
>>> On 23/10/15 15:56, Mark Cave-Ayland wrote:
>>>> From: Alexander Graf <agraf@suse.de>
>>>>
>>>> The lsxw instruction checks whether the desired string actually fits
>>>> into all defined registers. Unfortunately it does the calculation wrong,
>>>> resulting in illegal instruction traps for loads that really should fit.
>>>
>>> s/lsxw/lswx/ in the above text and in the title ... but I guess this
>>> could also be done when this patch gets picked up.
>>>
>>>> Fix it up, making Mac OS happier.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  target-ppc/mem_helper.c |    5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
>>>> index 6d37dae..7e1f234 100644
>>>> --- a/target-ppc/mem_helper.c
>>>> +++ b/target-ppc/mem_helper.c
>>>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
>>>>                   uint32_t ra, uint32_t rb)
>>>>  {
>>>>      if (likely(xer_bc != 0)) {
>>>> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
>>>> -                     (reg < rb && (reg + xer_bc) > rb))) {
>>>> +        int num_used_regs = (xer_bc + 3) / 4;
>>>> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
>>>> +                     (reg < rb && (reg + num_used_regs) > rb))) {
>>>>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
>>>>                                         POWERPC_EXCP_INVAL |
>>>>                                         POWERPC_EXCP_INVAL_LSWX);
>>>
>>> The calculation of num_used_regs looks fine to me ... but is the
>>> remaining part of the condition really right already?
>>>
>>> According to the PowerISA:
>>>
>>>  If RA or RB is in the range of registers to be loaded,
>>>  including the case in which RA=0, the instruction is
>>>  treated as if the instruction form were invalid. If RT=RA
>>>  or RT=RB, the instruction form is invalid.
>>>
>>> So I wonder whether the check for "ra != 0" is really necessary here?
>>> Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
>>> ra"? And "reg <= rb", too, of course?
>>>
>>> Also this code seems to completely ignore the case of the register
>>> wrap-around, where the sequence of registers jumps back to r0 ...
>>>
>>> So I'm basically fine with the num_used_regs fix for now, but I think
>>> this needs a big "FIXME" comment so that the remaining issues get
>>> cleaned up later?
>>
>> This was one of Alex's patches that was originally queued for ppc-next
>> before being dropped for missing the SoB, so I was expecting review to
>> find issues with other patches in the set rather than this one...
>>
>> Having said that, I'm not sure whether this was deliberate for
>> compatibility reasons or just an oversight. Unless David has any ideas
>> it might be that we have to wait for Alex to return before this series
>> can be included, but thanks for the review anyhow :)
> 
> Well, as I said, I'm basically fine with the patch, since it fixes one
> bug and the fix itself also looks fine. It's just that the surrounding
> code looks like it contains some more bugs... but these could also be
> fixed with a later patch, I guess.

Okay, great. I've fixed the comments in my local branch and will
resubmit on-list if David is happy with the changes.


Many thanks,

Mark.

Re: [Qemu-devel] [PATCH 01/13] PPC: Allow Rc bit to be set on mtspr

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]


On Fri, Oct 23, 2015 at 02:56:26PM +0100, Mark Cave-Ayland wrote:
> From: Alexander Graf <agraf@suse.de>
> 
> According to the ISA setting the Rc bit on mtspr is undefined behavior.
> Real 750 hardware simply ignores the bit and doesn't touch cr0 though.
> 
> Unfortunately, Mac OS 9 relies on this fact and executes a few mtspr
> instructions (to set XER for example) with Rc set.
> 
> So let's handle the bit the same way hardware does and ignore it.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target-ppc/translate.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index c2bc1a7..d1f0f13 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -9884,7 +9884,7 @@ GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x00000801, PPC_MISC),
>  GEN_HANDLER(mtmsrd, 0x1F, 0x12, 0x05, 0x001EF801, PPC_64B),
>  #endif
>  GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001FF801, PPC_MISC),
> -GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000001, PPC_MISC),
> +GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x00000000, PPC_MISC),
>  GEN_HANDLER(dcbf, 0x1F, 0x16, 0x02, 0x03C00001, PPC_CACHE),
>  GEN_HANDLER(dcbi, 0x1F, 0x16, 0x0E, 0x03E00001, PPC_CACHE),
>  GEN_HANDLER(dcbst, 0x1F, 0x16, 0x01, 0x03E00001, PPC_CACHE),

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 02/13] PPC: Fix lsxw bounds checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4196 bytes --]

On Tue, Nov 03, 2015 at 10:03:21PM +0100, Thomas Huth wrote:
> On 03/11/15 20:21, Mark Cave-Ayland wrote:
> > On 03/11/15 15:23, Thomas Huth wrote:
> > 
> >> On 23/10/15 15:56, Mark Cave-Ayland wrote:
> >>> From: Alexander Graf <agraf@suse.de>
> >>>
> >>> The lsxw instruction checks whether the desired string actually fits
> >>> into all defined registers. Unfortunately it does the calculation wrong,
> >>> resulting in illegal instruction traps for loads that really should fit.
> >>
> >> s/lsxw/lswx/ in the above text and in the title ... but I guess this
> >> could also be done when this patch gets picked up.
> >>
> >>> Fix it up, making Mac OS happier.
> >>>
> >>> Signed-off-by: Alexander Graf <agraf@suse.de>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>> ---
> >>>  target-ppc/mem_helper.c |    5 +++--
> >>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/target-ppc/mem_helper.c b/target-ppc/mem_helper.c
> >>> index 6d37dae..7e1f234 100644
> >>> --- a/target-ppc/mem_helper.c
> >>> +++ b/target-ppc/mem_helper.c
> >>> @@ -100,8 +100,9 @@ void helper_lswx(CPUPPCState *env, target_ulong addr, uint32_t reg,
> >>>                   uint32_t ra, uint32_t rb)
> >>>  {
> >>>      if (likely(xer_bc != 0)) {
> >>> -        if (unlikely((ra != 0 && reg < ra && (reg + xer_bc) > ra) ||
> >>> -                     (reg < rb && (reg + xer_bc) > rb))) {
> >>> +        int num_used_regs = (xer_bc + 3) / 4;
> >>> +        if (unlikely((ra != 0 && reg < ra && (reg + num_used_regs) > ra) ||
> >>> +                     (reg < rb && (reg + num_used_regs) > rb))) {
> >>>              helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
> >>>                                         POWERPC_EXCP_INVAL |
> >>>                                         POWERPC_EXCP_INVAL_LSWX);
> >>
> >> The calculation of num_used_regs looks fine to me ... but is the
> >> remaining part of the condition really right already?
> >>
> >> According to the PowerISA:
> >>
> >>  If RA or RB is in the range of registers to be loaded,
> >>  including the case in which RA=0, the instruction is
> >>  treated as if the instruction form were invalid. If RT=RA
> >>  or RT=RB, the instruction form is invalid.
> >>
> >> So I wonder whether the check for "ra != 0" is really necessary here?
> >> Also, shouldn't the code rather check for "reg <= ra" instead of "reg <
> >> ra"? And "reg <= rb", too, of course?
> >>
> >> Also this code seems to completely ignore the case of the register
> >> wrap-around, where the sequence of registers jumps back to r0 ...
> >>
> >> So I'm basically fine with the num_used_regs fix for now, but I think
> >> this needs a big "FIXME" comment so that the remaining issues get
> >> cleaned up later?
> > 
> > This was one of Alex's patches that was originally queued for ppc-next
> > before being dropped for missing the SoB, so I was expecting review to
> > find issues with other patches in the set rather than this one...
> > 
> > Having said that, I'm not sure whether this was deliberate for
> > compatibility reasons or just an oversight. Unless David has any ideas
> > it might be that we have to wait for Alex to return before this series
> > can be included, but thanks for the review anyhow :)
> 
> Well, as I said, I'm basically fine with the patch, since it fixes one
> bug and the fix itself also looks fine. It's just that the surrounding
> code looks like it contains some more bugs... but these could also be
> fixed with a later patch, I guess.

Right.  It does look like the ra != 0 check is unnnecessary, according
to the ISA.  I think it's clearer to change that in a separate patch,
though (and it will making things easier to deal with if we discover
some implementations *do* allow RT==RA==0 and there's software that
relies on it).

With the trivial fix to the title,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 03/13] PPC: mac99: Always add USB controller

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2000 bytes --]

On Tue, Nov 03, 2015 at 04:30:57PM +0100, Thomas Huth wrote:
> On 23/10/15 15:56, Mark Cave-Ayland wrote:
> > From: Alexander Graf <agraf@suse.de>
> > 
> > The mac99 machines always have a USB controller. Usually not having one around
> > doesn't hurt quite as much, but Mac OS 9 really really wants one or it crashes
> > on bootup.
> > 
> > So always add OHCI to make it happy.
> > 
> > Signed-off-by: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >  hw/ppc/mac_newworld.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > index 66d016c..1b9a573 100644
> > --- a/hw/ppc/mac_newworld.c
> > +++ b/hw/ppc/mac_newworld.c
> > @@ -371,12 +371,13 @@ static void ppc_core99_init(MachineState *machine)
> >          /* 970 gets a U3 bus */
> >          pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io());
> >          machine_arch = ARCH_MAC99_U3;
> > -        machine->usb |= defaults_enabled() && !machine->usb_disabled;
> >      } else {
> >          pci_bus = pci_pmac_init(pic, get_system_memory(), get_system_io());
> >          machine_arch = ARCH_MAC99;
> >      }
> >  
> > +    machine->usb |= defaults_enabled() && !machine->usb_disabled;
> > +
> >      /* Timebase Frequency */
> >      if (kvm_enabled()) {
> >          tbfreq = kvmppc_get_tbfreq();
> > 
> 
> According to https://en.wikipedia.org/wiki/New_World_ROM :
> 
> "The simplest way to distinguish a New World ROM Mac is that it will
> have a factory built-in USB port."
> 
> ... so it seems to me that this is right.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 04/13] cuda.c: fix CUDA ADB error packet format

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

On Fri, Oct 23, 2015 at 02:56:29PM +0100, Mark Cave-Ayland wrote:
> ADB error packets should be of the form (type, status, cmd) rather than just
> (type, status). This fixes ADB device detection under MacOS 9.

Hmm.. are there any public doc on these ADB / CUDA things that we can
look up to compare to?

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/macio/cuda.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 5d7043e..9ec19af 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -560,19 +560,21 @@ static void cuda_receive_packet_from_host(CUDAState *s,
>      switch(data[0]) {
>      case ADB_PACKET:
>          {
> -            uint8_t obuf[ADB_MAX_OUT_LEN + 2];
> +            uint8_t obuf[ADB_MAX_OUT_LEN + 3];
>              int olen;
>              olen = adb_request(&s->adb_bus, obuf + 2, data + 1, len - 1);
>              if (olen > 0) {
>                  obuf[0] = ADB_PACKET;
>                  obuf[1] = 0x00;
> +                cuda_send_packet_to_host(s, obuf, olen + 2);
>              } else {
>                  /* error */
>                  obuf[0] = ADB_PACKET;
>                  obuf[1] = -olen;
> +                obuf[2] = data[1];
>                  olen = 0;
> +                cuda_send_packet_to_host(s, obuf, olen + 3);

Using 'olen + 3' seems confusing here, rather than just '3', since you
just set olen = 0.

>              }
> -            cuda_send_packet_to_host(s, obuf, olen + 2);
>          }
>          break;
>      case CUDA_PACKET:

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 05/13] cuda.c: fix CUDA_PACKET response packet format

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3212 bytes --]

On Fri, Oct 23, 2015 at 02:56:30PM +0100, Mark Cave-Ayland wrote:
> According to comments in MOL, the response to a CUDA_PACKET should be one of
> the following:
> 
> Reply: CUDA_PACKET, status, cmd
> Error: ERROR_PACKET, status, CUDA_PACKET, cmd
> 
> Update cuda_receive_packet() accordingly to reflect this.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Code seems to match the description, but I don't have another source
to check what the right thing to do is for CUDA.

> ---
>  hw/misc/macio/cuda.c |   24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 9ec19af..88a0999 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -480,7 +480,7 @@ static void cuda_adb_poll(void *opaque)
>  static void cuda_receive_packet(CUDAState *s,
>                                  const uint8_t *data, int len)
>  {
> -    uint8_t obuf[16];
> +    uint8_t obuf[16] = { CUDA_PACKET, 0, data[0] };
>      int autopoll;
>      uint32_t ti;
>  
> @@ -497,23 +497,15 @@ static void cuda_receive_packet(CUDAState *s,
>                  timer_del(s->adb_poll_timer);
>              }
>          }
> -        obuf[0] = CUDA_PACKET;
> -        obuf[1] = data[1];
> -        cuda_send_packet_to_host(s, obuf, 2);
> +        cuda_send_packet_to_host(s, obuf, 3);
>          break;
>      case CUDA_SET_TIME:
>          ti = (((uint32_t)data[1]) << 24) + (((uint32_t)data[2]) << 16) + (((uint32_t)data[3]) << 8) + data[4];
>          s->tick_offset = ti - (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
> -        obuf[0] = CUDA_PACKET;
> -        obuf[1] = 0;
> -        obuf[2] = 0;
>          cuda_send_packet_to_host(s, obuf, 3);
>          break;
>      case CUDA_GET_TIME:
>          ti = s->tick_offset + (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
> -        obuf[0] = CUDA_PACKET;
> -        obuf[1] = 0;
> -        obuf[2] = 0;
>          obuf[3] = ti >> 24;
>          obuf[4] = ti >> 16;
>          obuf[5] = ti >> 8;
> @@ -524,20 +516,14 @@ static void cuda_receive_packet(CUDAState *s,
>      case CUDA_SET_DEVICE_LIST:
>      case CUDA_SET_AUTO_RATE:
>      case CUDA_SET_POWER_MESSAGES:
> -        obuf[0] = CUDA_PACKET;
> -        obuf[1] = 0;
> -        cuda_send_packet_to_host(s, obuf, 2);
> +        cuda_send_packet_to_host(s, obuf, 3);
>          break;
>      case CUDA_POWERDOWN:
> -        obuf[0] = CUDA_PACKET;
> -        obuf[1] = 0;
> -        cuda_send_packet_to_host(s, obuf, 2);
> +        cuda_send_packet_to_host(s, obuf, 3);
>          qemu_system_shutdown_request();
>          break;
>      case CUDA_RESET_SYSTEM:
> -        obuf[0] = CUDA_PACKET;
> -        obuf[1] = 0;
> -        cuda_send_packet_to_host(s, obuf, 2);
> +        cuda_send_packet_to_host(s, obuf, 3);
>          qemu_system_reset_request();
>          break;
>      default:

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 06/13] cuda.c: implement simple CUDA_GET_6805_ADDR command

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

On Fri, Oct 23, 2015 at 02:56:31PM +0100, Mark Cave-Ayland wrote:
> This simply returns an empty response with no error status.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Assuming the previous changes are correct,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>


> ---
>  hw/misc/macio/cuda.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 88a0999..4fe901b 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -499,6 +499,9 @@ static void cuda_receive_packet(CUDAState *s,
>          }
>          cuda_send_packet_to_host(s, obuf, 3);
>          break;
> +    case CUDA_GET_6805_ADDR:
> +        cuda_send_packet_to_host(s, obuf, 3);
> +        break;
>      case CUDA_SET_TIME:
>          ti = (((uint32_t)data[1]) << 24) + (((uint32_t)data[2]) << 16) + (((uint32_t)data[3]) << 8) + data[4];
>          s->tick_offset = ti - (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 07/13] cuda.c: implement dummy IIC access commands

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

On Fri, Oct 23, 2015 at 02:56:32PM +0100, Mark Cave-Ayland wrote:
> These are used by MacOS 9 on boot. Here we return an error except for 4-byte
> commands which write to the IIC bus.

A bit of rationale about why some of these give errors and some don't
would be nice.

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/macio/cuda.c |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 4fe901b..d3ec58a 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -529,6 +529,24 @@ static void cuda_receive_packet(CUDAState *s,
>          cuda_send_packet_to_host(s, obuf, 3);
>          qemu_system_reset_request();
>          break;
> +    case CUDA_COMBINED_FORMAT_IIC:
> +        obuf[0] = ERROR_PACKET;
> +        obuf[1] = 0x5;
> +        obuf[2] = CUDA_PACKET;
> +        obuf[3] = data[0];
> +        cuda_send_packet_to_host(s, obuf, 4);
> +        break;
> +    case CUDA_GET_SET_IIC:
> +        if (len == 4) {
> +            cuda_send_packet_to_host(s, obuf, 3);
> +        } else {
> +            obuf[0] = ERROR_PACKET;
> +            obuf[1] = 0x2;
> +            obuf[2] = CUDA_PACKET;
> +            obuf[3] = data[0];
> +            cuda_send_packet_to_host(s, obuf, 4);
> +        }
> +        break;
>      default:
>          break;
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 09/13] cuda.c: add defines for CUDA registers

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6588 bytes --]

On Fri, Oct 23, 2015 at 02:56:34PM +0100, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>


> ---
>  hw/misc/macio/cuda.c |   87 ++++++++++++++++++++++++++++++--------------------
>  1 file changed, 53 insertions(+), 34 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 4027713..d32afc6 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -110,6 +110,24 @@
>  /* CUDA returns time_t's offset from Jan 1, 1904, not 1970 */
>  #define RTC_OFFSET                      2082844800
>  
> +/* CUDA registers */
> +#define CUDA_REG_B       0x00
> +#define CUDA_REG_A       0x01
> +#define CUDA_REG_DIRB    0x02
> +#define CUDA_REG_DIRA    0x03
> +#define CUDA_REG_T1CL    0x04
> +#define CUDA_REG_T1CH    0x05
> +#define CUDA_REG_T1LL    0x06
> +#define CUDA_REG_T1LH    0x07
> +#define CUDA_REG_T2CL    0x08
> +#define CUDA_REG_T2CH    0x09
> +#define CUDA_REG_SR      0x0a
> +#define CUDA_REG_ACR     0x0b
> +#define CUDA_REG_PCR     0x0c
> +#define CUDA_REG_IFR     0x0d
> +#define CUDA_REG_IER     0x0e
> +#define CUDA_REG_ANH     0x0f
> +
>  static void cuda_update(CUDAState *s);
>  static void cuda_receive_packet_from_host(CUDAState *s,
>                                            const uint8_t *data, int len);
> @@ -226,66 +244,67 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
>  
>      addr = (addr >> 9) & 0xf;
>      switch(addr) {
> -    case 0:
> +    case CUDA_REG_B:
>          val = s->b;
>          break;
> -    case 1:
> +    case CUDA_REG_A:
>          val = s->a;
>          break;
> -    case 2:
> +    case CUDA_REG_DIRB:
>          val = s->dirb;
>          break;
> -    case 3:
> +    case CUDA_REG_DIRA:
>          val = s->dira;
>          break;
> -    case 4:
> +    case CUDA_REG_T1CL:
>          val = get_counter(&s->timers[0]) & 0xff;
>          s->ifr &= ~T1_INT;
>          cuda_update_irq(s);
>          break;
> -    case 5:
> +    case CUDA_REG_T1CH:
>          val = get_counter(&s->timers[0]) >> 8;
>          cuda_update_irq(s);
>          break;
> -    case 6:
> +    case CUDA_REG_T1LL:
>          val = s->timers[0].latch & 0xff;
>          break;
> -    case 7:
> +    case CUDA_REG_T1LH:
>          /* XXX: check this */
>          val = (s->timers[0].latch >> 8) & 0xff;
>          break;
> -    case 8:
> +    case CUDA_REG_T2CL:
>          val = get_counter(&s->timers[1]) & 0xff;
>          s->ifr &= ~T2_INT;
>          break;
> -    case 9:
> +    case CUDA_REG_T2CH:
>          val = get_counter(&s->timers[1]) >> 8;
>          break;
> -    case 10:
> +    case CUDA_REG_SR:
>          val = s->sr;
>          s->ifr &= ~(SR_INT | SR_CLOCK_INT | SR_DATA_INT);
>          cuda_update_irq(s);
>          break;
> -    case 11:
> +    case CUDA_REG_ACR:
>          val = s->acr;
>          break;
> -    case 12:
> +    case CUDA_REG_PCR:
>          val = s->pcr;
>          break;
> -    case 13:
> +    case CUDA_REG_IFR:
>          val = s->ifr;
> -        if (s->ifr & s->ier)
> +        if (s->ifr & s->ier) {
>              val |= 0x80;
> +        }
>          break;
> -    case 14:
> +    case CUDA_REG_IER:
>          val = s->ier | 0x80;
>          break;
>      default:
> -    case 15:
> +    case CUDA_REG_ANH:
>          val = s->anh;
>          break;
>      }
> -    if (addr != 13 || val != 0) {
> +    if (addr != CUDA_REG_IFR || val != 0) {
>          CUDA_DPRINTF("read: reg=0x%x val=%02x\n", (int)addr, val);
>      }
>  
> @@ -300,61 +319,61 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
>      CUDA_DPRINTF("write: reg=0x%x val=%02x\n", (int)addr, val);
>  
>      switch(addr) {
> -    case 0:
> +    case CUDA_REG_B:
>          s->b = val;
>          cuda_update(s);
>          break;
> -    case 1:
> +    case CUDA_REG_A:
>          s->a = val;
>          break;
> -    case 2:
> +    case CUDA_REG_DIRB:
>          s->dirb = val;
>          break;
> -    case 3:
> +    case CUDA_REG_DIRA:
>          s->dira = val;
>          break;
> -    case 4:
> +    case CUDA_REG_T1CL:
>          s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>          break;
> -    case 5:
> +    case CUDA_REG_T1CH:
>          s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
>          s->ifr &= ~T1_INT;
>          set_counter(s, &s->timers[0], s->timers[0].latch);
>          break;
> -    case 6:
> +    case CUDA_REG_T1LL:
>          s->timers[0].latch = (s->timers[0].latch & 0xff00) | val;
>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>          break;
> -    case 7:
> +    case CUDA_REG_T1LH:
>          s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
>          s->ifr &= ~T1_INT;
>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>          break;
> -    case 8:
> +    case CUDA_REG_T2CL:
>          s->timers[1].latch = val;
>          set_counter(s, &s->timers[1], val);
>          break;
> -    case 9:
> +    case CUDA_REG_T2CH:
>          set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
>          break;
> -    case 10:
> +    case CUDA_REG_SR:
>          s->sr = val;
>          break;
> -    case 11:
> +    case CUDA_REG_ACR:
>          s->acr = val;
>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>          cuda_update(s);
>          break;
> -    case 12:
> +    case CUDA_REG_PCR:
>          s->pcr = val;
>          break;
> -    case 13:
> +    case CUDA_REG_IFR:
>          /* reset bits */
>          s->ifr &= ~val;
>          cuda_update_irq(s);
>          break;
> -    case 14:
> +    case CUDA_REG_IER:
>          if (val & IER_SET) {
>              /* set bits */
>              s->ier |= val & 0x7f;
> @@ -365,7 +384,7 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
>          cuda_update_irq(s);
>          break;
>      default:
> -    case 15:
> +    case CUDA_REG_ANH:
>          s->anh = val;
>          break;
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 10/13] cuda.c: refactor get_tb() so that the time can be passed in

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Fri, Oct 23, 2015 at 02:56:35PM +0100, Mark Cave-Ayland wrote:
> This is in preparation for sharing the code between timers.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/misc/macio/cuda.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index d32afc6..7a1b10b 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -143,10 +143,9 @@ static void cuda_update_irq(CUDAState *s)
>      }
>  }
>  
> -static uint64_t get_tb(uint64_t freq)
> +static uint64_t get_tb(uint64_t time, uint64_t freq)
>  {
> -    return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> -                    freq, get_ticks_per_sec());
> +    return muldiv64(time, freq, get_ticks_per_sec());
>  }
>  
>  static unsigned int get_counter(CUDATimer *s)
> @@ -154,9 +153,10 @@ static unsigned int get_counter(CUDATimer *s)
>      int64_t d;
>      unsigned int counter;
>      uint64_t tb_diff;
> +    uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  
>      /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup. */
> -    tb_diff = get_tb(s->frequency) - s->load_time;
> +    tb_diff = get_tb(current_time, s->frequency) - s->load_time;
>      d = (tb_diff * 0xBF401675E5DULL) / (s->frequency << 24);
>  
>      if (s->index == 0) {
> @@ -176,7 +176,8 @@ static unsigned int get_counter(CUDATimer *s)
>  static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
>  {
>      CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
> -    ti->load_time = get_tb(s->frequency);
> +    ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                           s->frequency);
>      ti->counter_value = val;
>      cuda_timer_update(s, ti, ti->load_time);
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 11/13] cuda.c: rename get_counter() state variable from s to ti for consistency

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]

On Fri, Oct 23, 2015 at 02:56:36PM +0100, Mark Cave-Ayland wrote:
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

There are some other functions that also use 's' instead of 'ti', but
I don't think that's a reason not to improve consistency in one place.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/misc/macio/cuda.c |   18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 7a1b10b..687cb54 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -148,7 +148,7 @@ static uint64_t get_tb(uint64_t time, uint64_t freq)
>      return muldiv64(time, freq, get_ticks_per_sec());
>  }
>  
> -static unsigned int get_counter(CUDATimer *s)
> +static unsigned int get_counter(CUDATimer *ti)
>  {
>      int64_t d;
>      unsigned int counter;
> @@ -156,19 +156,19 @@ static unsigned int get_counter(CUDATimer *s)
>      uint64_t current_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>  
>      /* Reverse of the tb calculation algorithm that Mac OS X uses on bootup. */
> -    tb_diff = get_tb(current_time, s->frequency) - s->load_time;
> -    d = (tb_diff * 0xBF401675E5DULL) / (s->frequency << 24);
> +    tb_diff = get_tb(current_time, ti->frequency) - ti->load_time;
> +    d = (tb_diff * 0xBF401675E5DULL) / (ti->frequency << 24);
>  
> -    if (s->index == 0) {
> +    if (ti->index == 0) {
>          /* the timer goes down from latch to -1 (period of latch + 2) */
> -        if (d <= (s->counter_value + 1)) {
> -            counter = (s->counter_value - d) & 0xffff;
> +        if (d <= (ti->counter_value + 1)) {
> +            counter = (ti->counter_value - d) & 0xffff;
>          } else {
> -            counter = (d - (s->counter_value + 1)) % (s->latch + 2);
> -            counter = (s->latch - counter) & 0xffff;
> +            counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> +            counter = (ti->latch - counter) & 0xffff;
>          }
>      } else {
> -        counter = (s->counter_value - d) & 0xffff;
> +        counter = (ti->counter_value - d) & 0xffff;
>      }
>      return counter;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 12/13] cuda.c: fix T2 timer and enable its interrupt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4473 bytes --]

On Fri, Oct 23, 2015 at 02:56:37PM +0100, Mark Cave-Ayland wrote:
> Fix the counter loading logic and enable the T2 interrupt when the timer
> expires.

A mention of what uses T2, and therefore why this is useful would be
good.

> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/macio/cuda.c |   30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index 687cb54..d864b24 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -136,7 +136,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
>  
>  static void cuda_update_irq(CUDAState *s)
>  {
> -    if (s->ifr & s->ier & (SR_INT | T1_INT)) {
> +    if (s->ifr & s->ier & (SR_INT | T1_INT | T2_INT)) {
>          qemu_irq_raise(s->irq);
>      } else {
>          qemu_irq_lower(s->irq);
> @@ -175,7 +175,7 @@ static unsigned int get_counter(CUDATimer *ti)
>  
>  static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
>  {
> -    CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
> +    CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
>      ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>                             s->frequency);
>      ti->counter_value = val;
> @@ -220,7 +220,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
>  {
>      if (!ti->timer)
>          return;
> -    if ((s->acr & T1MODE) != T1MODE_CONT) {
> +    if (ti->index == 0 && (s->acr & T1MODE) != T1MODE_CONT) {
>          timer_del(ti->timer);
>      } else {
>          ti->next_irq_time = get_next_irq_time(ti, current_time);
> @@ -238,6 +238,16 @@ static void cuda_timer1(void *opaque)
>      cuda_update_irq(s);
>  }
>  
> +static void cuda_timer2(void *opaque)
> +{
> +    CUDAState *s = opaque;
> +    CUDATimer *ti = &s->timers[1];
> +
> +    cuda_timer_update(s, ti, ti->next_irq_time);
> +    s->ifr |= T2_INT;
> +    cuda_update_irq(s);
> +}
> +
>  static uint32_t cuda_readb(void *opaque, hwaddr addr)
>  {
>      CUDAState *s = opaque;
> @@ -276,6 +286,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
>      case CUDA_REG_T2CL:
>          val = get_counter(&s->timers[1]) & 0xff;
>          s->ifr &= ~T2_INT;
> +        cuda_update_irq(s);
>          break;
>      case CUDA_REG_T2CH:
>          val = get_counter(&s->timers[1]) >> 8;
> @@ -352,11 +363,12 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>          break;
>      case CUDA_REG_T2CL:
> -        s->timers[1].latch = val;
> -        set_counter(s, &s->timers[1], val);
> +        s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>          break;
>      case CUDA_REG_T2CH:
> -        set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
> +        s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
> +        s->ifr &= ~T2_INT;
> +        set_counter(s, &s->timers[1], s->timers[1].latch);

So the new code appears to be like that for T1CL / T1CH, which makes
sense.  However, T1CL has a cuda_timer_update() call.  Do you also
need that for T2CL?

>          break;
>      case CUDA_REG_SR:
>          s->sr = val;
> @@ -719,8 +731,7 @@ static void cuda_reset(DeviceState *dev)
>      s->timers[0].latch = 0xffff;
>      set_counter(s, &s->timers[0], 0xffff);
>  
> -    s->timers[1].latch = 0;
> -    set_counter(s, &s->timers[1], 0xffff);
> +    s->timers[1].latch = 0xffff;
>  }
>  
>  static void cuda_realizefn(DeviceState *dev, Error **errp)
> @@ -730,7 +741,8 @@ static void cuda_realizefn(DeviceState *dev, Error **errp)
>  
>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer1, s);
>      s->timers[0].frequency = s->frequency;
> -    s->timers[1].frequency = s->frequency;
> +    s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer2, s);
> +    s->timers[1].frequency = (SCALE_US * 6000) / 4700;

Where does this T2 frequency come from?

>      qemu_get_timedate(&tm, 0);
>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 13/13] cuda.c: add delay to setting of SR_INT bit

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4623 bytes --]

On Fri, Oct 23, 2015 at 02:56:38PM +0100, Mark Cave-Ayland wrote:
> MacOS 9 is racy when it comes to accessing the shift register. Fix this by
> introducing a small delay between data accesses and raising the SR_INT
> interrupt bit.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/misc/macio/cuda.c |   44 +++++++++++++++++++++++++++++++++-----------
>  hw/ppc/mac.h         |    3 +++
>  2 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> index d864b24..5156c72 100644
> --- a/hw/misc/macio/cuda.c
> +++ b/hw/misc/macio/cuda.c
> @@ -248,6 +248,31 @@ static void cuda_timer2(void *opaque)
>      cuda_update_irq(s);
>  }
>  
> +static void cuda_set_sr_int(void *opaque)
> +{
> +    CUDAState *s = opaque;
> +
> +    CUDA_DPRINTF("CUDA: %s:%d\n", __func__, __LINE__);
> +    s->ifr |= SR_INT;
> +    cuda_update_irq(s);
> +}
> +
> +static void cuda_delay_set_sr_int(CUDAState *s)
> +{
> +    int64_t expire;
> +
> +    if (s->dirb == 0xff) {
> +        /* Not in Mac OS, fire the IRQ directly */
> +        cuda_set_sr_int(s);
> +        return;
> +    }
> +
> +    CUDA_DPRINTF("CUDA: %s:%d\n", __func__, __LINE__);
> +
> +    expire = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 300 * SCALE_US;
> +    timer_mod(s->sr_delay_timer, expire);
> +}
> +
>  static uint32_t cuda_readb(void *opaque, hwaddr addr)
>  {
>      CUDAState *s = opaque;
> @@ -418,8 +443,7 @@ static void cuda_update(CUDAState *s)
>                  if (s->data_out_index < sizeof(s->data_out)) {
>                      CUDA_DPRINTF("send: %02x\n", s->sr);
>                      s->data_out[s->data_out_index++] = s->sr;
> -                    s->ifr |= SR_INT;
> -                    cuda_update_irq(s);
> +                    cuda_delay_set_sr_int(s);
>                  }
>              }
>          } else {
> @@ -432,8 +456,7 @@ static void cuda_update(CUDAState *s)
>                      if (s->data_in_index >= s->data_in_size) {
>                          s->b = (s->b | TREQ);
>                      }
> -                    s->ifr |= SR_INT;
> -                    cuda_update_irq(s);
> +                    cuda_delay_set_sr_int(s);
>                  }
>              }
>          }
> @@ -445,15 +468,13 @@ static void cuda_update(CUDAState *s)
>                  s->b = (s->b | TREQ);
>              else
>                  s->b = (s->b & ~TREQ);
> -            s->ifr |= SR_INT;
> -            cuda_update_irq(s);
> +            cuda_delay_set_sr_int(s);
>          } else {
>              if (!(s->last_b & TIP)) {
>                  /* handle end of host to cuda transfer */
>                  packet_received = (s->data_out_index > 0);
>                  /* always an IRQ at the end of transfer */
> -                s->ifr |= SR_INT;
> -                cuda_update_irq(s);
> +                cuda_delay_set_sr_int(s);
>              }
>              /* signal if there is data to read */
>              if (s->data_in_index < s->data_in_size) {
> @@ -490,8 +511,7 @@ static void cuda_send_packet_to_host(CUDAState *s,
>      s->data_in_size = len;
>      s->data_in_index = 0;
>      cuda_update(s);
> -    s->ifr |= SR_INT;
> -    cuda_update_irq(s);
> +    cuda_delay_set_sr_int(s);
>  }
>  
>  static void cuda_adb_poll(void *opaque)
> @@ -714,7 +734,7 @@ static void cuda_reset(DeviceState *dev)
>  
>      s->b = 0;
>      s->a = 0;
> -    s->dirb = 0;
> +    s->dirb = 0xff;
>      s->dira = 0;
>      s->sr = 0;
>      s->acr = 0;
> @@ -732,6 +752,8 @@ static void cuda_reset(DeviceState *dev)
>      set_counter(s, &s->timers[0], 0xffff);
>  
>      s->timers[1].latch = 0xffff;
> +
> +    s->sr_delay_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_set_sr_int, s);
>  }
>  
>  static void cuda_realizefn(DeviceState *dev, Error **errp)
> diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
> index 8bdba30..e375ed2 100644
> --- a/hw/ppc/mac.h
> +++ b/hw/ppc/mac.h
> @@ -103,6 +103,9 @@ typedef struct CUDAState {
>      uint8_t last_b;
>      uint8_t last_acr;
>  
> +    /* MacOS 9 is racy and requires a delay upon setting the SR_INT bit */
> +    QEMUTimer *sr_delay_timer;
> +
>      int data_in_size;
>      int data_in_index;
>      int data_out_index;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3730 bytes --]

On Fri, Oct 30, 2015 at 04:48:12PM +0000, Mark Cave-Ayland wrote:
> On 23/10/15 14:56, Mark Cave-Ayland wrote:
> 
> > This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
> > QEMU, the original version of which was posted to the qemu-devel list at the
> > end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
> > 
> > The patchset consisted of some simple patches from Alex and then a large set of
> > CUDA changes supplied as a single patch which were the result of Cormac analysing
> > MOL with Alex's help to try and further the boot process.
> > 
> > In their previous form, the patches were unsuitable for applying upstream since
> > while they furthered MacOS 9 boot, they also caused a couple of major regressions
> > such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
> > 
> > This reworked patchset fixes these regressions, includes some other clean-ups 
> > and more importantly now passes all of my OpenBIOS image boot tests with an 
> > OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
> > Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
> > I've uploaded a pre-compiled binary to 
> > https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
> > new MacOS 9 functionality.
> > 
> > Apologies for the delay in sending this out on-list, however due to recent
> > circumstances I've been without a reliable broadband connection for a couple
> > of weeks. However given that this is mostly a rework of the previous patchset 
> > and looks good in testing here, I'd definitely like it to be considered for
> > application during soft freeze.
> > 
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > 
> > Alexander Graf (3):
> >   PPC: Allow Rc bit to be set on mtspr
> >   PPC: Fix lsxw bounds checks
> >   PPC: mac99: Always add USB controller
> > 
> > Mark Cave-Ayland (10):
> >   cuda.c: fix CUDA ADB error packet format
> >   cuda.c: fix CUDA_PACKET response packet format
> >   cuda.c: implement simple CUDA_GET_6805_ADDR command
> >   cuda.c: implement dummy IIC access commands
> >   cuda.c: fix CUDA SR interrupt clearing
> >   cuda.c: add defines for CUDA registers
> >   cuda.c: refactor get_tb() so that the time can be passed in
> >   cuda.c: rename get_counter() state variable from s to ti for
> >     consistency
> >   cuda.c: fix T2 timer and enable its interrupt
> >   cuda.c: add delay to setting of SR_INT bit
> > 
> >  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
> >  hw/ppc/mac.h            |    3 +
> >  hw/ppc/mac_newworld.c   |    3 +-
> >  target-ppc/mem_helper.c |    5 +-
> >  target-ppc/translate.c  |    2 +-
> >  5 files changed, 163 insertions(+), 93 deletions(-)
> 
> Ping? Can anyone review this in Alex's absence? In the meantime I've
> added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
> good to get the GSoC work upstream for 2.5.

Sorry I've taken a while to get to this.  It looks pretty good, though
I've sent a handful of comments on individual patches.

I gathered from one of your replies that you do intend to do a
respin.  The current comments all look pretty trivial, so I expect
I'll be ok to apply your respin to ppc-next (which I'm looking after
in agraf's absence).  It would be nice to get a review from someone
more familiar with, or better able to test MacOS stuff.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 04/13] cuda.c: fix CUDA ADB error packet format

[Mark Cave-Ayland]

On 04/11/15 03:12, David Gibson wrote:

> On Fri, Oct 23, 2015 at 02:56:29PM +0100, Mark Cave-Ayland wrote:
>> ADB error packets should be of the form (type, status, cmd) rather than just
>> (type, status). This fixes ADB device detection under MacOS 9.
> 
> Hmm.. are there any public doc on these ADB / CUDA things that we can
> look up to compare to?

Sadly no. The main process for working during GSoC was to add logging
into various areas to see the point at which execution stopped, then
compare with MOL, code up the difference and see if this helps boot
progress.

Pretty much every other OS will read in the entire CUDA packet based
upon the length and check the status for errors, whereas it seems MacOS
9 additionally checks the right number of bytes are received in the
reply (even if they are never used).

>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/misc/macio/cuda.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index 5d7043e..9ec19af 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -560,19 +560,21 @@ static void cuda_receive_packet_from_host(CUDAState *s,
>>      switch(data[0]) {
>>      case ADB_PACKET:
>>          {
>> -            uint8_t obuf[ADB_MAX_OUT_LEN + 2];
>> +            uint8_t obuf[ADB_MAX_OUT_LEN + 3];
>>              int olen;
>>              olen = adb_request(&s->adb_bus, obuf + 2, data + 1, len - 1);
>>              if (olen > 0) {
>>                  obuf[0] = ADB_PACKET;
>>                  obuf[1] = 0x00;
>> +                cuda_send_packet_to_host(s, obuf, olen + 2);
>>              } else {
>>                  /* error */
>>                  obuf[0] = ADB_PACKET;
>>                  obuf[1] = -olen;
>> +                obuf[2] = data[1];
>>                  olen = 0;
>> +                cuda_send_packet_to_host(s, obuf, olen + 3);
> 
> Using 'olen + 3' seems confusing here, rather than just '3', since you
> just set olen = 0.

I do prefer the way it is at the moment, because all the calls to
cuda_send_packet_to_host() use a length of the form "olen + X" which
tells you the payload length (0) + the header length (X). So from this
we know that there are just the 3 standard header bytes and no
additional payload being returned by this particular transaction.

>>              }
>> -            cuda_send_packet_to_host(s, obuf, olen + 2);
>>          }
>>          break;
>>      case CUDA_PACKET:
> 

ATB,

Mark.

Re: [Qemu-devel] [PATCH 05/13] cuda.c: fix CUDA_PACKET response packet format

[Mark Cave-Ayland]

On 04/11/15 03:15, David Gibson wrote:

> On Fri, Oct 23, 2015 at 02:56:30PM +0100, Mark Cave-Ayland wrote:
>> According to comments in MOL, the response to a CUDA_PACKET should be one of
>> the following:
>>
>> Reply: CUDA_PACKET, status, cmd
>> Error: ERROR_PACKET, status, CUDA_PACKET, cmd
>>
>> Update cuda_receive_packet() accordingly to reflect this.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Code seems to match the description, but I don't have another source
> to check what the right thing to do is for CUDA.

Again there is no official documentation for this other than the
comments in MOL's src/drivers/via-cuda.c cuda_packet() function :(

>> ---
>>  hw/misc/macio/cuda.c |   24 +++++-------------------
>>  1 file changed, 5 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index 9ec19af..88a0999 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -480,7 +480,7 @@ static void cuda_adb_poll(void *opaque)
>>  static void cuda_receive_packet(CUDAState *s,
>>                                  const uint8_t *data, int len)
>>  {
>> -    uint8_t obuf[16];
>> +    uint8_t obuf[16] = { CUDA_PACKET, 0, data[0] };
>>      int autopoll;
>>      uint32_t ti;
>>  
>> @@ -497,23 +497,15 @@ static void cuda_receive_packet(CUDAState *s,
>>                  timer_del(s->adb_poll_timer);
>>              }
>>          }
>> -        obuf[0] = CUDA_PACKET;
>> -        obuf[1] = data[1];
>> -        cuda_send_packet_to_host(s, obuf, 2);
>> +        cuda_send_packet_to_host(s, obuf, 3);
>>          break;
>>      case CUDA_SET_TIME:
>>          ti = (((uint32_t)data[1]) << 24) + (((uint32_t)data[2]) << 16) + (((uint32_t)data[3]) << 8) + data[4];
>>          s->tick_offset = ti - (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
>> -        obuf[0] = CUDA_PACKET;
>> -        obuf[1] = 0;
>> -        obuf[2] = 0;
>>          cuda_send_packet_to_host(s, obuf, 3);
>>          break;
>>      case CUDA_GET_TIME:
>>          ti = s->tick_offset + (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / get_ticks_per_sec());
>> -        obuf[0] = CUDA_PACKET;
>> -        obuf[1] = 0;
>> -        obuf[2] = 0;
>>          obuf[3] = ti >> 24;
>>          obuf[4] = ti >> 16;
>>          obuf[5] = ti >> 8;
>> @@ -524,20 +516,14 @@ static void cuda_receive_packet(CUDAState *s,
>>      case CUDA_SET_DEVICE_LIST:
>>      case CUDA_SET_AUTO_RATE:
>>      case CUDA_SET_POWER_MESSAGES:
>> -        obuf[0] = CUDA_PACKET;
>> -        obuf[1] = 0;
>> -        cuda_send_packet_to_host(s, obuf, 2);
>> +        cuda_send_packet_to_host(s, obuf, 3);
>>          break;
>>      case CUDA_POWERDOWN:
>> -        obuf[0] = CUDA_PACKET;
>> -        obuf[1] = 0;
>> -        cuda_send_packet_to_host(s, obuf, 2);
>> +        cuda_send_packet_to_host(s, obuf, 3);
>>          qemu_system_shutdown_request();
>>          break;
>>      case CUDA_RESET_SYSTEM:
>> -        obuf[0] = CUDA_PACKET;
>> -        obuf[1] = 0;
>> -        cuda_send_packet_to_host(s, obuf, 2);
>> +        cuda_send_packet_to_host(s, obuf, 3);
>>          qemu_system_reset_request();
>>          break;
>>      default:


ATB,

Mark.

Re: [Qemu-devel] [PATCH 07/13] cuda.c: implement dummy IIC access commands

[Mark Cave-Ayland]

On 04/11/15 03:17, David Gibson wrote:

> On Fri, Oct 23, 2015 at 02:56:32PM +0100, Mark Cave-Ayland wrote:
>> These are used by MacOS 9 on boot. Here we return an error except for 4-byte
>> commands which write to the IIC bus.
> 
> A bit of rationale about why some of these give errors and some don't
> would be nice.

This was another change inspired by the relevant code in MOL. There are
comments suggesting that the 3 byte packet format is a (iicAddr, iicReg,
iicData) tuple which I could add in if required? Then again it seems the
MacOS 9 code just needs to receive some kind of reply in order for boot
to proceed, rather than the values actually doing anything.

>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/misc/macio/cuda.c |   18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index 4fe901b..d3ec58a 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -529,6 +529,24 @@ static void cuda_receive_packet(CUDAState *s,
>>          cuda_send_packet_to_host(s, obuf, 3);
>>          qemu_system_reset_request();
>>          break;
>> +    case CUDA_COMBINED_FORMAT_IIC:
>> +        obuf[0] = ERROR_PACKET;
>> +        obuf[1] = 0x5;
>> +        obuf[2] = CUDA_PACKET;
>> +        obuf[3] = data[0];
>> +        cuda_send_packet_to_host(s, obuf, 4);
>> +        break;
>> +    case CUDA_GET_SET_IIC:
>> +        if (len == 4) {
>> +            cuda_send_packet_to_host(s, obuf, 3);
>> +        } else {
>> +            obuf[0] = ERROR_PACKET;
>> +            obuf[1] = 0x2;
>> +            obuf[2] = CUDA_PACKET;
>> +            obuf[3] = data[0];
>> +            cuda_send_packet_to_host(s, obuf, 4);
>> +        }
>> +        break;
>>      default:
>>          break;
>>      }
> 

ATB,

Mark.

Re: [Qemu-devel] [PATCH 12/13] cuda.c: fix T2 timer and enable its interrupt

[Mark Cave-Ayland]

On 04/11/15 03:40, David Gibson wrote:

> On Fri, Oct 23, 2015 at 02:56:37PM +0100, Mark Cave-Ayland wrote:
>> Fix the counter loading logic and enable the T2 interrupt when the timer
>> expires.
> 
> A mention of what uses T2, and therefore why this is useful would be
> good.

There is a good chance that nothing has used T2 before MacOS 9 since
before this patch, it is impossible for the T2 timer interrupt to fire.
It can be seen that MacOS 9 does write to the relevant registers during
boot, and if the T2 interrupt is disabled then boot will hang.

>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/misc/macio/cuda.c |   30 +++++++++++++++++++++---------
>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>> index 687cb54..d864b24 100644
>> --- a/hw/misc/macio/cuda.c
>> +++ b/hw/misc/macio/cuda.c
>> @@ -136,7 +136,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
>>  
>>  static void cuda_update_irq(CUDAState *s)
>>  {
>> -    if (s->ifr & s->ier & (SR_INT | T1_INT)) {
>> +    if (s->ifr & s->ier & (SR_INT | T1_INT | T2_INT)) {
>>          qemu_irq_raise(s->irq);
>>      } else {
>>          qemu_irq_lower(s->irq);
>> @@ -175,7 +175,7 @@ static unsigned int get_counter(CUDATimer *ti)
>>  
>>  static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
>>  {
>> -    CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
>> +    CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
>>      ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>>                             s->frequency);
>>      ti->counter_value = val;
>> @@ -220,7 +220,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
>>  {
>>      if (!ti->timer)
>>          return;
>> -    if ((s->acr & T1MODE) != T1MODE_CONT) {
>> +    if (ti->index == 0 && (s->acr & T1MODE) != T1MODE_CONT) {
>>          timer_del(ti->timer);
>>      } else {
>>          ti->next_irq_time = get_next_irq_time(ti, current_time);
>> @@ -238,6 +238,16 @@ static void cuda_timer1(void *opaque)
>>      cuda_update_irq(s);
>>  }
>>  
>> +static void cuda_timer2(void *opaque)
>> +{
>> +    CUDAState *s = opaque;
>> +    CUDATimer *ti = &s->timers[1];
>> +
>> +    cuda_timer_update(s, ti, ti->next_irq_time);
>> +    s->ifr |= T2_INT;
>> +    cuda_update_irq(s);
>> +}
>> +
>>  static uint32_t cuda_readb(void *opaque, hwaddr addr)
>>  {
>>      CUDAState *s = opaque;
>> @@ -276,6 +286,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
>>      case CUDA_REG_T2CL:
>>          val = get_counter(&s->timers[1]) & 0xff;
>>          s->ifr &= ~T2_INT;
>> +        cuda_update_irq(s);
>>          break;
>>      case CUDA_REG_T2CH:
>>          val = get_counter(&s->timers[1]) >> 8;
>> @@ -352,11 +363,12 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
>>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>>          break;
>>      case CUDA_REG_T2CL:
>> -        s->timers[1].latch = val;
>> -        set_counter(s, &s->timers[1], val);
>> +        s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>>          break;
>>      case CUDA_REG_T2CH:
>> -        set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
>> +        s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
>> +        s->ifr &= ~T2_INT;
>> +        set_counter(s, &s->timers[1], s->timers[1].latch);
> 
> So the new code appears to be like that for T1CL / T1CH, which makes
> sense.  However, T1CL has a cuda_timer_update() call.  Do you also
> need that for T2CL?

This is a side-effect of combining the T1 and T2 code. Unlike T1, T2
appears to be free-running from its written value but generating an
interrupt just after zero-crossing. So in order to get the correct
interval, we write the value to the latch instead of the counter to get
the same effect with the shared timer code.

>>          break;
>>      case CUDA_REG_SR:
>>          s->sr = val;
>> @@ -719,8 +731,7 @@ static void cuda_reset(DeviceState *dev)
>>      s->timers[0].latch = 0xffff;
>>      set_counter(s, &s->timers[0], 0xffff);
>>  
>> -    s->timers[1].latch = 0;
>> -    set_counter(s, &s->timers[1], 0xffff);
>> +    s->timers[1].latch = 0xffff;
>>  }
>>  
>>  static void cuda_realizefn(DeviceState *dev, Error **errp)
>> @@ -730,7 +741,8 @@ static void cuda_realizefn(DeviceState *dev, Error **errp)
>>  
>>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer1, s);
>>      s->timers[0].frequency = s->frequency;
>> -    s->timers[1].frequency = s->frequency;
>> +    s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer2, s);
>> +    s->timers[1].frequency = (SCALE_US * 6000) / 4700;
> 
> Where does this T2 frequency come from?

My understanding of this is that with the shared timer code, the IRQ
timing is calculated based upon CUDA_TIMER_FREQ (4.7MHz / 6). Therefore
by setting the frequency to the inverse value ((SCALE_US * 6000) / 4700)
then this cancels out the effect of the timebase calculation algorithm
used in the counters. I believe this came from Alex so he would likely
be able to clarify this and give a much better explanation.

>>      qemu_get_timedate(&tm, 0);
>>      s->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
> 


ATB,

Mark.

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[Mark Cave-Ayland]

On 04/11/15 03:44, David Gibson wrote:

> On Fri, Oct 30, 2015 at 04:48:12PM +0000, Mark Cave-Ayland wrote:
>> On 23/10/15 14:56, Mark Cave-Ayland wrote:
>>
>>> This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
>>> QEMU, the original version of which was posted to the qemu-devel list at the
>>> end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
>>>
>>> The patchset consisted of some simple patches from Alex and then a large set of
>>> CUDA changes supplied as a single patch which were the result of Cormac analysing
>>> MOL with Alex's help to try and further the boot process.
>>>
>>> In their previous form, the patches were unsuitable for applying upstream since
>>> while they furthered MacOS 9 boot, they also caused a couple of major regressions
>>> such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
>>>
>>> This reworked patchset fixes these regressions, includes some other clean-ups 
>>> and more importantly now passes all of my OpenBIOS image boot tests with an 
>>> OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
>>> Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
>>> I've uploaded a pre-compiled binary to 
>>> https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
>>> new MacOS 9 functionality.
>>>
>>> Apologies for the delay in sending this out on-list, however due to recent
>>> circumstances I've been without a reliable broadband connection for a couple
>>> of weeks. However given that this is mostly a rework of the previous patchset 
>>> and looks good in testing here, I'd definitely like it to be considered for
>>> application during soft freeze.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>> Alexander Graf (3):
>>>   PPC: Allow Rc bit to be set on mtspr
>>>   PPC: Fix lsxw bounds checks
>>>   PPC: mac99: Always add USB controller
>>>
>>> Mark Cave-Ayland (10):
>>>   cuda.c: fix CUDA ADB error packet format
>>>   cuda.c: fix CUDA_PACKET response packet format
>>>   cuda.c: implement simple CUDA_GET_6805_ADDR command
>>>   cuda.c: implement dummy IIC access commands
>>>   cuda.c: fix CUDA SR interrupt clearing
>>>   cuda.c: add defines for CUDA registers
>>>   cuda.c: refactor get_tb() so that the time can be passed in
>>>   cuda.c: rename get_counter() state variable from s to ti for
>>>     consistency
>>>   cuda.c: fix T2 timer and enable its interrupt
>>>   cuda.c: add delay to setting of SR_INT bit
>>>
>>>  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
>>>  hw/ppc/mac.h            |    3 +
>>>  hw/ppc/mac_newworld.c   |    3 +-
>>>  target-ppc/mem_helper.c |    5 +-
>>>  target-ppc/translate.c  |    2 +-
>>>  5 files changed, 163 insertions(+), 93 deletions(-)
>>
>> Ping? Can anyone review this in Alex's absence? In the meantime I've
>> added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
>> good to get the GSoC work upstream for 2.5.
> 
> Sorry I've taken a while to get to this.  It looks pretty good, though
> I've sent a handful of comments on individual patches.
> 
> I gathered from one of your replies that you do intend to do a
> respin.  The current comments all look pretty trivial, so I expect
> I'll be ok to apply your respin to ppc-next (which I'm looking after
> in agraf's absence).  It would be nice to get a review from someone
> more familiar with, or better able to test MacOS stuff.

Great. I've sent a further few replies, so if you're happy with the
answers let me know and I'll send a v2 tomorrow.

In terms of testing, as I mentioned above it doesn't regress any of my
working OpenBIOS test-suite images which is a good sign. I'm not sure
who else would be able to review the MacOS side. Both Segher and Ben. H
have looked at parts of the original code changes during GSoC, although
that was from a debugging rather than a code review perspective.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4603 bytes --]

On Wed, Nov 04, 2015 at 11:32:02PM +0000, Mark Cave-Ayland wrote:
> On 04/11/15 03:44, David Gibson wrote:
> 
> > On Fri, Oct 30, 2015 at 04:48:12PM +0000, Mark Cave-Ayland wrote:
> >> On 23/10/15 14:56, Mark Cave-Ayland wrote:
> >>
> >>> This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
> >>> QEMU, the original version of which was posted to the qemu-devel list at the
> >>> end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
> >>>
> >>> The patchset consisted of some simple patches from Alex and then a large set of
> >>> CUDA changes supplied as a single patch which were the result of Cormac analysing
> >>> MOL with Alex's help to try and further the boot process.
> >>>
> >>> In their previous form, the patches were unsuitable for applying upstream since
> >>> while they furthered MacOS 9 boot, they also caused a couple of major regressions
> >>> such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
> >>>
> >>> This reworked patchset fixes these regressions, includes some other clean-ups 
> >>> and more importantly now passes all of my OpenBIOS image boot tests with an 
> >>> OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
> >>> Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
> >>> I've uploaded a pre-compiled binary to 
> >>> https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
> >>> new MacOS 9 functionality.
> >>>
> >>> Apologies for the delay in sending this out on-list, however due to recent
> >>> circumstances I've been without a reliable broadband connection for a couple
> >>> of weeks. However given that this is mostly a rework of the previous patchset 
> >>> and looks good in testing here, I'd definitely like it to be considered for
> >>> application during soft freeze.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>>
> >>> Alexander Graf (3):
> >>>   PPC: Allow Rc bit to be set on mtspr
> >>>   PPC: Fix lsxw bounds checks
> >>>   PPC: mac99: Always add USB controller
> >>>
> >>> Mark Cave-Ayland (10):
> >>>   cuda.c: fix CUDA ADB error packet format
> >>>   cuda.c: fix CUDA_PACKET response packet format
> >>>   cuda.c: implement simple CUDA_GET_6805_ADDR command
> >>>   cuda.c: implement dummy IIC access commands
> >>>   cuda.c: fix CUDA SR interrupt clearing
> >>>   cuda.c: add defines for CUDA registers
> >>>   cuda.c: refactor get_tb() so that the time can be passed in
> >>>   cuda.c: rename get_counter() state variable from s to ti for
> >>>     consistency
> >>>   cuda.c: fix T2 timer and enable its interrupt
> >>>   cuda.c: add delay to setting of SR_INT bit
> >>>
> >>>  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
> >>>  hw/ppc/mac.h            |    3 +
> >>>  hw/ppc/mac_newworld.c   |    3 +-
> >>>  target-ppc/mem_helper.c |    5 +-
> >>>  target-ppc/translate.c  |    2 +-
> >>>  5 files changed, 163 insertions(+), 93 deletions(-)
> >>
> >> Ping? Can anyone review this in Alex's absence? In the meantime I've
> >> added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
> >> good to get the GSoC work upstream for 2.5.
> > 
> > Sorry I've taken a while to get to this.  It looks pretty good, though
> > I've sent a handful of comments on individual patches.
> > 
> > I gathered from one of your replies that you do intend to do a
> > respin.  The current comments all look pretty trivial, so I expect
> > I'll be ok to apply your respin to ppc-next (which I'm looking after
> > in agraf's absence).  It would be nice to get a review from someone
> > more familiar with, or better able to test MacOS stuff.
> 
> Great. I've sent a further few replies, so if you're happy with the
> answers let me know and I'll send a v2 tomorrow.

Haven't seen v2.  Did something sidetrack you, or did I manage to miss
it?
> 
> In terms of testing, as I mentioned above it doesn't regress any of my
> working OpenBIOS test-suite images which is a good sign. I'm not sure
> who else would be able to review the MacOS side. Both Segher and Ben. H
> have looked at parts of the original code changes during GSoC, although
> that was from a debugging rather than a code review perspective.
> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[Mark Cave-Ayland]

On 11/11/15 02:11, David Gibson wrote:

> On Wed, Nov 04, 2015 at 11:32:02PM +0000, Mark Cave-Ayland wrote:
>> On 04/11/15 03:44, David Gibson wrote:
>>
>>> On Fri, Oct 30, 2015 at 04:48:12PM +0000, Mark Cave-Ayland wrote:
>>>> On 23/10/15 14:56, Mark Cave-Ayland wrote:
>>>>
>>>>> This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
>>>>> QEMU, the original version of which was posted to the qemu-devel list at the
>>>>> end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
>>>>>
>>>>> The patchset consisted of some simple patches from Alex and then a large set of
>>>>> CUDA changes supplied as a single patch which were the result of Cormac analysing
>>>>> MOL with Alex's help to try and further the boot process.
>>>>>
>>>>> In their previous form, the patches were unsuitable for applying upstream since
>>>>> while they furthered MacOS 9 boot, they also caused a couple of major regressions
>>>>> such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
>>>>>
>>>>> This reworked patchset fixes these regressions, includes some other clean-ups 
>>>>> and more importantly now passes all of my OpenBIOS image boot tests with an 
>>>>> OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
>>>>> Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
>>>>> I've uploaded a pre-compiled binary to 
>>>>> https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
>>>>> new MacOS 9 functionality.
>>>>>
>>>>> Apologies for the delay in sending this out on-list, however due to recent
>>>>> circumstances I've been without a reliable broadband connection for a couple
>>>>> of weeks. However given that this is mostly a rework of the previous patchset 
>>>>> and looks good in testing here, I'd definitely like it to be considered for
>>>>> application during soft freeze.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>
>>>>> Alexander Graf (3):
>>>>>   PPC: Allow Rc bit to be set on mtspr
>>>>>   PPC: Fix lsxw bounds checks
>>>>>   PPC: mac99: Always add USB controller
>>>>>
>>>>> Mark Cave-Ayland (10):
>>>>>   cuda.c: fix CUDA ADB error packet format
>>>>>   cuda.c: fix CUDA_PACKET response packet format
>>>>>   cuda.c: implement simple CUDA_GET_6805_ADDR command
>>>>>   cuda.c: implement dummy IIC access commands
>>>>>   cuda.c: fix CUDA SR interrupt clearing
>>>>>   cuda.c: add defines for CUDA registers
>>>>>   cuda.c: refactor get_tb() so that the time can be passed in
>>>>>   cuda.c: rename get_counter() state variable from s to ti for
>>>>>     consistency
>>>>>   cuda.c: fix T2 timer and enable its interrupt
>>>>>   cuda.c: add delay to setting of SR_INT bit
>>>>>
>>>>>  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
>>>>>  hw/ppc/mac.h            |    3 +
>>>>>  hw/ppc/mac_newworld.c   |    3 +-
>>>>>  target-ppc/mem_helper.c |    5 +-
>>>>>  target-ppc/translate.c  |    2 +-
>>>>>  5 files changed, 163 insertions(+), 93 deletions(-)
>>>>
>>>> Ping? Can anyone review this in Alex's absence? In the meantime I've
>>>> added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
>>>> good to get the GSoC work upstream for 2.5.
>>>
>>> Sorry I've taken a while to get to this.  It looks pretty good, though
>>> I've sent a handful of comments on individual patches.
>>>
>>> I gathered from one of your replies that you do intend to do a
>>> respin.  The current comments all look pretty trivial, so I expect
>>> I'll be ok to apply your respin to ppc-next (which I'm looking after
>>> in agraf's absence).  It would be nice to get a review from someone
>>> more familiar with, or better able to test MacOS stuff.
>>
>> Great. I've sent a further few replies, so if you're happy with the
>> answers let me know and I'll send a v2 tomorrow.
> 
> Haven't seen v2.  Did something sidetrack you, or did I manage to miss
> it?

Hi David,

I am still waiting for feedback (and Reviewed-by tags) for a few replies
to your initial patch review:

https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00855.html
https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00856.html
https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00862.html

In particular would you like me to add the comments about MOL to the
commit messages? And I wasn't sure from your comments whether you were
still looking for someone else more able to test the MacOS stuff?

That said, if you're happy with my responses I can respin later today if
you are able to send a pull request before tomorrow (which is when
freeze hits)? I have enough free time over the next couple of weeks to
handle any issues that may arise, although I'm fairly confident in these
patches as I've been running them locally over my OpenBIOS test images
for a few weeks now.


ATB,

Mark.

Re: [Qemu-devel] [PATCH 12/13] cuda.c: fix T2 timer and enable its interrupt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6626 bytes --]

On Wed, Nov 04, 2015 at 11:25:43PM +0000, Mark Cave-Ayland wrote:
> On 04/11/15 03:40, David Gibson wrote:
> 
> > On Fri, Oct 23, 2015 at 02:56:37PM +0100, Mark Cave-Ayland wrote:
> >> Fix the counter loading logic and enable the T2 interrupt when the timer
> >> expires.
> > 
> > A mention of what uses T2, and therefore why this is useful would be
> > good.
> 
> There is a good chance that nothing has used T2 before MacOS 9 since
> before this patch, it is impossible for the T2 timer interrupt to fire.
> It can be seen that MacOS 9 does write to the relevant registers during
> boot, and if the T2 interrupt is disabled then boot will hang.

Sorry, I wasn't clear.  My point is that anyone looking back on this
commit in the future won't necessarily have the context of the series
its part of here.  So, the individual commit message should explain
that T2 is being enabled in order to support MacOS9.

*I* know this is about MacOS9 enablement, but that's not clear from the
commit & message itself.

> 
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>  hw/misc/macio/cuda.c |   30 +++++++++++++++++++++---------
> >>  1 file changed, 21 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
> >> index 687cb54..d864b24 100644
> >> --- a/hw/misc/macio/cuda.c
> >> +++ b/hw/misc/macio/cuda.c
> >> @@ -136,7 +136,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
> >>  
> >>  static void cuda_update_irq(CUDAState *s)
> >>  {
> >> -    if (s->ifr & s->ier & (SR_INT | T1_INT)) {
> >> +    if (s->ifr & s->ier & (SR_INT | T1_INT | T2_INT)) {
> >>          qemu_irq_raise(s->irq);
> >>      } else {
> >>          qemu_irq_lower(s->irq);
> >> @@ -175,7 +175,7 @@ static unsigned int get_counter(CUDATimer *ti)
> >>  
> >>  static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
> >>  {
> >> -    CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
> >> +    CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
> >>      ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> >>                             s->frequency);
> >>      ti->counter_value = val;
> >> @@ -220,7 +220,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
> >>  {
> >>      if (!ti->timer)
> >>          return;
> >> -    if ((s->acr & T1MODE) != T1MODE_CONT) {
> >> +    if (ti->index == 0 && (s->acr & T1MODE) != T1MODE_CONT) {
> >>          timer_del(ti->timer);
> >>      } else {
> >>          ti->next_irq_time = get_next_irq_time(ti, current_time);
> >> @@ -238,6 +238,16 @@ static void cuda_timer1(void *opaque)
> >>      cuda_update_irq(s);
> >>  }
> >>  
> >> +static void cuda_timer2(void *opaque)
> >> +{
> >> +    CUDAState *s = opaque;
> >> +    CUDATimer *ti = &s->timers[1];
> >> +
> >> +    cuda_timer_update(s, ti, ti->next_irq_time);
> >> +    s->ifr |= T2_INT;
> >> +    cuda_update_irq(s);
> >> +}
> >> +
> >>  static uint32_t cuda_readb(void *opaque, hwaddr addr)
> >>  {
> >>      CUDAState *s = opaque;
> >> @@ -276,6 +286,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
> >>      case CUDA_REG_T2CL:
> >>          val = get_counter(&s->timers[1]) & 0xff;
> >>          s->ifr &= ~T2_INT;
> >> +        cuda_update_irq(s);
> >>          break;
> >>      case CUDA_REG_T2CH:
> >>          val = get_counter(&s->timers[1]) >> 8;
> >> @@ -352,11 +363,12 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
> >>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
> >>          break;
> >>      case CUDA_REG_T2CL:
> >> -        s->timers[1].latch = val;
> >> -        set_counter(s, &s->timers[1], val);
> >> +        s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
> >>          break;
> >>      case CUDA_REG_T2CH:
> >> -        set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
> >> +        s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
> >> +        s->ifr &= ~T2_INT;
> >> +        set_counter(s, &s->timers[1], s->timers[1].latch);
> > 
> > So the new code appears to be like that for T1CL / T1CH, which makes
> > sense.  However, T1CL has a cuda_timer_update() call.  Do you also
> > need that for T2CL?
> 
> This is a side-effect of combining the T1 and T2 code. Unlike T1, T2
> appears to be free-running from its written value but generating an
> interrupt just after zero-crossing. So in order to get the correct
> interval, we write the value to the latch instead of the counter to get
> the same effect with the shared timer code.

Ok.  I think you need to put that paragraph into a comment - without
that subtle background information, it looks like an obvious bug.

> >>          break;
> >>      case CUDA_REG_SR:
> >>          s->sr = val;
> >> @@ -719,8 +731,7 @@ static void cuda_reset(DeviceState *dev)
> >>      s->timers[0].latch = 0xffff;
> >>      set_counter(s, &s->timers[0], 0xffff);
> >>  
> >> -    s->timers[1].latch = 0;
> >> -    set_counter(s, &s->timers[1], 0xffff);
> >> +    s->timers[1].latch = 0xffff;
> >>  }
> >>  
> >>  static void cuda_realizefn(DeviceState *dev, Error **errp)
> >> @@ -730,7 +741,8 @@ static void cuda_realizefn(DeviceState *dev, Error **errp)
> >>  
> >>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer1, s);
> >>      s->timers[0].frequency = s->frequency;
> >> -    s->timers[1].frequency = s->frequency;
> >> +    s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer2, s);
> >> +    s->timers[1].frequency = (SCALE_US * 6000) / 4700;
> > 
> > Where does this T2 frequency come from?
> 
> My understanding of this is that with the shared timer code, the IRQ
> timing is calculated based upon CUDA_TIMER_FREQ (4.7MHz / 6). Therefore
> by setting the frequency to the inverse value ((SCALE_US * 6000) / 4700)
> then this cancels out the effect of the timebase calculation algorithm
> used in the counters. I believe this came from Alex so he would likely
> be able to clarify this and give a much better explanation.

Hmm, yeah, I'm having trouble following that.  I won't hold the series
up over it, but a clearer rationale would help here.

I take it that T2 has a fixed frequency, whereas T1 is adjustable,
based on the code snippet above?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]

On Wed, Nov 11, 2015 at 06:29:09AM +0000, Mark Cave-Ayland wrote:
> On 11/11/15 02:11, David Gibson wrote:
> 
> > On Wed, Nov 04, 2015 at 11:32:02PM +0000, Mark Cave-Ayland wrote:
> >> On 04/11/15 03:44, David Gibson wrote:
> >>
> >>> On Fri, Oct 30, 2015 at 04:48:12PM +0000, Mark Cave-Ayland wrote:
> >>>> On 23/10/15 14:56, Mark Cave-Ayland wrote:
> >>>>
> >>>>> This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
> >>>>> QEMU, the original version of which was posted to the qemu-devel list at the
> >>>>> end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
> >>>>>
> >>>>> The patchset consisted of some simple patches from Alex and then a large set of
> >>>>> CUDA changes supplied as a single patch which were the result of Cormac analysing
> >>>>> MOL with Alex's help to try and further the boot process.
> >>>>>
> >>>>> In their previous form, the patches were unsuitable for applying upstream since
> >>>>> while they furthered MacOS 9 boot, they also caused a couple of major regressions
> >>>>> such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
> >>>>>
> >>>>> This reworked patchset fixes these regressions, includes some other clean-ups 
> >>>>> and more importantly now passes all of my OpenBIOS image boot tests with an 
> >>>>> OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
> >>>>> Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
> >>>>> I've uploaded a pre-compiled binary to 
> >>>>> https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
> >>>>> new MacOS 9 functionality.
> >>>>>
> >>>>> Apologies for the delay in sending this out on-list, however due to recent
> >>>>> circumstances I've been without a reliable broadband connection for a couple
> >>>>> of weeks. However given that this is mostly a rework of the previous patchset 
> >>>>> and looks good in testing here, I'd definitely like it to be considered for
> >>>>> application during soft freeze.
> >>>>>
> >>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>>>>
> >>>>> Alexander Graf (3):
> >>>>>   PPC: Allow Rc bit to be set on mtspr
> >>>>>   PPC: Fix lsxw bounds checks
> >>>>>   PPC: mac99: Always add USB controller
> >>>>>
> >>>>> Mark Cave-Ayland (10):
> >>>>>   cuda.c: fix CUDA ADB error packet format
> >>>>>   cuda.c: fix CUDA_PACKET response packet format
> >>>>>   cuda.c: implement simple CUDA_GET_6805_ADDR command
> >>>>>   cuda.c: implement dummy IIC access commands
> >>>>>   cuda.c: fix CUDA SR interrupt clearing
> >>>>>   cuda.c: add defines for CUDA registers
> >>>>>   cuda.c: refactor get_tb() so that the time can be passed in
> >>>>>   cuda.c: rename get_counter() state variable from s to ti for
> >>>>>     consistency
> >>>>>   cuda.c: fix T2 timer and enable its interrupt
> >>>>>   cuda.c: add delay to setting of SR_INT bit
> >>>>>
> >>>>>  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
> >>>>>  hw/ppc/mac.h            |    3 +
> >>>>>  hw/ppc/mac_newworld.c   |    3 +-
> >>>>>  target-ppc/mem_helper.c |    5 +-
> >>>>>  target-ppc/translate.c  |    2 +-
> >>>>>  5 files changed, 163 insertions(+), 93 deletions(-)
> >>>>
> >>>> Ping? Can anyone review this in Alex's absence? In the meantime I've
> >>>> added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
> >>>> good to get the GSoC work upstream for 2.5.
> >>>
> >>> Sorry I've taken a while to get to this.  It looks pretty good, though
> >>> I've sent a handful of comments on individual patches.
> >>>
> >>> I gathered from one of your replies that you do intend to do a
> >>> respin.  The current comments all look pretty trivial, so I expect
> >>> I'll be ok to apply your respin to ppc-next (which I'm looking after
> >>> in agraf's absence).  It would be nice to get a review from someone
> >>> more familiar with, or better able to test MacOS stuff.
> >>
> >> Great. I've sent a further few replies, so if you're happy with the
> >> answers let me know and I'll send a v2 tomorrow.
> > 
> > Haven't seen v2.  Did something sidetrack you, or did I manage to miss
> > it?
> 
> Hi David,
> 
> I am still waiting for feedback (and Reviewed-by tags) for a few replies
> to your initial patch review:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00855.html
> https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00856.html
> https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00862.html

Sorry, I managed to miss these.  The first two replies seem good, I'll
send a few comments on the third now.

> In particular would you like me to add the comments about MOL to the
> commit messages?

But yes, referencing MOL in the messages would be good so we can see
where this came from in the future.

> And I wasn't sure from your comments whether you were
> still looking for someone else more able to test the MacOS stuff?

Well, that would be nice, but in the absence of any volunteers, I'll
take the series anyway (provided I don't spot breakage of other
platforms, obviously).

> That said, if you're happy with my responses I can respin later today if
> you are able to send a pull request before tomorrow (which is when
> freeze hits)? I have enough free time over the next couple of weeks to
> handle any issues that may arise, although I'm fairly confident in these
> patches as I've been running them locally over my OpenBIOS test images
> for a few weeks now.
> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 00/13] Mac OS 9 compatibility improvements (upstream rework)

[Mark Cave-Ayland]

On 11/11/15 06:52, David Gibson wrote:

> On Wed, Nov 11, 2015 at 06:29:09AM +0000, Mark Cave-Ayland wrote:
>> On 11/11/15 02:11, David Gibson wrote:
>>
>>> On Wed, Nov 04, 2015 at 11:32:02PM +0000, Mark Cave-Ayland wrote:
>>>> On 04/11/15 03:44, David Gibson wrote:
>>>>
>>>>> On Fri, Oct 30, 2015 at 04:48:12PM +0000, Mark Cave-Ayland wrote:
>>>>>> On 23/10/15 14:56, Mark Cave-Ayland wrote:
>>>>>>
>>>>>>> This is a rework of Cormac O'Brien's GSoC project to try and boot MacOS 9 under
>>>>>>> QEMU, the original version of which was posted to the qemu-devel list at the
>>>>>>> end of August (https://lists.gnu.org/archive/html/qemu-devel/2015-08/msg02521.html).
>>>>>>>
>>>>>>> The patchset consisted of some simple patches from Alex and then a large set of
>>>>>>> CUDA changes supplied as a single patch which were the result of Cormac analysing
>>>>>>> MOL with Alex's help to try and further the boot process.
>>>>>>>
>>>>>>> In their previous form, the patches were unsuitable for applying upstream since
>>>>>>> while they furthered MacOS 9 boot, they also caused a couple of major regressions
>>>>>>> such as breaking the mouse and causing Darwin/OS X boot to panic on startup.
>>>>>>>
>>>>>>> This reworked patchset fixes these regressions, includes some other clean-ups 
>>>>>>> and more importantly now passes all of my OpenBIOS image boot tests with an 
>>>>>>> OpenBIOS binary from SVN trunk (separate pull request to be sent shortly).
>>>>>>> Whilst OpenBIOS still needs one additional patch to run the MacOS 9 bootloader,
>>>>>>> I've uploaded a pre-compiled binary to 
>>>>>>> https://www.ilande.co.uk/tmp/openbios-ppc for people interested in testing the 
>>>>>>> new MacOS 9 functionality.
>>>>>>>
>>>>>>> Apologies for the delay in sending this out on-list, however due to recent
>>>>>>> circumstances I've been without a reliable broadband connection for a couple
>>>>>>> of weeks. However given that this is mostly a rework of the previous patchset 
>>>>>>> and looks good in testing here, I'd definitely like it to be considered for
>>>>>>> application during soft freeze.
>>>>>>>
>>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>>>
>>>>>>> Alexander Graf (3):
>>>>>>>   PPC: Allow Rc bit to be set on mtspr
>>>>>>>   PPC: Fix lsxw bounds checks
>>>>>>>   PPC: mac99: Always add USB controller
>>>>>>>
>>>>>>> Mark Cave-Ayland (10):
>>>>>>>   cuda.c: fix CUDA ADB error packet format
>>>>>>>   cuda.c: fix CUDA_PACKET response packet format
>>>>>>>   cuda.c: implement simple CUDA_GET_6805_ADDR command
>>>>>>>   cuda.c: implement dummy IIC access commands
>>>>>>>   cuda.c: fix CUDA SR interrupt clearing
>>>>>>>   cuda.c: add defines for CUDA registers
>>>>>>>   cuda.c: refactor get_tb() so that the time can be passed in
>>>>>>>   cuda.c: rename get_counter() state variable from s to ti for
>>>>>>>     consistency
>>>>>>>   cuda.c: fix T2 timer and enable its interrupt
>>>>>>>   cuda.c: add delay to setting of SR_INT bit
>>>>>>>
>>>>>>>  hw/misc/macio/cuda.c    |  243 ++++++++++++++++++++++++++++++-----------------
>>>>>>>  hw/ppc/mac.h            |    3 +
>>>>>>>  hw/ppc/mac_newworld.c   |    3 +-
>>>>>>>  target-ppc/mem_helper.c |    5 +-
>>>>>>>  target-ppc/translate.c  |    2 +-
>>>>>>>  5 files changed, 163 insertions(+), 93 deletions(-)
>>>>>>
>>>>>> Ping? Can anyone review this in Alex's absence? In the meantime I've
>>>>>> added it to wiki at http://wiki.qemu.org/Planning/2.5 as it would be
>>>>>> good to get the GSoC work upstream for 2.5.
>>>>>
>>>>> Sorry I've taken a while to get to this.  It looks pretty good, though
>>>>> I've sent a handful of comments on individual patches.
>>>>>
>>>>> I gathered from one of your replies that you do intend to do a
>>>>> respin.  The current comments all look pretty trivial, so I expect
>>>>> I'll be ok to apply your respin to ppc-next (which I'm looking after
>>>>> in agraf's absence).  It would be nice to get a review from someone
>>>>> more familiar with, or better able to test MacOS stuff.
>>>>
>>>> Great. I've sent a further few replies, so if you're happy with the
>>>> answers let me know and I'll send a v2 tomorrow.
>>>
>>> Haven't seen v2.  Did something sidetrack you, or did I manage to miss
>>> it?
>>
>> Hi David,
>>
>> I am still waiting for feedback (and Reviewed-by tags) for a few replies
>> to your initial patch review:
>>
>> https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00855.html
>> https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00856.html
>> https://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg00862.html
> 
> Sorry, I managed to miss these.  The first two replies seem good, I'll
> send a few comments on the third now.
> 
>> In particular would you like me to add the comments about MOL to the
>> commit messages?
> 
> But yes, referencing MOL in the messages would be good so we can see
> where this came from in the future.
> 
>> And I wasn't sure from your comments whether you were
>> still looking for someone else more able to test the MacOS stuff?
> 
> Well, that would be nice, but in the absence of any volunteers, I'll
> take the series anyway (provided I don't spot breakage of other
> platforms, obviously).

Thanks a lot for taking another look at these. I've just arrived in work
now (ahhh timezones!) but will aim to get the updated v2 based upon your
comments out either lunchtime or early evening today at the latest.


Many thanks,

Mark.

Re: [Qemu-devel] [PATCH 12/13] cuda.c: fix T2 timer and enable its interrupt

[Mark Cave-Ayland]

On 11/11/15 06:52, David Gibson wrote:

> On Wed, Nov 04, 2015 at 11:25:43PM +0000, Mark Cave-Ayland wrote:
>> On 04/11/15 03:40, David Gibson wrote:
>>
>>> On Fri, Oct 23, 2015 at 02:56:37PM +0100, Mark Cave-Ayland wrote:
>>>> Fix the counter loading logic and enable the T2 interrupt when the timer
>>>> expires.
>>>
>>> A mention of what uses T2, and therefore why this is useful would be
>>> good.
>>
>> There is a good chance that nothing has used T2 before MacOS 9 since
>> before this patch, it is impossible for the T2 timer interrupt to fire.
>> It can be seen that MacOS 9 does write to the relevant registers during
>> boot, and if the T2 interrupt is disabled then boot will hang.
> 
> Sorry, I wasn't clear.  My point is that anyone looking back on this
> commit in the future won't necessarily have the context of the series
> its part of here.  So, the individual commit message should explain
> that T2 is being enabled in order to support MacOS9.
> 
> *I* know this is about MacOS9 enablement, but that's not clear from the
> commit & message itself.

Okay I've added a reference to MacOS 9 in the updated commit message.

>>
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>  hw/misc/macio/cuda.c |   30 +++++++++++++++++++++---------
>>>>  1 file changed, 21 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
>>>> index 687cb54..d864b24 100644
>>>> --- a/hw/misc/macio/cuda.c
>>>> +++ b/hw/misc/macio/cuda.c
>>>> @@ -136,7 +136,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
>>>>  
>>>>  static void cuda_update_irq(CUDAState *s)
>>>>  {
>>>> -    if (s->ifr & s->ier & (SR_INT | T1_INT)) {
>>>> +    if (s->ifr & s->ier & (SR_INT | T1_INT | T2_INT)) {
>>>>          qemu_irq_raise(s->irq);
>>>>      } else {
>>>>          qemu_irq_lower(s->irq);
>>>> @@ -175,7 +175,7 @@ static unsigned int get_counter(CUDATimer *ti)
>>>>  
>>>>  static void set_counter(CUDAState *s, CUDATimer *ti, unsigned int val)
>>>>  {
>>>> -    CUDA_DPRINTF("T%d.counter=%d\n", 1 + (ti->timer == NULL), val);
>>>> +    CUDA_DPRINTF("T%d.counter=%d\n", 1 + ti->index, val);
>>>>      ti->load_time = get_tb(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>>>>                             s->frequency);
>>>>      ti->counter_value = val;
>>>> @@ -220,7 +220,7 @@ static void cuda_timer_update(CUDAState *s, CUDATimer *ti,
>>>>  {
>>>>      if (!ti->timer)
>>>>          return;
>>>> -    if ((s->acr & T1MODE) != T1MODE_CONT) {
>>>> +    if (ti->index == 0 && (s->acr & T1MODE) != T1MODE_CONT) {
>>>>          timer_del(ti->timer);
>>>>      } else {
>>>>          ti->next_irq_time = get_next_irq_time(ti, current_time);
>>>> @@ -238,6 +238,16 @@ static void cuda_timer1(void *opaque)
>>>>      cuda_update_irq(s);
>>>>  }
>>>>  
>>>> +static void cuda_timer2(void *opaque)
>>>> +{
>>>> +    CUDAState *s = opaque;
>>>> +    CUDATimer *ti = &s->timers[1];
>>>> +
>>>> +    cuda_timer_update(s, ti, ti->next_irq_time);
>>>> +    s->ifr |= T2_INT;
>>>> +    cuda_update_irq(s);
>>>> +}
>>>> +
>>>>  static uint32_t cuda_readb(void *opaque, hwaddr addr)
>>>>  {
>>>>      CUDAState *s = opaque;
>>>> @@ -276,6 +286,7 @@ static uint32_t cuda_readb(void *opaque, hwaddr addr)
>>>>      case CUDA_REG_T2CL:
>>>>          val = get_counter(&s->timers[1]) & 0xff;
>>>>          s->ifr &= ~T2_INT;
>>>> +        cuda_update_irq(s);
>>>>          break;
>>>>      case CUDA_REG_T2CH:
>>>>          val = get_counter(&s->timers[1]) >> 8;
>>>> @@ -352,11 +363,12 @@ static void cuda_writeb(void *opaque, hwaddr addr, uint32_t val)
>>>>          cuda_timer_update(s, &s->timers[0], qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
>>>>          break;
>>>>      case CUDA_REG_T2CL:
>>>> -        s->timers[1].latch = val;
>>>> -        set_counter(s, &s->timers[1], val);
>>>> +        s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>>>>          break;
>>>>      case CUDA_REG_T2CH:
>>>> -        set_counter(s, &s->timers[1], (val << 8) | s->timers[1].latch);
>>>> +        s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
>>>> +        s->ifr &= ~T2_INT;
>>>> +        set_counter(s, &s->timers[1], s->timers[1].latch);
>>>
>>> So the new code appears to be like that for T1CL / T1CH, which makes
>>> sense.  However, T1CL has a cuda_timer_update() call.  Do you also
>>> need that for T2CL?
>>
>> This is a side-effect of combining the T1 and T2 code. Unlike T1, T2
>> appears to be free-running from its written value but generating an
>> interrupt just after zero-crossing. So in order to get the correct
>> interval, we write the value to the latch instead of the counter to get
>> the same effect with the shared timer code.
> 
> Ok.  I think you need to put that paragraph into a comment - without
> that subtle background information, it looks like an obvious bug.

Done.

>>>>          break;
>>>>      case CUDA_REG_SR:
>>>>          s->sr = val;
>>>> @@ -719,8 +731,7 @@ static void cuda_reset(DeviceState *dev)
>>>>      s->timers[0].latch = 0xffff;
>>>>      set_counter(s, &s->timers[0], 0xffff);
>>>>  
>>>> -    s->timers[1].latch = 0;
>>>> -    set_counter(s, &s->timers[1], 0xffff);
>>>> +    s->timers[1].latch = 0xffff;
>>>>  }
>>>>  
>>>>  static void cuda_realizefn(DeviceState *dev, Error **errp)
>>>> @@ -730,7 +741,8 @@ static void cuda_realizefn(DeviceState *dev, Error **errp)
>>>>  
>>>>      s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer1, s);
>>>>      s->timers[0].frequency = s->frequency;
>>>> -    s->timers[1].frequency = s->frequency;
>>>> +    s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, cuda_timer2, s);
>>>> +    s->timers[1].frequency = (SCALE_US * 6000) / 4700;
>>>
>>> Where does this T2 frequency come from?
>>
>> My understanding of this is that with the shared timer code, the IRQ
>> timing is calculated based upon CUDA_TIMER_FREQ (4.7MHz / 6). Therefore
>> by setting the frequency to the inverse value ((SCALE_US * 6000) / 4700)
>> then this cancels out the effect of the timebase calculation algorithm
>> used in the counters. I believe this came from Alex so he would likely
>> be able to clarify this and give a much better explanation.
> 
> Hmm, yeah, I'm having trouble following that.  I won't hold the series
> up over it, but a clearer rationale would help here.
> 
> I take it that T2 has a fixed frequency, whereas T1 is adjustable,
> based on the code snippet above?

Yes, my understanding from studying the code is that both timers run off
a fixed frequency clock with the difference being that T1 allows both
the counter and latch to be updated (with an optional continuous mode),
while T2 is free running. Both timers can be configured to interrupt
just after the zero-crossing point if required.


ATB,

Mark.