[PATCH RFC] ppc: fix CUDA ADB packet header format
Previous versions of QEMU use a 2-byte header for CUDA ADB packets where it should have used a 3-byte one, so this commit allows cuda_adb_req() to differentiate between the two formats and act accordingly. Signed-off-by: Cormac O'Brien <cormac@c-obrien.org> --- drivers/cuda.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/cuda.c b/drivers/cuda.c index 9555dea..deceacf 100644 --- a/drivers/cuda.c +++ b/drivers/cuda.c @@ -144,8 +144,18 @@ static int cuda_adb_req (void *host, const uint8_t *snd_buf, int len, // CUDA_DPRINTF("len: %d %02x\n", len, snd_buf[0]); len = cuda_request(host, ADB_PACKET, snd_buf, len, buffer); if (len > 1 && buffer[0] == ADB_PACKET) { - pos = buffer + 2; - len -= 2; + /* QEMU's previous model used a 2-byte header where it should use a + * 3-byte one, so we check to see what kind of header we ought to use. + */ + if (len > 2 && buffer[2] == snd_buf[0]) { + /* Correct 3-byte header */ + pos = buffer + 3; + len -= 3; + } else { + /* Old 2-byte header */ + pos = buffer + 2; + len -= 2; + } } else { pos = buffer + 1; len = -1; -- 2.4.5
On 10/07/15 18:49, Cormac O'Brien wrote:
Previous versions of QEMU use a 2-byte header for CUDA ADB packets where it should have used a 3-byte one, so this commit allows cuda_adb_req() to differentiate between the two formats and act accordingly.
Signed-off-by: Cormac O'Brien <cormac@c-obrien.org> --- drivers/cuda.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/cuda.c b/drivers/cuda.c index 9555dea..deceacf 100644 --- a/drivers/cuda.c +++ b/drivers/cuda.c @@ -144,8 +144,18 @@ static int cuda_adb_req (void *host, const uint8_t *snd_buf, int len, // CUDA_DPRINTF("len: %d %02x\n", len, snd_buf[0]); len = cuda_request(host, ADB_PACKET, snd_buf, len, buffer); if (len > 1 && buffer[0] == ADB_PACKET) { - pos = buffer + 2; - len -= 2; + /* QEMU's previous model used a 2-byte header where it should use a + * 3-byte one, so we check to see what kind of header we ought to use. + */ + if (len > 2 && buffer[2] == snd_buf[0]) { + /* Correct 3-byte header */ + pos = buffer + 3; + len -= 3; + } else { + /* Old 2-byte header */ + pos = buffer + 2; + len -= 2; + } } else { pos = buffer + 1; len = -1;
Technically the patch looks good to me - due to the nature of the change, can you confirm that this patch doesn't cause regressions in any other OS images during testing? Even better, is there a reference to a 3-byte header in any of the open OS sources to confirm that this is definitely the correct solution? The only change I would like to see is a reference to specific QEMU versions in the comments and commit log, so instead of referencing "QEMU's previous model" it would be better to state "QEMU versions <= 2.4" instead. This just makes it a bit easier to understand to implications of removing the hack at a later date. ATB, Mark.
Le 12/07/2015 14:34, Mark Cave-Ayland a écrit :
On 10/07/15 18:49, Cormac O'Brien wrote:
Previous versions of QEMU use a 2-byte header for CUDA ADB packets where it should have used a 3-byte one, so this commit allows cuda_adb_req() to differentiate between the two formats and act accordingly.
Signed-off-by: Cormac O'Brien <cormac@c-obrien.org> --- drivers/cuda.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/cuda.c b/drivers/cuda.c index 9555dea..deceacf 100644 --- a/drivers/cuda.c +++ b/drivers/cuda.c @@ -144,8 +144,18 @@ static int cuda_adb_req (void *host, const uint8_t *snd_buf, int len, // CUDA_DPRINTF("len: %d %02x\n", len, snd_buf[0]); len = cuda_request(host, ADB_PACKET, snd_buf, len, buffer); if (len > 1 && buffer[0] == ADB_PACKET) { - pos = buffer + 2; - len -= 2; + /* QEMU's previous model used a 2-byte header where it should use a + * 3-byte one, so we check to see what kind of header we ought to use. + */ + if (len > 2 && buffer[2] == snd_buf[0]) { + /* Correct 3-byte header */ + pos = buffer + 3; + len -= 3; + } else { + /* Old 2-byte header */ + pos = buffer + 2; + len -= 2; + } } else { pos = buffer + 1; len = -1;
Technically the patch looks good to me - due to the nature of the change, can you confirm that this patch doesn't cause regressions in any other OS images during testing? Even better, is there a reference to a 3-byte header in any of the open OS sources to confirm that this is definitely the correct solution?
Perhaps Alex can explain his changes in QEMU: https://github.com/agraf/qemu/commit/26a9dfedd107a8716e833f2e959500c0b8a435e... But in kernel I see only 2-byte header: http://lxr.free-electrons.com/source/drivers/macintosh/adb.c#L424 http://lxr.free-electrons.com/source/drivers/macintosh/via-cuda.c#L354 The first byte is ADB_PACKET (0) followed by the ADB command. ADB command is a 8 bit command packet (4 bit address + 2 bit command + 2 bit register), followed by 2 to 8 bytes data packet. We can find some pseudo-3-bytes header in AppleCuda source, but it's not: http://www.opensource.apple.com/tarballs/AppleCuda/AppleCuda-100.0.3.tar.gz IOReturn IOCudaADBController::setAutoPollEnable ( bool enable ) { cuda_request_t cmd; adb_init_request(&cmd); ADB_BUILD_CMD3(&cmd, ADB_PACKET_PSEUDO, ADB_PSEUDOCMD_START_STOP_AUTO_POLL, (enable ? 1 : 0)); return CudaDriver->doSyncRequest(&cmd); } But ADB_PACKET_PSEUDO is 1, ADB_PSEUDOCMD_START_STOP_AUTO_POLL is 1. In Linux we have: http://lxr.free-electrons.com/source/drivers/macintosh/via-cuda.c#L137 137 cuda_request(&req, NULL, 3, CUDA_PACKET, CUDA_AUTOPOLL, 1); where CUDA_PACKET is 1, and CUDA_AUTOPOLL is 1. So Apple ADB_PACKET_PSEUDO == Linux CUDA_PACKET. IMHO, it's not 3-byte header for ADB, but for the cuda controller. In AppleCuda source, ADB read and write are only done with 2-byte header: IOCudaADBController::readFromDevice () ... ADB_BUILD_CMD2(&cmd, ADB_PACKET_ADB, (ADB_ADBCMD_READ_ADB | (address << 4) | (adbRegister & 3))); ... IOCudaADBController::writeToDevice () ... ADB_BUILD_CMD2(&cmd, ADB_PACKET_ADB, (ADB_ADBCMD_WRITE_ADB | (address << 4) | (adbRegister & 3))); ... Laurent
On 12/07/15 22:05, Laurent Vivier wrote:
Le 12/07/2015 14:34, Mark Cave-Ayland a écrit :
On 10/07/15 18:49, Cormac O'Brien wrote:
Previous versions of QEMU use a 2-byte header for CUDA ADB packets where it should have used a 3-byte one, so this commit allows cuda_adb_req() to differentiate between the two formats and act accordingly.
Signed-off-by: Cormac O'Brien <cormac@c-obrien.org> --- drivers/cuda.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/cuda.c b/drivers/cuda.c index 9555dea..deceacf 100644 --- a/drivers/cuda.c +++ b/drivers/cuda.c @@ -144,8 +144,18 @@ static int cuda_adb_req (void *host, const uint8_t *snd_buf, int len, // CUDA_DPRINTF("len: %d %02x\n", len, snd_buf[0]); len = cuda_request(host, ADB_PACKET, snd_buf, len, buffer); if (len > 1 && buffer[0] == ADB_PACKET) { - pos = buffer + 2; - len -= 2; + /* QEMU's previous model used a 2-byte header where it should use a + * 3-byte one, so we check to see what kind of header we ought to use. + */ + if (len > 2 && buffer[2] == snd_buf[0]) { + /* Correct 3-byte header */ + pos = buffer + 3; + len -= 3; + } else { + /* Old 2-byte header */ + pos = buffer + 2; + len -= 2; + } } else { pos = buffer + 1; len = -1;
Technically the patch looks good to me - due to the nature of the change, can you confirm that this patch doesn't cause regressions in any other OS images during testing? Even better, is there a reference to a 3-byte header in any of the open OS sources to confirm that this is definitely the correct solution?
Perhaps Alex can explain his changes in QEMU:
https://github.com/agraf/qemu/commit/26a9dfedd107a8716e833f2e959500c0b8a435e...
But in kernel I see only 2-byte header:
http://lxr.free-electrons.com/source/drivers/macintosh/adb.c#L424 http://lxr.free-electrons.com/source/drivers/macintosh/via-cuda.c#L354
The first byte is ADB_PACKET (0) followed by the ADB command.
ADB command is a 8 bit command packet (4 bit address + 2 bit command + 2 bit register), followed by 2 to 8 bytes data packet.
We can find some pseudo-3-bytes header in AppleCuda source, but it's not:
http://www.opensource.apple.com/tarballs/AppleCuda/AppleCuda-100.0.3.tar.gz
IOReturn IOCudaADBController::setAutoPollEnable ( bool enable ) { cuda_request_t cmd;
adb_init_request(&cmd); ADB_BUILD_CMD3(&cmd, ADB_PACKET_PSEUDO, ADB_PSEUDOCMD_START_STOP_AUTO_POLL, (enable ? 1 : 0));
return CudaDriver->doSyncRequest(&cmd); }
But ADB_PACKET_PSEUDO is 1, ADB_PSEUDOCMD_START_STOP_AUTO_POLL is 1.
In Linux we have:
http://lxr.free-electrons.com/source/drivers/macintosh/via-cuda.c#L137
137 cuda_request(&req, NULL, 3, CUDA_PACKET, CUDA_AUTOPOLL, 1);
where CUDA_PACKET is 1, and CUDA_AUTOPOLL is 1. So Apple ADB_PACKET_PSEUDO == Linux CUDA_PACKET.
IMHO, it's not 3-byte header for ADB, but for the cuda controller.
In AppleCuda source, ADB read and write are only done with 2-byte header:
IOCudaADBController::readFromDevice () ... ADB_BUILD_CMD2(&cmd, ADB_PACKET_ADB, (ADB_ADBCMD_READ_ADB | (address << 4) | (adbRegister & 3))); ... IOCudaADBController::writeToDevice () ... ADB_BUILD_CMD2(&cmd, ADB_PACKET_ADB, (ADB_ADBCMD_WRITE_ADB | (address << 4) | (adbRegister & 3))); ...
I think you're right here - it's the CUDA encapsulation of the ADB protocol which is suspect.
From the links you included above, it seems like the correct thing to do is to detect ADB_PACKET_PSEUDO as opposed to ADB_PACKET and use that to determine that we need to handle a 3-byte payload instead of looking at the buffer contents.
Alex: so I believe your fix to QEMU's cuda_receive_packet() is correct to bump the packet length to 3 for CUDA_AUTOPOLL, it's just the corresponding OpenBIOS patch that needs to be tweaked to handle ADB_PACKET_PSEUDO accordingly. ATB, Mark.
participants (3)
-
Cormac O'Brien -
Laurent Vivier -
Mark Cave-Ayland