[Qemu-devel] [PATCH] macio ide: Do remainder access asynchronously

2014-05-26 Thread Alexander Graf
The macio IDE controller has some pretty nasty magic in its implementation to
allow for unaligned sector accesses. We used to handle these accesses
synchronously inside the IO callback handler.

However, the block infrastructure changed below our feet and now it's impossible
to call a synchronous block read/write from the aio callback handler of a
previous block access.

Work around that limitation by making the unaligned handling bits also go
through our asynchronous handler.

This fixes booting Mac OS X for me.

Reported-by: John Arbuckle programmingk...@gmail.com
Signed-off-by: Alexander Graf ag...@suse.de
---
 hw/ide/macio.c | 50 --
 hw/misc/macio/mac_dbdma.c  |  6 ++
 include/hw/ppc/mac_dbdma.h |  5 +
 3 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index af57168..c14a1dd 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -193,6 +193,11 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 goto done;
 }
 
+if (--io-requests) {
+/* More requests still in flight */
+return;
+}
+
 if (!m-dma_active) {
 MACIO_DPRINTF(waiting for data (%#x - %#x - %x)\n,
   s-nsector, io-len, s-status);
@@ -212,6 +217,13 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 s-nsector -= n;
 }
 
+if (io-finish_remain_read) {
+/* Finish a stale read from the last iteration */
+io-finish_remain_read = false;
+cpu_physical_memory_write(io-finish_addr, io-remainder,
+  io-finish_len);
+}
+
 MACIO_DPRINTF(remainder: %d io-len: %d nsector: %d 
   sector_num: % PRId64 \n,
   io-remainder_len, io-len, s-nsector, sector_num);
@@ -229,7 +241,6 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 break;
 case IDE_DMA_WRITE:
 cpu_physical_memory_read(io-addr, p, remainder_len);
-bdrv_write(s-bs, sector_num - 1, io-remainder, 1);
 break;
 case IDE_DMA_TRIM:
 break;
@@ -237,6 +248,15 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 io-addr += remainder_len;
 io-len -= remainder_len;
 io-remainder_len -= remainder_len;
+
+if (s-dma_cmd == IDE_DMA_WRITE  !io-remainder_len) {
+io-requests++;
+qemu_iovec_reset(io-iov);
+qemu_iovec_add(io-iov, io-remainder, 0x200);
+
+m-aiocb = bdrv_aio_writev(s-bs, sector_num - 1, io-iov, 1,
+   pmac_ide_transfer_cb, io);
+}
 }
 
 if (s-nsector == 0  !io-remainder_len) {
@@ -267,20 +287,25 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 
 switch (s-dma_cmd) {
 case IDE_DMA_READ:
-bdrv_read(s-bs, sector_num + nsector, io-remainder, 1);
-cpu_physical_memory_write(io-addr + io-len - unaligned,
-  io-remainder, unaligned);
+io-requests++;
+io-finish_addr = io-addr + io-len - unaligned;
+io-finish_len = unaligned;
+io-finish_remain_read = true;
+qemu_iovec_reset(io-iov);
+qemu_iovec_add(io-iov, io-remainder, 0x200);
+
+m-aiocb = bdrv_aio_readv(s-bs, sector_num + nsector, io-iov, 1,
+  pmac_ide_transfer_cb, io);
 break;
 case IDE_DMA_WRITE:
 /* cache the contents in our io struct */
 cpu_physical_memory_read(io-addr + io-len - unaligned,
- io-remainder, unaligned);
+ io-remainder + io-remainder_len,
+ unaligned);
 break;
 case IDE_DMA_TRIM:
 break;
 }
-
-io-len -= unaligned;
 }
 
 MACIO_DPRINTF(io-len = %#x\n, io-len);
@@ -292,10 +317,12 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
 io-remainder_len = (0x200 - unaligned)  0x1ff;
 MACIO_DPRINTF(set remainder to: %d\n, io-remainder_len);
 
-/* We would read no data from the block layer, thus not get a callback.
-   Just fake completion manually. */
+/* Only subsector reads happening */
 if (!io-len) {
-pmac_ide_transfer_cb(opaque, 0);
+if (!io-requests) {
+io-requests++;
+pmac_ide_transfer_cb(opaque, ret);
+}
 return;
 }
 
@@ -319,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
DMA_DIRECTION_TO_DEVICE);
 break;
 }
+
+io-requests++;
 return;
 
 done:
@@ -374,6 +403,7 @@ static void pmac_ide_transfer(DBDMA_io *io)
 break;
 }
 
+io-requests++;
 pmac_ide_transfer_cb(io, 0);
 }
 
diff --git a/hw/misc/macio/mac_dbdma.c b/hw/misc/macio/mac_dbdma.c
index 

Re: [Qemu-devel] [PATCH] macio ide: Do remainder access asynchronously

2014-05-26 Thread Mark Cave-Ayland

On 26/05/14 09:32, Alexander Graf wrote:


The macio IDE controller has some pretty nasty magic in its implementation to
allow for unaligned sector accesses. We used to handle these accesses
synchronously inside the IO callback handler.

However, the block infrastructure changed below our feet and now it's impossible
to call a synchronous block read/write from the aio callback handler of a
previous block access.

Work around that limitation by making the unaligned handling bits also go
through our asynchronous handler.

This fixes booting Mac OS X for me.


Hmmm nasty. I've never had a problem booting my two Darwin test images, 
but I've quickly tried the patch on my complete set of test suite 
booting up to the installer and haven't seen any regressions yet.


Incidentally, whilst investigating Zoltan's bug with the ATAPI DMA I 
looked into where we were with byte-aligned DMA, and with Kevin's 
patches applied it's fairly close.


The underlying bdrv_*() functions now use the unaligned-aware functions, 
and the DMA API takes a QEMUSGList as input which is good. However, 
moving down between the layers we switch back to a BlockDriverState 
before calling the unaligned functions so we lose the SGList and switch 
to lba/count again.


It looks like most of it is there, it just needs a few tweaks to get the 
SGList all the way down the unaligned functions. And I will definitely 
be happy on the day we get to rip out all of the unaligned code from 
macio :)



ATB,

Mark.




Re: [Qemu-devel] [PATCH] macio ide: Do remainder access asynchronously

2014-05-26 Thread Alexander Graf


On 26.05.14 15:56, Mark Cave-Ayland wrote:

On 26/05/14 09:32, Alexander Graf wrote:

The macio IDE controller has some pretty nasty magic in its 
implementation to

allow for unaligned sector accesses. We used to handle these accesses
synchronously inside the IO callback handler.

However, the block infrastructure changed below our feet and now it's 
impossible
to call a synchronous block read/write from the aio callback handler 
of a

previous block access.

Work around that limitation by making the unaligned handling bits 
also go

through our asynchronous handler.

This fixes booting Mac OS X for me.


Hmmm nasty. I've never had a problem booting my two Darwin test 
images, but I've quickly tried the patch on my complete set of test 
suite booting up to the installer and haven't seen any regressions yet.


Incidentally, whilst investigating Zoltan's bug with the ATAPI DMA I 
looked into where we were with byte-aligned DMA, and with Kevin's 
patches applied it's fairly close.


The underlying bdrv_*() functions now use the unaligned-aware 
functions, and the DMA API takes a QEMUSGList as input which is good. 
However, moving down between the layers we switch back to a 
BlockDriverState before calling the unaligned functions so we lose the 
SGList and switch to lba/count again.


It looks like most of it is there, it just needs a few tweaks to get 
the SGList all the way down the unaligned functions. And I will 
definitely be happy on the day we get to rip out all of the unaligned 
code from macio :)


Yeah, count me in on that sentiment too :).


Alex