[Qemu-devel] [PATCH] nvme: fix copy direction in DMA reads going to CMB

[Qemu-devel] [PATCH] nvme: fix copy direction in DMA reads going to CMB

[Klaus Birkelund Jensen]

`nvme_dma_read_prp` erronously used `qemu_iovec_*to*_buf` instead of
`qemu_iovec_*from*_buf` when the request involved the controller memory
buffer.

Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7caf92532a09..63a5b58849fb 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -238,7 +238,7 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
         }
         qemu_sglist_destroy(&qsg);
     } else {
-        if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) {
+        if (unlikely(qemu_iovec_from_buf(&iov, 0, ptr, len) != len)) {
             trace_nvme_err_invalid_dma();
             status = NVME_INVALID_FIELD | NVME_DNR;
         }
-- 
2.21.0

Re: [Qemu-devel] [PATCH] nvme: fix copy direction in DMA reads going to CMB

[Heitke, Kenneth]

On 5/18/2019 1:39 AM, Klaus Birkelund Jensen wrote:
> `nvme_dma_read_prp` erronously used `qemu_iovec_*to*_buf` instead of
> `qemu_iovec_*from*_buf` when the request involved the controller memory
> buffer.
> 
> Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>
> ---
>   hw/block/nvme.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 7caf92532a09..63a5b58849fb 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -238,7 +238,7 @@ static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
>           }
>           qemu_sglist_destroy(&qsg);
>       } else {
> -        if (unlikely(qemu_iovec_to_buf(&iov, 0, ptr, len) != len)) {
> +        if (unlikely(qemu_iovec_from_buf(&iov, 0, ptr, len) != len)) {
>               trace_nvme_err_invalid_dma();
>               status = NVME_INVALID_FIELD | NVME_DNR;
>           }
> 

Reviewed-by: Kenneth Heitke <kenneth.heitke@intel.com>

Re: [Qemu-devel] [PATCH] nvme: fix copy direction in DMA reads going to CMB

[Kevin Wolf]

Am 18.05.2019 um 09:39 hat Klaus Birkelund Jensen geschrieben:
> `nvme_dma_read_prp` erronously used `qemu_iovec_*to*_buf` instead of
> `qemu_iovec_*from*_buf` when the request involved the controller memory
> buffer.
> 
> Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] nvme: fix copy direction in DMA reads going to CMB

[Keith Busch]

On Sat, May 18, 2019 at 09:39:05AM +0200, Klaus Birkelund Jensen wrote:
> `nvme_dma_read_prp` erronously used `qemu_iovec_*to*_buf` instead of
> `qemu_iovec_*from*_buf` when the request involved the controller memory
> buffer.
> 
> Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>

I was wondering how this mistake got by for so long, and it looks like
the only paths here require an admin command with dev->host transfer
to CMB. That's just not done in any host implementation I'm aware of
since it'd make it more difficult to use for no particular gain AFAICS,
so I'd be curious to hear if you have a legit implementation doing this.

Anyway, thanks for the fix.

Re: [Qemu-devel] [PATCH] nvme: fix copy direction in DMA reads going to CMB

[Klaus Birkelund]

On Mon, May 20, 2019 at 09:05:57AM -0600, Keith Busch wrote:
> On Sat, May 18, 2019 at 09:39:05AM +0200, Klaus Birkelund Jensen wrote:
> > `nvme_dma_read_prp` erronously used `qemu_iovec_*to*_buf` instead of
> > `qemu_iovec_*from*_buf` when the request involved the controller memory
> > buffer.
> > 
> > Signed-off-by: Klaus Birkelund Jensen <klaus.jensen@cnexlabs.com>
> 
> I was wondering how this mistake got by for so long, and it looks like
> the only paths here require an admin command with dev->host transfer
> to CMB. That's just not done in any host implementation I'm aware of
> since it'd make it more difficult to use for no particular gain AFAICS,
> so I'd be curious to hear if you have a legit implementation doing this.
> 

I'm just trying to get the device to be as compliant as possible, but I
don't know why you'd have any reason to do a, say Get Feature, to the
CMB.