[OpenBIOS] [PATCH RFC] ppc: fix CUDA ADB packet header format

[Mark Cave-Ayland]

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 at 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/26a9dfedd107a8716e833f2e959500c0b8a435e7
> 
> 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.