Re: [Qemu-devel] [PATCH 3/9] dma-helpers: Do not truncate small qiovs

[Paolo Bonzini <pbonzini@redhat.com>]

On 16/12/2015 17:55, Alex Pyrgiotis wrote:
> If the size of the qiov is smaller than the sector size, do not truncate
> the qiov, which would effectively make it empty. Instead, allow it to
> pass as is.
> 
> This is necessary for SCSI requests like READ CAPACITY which have small
> buffers, e.g. 32 bytes.
> 
> Signed-off-by: Alex Pyrgiotis <apyrgio@arrikto.com>
> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com>
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index e1ea7b3..b8f2ae0 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -162,7 +162,16 @@ static void dma_map_sg(DMAAIOCB *dbs)
>          return;
>      }
>  
> -    if (dbs->iov.size & ~BDRV_SECTOR_MASK) {
> +    /*
> +     * If the size of the qiov is not a multiple of the sector size, truncate
> +     * the qiov.
> +     *
> +     * NOTE: If the qiov is less than a sector, we can assume that there is a
> +     * reason for it, e.g., a SCSI request such as READ CAPACITY, and we should
> +     * not truncate it.
> +     */

I'm afraid this is wrong.  It is legal to send SCSI requests that are
e.g. 513 bytes in size.  The sector limit is arbitrary.

I think the "if" statement is wrong too, however.  In particular it has
not broken IDE TRIM only because the ATA standard makes 512 bytes the
unit of DMA transfer.  Rather, it is the IDE device that should discard
extra data in the QEMUSGList.

Paolo

> +    if (dbs->iov.size & ~BDRV_SECTOR_MASK &&
> +            dbs->iov.size > BDRV_SECTOR_SIZE) {
>          qemu_iovec_discard_back(&dbs->iov, dbs->iov.size & ~BDRV_SECTOR_MASK);
>      }
>  }
>