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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

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.

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.

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.


HTH,

Mark.