Re: [Qemu-devel] [PATCH for-2.4] macio: re-add TRIM support

[Mark Cave-Ayland]

On 29/07/15 20:27, Aurelien Jarno wrote:

> Commit bd4214fc dropped TRIM support by mistake. Given it is still
> advertised to the host when using a drive with discard=on, this cause
> the IDE bus to hang when the host issues a TRIM command.
> 
> This patch fixes that by re-adding the TRIM code, ported to the new
> new DMA implementation.
> 
> Cc: Mark Cave-Ayland <address@hidden>
> Cc: John Snow <address@hidden>
> Signed-off-by: Aurelien Jarno <address@hidden>
> ---
>  hw/ide/macio.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index a55a479..66ac2ba 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -208,6 +208,33 @@ static void pmac_dma_write(BlockBackend *blk,
>                                cb, io);
>  }
>  
> +static void pmac_dma_trim(BlockBackend *blk,
> +                        int64_t offset, int bytes,
> +                        void (*cb)(void *opaque, int ret), void *opaque)
> +{
> +    DBDMA_io *io = opaque;
> +    MACIOIDEState *m = io->opaque;
> +    IDEState *s = idebus_active_if(&m->bus);
> +    dma_addr_t dma_addr, dma_len;
> +    void *mem;
> +
> +    qemu_iovec_destroy(&io->iov);
> +    qemu_iovec_init(&io->iov, io->len / MACIO_PAGE_SIZE + 1);
> +
> +    dma_addr = io->addr;
> +    dma_len = io->len;
> +    mem = dma_memory_map(&address_space_memory, dma_addr, &dma_len,
> +                         DMA_DIRECTION_TO_DEVICE);
> +
> +    qemu_iovec_add(&io->iov, mem, io->len);
> +    s->io_buffer_size -= io->len;
> +    s->io_buffer_index += io->len;
> +    io->len = 0;
> +
> +    m->aiocb = ide_issue_trim(blk, (offset >> 9), &io->iov, (bytes >> 9),
> +                              cb, io);
> +}
> +
>  static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
>  {
>      DBDMA_io *io = opaque;
> @@ -313,6 +340,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>          pmac_dma_write(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      case IDE_DMA_TRIM:
> +        pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
>      }

Apologies for the slow response on this (it has been a busy week here).
The basic patch itself looks good, my only question would be about the
effect on the Darwin/OS X family, since like the previous code the TRIM
request is rounded to the nearest sector whereas these OSs do use
byte-aligned DMA buffers.

Having said that, since the option has to be explicitly enabled for the
guest (and does the OS driver even support TRIM on these OSs?) then I
think this patch is fine, and I can confirm it passes my tests here.


ATB,

Mark.