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

[John Snow <jsnow@redhat.com>]

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 <mark.cave-ayland@ilande.co.uk>
>>>
>>> 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 <mark.cave-ayland@ilande.co.uk>
>>> Acked-by: John Snow <jsnow@redhat.com>
>>> Message-id: 1425939893-14404-3-git-send-email-mark.cave-ayland@ilande.co.uk
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  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.

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 ?

--js