Re: [Qemu-devel] [PULL 18/19] macio: move unaligned DMA write code into separate pmac_dma_write() function

[Mark Cave-Ayland]

On 31/07/15 21:37, John Snow wrote:

> On 07/28/2015 04:52 AM, Mark Cave-Ayland wrote:
>> On 27/07/15 23:00, Aurelien Jarno wrote:
>>
>>> On 2015-05-22 15:59, John Snow wrote:
>>>> From: Mark Cave-Ayland <address@hidden>
>>>>
>>>> Similarly switch the macio IDE routines over to use the new function and
>>>> tidy-up the remaining code as required.
>>>>
>>>> [Maintainer edit: printf format codes adjusted for 32/64bit. --js]
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>>>> Acked-by: John Snow <address@hidden>
>>>> Message-id: address@hidden
>>>> Signed-off-by: John Snow <address@hidden>
>>>> ---
>>>>  hw/ide/macio.c             | 268 
>>>> +++++++++++++++++++++------------------------
>>>>  include/hw/ppc/mac_dbdma.h |   4 -
>>>>  2 files changed, 125 insertions(+), 147 deletions(-)
>>>
>>> This patch has removed TRIM support without any obvious reason, and
>>> without mentioning it in the changelog. As a consequence guests with
>>> TRIM enabled now fail to boot:
>>>
>>> | [   46.916047] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x0
>>> | [   46.916545] ata1.00: failed command: DATA SET MANAGEMENT
>>> | [   46.916794] ata1.00: cmd 06/01:01:00:00:00/00:00:00:00:00/a0 tag 0 dma 
>>> 512 out
>>> | [   46.916794]          res 40/00:01:00:00:00/00:00:00:00:00/e0 Emask 
>>> 0x20 (host bus error)
>>> | [   46.917219] ata1.00: status: { DRDY }
>>> | [   51.957389] ata1.00: qc timeout (cmd 0xec)
>>> | [   51.958076] ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
>>> | [   51.958551] ata1.00: revalidation failed (errno=-5)
>>> | [   56.996713] ata1: link is slow to respond, please be patient (ready=0)
>>> | [   61.981042] ata1: device not ready (errno=-16), forcing hardreset
>>> | [   61.981669] ata1: soft resetting link
>>> | [   62.137894] ata1.00: configured for MWDMA2
>>> | [   62.138294] ata1.00: device reported invalid CHS sector 0
>>> | [   62.139045] sd 0:0:0:0: [sda]  
>>> | [   62.139128] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
>>> | [   62.139243] sd 0:0:0:0: [sda]  
>>> | [   62.139346] Sense Key : Aborted Command [current] [descriptor]
>>> | [   62.139581] Descriptor sense data with sense descriptors (in hex):
>>> | [   62.139670]         72 0b 00 00 00 00 00 0c 00 0a 80 00 00 00 00 00 
>>> | [   62.139812]         00 00 00 00 
>>> | [   62.139897] sd 0:0:0:0: [sda]  
>>> | [   62.140009] Add. Sense: No additional sense information
>>> | [   62.140115] sd 0:0:0:0: [sda] CDB: 
>>> | [   62.140204] Write same(16): 93 08 00 00 00 00 03 c0 00 48 00 3f ff b8 
>>> 00 00
>>> | [   62.140661] end_request: I/O error, dev sda, sector 62914632
>>> | [   62.141270] ata1: EH complete
>>
>> Hi Aurelien,
>>
>> Thanks for the heads-up. I have a fairly comprehensive suite of various
>> OS test images I use for OpenBIOS testing and evidently not a single one
>> of them issues a TRIM command as I don't see any regressions here. Can
>> you point me towards the particular test image you are using?
>>
>>
>> ATB,
>>
>> Mark.
>>
> 
> Hi Mark:
> 
> This series also exposes to me (unfortunately) the fact that we aren't
> unmapping the memory space we're picking up for the DMA.

Indeed I think you're right - it seems that for unaligned cases
dma_memory_map() can create a bounce region rather than providing a
pointer to the memory directly. I'm not exactly sure that we're
triggering this at the moment since I don't see memory usage ballooning
during use, but it's something that should be done for consistency (not
least that the unmap call marks the DMA memory as dirty).

> It wouldn't be too hard to unmap in the pmac_ide_transfer_cb with
> something like ...
> 
> dma_memory_unmap(&address_space_memory, XXXX, io->len, s->dma_cmd ==
> IDE_DMA_READ ? DMA_DIRECTION_FROM_DEVICE : DMA_DIRECTION_TO_DEVICE, io->len)
> 
> As the XXXX is the dead giveaway, though, we've actually leaked the
> pointer -- we've given it to qemu_iovec_add, but I don't think there's a
> way to get it back, so we'll need to stash it /somewhere/.
> 
> In DBDMA_io ?

That sounds like a reasonable approach. I'm extremely low on QEMU cycle
for the next 2 weeks or so, but if you post something I'll try my best
to review it.


ATB,

Mark.