diff mbox

[PULL,075/118] macio: handle non-block ATAPI DMA transfers

Message ID 53A44548.7020605@ilande.co.uk
State New
Headers show

Commit Message

Mark Cave-Ayland June 20, 2014, 2:29 p.m. UTC
On 04/06/14 13:44, Alexander Graf wrote:

> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Currently the macio DMA routines assume that all DMA requests are for read/write
> block transfers. This is not always the case for ATAPI, for example when
> requesting a TOC where the response is generated directly in the IDE buffer.
>
> Detect these non-block ATAPI DMA transfers (where no lba is specified in the
> command) and copy the results directly into RAM as indicated by the DBDMA
> descriptor. This fixes CDROM access under MorphOS.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Alexander Graf <agraf@suse.de>

I've just done a complete round of OpenBIOS tests and it looks as if 
this patch introduces random hang failures (60% or so) into my Darwin 
image boot tests.

The issue seems to be related as to the timing of the DMA callback 
relative to reception of the IDE command; if the IDE transfer is invoked 
first (as happens in Darwin) then we hang after sending the IDE command 
which is likely because the DMA is not being completed correctly. There 
is also another difference with MorphOS in that when the IDE transfer 
callback is invoked first then s->packet_transfer_size == 0 and so the 
memory copy would always copy 0 bytes.

After some experimentation, I've come up with the attached patch which 
should retain the DMA memory copy, but instead invokes a further round 
of pmac_ide_atapi_transfer_cb() with io->len == 0 and s->io_buffer_size 
== 0 which should terminate the DMA request correctly. At the very least 
it seems to fix the hang on boot with my Darwin images.

Zoltan, please can you test the attached patch to see if this still 
allows MorphOS to boot?


ATB,

Mark.

Comments

BALATON Zoltan June 20, 2014, 7:17 p.m. UTC | #1
On Fri, 20 Jun 2014, Mark Cave-Ayland wrote:
> Zoltan, please can you test the attached patch to see if this still allows 
> MorphOS to boot?

Unfortunately it seems MorphOS cannot boot with this patch. It hangs while 
trying to read the TOC from the CD. Debug output with DEBUG_MACIO and 
DEBUG_DBDMA enabled shows:

DBDMA: writel 0x0000000000000d0c <= 0x00e51970
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e51970

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 0x7f0997120f28
     req_count 0x0324
     command 0x3000
     phy_addr 0x00e7b0bc
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe7b0bc key 0x0

non-block ATAPI DMA transfer size: 804
io_buffer_size = 0
remainder: 0 io->len: 0 size: 20
end of DMA
done DMA
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e51970
DBDMA: xfer_status 0x00008400 res_count 0x0000
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e51980
DBDMA: channel_run
dbdma_cmd 0x7f0997120f28
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000

and no further ide activity from here whereas without the patch when it 
boots I see these logs:

DBDMA: writel 0x0000000000000d0c <= 0x00e50090
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e50090

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 0x7f56695a7f28
     req_count 0x0324
     command 0x3000
     phy_addr 0x00e4f8fc
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe4f8fc key 0x0

non-block ATAPI DMA transfer size: 20
end of non-block ATAPI DMA transfer
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e50090
DBDMA: xfer_status 0x00008400 res_count 0x0324
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e500a0
DBDMA: channel_run
dbdma_cmd 0x7f56695a7f28
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: writel 0x0000000000000d00 <= 0x98000000
DBDMA: channel 0x1a reg 0x0
DBDMA:     status 0x00000000
DBDMA: writel 0x0000000000000d0c <= 0x00e50090
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e50090

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 0x7f56695a7f28
     req_count 0x0800
     command 0x3000
     phy_addr 0x00e8d7c0
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe8d7c0 key 0x0

io_buffer_size = 0
remainder: 0 io->len: 2048 size: 2048
io->len = 0x800
set remainder to: 0
sector_num=64 size=2048, cmd_cmd=0
io_buffer_size = 0x800
remainder: 0 io->len: 0 size: 0
end of transfer
end of DMA
done DMA
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e50090
DBDMA: xfer_status 0x00008400 res_count 0x0000
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e500a0
DBDMA: channel_run
dbdma_cmd 0x7f56695a7f28
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000

and a lot of similar stuff after this. If this is not enough to understand 
the problem and you need more details please tell me what to look for.

Regards,
BALATON Zoltan
Mark Cave-Ayland June 20, 2014, 7:27 p.m. UTC | #2
On 20/06/14 20:17, BALATON Zoltan wrote:

> On Fri, 20 Jun 2014, Mark Cave-Ayland wrote:
>> Zoltan, please can you test the attached patch to see if this still
>> allows MorphOS to boot?
>
> Unfortunately it seems MorphOS cannot boot with this patch. It hangs
> while trying to read the TOC from the CD. Debug output with DEBUG_MACIO
> and DEBUG_DBDMA enabled shows:

And also with ATAPI debugging enabled? I suspect the problem is with the 
interaction between the DMA/ATAPI systems again.

> DBDMA: writel 0x0000000000000d0c <= 0x00e51970
> DBDMA: channel 0x1a reg 0x3
> DBDMA: dbdma_cmdptr_load 0x00e51970
>
> 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 0x7f0997120f28
>      req_count 0x0324
>      command 0x3000
>      phy_addr 0x00e7b0bc
>      cmd_dep 0x00000000
>      res_count 0x0000
>      xfer_status 0x0000
> DBDMA: start_input
> DBDMA: addr 0xe7b0bc key 0x0
>
> non-block ATAPI DMA transfer size: 804
> io_buffer_size = 0
> remainder: 0 io->len: 0 size: 20
> end of DMA
> done DMA
> DBDMA: dbdma_end
> DBDMA: conditional_wait
> DBDMA: dbdma_cmdptr_save 0x00e51970
> DBDMA: xfer_status 0x00008400 res_count 0x0000
> DBDMA: conditional_interrupt
> DBDMA: conditional_branch
> DBDMA: dbdma_cmdptr_load 0x00e51980
> DBDMA: channel_run
> dbdma_cmd 0x7f0997120f28
>      req_count 0x0000
>      command 0x7000
>      phy_addr 0x00000000
>      cmd_dep 0x00000000
>      res_count 0x0000
>      xfer_status 0x0000
>
> and no further ide activity from here whereas without the patch when it
> boots I see these logs:
>
> DBDMA: writel 0x0000000000000d0c <= 0x00e50090
> DBDMA: channel 0x1a reg 0x3
> DBDMA: dbdma_cmdptr_load 0x00e50090
>
> 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 0x7f56695a7f28
>      req_count 0x0324
>      command 0x3000
>      phy_addr 0x00e4f8fc
>      cmd_dep 0x00000000
>      res_count 0x0000
>      xfer_status 0x0000
> DBDMA: start_input
> DBDMA: addr 0xe4f8fc key 0x0
>
> non-block ATAPI DMA transfer size: 20
> end of non-block ATAPI DMA transfer
> DBDMA: dbdma_end
> DBDMA: conditional_wait
> DBDMA: dbdma_cmdptr_save 0x00e50090
> DBDMA: xfer_status 0x00008400 res_count 0x0324
> DBDMA: conditional_interrupt
> DBDMA: conditional_branch
> DBDMA: dbdma_cmdptr_load 0x00e500a0
> DBDMA: channel_run
> dbdma_cmd 0x7f56695a7f28
>      req_count 0x0000
>      command 0x7000
>      phy_addr 0x00000000
>      cmd_dep 0x00000000
>      res_count 0x0000
>      xfer_status 0x0000
> DBDMA: writel 0x0000000000000d00 <= 0x98000000
> DBDMA: channel 0x1a reg 0x0
> DBDMA:     status 0x00000000
> DBDMA: writel 0x0000000000000d0c <= 0x00e50090
> DBDMA: channel 0x1a reg 0x3
> DBDMA: dbdma_cmdptr_load 0x00e50090
>
> 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 0x7f56695a7f28
>      req_count 0x0800
>      command 0x3000
>      phy_addr 0x00e8d7c0
>      cmd_dep 0x00000000
>      res_count 0x0000
>      xfer_status 0x0000
> DBDMA: start_input
> DBDMA: addr 0xe8d7c0 key 0x0
>
> io_buffer_size = 0
> remainder: 0 io->len: 2048 size: 2048
> io->len = 0x800
> set remainder to: 0
> sector_num=64 size=2048, cmd_cmd=0
> io_buffer_size = 0x800
> remainder: 0 io->len: 0 size: 0
> end of transfer
> end of DMA
> done DMA
> DBDMA: dbdma_end
> DBDMA: conditional_wait
> DBDMA: dbdma_cmdptr_save 0x00e50090
> DBDMA: xfer_status 0x00008400 res_count 0x0000
> DBDMA: conditional_interrupt
> DBDMA: conditional_branch
> DBDMA: dbdma_cmdptr_load 0x00e500a0
> DBDMA: channel_run
> dbdma_cmd 0x7f56695a7f28
>      req_count 0x0000
>      command 0x7000
>      phy_addr 0x00000000
>      cmd_dep 0x00000000
>      res_count 0x0000
>      xfer_status 0x0000
>
> and a lot of similar stuff after this. If this is not enough to
> understand the problem and you need more details please tell me what to
> look for.

I'm afraid as you're the only person that can boot MorphOS this far then 
we need you to diagnose and suggest a suitable alternative by comparing 
the before and after output. Since MacOS is already a supported client 
then if no solution can be found then it is likely that this patch will 
be reverted :(

In order for a patch to be accepted then it will need to both pass your 
MorphOS tests and also boot darwinppc-602.iso which should cover the two 
cases of either the ATAPI or the DMA transfer occurring first.


HTH,

Mark.
BALATON Zoltan June 21, 2014, 12:57 a.m. UTC | #3
On Fri, 20 Jun 2014, Mark Cave-Ayland wrote:
> And also with ATAPI debugging enabled? I suspect the problem is with the 
> interaction between the DMA/ATAPI systems again.

I forgot that one. I've now rerun with DEBUG_IDE_ATAPI enabled. Here is 
with the patch failing:

DBDMA: writel 0x0000000000000d0c <= 0x00e599f0
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e599f0
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 0x7f4cf1325228
     req_count 0x0324
     command 0x3000
     phy_addr 0x00e57d74
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe57d74 key 0x0

non-block ATAPI DMA transfer size: 804
io_buffer_size = 0
remainder: 0 io->len: 0 size: 20
end of DMA
done DMA
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e599f0
DBDMA: xfer_status 0x00008400 res_count 0x0000
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e59a00
DBDMA: channel_run
dbdma_cmd 0x7f4cf1325228
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000


and without the patch when it works:


DBDMA: writel 0x0000000000000d0c <= 0x00e89ce0
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e89ce0
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 0x7fc4dcef2228
     req_count 0x0324
     command 0x3000
     phy_addr 0x00e8ff94
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe8ff94 key 0x0

non-block ATAPI DMA transfer size: 20
end of non-block ATAPI DMA transfer
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e89ce0
DBDMA: xfer_status 0x00008400 res_count 0x0324
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e89cf0
DBDMA: channel_run
dbdma_cmd 0x7fc4dcef2228
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: readl 0x0000000000000d04 => 0x00008000
DBDMA: channel 0x1a reg 0x1
DBDMA: writel 0x0000000000000d00 <= 0x98000000
DBDMA: channel 0x1a reg 0x0
DBDMA:     status 0x00000000
DBDMA: writel 0x0000000000000d0c <= 0x00e89ce0
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e89ce0
ATAPI limit=0x8000 packet: 28 00 00 00 00 10 00 00 01 00 00 00
read dma: LBA=16 nb_sectors=1

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 0x7fc4dcef2228
     req_count 0x0800
     command 0x3000
     phy_addr 0x00e7bc20
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe7bc20 key 0x0

io_buffer_size = 0
remainder: 0 io->len: 2048 size: 2048
io->len = 0x800
set remainder to: 0
sector_num=64 size=2048, cmd_cmd=0
io_buffer_size = 0x800
remainder: 0 io->len: 0 size: 0
end of transfer
end of DMA
done DMA
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e89ce0
DBDMA: xfer_status 0x00008400 res_count 0x0000
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e89cf0
DBDMA: channel_run
dbdma_cmd 0x7fc4dcef2228
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: writel 0x0000000000000d00 <= 0x98000000
DBDMA: channel 0x1a reg 0x0
DBDMA:     status 0x00000000
ATAPI limit=0x8000 packet: 00 00 00 00 00 00 00 00 00 00 00 00
DBDMA: writel 0x0000000000000d0c <= 0x00e89ce0
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e89ce0
ATAPI limit=0x8000 packet: 43 00 00 00 00 00 00 03 24 00 00 00

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 0x7fc4dcef2228
     req_count 0x0324
     command 0x3000
     phy_addr 0x00e98dd4
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: start_input
DBDMA: addr 0xe98dd4 key 0x0

non-block ATAPI DMA transfer size: 20
end of non-block ATAPI DMA transfer
DBDMA: dbdma_end
DBDMA: conditional_wait
DBDMA: dbdma_cmdptr_save 0x00e89ce0
DBDMA: xfer_status 0x00008400 res_count 0x0324
DBDMA: conditional_interrupt
DBDMA: conditional_branch
DBDMA: dbdma_cmdptr_load 0x00e89cf0
DBDMA: channel_run
dbdma_cmd 0x7fc4dcef2228
     req_count 0x0000
     command 0x7000
     phy_addr 0x00000000
     cmd_dep 0x00000000
     res_count 0x0000
     xfer_status 0x0000
DBDMA: writel 0x0000000000000d00 <= 0x98000000
DBDMA: channel 0x1a reg 0x0
DBDMA:     status 0x00000000
DBDMA: writel 0x0000000000000d0c <= 0x00e89ce0
DBDMA: channel 0x1a reg 0x3
DBDMA: dbdma_cmdptr_load 0x00e89ce0
ATAPI limit=0x8000 packet: 28 00 00 00 00 10 00 00 01 00 00 00
read dma: LBA=16 nb_sectors=1


The only difference in the common part I can see is the different transfer 
size.

> I'm afraid as you're the only person that can boot MorphOS this far then we 
> need you to diagnose and suggest a suitable alternative by comparing the 
> before and after output. Since MacOS is already a supported client then if no 
> solution can be found then it is likely that this patch will be reverted :(

Unfortunately I can't make sense of the mess in macio.c so I cannot 
suggest a solution as I don't understand what's happening and how should 
it work. I can help in testing and trying things you tell me to do but I 
cannot solve it by myself. (It would probably help if the unaligned hacks 
could be removed if this is already possible with the current block layer 
as that should simplify the code so it could be understood. I can't do 
that either though as I don't know what needs to be done for this.)

I wish more people could boot MorphOS by now but that still needs the 
patches I've sent for OpenBIOS and QEMU that are not merged yet and 
there's also the problem with the MMU exceptions that we don't have a 
solution for yet.

Regards,
BALATON Zoltan
Alexander Graf June 23, 2014, 4:31 p.m. UTC | #4
On 20.06.14 21:27, Mark Cave-Ayland wrote:
> On 20/06/14 20:17, BALATON Zoltan wrote:
>
>> On Fri, 20 Jun 2014, Mark Cave-Ayland wrote:
>>> Zoltan, please can you test the attached patch to see if this still
>>> allows MorphOS to boot?
>>
>> Unfortunately it seems MorphOS cannot boot with this patch. It hangs
>> while trying to read the TOC from the CD. Debug output with DEBUG_MACIO
>> and DEBUG_DBDMA enabled shows:
>
> And also with ATAPI debugging enabled? I suspect the problem is with 
> the interaction between the DMA/ATAPI systems again.
>
>> DBDMA: writel 0x0000000000000d0c <= 0x00e51970
>> DBDMA: channel 0x1a reg 0x3
>> DBDMA: dbdma_cmdptr_load 0x00e51970
>>
>> 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 0x7f0997120f28
>>      req_count 0x0324
>>      command 0x3000
>>      phy_addr 0x00e7b0bc
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: start_input
>> DBDMA: addr 0xe7b0bc key 0x0
>>
>> non-block ATAPI DMA transfer size: 804
>> io_buffer_size = 0
>> remainder: 0 io->len: 0 size: 20
>> end of DMA
>> done DMA
>> DBDMA: dbdma_end
>> DBDMA: conditional_wait
>> DBDMA: dbdma_cmdptr_save 0x00e51970
>> DBDMA: xfer_status 0x00008400 res_count 0x0000
>> DBDMA: conditional_interrupt
>> DBDMA: conditional_branch
>> DBDMA: dbdma_cmdptr_load 0x00e51980
>> DBDMA: channel_run
>> dbdma_cmd 0x7f0997120f28
>>      req_count 0x0000
>>      command 0x7000
>>      phy_addr 0x00000000
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>>
>> and no further ide activity from here whereas without the patch when it
>> boots I see these logs:
>>
>> DBDMA: writel 0x0000000000000d0c <= 0x00e50090
>> DBDMA: channel 0x1a reg 0x3
>> DBDMA: dbdma_cmdptr_load 0x00e50090
>>
>> 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 0x7f56695a7f28
>>      req_count 0x0324
>>      command 0x3000
>>      phy_addr 0x00e4f8fc
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: start_input
>> DBDMA: addr 0xe4f8fc key 0x0
>>
>> non-block ATAPI DMA transfer size: 20
>> end of non-block ATAPI DMA transfer
>> DBDMA: dbdma_end
>> DBDMA: conditional_wait
>> DBDMA: dbdma_cmdptr_save 0x00e50090
>> DBDMA: xfer_status 0x00008400 res_count 0x0324
>> DBDMA: conditional_interrupt
>> DBDMA: conditional_branch
>> DBDMA: dbdma_cmdptr_load 0x00e500a0
>> DBDMA: channel_run
>> dbdma_cmd 0x7f56695a7f28
>>      req_count 0x0000
>>      command 0x7000
>>      phy_addr 0x00000000
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: writel 0x0000000000000d00 <= 0x98000000
>> DBDMA: channel 0x1a reg 0x0
>> DBDMA:     status 0x00000000
>> DBDMA: writel 0x0000000000000d0c <= 0x00e50090
>> DBDMA: channel 0x1a reg 0x3
>> DBDMA: dbdma_cmdptr_load 0x00e50090
>>
>> 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 0x7f56695a7f28
>>      req_count 0x0800
>>      command 0x3000
>>      phy_addr 0x00e8d7c0
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>> DBDMA: start_input
>> DBDMA: addr 0xe8d7c0 key 0x0
>>
>> io_buffer_size = 0
>> remainder: 0 io->len: 2048 size: 2048
>> io->len = 0x800
>> set remainder to: 0
>> sector_num=64 size=2048, cmd_cmd=0
>> io_buffer_size = 0x800
>> remainder: 0 io->len: 0 size: 0
>> end of transfer
>> end of DMA
>> done DMA
>> DBDMA: dbdma_end
>> DBDMA: conditional_wait
>> DBDMA: dbdma_cmdptr_save 0x00e50090
>> DBDMA: xfer_status 0x00008400 res_count 0x0000
>> DBDMA: conditional_interrupt
>> DBDMA: conditional_branch
>> DBDMA: dbdma_cmdptr_load 0x00e500a0
>> DBDMA: channel_run
>> dbdma_cmd 0x7f56695a7f28
>>      req_count 0x0000
>>      command 0x7000
>>      phy_addr 0x00000000
>>      cmd_dep 0x00000000
>>      res_count 0x0000
>>      xfer_status 0x0000
>>
>> and a lot of similar stuff after this. If this is not enough to
>> understand the problem and you need more details please tell me what to
>> look for.
>
> I'm afraid as you're the only person that can boot MorphOS this far 
> then we need you to diagnose and suggest a suitable alternative by 
> comparing the before and after output. Since MacOS is already a 
> supported client then if no solution can be found then it is likely 
> that this patch will be reverted :(

So should I revert the patch for now? We're already in soft freeze.


Alex
BALATON Zoltan June 23, 2014, 7:26 p.m. UTC | #5
On Mon, 23 Jun 2014, Alexander Graf wrote:
> On 20.06.14 21:27, Mark Cave-Ayland wrote:
>> On 20/06/14 20:17, BALATON Zoltan wrote:
>> 
>>> On Fri, 20 Jun 2014, Mark Cave-Ayland wrote:
>>>> Zoltan, please can you test the attached patch to see if this still
>>>> allows MorphOS to boot?
>>> 
>>> Unfortunately it seems MorphOS cannot boot with this patch. It hangs
>>> while trying to read the TOC from the CD. Debug output with DEBUG_MACIO
>>> and DEBUG_DBDMA enabled shows:
>> 
>> And also with ATAPI debugging enabled? I suspect the problem is with the 
>> interaction between the DMA/ATAPI systems again.
>> 
>>> DBDMA: writel 0x0000000000000d0c <= 0x00e51970
>>> DBDMA: channel 0x1a reg 0x3
>>> DBDMA: dbdma_cmdptr_load 0x00e51970
>>> 
>>> 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 0x7f0997120f28
>>>      req_count 0x0324
>>>      command 0x3000
>>>      phy_addr 0x00e7b0bc
>>>      cmd_dep 0x00000000
>>>      res_count 0x0000
>>>      xfer_status 0x0000
>>> DBDMA: start_input
>>> DBDMA: addr 0xe7b0bc key 0x0
>>> 
>>> non-block ATAPI DMA transfer size: 804
>>> io_buffer_size = 0
>>> remainder: 0 io->len: 0 size: 20
>>> end of DMA
>>> done DMA
>>> DBDMA: dbdma_end
>>> DBDMA: conditional_wait
>>> DBDMA: dbdma_cmdptr_save 0x00e51970
>>> DBDMA: xfer_status 0x00008400 res_count 0x0000
>>> DBDMA: conditional_interrupt
>>> DBDMA: conditional_branch
>>> DBDMA: dbdma_cmdptr_load 0x00e51980
>>> DBDMA: channel_run
>>> dbdma_cmd 0x7f0997120f28
>>>      req_count 0x0000
>>>      command 0x7000
>>>      phy_addr 0x00000000
>>>      cmd_dep 0x00000000
>>>      res_count 0x0000
>>>      xfer_status 0x0000
>>> 
>>> and no further ide activity from here whereas without the patch when it
>>> boots I see these logs:
>>> 
>>> DBDMA: writel 0x0000000000000d0c <= 0x00e50090
>>> DBDMA: channel 0x1a reg 0x3
>>> DBDMA: dbdma_cmdptr_load 0x00e50090
>>> 
>>> 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 0x7f56695a7f28
>>>      req_count 0x0324
>>>      command 0x3000
>>>      phy_addr 0x00e4f8fc
>>>      cmd_dep 0x00000000
>>>      res_count 0x0000
>>>      xfer_status 0x0000
>>> DBDMA: start_input
>>> DBDMA: addr 0xe4f8fc key 0x0
>>> 
>>> non-block ATAPI DMA transfer size: 20
>>> end of non-block ATAPI DMA transfer
>>> DBDMA: dbdma_end
>>> DBDMA: conditional_wait
>>> DBDMA: dbdma_cmdptr_save 0x00e50090
>>> DBDMA: xfer_status 0x00008400 res_count 0x0324
>>> DBDMA: conditional_interrupt
>>> DBDMA: conditional_branch
>>> DBDMA: dbdma_cmdptr_load 0x00e500a0
>>> DBDMA: channel_run
>>> dbdma_cmd 0x7f56695a7f28
>>>      req_count 0x0000
>>>      command 0x7000
>>>      phy_addr 0x00000000
>>>      cmd_dep 0x00000000
>>>      res_count 0x0000
>>>      xfer_status 0x0000
>>> DBDMA: writel 0x0000000000000d00 <= 0x98000000
>>> DBDMA: channel 0x1a reg 0x0
>>> DBDMA:     status 0x00000000
>>> DBDMA: writel 0x0000000000000d0c <= 0x00e50090
>>> DBDMA: channel 0x1a reg 0x3
>>> DBDMA: dbdma_cmdptr_load 0x00e50090
>>> 
>>> 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 0x7f56695a7f28
>>>      req_count 0x0800
>>>      command 0x3000
>>>      phy_addr 0x00e8d7c0
>>>      cmd_dep 0x00000000
>>>      res_count 0x0000
>>>      xfer_status 0x0000
>>> DBDMA: start_input
>>> DBDMA: addr 0xe8d7c0 key 0x0
>>> 
>>> io_buffer_size = 0
>>> remainder: 0 io->len: 2048 size: 2048
>>> io->len = 0x800
>>> set remainder to: 0
>>> sector_num=64 size=2048, cmd_cmd=0
>>> io_buffer_size = 0x800
>>> remainder: 0 io->len: 0 size: 0
>>> end of transfer
>>> end of DMA
>>> done DMA
>>> DBDMA: dbdma_end
>>> DBDMA: conditional_wait
>>> DBDMA: dbdma_cmdptr_save 0x00e50090
>>> DBDMA: xfer_status 0x00008400 res_count 0x0000
>>> DBDMA: conditional_interrupt
>>> DBDMA: conditional_branch
>>> DBDMA: dbdma_cmdptr_load 0x00e500a0
>>> DBDMA: channel_run
>>> dbdma_cmd 0x7f56695a7f28
>>>      req_count 0x0000
>>>      command 0x7000
>>>      phy_addr 0x00000000
>>>      cmd_dep 0x00000000
>>>      res_count 0x0000
>>>      xfer_status 0x0000
>>> 
>>> and a lot of similar stuff after this. If this is not enough to
>>> understand the problem and you need more details please tell me what to
>>> look for.
>> 
>> I'm afraid as you're the only person that can boot MorphOS this far then we 
>> need you to diagnose and suggest a suitable alternative by comparing the 
>> before and after output. Since MacOS is already a supported client then if 
>> no solution can be found then it is likely that this patch will be reverted 
>> :(
>
> So should I revert the patch for now? We're already in soft freeze.

It would be nicer if it could be fixed instead of reverting. You could 
help detangling the macio.c code for a start.

Regards,
BALATON Zoltan
BALATON Zoltan June 23, 2014, 9:30 p.m. UTC | #6
On Fri, 20 Jun 2014, Mark Cave-Ayland wrote:
> On 04/06/14 13:44, Alexander Graf wrote:
>
>> From: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> 
>> Currently the macio DMA routines assume that all DMA requests are for 
>> read/write
>> block transfers. This is not always the case for ATAPI, for example when
>> requesting a TOC where the response is generated directly in the IDE 
>> buffer.
>> 
>> Detect these non-block ATAPI DMA transfers (where no lba is specified in 
>> the
>> command) and copy the results directly into RAM as indicated by the DBDMA
>> descriptor. This fixes CDROM access under MorphOS.
>> 
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> I've just done a complete round of OpenBIOS tests and it looks as if this 
> patch introduces random hang failures (60% or so) into my Darwin image boot 
> tests.
>
> The issue seems to be related as to the timing of the DMA callback relative 
> to reception of the IDE command; if the IDE transfer is invoked first (as 
> happens in Darwin) then we hang after sending the IDE command which is likely 
> because the DMA is not being completed correctly. There is also another 
> difference with MorphOS in that when the IDE transfer callback is invoked 
> first then s->packet_transfer_size == 0 and so the memory copy would always 
> copy 0 bytes.
>
> After some experimentation, I've come up with the attached patch which should 
> retain the DMA memory copy, but instead invokes a further round of 
> pmac_ide_atapi_transfer_cb() with io->len == 0 and s->io_buffer_size == 0 
> which should terminate the DMA request correctly. At the very least it seems 
> to fix the hang on boot with my Darwin images.
>
> Zoltan, please can you test the attached patch to see if this still allows 
> MorphOS to boot?

Also this patch seems to slow down Finnix boot very much.

Regards,
BALATON Zoltan
Mark Cave-Ayland June 23, 2014, 10:41 p.m. UTC | #7
On 23/06/14 20:26, BALATON Zoltan wrote:

(add Kevin to CC)

>>> I'm afraid as you're the only person that can boot MorphOS this far
>>> then we need you to diagnose and suggest a suitable alternative by
>>> comparing the before and after output. Since MacOS is already a
>>> supported client then if no solution can be found then it is likely
>>> that this patch will be reverted :(
>>
>> So should I revert the patch for now? We're already in soft freeze.

Well let's see if Zoltan can make any headway with debugging over the 
next few days; if there's no progress by the weekend then sadly my 
recommendation would be to revert in time for -rc0 as this definitely 
causes intermittent boot failures in Darwin for me.

> It would be nicer if it could be fixed instead of reverting. You could
> help detangling the macio.c code for a start.

Just to clarify here: the macio/DBDMA code is quite complicated, but 
this is because this device has to work around to the fact that 
currently the DMA I/O routines currently need sector alignment whereas 
macio requires byte-level alignment. There has been quite a lot of work 
at the lower levels to support byte-level alignment (see Kevin's series 
at http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg02163.html) 
but until we can specify transfers to byte granularity in the 
dma_bdrv_*() APIs then there isn't much we can do to clean up the 
macio.c code.

Kevin, are there any plans to bubble the byte-granularity block layer 
changes up to the dma_bdrv_*() APIs in the near future?

Please bear in mind that QEMU supports a large number of OSs, and there 
is already an enthusiastic group of people using Alex's OS X work (see 
emaculation for many examples) so introducing an intermittent fault on a 
supported OS is not an option here.

I should also re-emphasise that Alex/Andreas work on many different 
parts of QEMU, and my work is currently unsponsored so while we are all 
keen to improve QEMU to the point where it can emulate new OSs such as 
MorphOS, it's not the case that we can simply drop what we are doing at 
the time to focus on an issue that affects a single OS which is new and 
currently unsupported.

Now I think it's fair to say that I've spent quite a few hours helping 
you and coming up with the original version of this patch, and I'm glad 
that you are now seeing success with this. But what is important to us 
right now heading towards a release is that patches don't cause any 
regressions.

All I can say is that debugging this stuff isn't easy, particularly with 
MorphOS which has some rather unusual behaviours. But what we really 
need from you now over the next few days is for you to compare the debug 
output between the working and non-working cases and figure out if we 
can fix this in time for the 2.1 release. You have everything you need 
(including my acceptance test of booting both MorphOS and Darwin ISOs), 
so time to take a deep breath and begin what should be a challenging yet 
ultimately rewarding debugging process :)


ATB,

Mark.
Kevin Wolf June 24, 2014, 10:35 a.m. UTC | #8
Am 24.06.2014 um 00:41 hat Mark Cave-Ayland geschrieben:
> On 23/06/14 20:26, BALATON Zoltan wrote:
> 
> (add Kevin to CC)
> 
> >>>I'm afraid as you're the only person that can boot MorphOS this far
> >>>then we need you to diagnose and suggest a suitable alternative by
> >>>comparing the before and after output. Since MacOS is already a
> >>>supported client then if no solution can be found then it is likely
> >>>that this patch will be reverted :(
> >>
> >>So should I revert the patch for now? We're already in soft freeze.
> 
> Well let's see if Zoltan can make any headway with debugging over
> the next few days; if there's no progress by the weekend then sadly
> my recommendation would be to revert in time for -rc0 as this
> definitely causes intermittent boot failures in Darwin for me.
> 
> >It would be nicer if it could be fixed instead of reverting. You could
> >help detangling the macio.c code for a start.
> 
> Just to clarify here: the macio/DBDMA code is quite complicated, but
> this is because this device has to work around to the fact that
> currently the DMA I/O routines currently need sector alignment
> whereas macio requires byte-level alignment. There has been quite a
> lot of work at the lower levels to support byte-level alignment (see
> Kevin's series at
> http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg02163.html)
> but until we can specify transfers to byte granularity in the
> dma_bdrv_*() APIs then there isn't much we can do to clean up the
> macio.c code.
> 
> Kevin, are there any plans to bubble the byte-granularity block
> layer changes up to the dma_bdrv_*() APIs in the near future?

Not to my knowledge.

Block devices allowing byte-granularity accesses (or can you even call
them block devices any more then?) seem to be something rather uncommon.
So far this macio thing seems to be the only one that needs such
functionality. Which probably means that if you guys don't do the
conversion, nobody will.

> Please bear in mind that QEMU supports a large number of OSs, and
> there is already an enthusiastic group of people using Alex's OS X
> work (see emaculation for many examples) so introducing an
> intermittent fault on a supported OS is not an option here.
> 
> I should also re-emphasise that Alex/Andreas work on many different
> parts of QEMU, and my work is currently unsponsored so while we are
> all keen to improve QEMU to the point where it can emulate new OSs
> such as MorphOS, it's not the case that we can simply drop what we
> are doing at the time to focus on an issue that affects a single OS
> which is new and currently unsupported.
> 
> Now I think it's fair to say that I've spent quite a few hours
> helping you and coming up with the original version of this patch,
> and I'm glad that you are now seeing success with this. But what is
> important to us right now heading towards a release is that patches
> don't cause any regressions.

Yes, I think you're making an important point here.

Kevin
BALATON Zoltan June 24, 2014, 10:53 a.m. UTC | #9
On Mon, 23 Jun 2014, Mark Cave-Ayland wrote:
> On 23/06/14 20:26, BALATON Zoltan wrote:
>
> (add Kevin to CC)
>
>>>> I'm afraid as you're the only person that can boot MorphOS this far
>>>> then we need you to diagnose and suggest a suitable alternative by
>>>> comparing the before and after output. Since MacOS is already a
>>>> supported client then if no solution can be found then it is likely
>>>> that this patch will be reverted :(
>>> 
>>> So should I revert the patch for now? We're already in soft freeze.
>
> Well let's see if Zoltan can make any headway with debugging over the next 
> few days; if there's no progress by the weekend then sadly my recommendation 
> would be to revert in time for -rc0 as this definitely causes intermittent 
> boot failures in Darwin for me.
>
>> It would be nicer if it could be fixed instead of reverting. You could
>> help detangling the macio.c code for a start.
>
> Just to clarify here: the macio/DBDMA code is quite complicated, but this is 
> because this device has to work around to the fact that currently the DMA I/O 
> routines currently need sector alignment whereas macio requires byte-level 
> alignment. There has been quite a lot of work at the lower levels to support 
> byte-level alignment (see Kevin's series at 
> http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg02163.html) but until 
> we can specify transfers to byte granularity in the dma_bdrv_*() APIs then 
> there isn't much we can do to clean up the macio.c code.
>
> Kevin, are there any plans to bubble the byte-granularity block layer changes 
> up to the dma_bdrv_*() APIs in the near future?
>
> Please bear in mind that QEMU supports a large number of OSs, and there is 
> already an enthusiastic group of people using Alex's OS X work (see 
> emaculation for many examples) so introducing an intermittent fault on a 
> supported OS is not an option here.
>
> I should also re-emphasise that Alex/Andreas work on many different parts of 
> QEMU, and my work is currently unsponsored so while we are all keen to 
> improve QEMU to the point where it can emulate new OSs such as MorphOS, it's 
> not the case that we can simply drop what we are doing at the time to focus 
> on an issue that affects a single OS which is new and currently unsupported.

I also work unsponsored on this and not sure how long can I still find 
time for it. I've already spent much more with this than I originally 
planned as I'm doing it since end of this February already. So I'd like my 
work so far to get upstream so that if I have to finish it's not lost and 
others could use and build on it. If there's no chance that this can be 
achieved by 2.1 then you could revert this patch and get back to it in 2.2 
but that would delay things by months again. My patches are on the list 
for quite some time so it's not like I'm asking you to work on this in the 
last minute and this bug was reported on May 4th. I appreciate your help 
so far very much and don't exepct this to be highest priority but I'd 
like to make some progress too.

> Now I think it's fair to say that I've spent quite a few hours helping you 
> and coming up with the original version of this patch, and I'm glad that you

Now doubt about that, thank you very much again.

> are now seeing success with this. But what is important to us right now 
> heading towards a release is that patches don't cause any regressions.
>
> All I can say is that debugging this stuff isn't easy, particularly with 
> MorphOS which has some rather unusual behaviours. But what we really need 
> from you now over the next few days is for you to compare the debug output 
> between the working and non-working cases and figure out if we can fix this 
> in time for the 2.1 release. You have everything you need (including my 
> acceptance test of booting both MorphOS and Darwin ISOs), so time to take a 
> deep breath and begin what should be a challenging yet ultimately rewarding 
> debugging process :)

I'm still working on finding a solution for the exception problems with 
OpenBIOS that prevent MorphOS from working and I failed to understand the 
whole working of macio, DBDMA and the whole block layer so far but I can 
try to debug it. Can you tell how to reproduce the problem with Darwin? 
The Darwin images don't seem to work with -M mac99 either before or after 
the patch so no regressions there.

Regards,
BALATON Zoltan
Alexander Graf June 24, 2014, 11:02 a.m. UTC | #10
On 24.06.14 12:53, BALATON Zoltan wrote:
> On Mon, 23 Jun 2014, Mark Cave-Ayland wrote:
>> On 23/06/14 20:26, BALATON Zoltan wrote:
>>
>> (add Kevin to CC)
>>
>>>>> I'm afraid as you're the only person that can boot MorphOS this far
>>>>> then we need you to diagnose and suggest a suitable alternative by
>>>>> comparing the before and after output. Since MacOS is already a
>>>>> supported client then if no solution can be found then it is likely
>>>>> that this patch will be reverted :(
>>>>
>>>> So should I revert the patch for now? We're already in soft freeze.
>>
>> Well let's see if Zoltan can make any headway with debugging over the 
>> next few days; if there's no progress by the weekend then sadly my 
>> recommendation would be to revert in time for -rc0 as this definitely 
>> causes intermittent boot failures in Darwin for me.
>>
>>> It would be nicer if it could be fixed instead of reverting. You could
>>> help detangling the macio.c code for a start.
>>
>> Just to clarify here: the macio/DBDMA code is quite complicated, but 
>> this is because this device has to work around to the fact that 
>> currently the DMA I/O routines currently need sector alignment 
>> whereas macio requires byte-level alignment. There has been quite a 
>> lot of work at the lower levels to support byte-level alignment (see 
>> Kevin's series at 
>> http://lists.gnu.org/archive/html/qemu-devel/2014-01/msg02163.html) 
>> but until we can specify transfers to byte granularity in the 
>> dma_bdrv_*() APIs then there isn't much we can do to clean up the 
>> macio.c code.
>>
>> Kevin, are there any plans to bubble the byte-granularity block layer 
>> changes up to the dma_bdrv_*() APIs in the near future?
>>
>> Please bear in mind that QEMU supports a large number of OSs, and 
>> there is already an enthusiastic group of people using Alex's OS X 
>> work (see emaculation for many examples) so introducing an 
>> intermittent fault on a supported OS is not an option here.
>>
>> I should also re-emphasise that Alex/Andreas work on many different 
>> parts of QEMU, and my work is currently unsponsored so while we are 
>> all keen to improve QEMU to the point where it can emulate new OSs 
>> such as MorphOS, it's not the case that we can simply drop what we 
>> are doing at the time to focus on an issue that affects a single OS 
>> which is new and currently unsupported.
>
> I also work unsponsored on this and not sure how long can I still find 
> time for it. I've already spent much more with this than I originally 
> planned as I'm doing it since end of this February already. So I'd 
> like my work so far to get upstream so that if I have to finish it's 
> not lost and others could use and build on it. If there's no chance 
> that this can be achieved by 2.1 then you could revert this patch and 
> get back to it in 2.2 but that would delay things by months again. My 
> patches are on the list for quite some time so it's not like I'm 
> asking you to work on this in the last minute and this bug was 
> reported on May 4th. I appreciate your help so far very much and don't 
> exepct this to be highest priority but I'd like to make some progress 
> too.
>
>> Now I think it's fair to say that I've spent quite a few hours 
>> helping you and coming up with the original version of this patch, 
>> and I'm glad that you
>
> Now doubt about that, thank you very much again.
>
>> are now seeing success with this. But what is important to us right 
>> now heading towards a release is that patches don't cause any 
>> regressions.
>>
>> All I can say is that debugging this stuff isn't easy, particularly 
>> with MorphOS which has some rather unusual behaviours. But what we 
>> really need from you now over the next few days is for you to compare 
>> the debug output between the working and non-working cases and figure 
>> out if we can fix this in time for the 2.1 release. You have 
>> everything you need (including my acceptance test of booting both 
>> MorphOS and Darwin ISOs), so time to take a deep breath and begin 
>> what should be a challenging yet ultimately rewarding debugging 
>> process :)
>
> I'm still working on finding a solution for the exception problems 
> with OpenBIOS that prevent MorphOS from working and I failed to 
> understand the whole working of macio, DBDMA and the whole block layer 
> so far

That one's reasonably simple. On a real Mac, we have different chips 
implementing IDE and DMA.

Traditionally in a non-DMA system, you would have an IDE controller that 
has a small buffer. When you issue a command, that buffer would get 
filled and you can poke the IDE controller byte-by-byte to retrieve that 
buffer.

Then people realized that this doesn't perform too well, so they started 
implementing DMA controllers. The DBDMA controller does this task of 
poking the IDE controller for you. So instead of hogging the CPU with 
byte-by-byte transfers from the IDE controller, you now occupy the DMA 
controller with them, freeing the CPU to do other work.

The way DBDMA works is that you put in something similar to a 
scatter-gather list: A list of chunks to read / write and where in 
memory those chunks live. DBDMA then goes over its list and does the 
pokes. So for example if the list is

   [ memaddr = 0x12000 | len = 500 ]
   [ memaddr = 0x13098 | len = 12 ]

then it reads 500 bytes from IDE, writes them at memory offset 0x12000 
and after that reads another 12 bytes from IDE and puts them at memory 
offset 0x13098.

The reason we have such complicated code for real DMA is that we can't 
model this easily with our direct block-to-memory API. That one can only 
work on a 512 byte granularity. So when we see unaligned accesses like 
above, we have to split them out and handle them lazily.


Alex
Kevin Wolf June 24, 2014, 11:22 a.m. UTC | #11
Am 24.06.2014 um 13:02 hat Alexander Graf geschrieben:
> The way DBDMA works is that you put in something similar to a
> scatter-gather list: A list of chunks to read / write and where in
> memory those chunks live. DBDMA then goes over its list and does the
> pokes. So for example if the list is
> 
>   [ memaddr = 0x12000 | len = 500 ]
>   [ memaddr = 0x13098 | len = 12 ]
> 
> then it reads 500 bytes from IDE, writes them at memory offset
> 0x12000 and after that reads another 12 bytes from IDE and puts them
> at memory offset 0x13098.
> 
> The reason we have such complicated code for real DMA is that we
> can't model this easily with our direct block-to-memory API. That
> one can only work on a 512 byte granularity. So when we see
> unaligned accesses like above, we have to split them out and handle
> them lazily.

Wait... What kind of granularity are you talking about?

We do need disk accesses with a 512 byte granularity, because the API
takes a sector number. This is also what real IDE disks do, they don't
provide byte access.

However, for the memory, I can't see why you couldn't pass a s/g list
like what you wrote above to the DMA functions. This is not unusual at
all and is the same as ide/pci.c does. There is no 512-byte alignment
needed for the individual s/g list entries, only the total size should
obviously be a multiple of 512 in the general case (otherwise the list
would be too short or too long for the request).

If this is really what we're talking about, then I think your problem is
just that you try to handle the 500 byte and the 12 byte as individual
requests instead of building up the s/g list and then sending a single
request.

Kevin
Alexander Graf June 24, 2014, 11:27 a.m. UTC | #12
On 24.06.14 13:22, Kevin Wolf wrote:
> Am 24.06.2014 um 13:02 hat Alexander Graf geschrieben:
>> The way DBDMA works is that you put in something similar to a
>> scatter-gather list: A list of chunks to read / write and where in
>> memory those chunks live. DBDMA then goes over its list and does the
>> pokes. So for example if the list is
>>
>>    [ memaddr = 0x12000 | len = 500 ]
>>    [ memaddr = 0x13098 | len = 12 ]
>>
>> then it reads 500 bytes from IDE, writes them at memory offset
>> 0x12000 and after that reads another 12 bytes from IDE and puts them
>> at memory offset 0x13098.
>>
>> The reason we have such complicated code for real DMA is that we
>> can't model this easily with our direct block-to-memory API. That
>> one can only work on a 512 byte granularity. So when we see
>> unaligned accesses like above, we have to split them out and handle
>> them lazily.
> Wait... What kind of granularity are you talking about?
>
> We do need disk accesses with a 512 byte granularity, because the API
> takes a sector number. This is also what real IDE disks do, they don't
> provide byte access.
>
> However, for the memory, I can't see why you couldn't pass a s/g list
> like what you wrote above to the DMA functions. This is not unusual at
> all and is the same as ide/pci.c does. There is no 512-byte alignment
> needed for the individual s/g list entries, only the total size should
> obviously be a multiple of 512 in the general case (otherwise the list
> would be too short or too long for the request).
>
> If this is really what we're talking about, then I think your problem is
> just that you try to handle the 500 byte and the 12 byte as individual
> requests instead of building up the s/g list and then sending a single
> request.

The 500 and 12 byte requests can come in as separate requests that 
require previous requests to have finished. What Mac OS X does for 
example is

   [ memaddr = 0x2000 | len = 1024 ]
   [ memaddr = 0x1000 | len = 510 ]

<wait for ack>

   [ memaddr = 0x10fe | len = 2 ]
   [ memaddr = 0x3000 | len = 2048 ]

If it was as simple as creating a working sglist, I would've certainly 
done so long ago :).


Alex
Kevin Wolf June 24, 2014, 12:07 p.m. UTC | #13
Am 24.06.2014 um 13:27 hat Alexander Graf geschrieben:
> 
> On 24.06.14 13:22, Kevin Wolf wrote:
> >Am 24.06.2014 um 13:02 hat Alexander Graf geschrieben:
> >>The way DBDMA works is that you put in something similar to a
> >>scatter-gather list: A list of chunks to read / write and where in
> >>memory those chunks live. DBDMA then goes over its list and does the
> >>pokes. So for example if the list is
> >>
> >>   [ memaddr = 0x12000 | len = 500 ]
> >>   [ memaddr = 0x13098 | len = 12 ]
> >>
> >>then it reads 500 bytes from IDE, writes them at memory offset
> >>0x12000 and after that reads another 12 bytes from IDE and puts them
> >>at memory offset 0x13098.
> >>
> >>The reason we have such complicated code for real DMA is that we
> >>can't model this easily with our direct block-to-memory API. That
> >>one can only work on a 512 byte granularity. So when we see
> >>unaligned accesses like above, we have to split them out and handle
> >>them lazily.
> >Wait... What kind of granularity are you talking about?
> >
> >We do need disk accesses with a 512 byte granularity, because the API
> >takes a sector number. This is also what real IDE disks do, they don't
> >provide byte access.
> >
> >However, for the memory, I can't see why you couldn't pass a s/g list
> >like what you wrote above to the DMA functions. This is not unusual at
> >all and is the same as ide/pci.c does. There is no 512-byte alignment
> >needed for the individual s/g list entries, only the total size should
> >obviously be a multiple of 512 in the general case (otherwise the list
> >would be too short or too long for the request).
> >
> >If this is really what we're talking about, then I think your problem is
> >just that you try to handle the 500 byte and the 12 byte as individual
> >requests instead of building up the s/g list and then sending a single
> >request.
> 
> The 500 and 12 byte requests can come in as separate requests that
> require previous requests to have finished. What Mac OS X does for
> example is
> 
>   [ memaddr = 0x2000 | len = 1024 ]
>   [ memaddr = 0x1000 | len = 510 ]
> 
> <wait for ack>
> 
>   [ memaddr = 0x10fe | len = 2 ]
>   [ memaddr = 0x3000 | len = 2048 ]
> 
> If it was as simple as creating a working sglist, I would've
> certainly done so long ago :).

Thanks, that's the explanation that was missing for me (I'm sure you
explained it more than once to me in the past few years, but I keep
forgetting).

This means, however, that exposing the byte access in the block layer is
probably not what you want. Otherwise you would read the same sector
twice from the image (assuming cache=none, so the backend must have
512-byte alignment). If you do the handling in the device emulation you
can read the full request once and then only do the DMA part with a byte
granularity. I suppose this is the complicated code that you have today?

Kevin
Alexander Graf June 24, 2014, 12:10 p.m. UTC | #14
On 24.06.14 14:07, Kevin Wolf wrote:
> Am 24.06.2014 um 13:27 hat Alexander Graf geschrieben:
>> On 24.06.14 13:22, Kevin Wolf wrote:
>>> Am 24.06.2014 um 13:02 hat Alexander Graf geschrieben:
>>>> The way DBDMA works is that you put in something similar to a
>>>> scatter-gather list: A list of chunks to read / write and where in
>>>> memory those chunks live. DBDMA then goes over its list and does the
>>>> pokes. So for example if the list is
>>>>
>>>>    [ memaddr = 0x12000 | len = 500 ]
>>>>    [ memaddr = 0x13098 | len = 12 ]
>>>>
>>>> then it reads 500 bytes from IDE, writes them at memory offset
>>>> 0x12000 and after that reads another 12 bytes from IDE and puts them
>>>> at memory offset 0x13098.
>>>>
>>>> The reason we have such complicated code for real DMA is that we
>>>> can't model this easily with our direct block-to-memory API. That
>>>> one can only work on a 512 byte granularity. So when we see
>>>> unaligned accesses like above, we have to split them out and handle
>>>> them lazily.
>>> Wait... What kind of granularity are you talking about?
>>>
>>> We do need disk accesses with a 512 byte granularity, because the API
>>> takes a sector number. This is also what real IDE disks do, they don't
>>> provide byte access.
>>>
>>> However, for the memory, I can't see why you couldn't pass a s/g list
>>> like what you wrote above to the DMA functions. This is not unusual at
>>> all and is the same as ide/pci.c does. There is no 512-byte alignment
>>> needed for the individual s/g list entries, only the total size should
>>> obviously be a multiple of 512 in the general case (otherwise the list
>>> would be too short or too long for the request).
>>>
>>> If this is really what we're talking about, then I think your problem is
>>> just that you try to handle the 500 byte and the 12 byte as individual
>>> requests instead of building up the s/g list and then sending a single
>>> request.
>> The 500 and 12 byte requests can come in as separate requests that
>> require previous requests to have finished. What Mac OS X does for
>> example is
>>
>>    [ memaddr = 0x2000 | len = 1024 ]
>>    [ memaddr = 0x1000 | len = 510 ]
>>
>> <wait for ack>
>>
>>    [ memaddr = 0x10fe | len = 2 ]
>>    [ memaddr = 0x3000 | len = 2048 ]
>>
>> If it was as simple as creating a working sglist, I would've
>> certainly done so long ago :).
> Thanks, that's the explanation that was missing for me (I'm sure you
> explained it more than once to me in the past few years, but I keep
> forgetting).
>
> This means, however, that exposing the byte access in the block layer is
> probably not what you want. Otherwise you would read the same sector
> twice from the image (assuming cache=none, so the backend must have
> 512-byte alignment). If you do the handling in the device emulation you
> can read the full request once and then only do the DMA part with a byte
> granularity. I suppose this is the complicated code that you have today?

Yes and no. We are trying to be slightly smarter than that. For all the 
aligned pieces in between the just have a fast path doing straight DMA 
to/from memory. For the tiny unaligned chunks, we read into a temporary 
buffer and do DMA byte-wise from that one manually. For writes it's the 
reverse - we only issue the full sector IDE transfer after our temporary 
buffer is fully filled.

I think it's perfectly reasonable to read the sector twice from the 
image though if that makes the DBDMA emulation code easier to maintain 
;). Right now there are just way too many tiny corner cases lingering.


Alex
Mark Cave-Ayland June 25, 2014, 8:17 p.m. UTC | #15
On 24/06/14 11:53, BALATON Zoltan wrote:

>> All I can say is that debugging this stuff isn't easy, particularly
>> with MorphOS which has some rather unusual behaviours. But what we
>> really need from you now over the next few days is for you to compare
>> the debug output between the working and non-working cases and figure
>> out if we can fix this in time for the 2.1 release. You have
>> everything you need (including my acceptance test of booting both
>> MorphOS and Darwin ISOs), so time to take a deep breath and begin what
>> should be a challenging yet ultimately rewarding debugging process :)
>
> I'm still working on finding a solution for the exception problems with
> OpenBIOS that prevent MorphOS from working and I failed to understand
> the whole working of macio, DBDMA and the whole block layer so far but I
> can try to debug it. Can you tell how to reproduce the problem with
> Darwin? The Darwin images don't seem to work with -M mac99 either before
> or after the patch so no regressions there.

It's fairly simple to reproduce here:

qemu-system-ppc -M g3beige -cdrom darwinppc-602.iso -boot d
qemu-system-ppc -M g3beige -cdrom darwinppc-801.iso -boot d
qemu-system-ppc -M mac99 -cdrom darwinppc-801.iso -boot d

For -M g3beige then darwinppc-602.iso tends to hang just after the "ADB 
present" line just before it finds the CDROM.

Rather annoyingly it seems to be a lot trickier to reproduce today than 
it was with my original tests, currently 1 in 8 boots compared to 1 in 3 
when I did the OpenBIOS tests. Delays introduced by enabling debugging 
in pmac_ide_transfer() seem to make it easier to trigger, as does 
compiling with -O0 -g (slower) and also dropping the kernel FS cache.

Maybe it's an existing timing bug that happens to be exacerbated by the 
patch? :/

Notes:
Darwin 6.02 doesn't support -M mac99 (always hangs) AFAICT.
Darwin 8.01 works but with -M mac99 IDE detection can take up to 30s or so.


ATB,

Mark.
BALATON Zoltan June 25, 2014, 9:48 p.m. UTC | #16
On Wed, 25 Jun 2014, Mark Cave-Ayland wrote:
> On 24/06/14 11:53, BALATON Zoltan wrote:
>>> All I can say is that debugging this stuff isn't easy, particularly
>>> with MorphOS which has some rather unusual behaviours. But what we
>>> really need from you now over the next few days is for you to compare
>>> the debug output between the working and non-working cases and figure
>>> out if we can fix this in time for the 2.1 release. You have
>>> everything you need (including my acceptance test of booting both
>>> MorphOS and Darwin ISOs), so time to take a deep breath and begin what
>>> should be a challenging yet ultimately rewarding debugging process :)
>> 
>> I'm still working on finding a solution for the exception problems with
>> OpenBIOS that prevent MorphOS from working and I failed to understand
>> the whole working of macio, DBDMA and the whole block layer so far but I
>> can try to debug it. Can you tell how to reproduce the problem with
>> Darwin? The Darwin images don't seem to work with -M mac99 either before
>> or after the patch so no regressions there.
>
> It's fairly simple to reproduce here:
>
> qemu-system-ppc -M g3beige -cdrom darwinppc-602.iso -boot d
> qemu-system-ppc -M g3beige -cdrom darwinppc-801.iso -boot d
> qemu-system-ppc -M mac99 -cdrom darwinppc-801.iso -boot d
>
> For -M g3beige then darwinppc-602.iso tends to hang just after the "ADB 
> present" line just before it finds the CDROM.
>
> Rather annoyingly it seems to be a lot trickier to reproduce today than it 
> was with my original tests, currently 1 in 8 boots compared to 1 in 3 when I 
> did the OpenBIOS tests. Delays introduced by enabling debugging in 
> pmac_ide_transfer() seem to make it easier to trigger, as does compiling with 
> -O0 -g (slower) and also dropping the kernel FS cache.
>
> Maybe it's an existing timing bug that happens to be exacerbated by the 
> patch? :/

For me darwinppc-602.iso seems to more consistently hang although it did 
boot once but I had no debug logs that time. The diff between a boot with 
the last two patches reverted (your non-block ATAPI patch and Alex's async 
remainder) and HEAD shows this in case it gives a hint to someone:

--- debug-console.log.nopatch
+++ debug-console.log.fail
  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 0x7fc7142f0560
+dbdma_cmd 0x7f58257f4b48
      req_count 0x0930
      command 0x2000
-    phy_addr 0x017ca000
+    phy_addr 0x017cb000
      cmd_dep 0x00000000
      res_count 0x0000
      xfer_status 0x0000
  DBDMA: start_input
-DBDMA: addr 0x17ca000 key 0x0
+DBDMA: addr 0x17cb000 key 0x0

-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 0x7fc7142f0560
-    req_count 0x0930
-    command 0x2000
-    phy_addr 0x017ca000
-    cmd_dep 0x00000000
-    res_count 0x0000
-    xfer_status 0x0000
-DBDMA: start_input
-DBDMA: addr 0x17ca000 key 0x0
-
-io_buffer_size = 0
-remainder: 0 io->len: 2352 size: 2352
-precopying unaligned 304 bytes to 0x17ca800
-io->len = 0x800
-set remainder to: 0
-sector_num=0 size=2352, cmd_cmd=0
-io_buffer_size = 0x930
-remainder: 0 io->len: 0 size: 0
-end of transfer
-end of DMA
-done DMA
+non-block ATAPI DMA transfer size: 0
+end of non-block ATAPI DMA transfer
  DBDMA: dbdma_end
  DBDMA: conditional_wait
  DBDMA: dbdma_cmdptr_save 0x011f8010
-DBDMA: xfer_status 0x00008400 res_count 0x0000
+DBDMA: xfer_status 0x00008400 res_count 0x0930
  DBDMA: conditional_interrupt
  DBDMA: conditional_branch
  DBDMA: dbdma_cmdptr_load 0x011f8020
  DBDMA: channel_run
-dbdma_cmd 0x7fc7142f0560
+dbdma_cmd 0x7f58257f4b48
      req_count 0x0000
      command 0x6030
      phy_addr 0x00000000
      cmd_dep 0x00000000
      res_count 0x0000
      xfer_status 0x0000
  DBDMA: conditional_wait
  DBDMA: dbdma_cmdptr_save 0x011f8020
  DBDMA: xfer_status 0x00008400 res_count 0x0000
  DBDMA: conditional_interrupt
  DBDMA: conditional_interrupt: raise
  DBDMA: conditional_branch
  DBDMA: dbdma_cmdptr_load 0x011f8030
  DBDMA: DBDMA_run_bh
  DBDMA: channel_run
-dbdma_cmd 0x7fc7142f0560
+dbdma_cmd 0x7f58257f4b48
      req_count 0x0000
      command 0x7000
      phy_addr 0x00000000
      cmd_dep 0x00000000
      res_count 0x0000
      xfer_status 0x0000
-DBDMA: writel 0x0000000000000d00 <= 0xa0002000
-DBDMA: channel 0x1a reg 0x0
-DBDMA:     status 0x00000000
-DBDMA: readl 0x0000000000000d04 => 0x00000000
-DBDMA: channel 0x1a reg 0x1
-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 0x7fc7142f0560
-    req_count 0x0800
-    command 0x2000
-    phy_addr 0x01526800
-    cmd_dep 0x00000000
-    res_count 0x0000
-    xfer_status 0x0000
-DBDMA: start_input
-DBDMA: addr 0x1526800 key 0x0
-
-waiting for data (0x1 - 0x800 - 58)
-ATAPI limit=0x800 packet: 28 00 00 00 00 00 00 00 01 00 00 00
+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 0x7fc7142f0560
-    req_count 0x0800
-    command 0x2000
-    phy_addr 0x01526800
-    cmd_dep 0x00000000
-    res_count 0x0000
-    xfer_status 0x0000
-DBDMA: start_input
-DBDMA: addr 0x1526800 key 0x0
-
-io_buffer_size = 0
-remainder: 0 io->len: 2048 size: 2048
-io->len = 0x800
-set remainder to: 0
-sector_num=0 size=2048, cmd_cmd=0
-io_buffer_size = 0x800
-remainder: 0 io->len: 0 size: 0
-end of transfer
-end of DMA
-done DMA
-DBDMA: dbdma_end
-DBDMA: conditional_wait
-DBDMA: dbdma_cmdptr_save 0x011f8010
-DBDMA: xfer_status 0x00008400 res_count 0x0000
-DBDMA: conditional_interrupt
-DBDMA: conditional_branch

I'm not sure but could it be that it mistakes a transfer to a non-block 
transfer for some reason?

> Notes:
> Darwin 6.02 doesn't support -M mac99 (always hangs) AFAICT.
> Darwin 8.01 works but with -M mac99 IDE detection can take up to 30s or so.

I thought both of these hung but I did not wait 30 seconds.

Regards,
BALATON Zoltan
diff mbox

Patch

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index c14a1dd..39bc7fd 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -367,27 +367,21 @@  static void pmac_ide_transfer(DBDMA_io *io)
     s->io_buffer_size = 0;
     if (s->drive_kind == IDE_CD) {
 
+        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
+
         /* Handle non-block ATAPI DMA transfers */
         if (s->lba == -1) {
-            s->io_buffer_size = MIN(io->len, s->packet_transfer_size);
-            bdrv_acct_start(s->bs, &s->acct, s->io_buffer_size,
-                            BDRV_ACCT_READ);
             MACIO_DPRINTF("non-block ATAPI DMA transfer size: %d\n",
-                          s->io_buffer_size);
+                          io->len);
 
-            /* Copy ATAPI buffer directly to RAM and finish */
+            /* Copy ATAPI buffer directly to RAM */
             cpu_physical_memory_write(io->addr, s->io_buffer,
-                                      s->io_buffer_size);
-            ide_atapi_cmd_ok(s);
-            m->dma_active = false;
-
-            MACIO_DPRINTF("end of non-block ATAPI DMA transfer\n");
-            bdrv_acct_done(s->bs, &s->acct);
-            io->dma_end(io);
-            return;
+                                      io->len);
+
+            /* Finish on next callback */
+            io->len = 0;
         }
 
-        bdrv_acct_start(s->bs, &s->acct, io->len, BDRV_ACCT_READ);
         pmac_ide_atapi_transfer_cb(io, 0);
         return;
     }