Re: [Qemu-devel] [Qemu-ppc] macio ide question/bug report

[BALATON Zoltan <balaton@eik.bme.hu>]

On Mon, 12 May 2014, Mark Cave-Ayland wrote:
> On 10/05/14 13:30, BALATON Zoltan wrote:
>> That patch would be 80fc95d8bdaf3392106b131a97ca701fd374489a in QEMU
>> master. I've tried reverting it and Darwin still boots (without -M
>> mac99) up to the point where it asks to install as before but I don't
>> know how good a test is this as I'm not sure it does unaligned accesses
>> up to this point. I see accesses like this:
>> 
>> pmac_ide_transfer(ATAPI) lba=ffffffff, buffer_index=30, len=930
>> pmac_ide_transfer(ATAPI) lba=0, buffer_index=0, len=930
>
> Here is an example of an unaligned access, where the length is not a multiple 
> of 0x200 (512 bytes) which was required for the block layer DMA APIs at the 
> time it was written. If you can get to the same point with the patch 
> reverted, then it means Kevin's patchset for byte-level rather than 
> sector-level granularity for block requests is working.

These logs were with the above commit reverted so it's probably not needed 
any more. I've tried booting Darwin 6.0.2, Darwin 8.0.1 up to the 
installation (did not install it) and Finnix 109 and they seem to work as 
before.

> What's important is the sum of the start_input size descriptors now matches 
> the size of the ATA request, so the dma_* callbacks can now be used directly 
> with byte precision rather than clobbering the memory up to the next multiple 
> of 0x200.
>
>> This however does not fix MorphOS which fails as before. I've tried this
>> patch too (on top of the previous revert) but that did not work either:
>> 
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index d3395f4..bce14fc 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -56,6 +56,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque,
>> int ret)
>>       DBDMA_io *io = opaque;
>>       MACIOIDEState *m = io->opaque;
>>       IDEState *s = idebus_active_if(&m->bus);
>> +    int64_t sector_num;
>>
>>       if (ret < 0) {
>>           m->aiocb = NULL;
>> @@ -107,13 +108,16 @@ static void pmac_ide_atapi_transfer_cb(void
>> *opaque, int r
>>       qemu_sglist_add(&s->sg, io->addr, io->len);
>>       io->addr += io->len;
>>       io->len = 0;
>> -
>> -    MACIO_DPRINTF("sector_num=%d size=%d, cmd_cmd=%d\n",
>> -                  (s->lba << 2) + (s->io_buffer_index >> 9),
>> +    if (s->lba >= 0)
>> +        sector_num = (s->lba << 2) + (s->io_buffer_index >> 9);
>> +    else
>> +        sector_num = s->lba;
>> +    MACIO_DPRINTF("sector_num=%ld size=%d, cmd_cmd=%d\n",
>> +                  sector_num,
>>                     s->packet_transfer_size, s->dma_cmd);
>>
>>       m->aiocb = dma_bdrv_read(s->bs, &s->sg,
>> -                             (int64_t)(s->lba << 2) +
>> (s->io_buffer_index >> 9)
>> +                             sector_num,
>>                                pmac_ide_atapi_transfer_cb, io);
>>       return;
>> 
>> This still produces an error in MorphOS as before:
>> 
>> DBDMA: writel 0x0000000000000d0c <= 0x00e5ac80
>> DBDMA: channel 0x1a reg 0x3
>> DBDMA: dbdma_cmdptr_load 0x00e5ac80
>> ATAPI limit=0x8000 packet: 43 00 00 00 00 00 00 03 24 00 00 00
>> 
>> DBDMA: DBDMA_run_bh
>> DBDMA: writel 0x0000000000000d00 <= 0x80008000
>> DBDMA: channel 0x1a reg 0x0
>> DBDMA:     status 0x00008400
>> DBDMA: readl 0x0000000000000d00 => 0x80008000
>> DBDMA: channel 0x1a reg 0x0
>> DBDMA: DBDMA_run_bh
>> DBDMA: channel_run
>> dbdma_cmd 0x7f7dddff5ae0
>>      req_count 0x0324
>>      command 0x3000
>>      phy_addr 0x00e7dddc
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: start_input
>> DBDMA: addr 0xe7dddc key 0x0
>> pmac_ide_transfer(ATAPI) lba=ffffffff, buffer_index=0, len=324
>
> ^^^^^
>
>> io_buffer_size = 0
>> io->len = 0x324
>> sector_num=-1 size=20, cmd_cmd=0
>> atapi_cmd_error: sense=0x5 asc=0x21
>> done DMA
>> DBDMA: dbdma_end
>> 
>> Interestingly a similar read works for Darwin which does this:
>> 
>> ATAPI limit=0x0 packet: bb 00 ff ff 00 00 00 00 00 00 00 00
>> ATAPI limit=0xfffe packet: 43 02 00 00 00 00 00 ff fe 80 00 00
>> reply: tx_size=48 elem_tx_size=0 index=0
>> byte_count_limit=65534
>> status=0x58
>> reply: tx_size=0 elem_tx_size=0 index=48
>> status=0x50
>> ATAPI limit=0x30 packet: 43 02 00 00 00 00 00 00 30 80 00 00
>> reply: tx_size=48 elem_tx_size=0 index=0
>> byte_count_limit=48
>> status=0x58
>> reply: tx_size=0 elem_tx_size=0 index=48
>> status=0x50
>> DBDMA: readl 0x0000000000000d04 => 0x00000000
>> DBDMA: channel 0x1a reg 0x1
>> DBDMA: writel 0x0000000000000d08 <= 0x00000000
>> DBDMA: channel 0x1a reg 0x2
>> DBDMA: writel 0x0000000000000d0c <= 0x011f8010
>> DBDMA: channel 0x1a reg 0x3
>> DBDMA: dbdma_cmdptr_load 0x011f8010
>> DBDMA: writel 0x0000000000000d00 <= 0x90009000
>> DBDMA: channel 0x1a reg 0x0
>> DBDMA:     status 0x00009400
>> DBDMA: DBDMA_run_bh
>> DBDMA: channel_run
>> dbdma_cmd 0x7ffa05173c60
>>      req_count 0x0930
>>      command 0x2000
>>      phy_addr 0x017cc000
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: start_input
>> DBDMA: addr 0x17cc000 key 0x0
>> pmac_ide_transfer(ATAPI) lba=ffffffff, buffer_index=30, len=930
>> waiting for data (0x3 - 0x930 - 50)
>
> ^^^^^
>
>> ATAPI limit=0x930 packet: be 00 00 00 00 00 00 00 01 f8 00 00
>> read dma: LBA=0 nb_sectors=1
>> 
>> DBDMA: DBDMA_run_bh
>> DBDMA: channel_run
>> dbdma_cmd 0x7ffa05173c60
>>      req_count 0x0930
>>      command 0x2000
>>      phy_addr 0x017cc000
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: start_input
>> DBDMA: addr 0x17cc000 key 0x0
>> pmac_ide_transfer(ATAPI) lba=0, buffer_index=0, len=930
>> io_buffer_size = 0
>> io->len = 0x930
>> sector_num=0 size=2352, cmd_cmd=0
>> io_buffer_size = 0x930
>> end of transfer
>> end of DMA
>> done DMA
>> DBDMA: dbdma_end
>
> From the limited block layer knowledge I have, it seems that lba == -1 is a 
> default value meaning the ATA request hasn't been submitted.

I don't know but according to hw/ide/atapi.c lba=-1 is a special value 
meaning that no sector needs to be read but only the results are 
requested. I don't know if this is only an atapi thing or how the block 
layer handles it but I believe that it is used for reading the TOC from 
the disk.

> Take a look at the lines I marked above: it looks to me as if in the 2nd 
> request, the DMA controller is running but realises that it is still waiting 
> for the ATA request to complete before continuing which is why you see 
> "waiting for data" followed by that ATAPI packet. In your first case, 
> pmac_ide_transfer() incorrectly believes the ATA request has already 
> completed which is why it tries to continue and then chokes on the invalid 
> lba value of -1.

MorphOS and Darwin are definitely doing things differently. I hope someone 
who understands what is happening can explain it why one of them works 
while the other doesn't.

Regards,
BALATON Zoltan