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;}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.