[PATCH 0/1] pci: pass along the return value of dma_memory_rw

[PATCH 0/1] pci: pass along the return value of dma_memory_rw

[Klaus Jensen]

Hi,

While working on fixing the emulated nvme device to pass more tests in
the blktests suite, I discovered that the pci_dma_rw function ignores
the return value of dma_memory_rw.

The nvme device needs to handle DMA errors gracefully in order to pass
the block/011 test ("disable PCI device while doing I/O") in the
blktests suite. This is only possible if the device knows if the DMA
transfer was successful or not.

I can't see what the reason for ignoring the return value would be. But
if there is a good reason, please enlighten me :)


Klaus Jensen (1):
  pci: pass along the return value of dma_memory_rw

 include/hw/pci/pci.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.23.0

[PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Klaus Jensen]

Some might actually care about the return value of dma_memory_rw. So
let us pass it along instead of ignoring it.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 include/hw/pci/pci.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index f3f0ffd5fb78..4e95bb847857 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -779,8 +779,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
 static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
                              void *buf, dma_addr_t len, DMADirection dir)
 {
-    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
-    return 0;
+    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
 }
 
 static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
-- 
2.23.0

Re: [PATCH 0/1] pci: pass along the return value of dma_memory_rw

[Klaus Birkelund]

On Fri, Oct 11, 2019 at 09:01:40AM +0200, Klaus Jensen wrote:
> Hi,
> 
> While working on fixing the emulated nvme device to pass more tests in
> the blktests suite, I discovered that the pci_dma_rw function ignores
> the return value of dma_memory_rw.
> 
> The nvme device needs to handle DMA errors gracefully in order to pass
> the block/011 test ("disable PCI device while doing I/O") in the
> blktests suite. This is only possible if the device knows if the DMA
> transfer was successful or not.
> 
> I can't see what the reason for ignoring the return value would be. But
> if there is a good reason, please enlighten me :)
> 
> 
> Klaus Jensen (1):
>   pci: pass along the return value of dma_memory_rw
> 
>  include/hw/pci/pci.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> -- 
> 2.23.0
> 

Gentle ping.

https://patchwork.kernel.org/patch/11184911/

Re: [PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Philippe Mathieu-Daudé]

On 10/11/19 9:01 AM, Klaus Jensen wrote:
> Some might actually care about the return value of dma_memory_rw. So
> let us pass it along instead of ignoring it.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   include/hw/pci/pci.h | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index f3f0ffd5fb78..4e95bb847857 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -779,8 +779,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
>                                void *buf, dma_addr_t len, DMADirection dir)
>   {
> -    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> -    return 0;
> +    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
>   }
>   
>   static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Klaus Birkelund]

On Thu, Oct 24, 2019 at 01:13:36AM +0200, Philippe Mathieu-Daudé wrote:
> On 10/11/19 9:01 AM, Klaus Jensen wrote:
> > Some might actually care about the return value of dma_memory_rw. So
> > let us pass it along instead of ignoring it.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   include/hw/pci/pci.h | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index f3f0ffd5fb78..4e95bb847857 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -779,8 +779,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> >   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> >                                void *buf, dma_addr_t len, DMADirection dir)
> >   {
> > -    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > -    return 0;
> > +    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> >   }
> >   static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> > 
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Gentle ping on this.

This fix is required for the nvme device to start passing some of the
nasty tests from blktests that flips bus mastering while doing I/O.


Cheers,
Klaus

Re: [PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Michael S. Tsirkin]

On Mon, Nov 11, 2019 at 10:30:07AM +0100, Klaus Birkelund wrote:
> On Thu, Oct 24, 2019 at 01:13:36AM +0200, Philippe Mathieu-Daudé wrote:
> > On 10/11/19 9:01 AM, Klaus Jensen wrote:
> > > Some might actually care about the return value of dma_memory_rw. So
> > > let us pass it along instead of ignoring it.
> > > 
> > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > ---
> > >   include/hw/pci/pci.h | 3 +--
> > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index f3f0ffd5fb78..4e95bb847857 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -779,8 +779,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> > >   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> > >                                void *buf, dma_addr_t len, DMADirection dir)
> > >   {
> > > -    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > -    return 0;
> > > +    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > >   }
> > >   static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> > > 
> > 
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Gentle ping on this.
> 
> This fix is required for the nvme device to start passing some of the
> nasty tests from blktests that flips bus mastering while doing I/O.
> 
> 
> Cheers,
> Klaus

So I looked and it does not seem like anyone at all checks the
return value. While this makes the patch safe, how come it
helps anyone at all?
Maybe this is just infrastructure to allow checks in the
future, in this case do we need this for the freeze?
Or can it wait until after the release?

-- 
MST

Re: [PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Klaus Birkelund]

On Mon, Nov 11, 2019 at 05:16:41AM -0500, Michael S. Tsirkin wrote:
> On Mon, Nov 11, 2019 at 10:30:07AM +0100, Klaus Birkelund wrote:
> > On Thu, Oct 24, 2019 at 01:13:36AM +0200, Philippe Mathieu-Daudé wrote:
> > > On 10/11/19 9:01 AM, Klaus Jensen wrote:
> > > > Some might actually care about the return value of dma_memory_rw. So
> > > > let us pass it along instead of ignoring it.
> > > > 
> > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > ---
> > > >   include/hw/pci/pci.h | 3 +--
> > > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > index f3f0ffd5fb78..4e95bb847857 100644
> > > > --- a/include/hw/pci/pci.h
> > > > +++ b/include/hw/pci/pci.h
> > > > @@ -779,8 +779,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> > > >   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> > > >                                void *buf, dma_addr_t len, DMADirection dir)
> > > >   {
> > > > -    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > > -    return 0;
> > > > +    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > >   }
> > > >   static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> > > > 
> > > 
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > 
> > Gentle ping on this.
> > 
> > This fix is required for the nvme device to start passing some of the
> > nasty tests from blktests that flips bus mastering while doing I/O.
> > 
> > 
> > Cheers,
> > Klaus
> 
> So I looked and it does not seem like anyone at all checks the
> return value. While this makes the patch safe, how come it
> helps anyone at all?

I have a series[1] under review for the nvme device (I should have
mentioned that). There is a certain test (block/011) from blktests[2],
that disables the PCI device while doing I/O by flipping the bus master
register. QEMU correctly understands this which causes the dma_memory_rw
calls to fail while the device is not a bus master. For the NVMe device
to pass that test, it needs to know that it could not do the DMA,
otherwise it will just think that a completion queue entry was
successfully posted or data was correctly read.

> Maybe this is just infrastructure to allow checks in the
> future, in this case do we need this for the freeze?
> Or can it wait until after the release?
> 

The series I'm mentioning won't be going into the release, so yeah - it
can surely wait. I was just pinging to make sure someone would pick it
up at some point :)
 
    [1]: https://patchwork.kernel.org/cover/11190045/
    [2]: https://github.com/osandov/blktests

Re: [PATCH 1/1] pci: pass along the return value of dma_memory_rw

[Michael S. Tsirkin]

On Mon, Nov 11, 2019 at 11:33:17AM +0100, Klaus Birkelund wrote:
> On Mon, Nov 11, 2019 at 05:16:41AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Nov 11, 2019 at 10:30:07AM +0100, Klaus Birkelund wrote:
> > > On Thu, Oct 24, 2019 at 01:13:36AM +0200, Philippe Mathieu-Daudé wrote:
> > > > On 10/11/19 9:01 AM, Klaus Jensen wrote:
> > > > > Some might actually care about the return value of dma_memory_rw. So
> > > > > let us pass it along instead of ignoring it.
> > > > > 
> > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > > ---
> > > > >   include/hw/pci/pci.h | 3 +--
> > > > >   1 file changed, 1 insertion(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > > index f3f0ffd5fb78..4e95bb847857 100644
> > > > > --- a/include/hw/pci/pci.h
> > > > > +++ b/include/hw/pci/pci.h
> > > > > @@ -779,8 +779,7 @@ static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> > > > >   static inline int pci_dma_rw(PCIDevice *dev, dma_addr_t addr,
> > > > >                                void *buf, dma_addr_t len, DMADirection dir)
> > > > >   {
> > > > > -    dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > > > -    return 0;
> > > > > +    return dma_memory_rw(pci_get_address_space(dev), addr, buf, len, dir);
> > > > >   }
> > > > >   static inline int pci_dma_read(PCIDevice *dev, dma_addr_t addr,
> > > > > 
> > > > 
> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > 
> > > Gentle ping on this.
> > > 
> > > This fix is required for the nvme device to start passing some of the
> > > nasty tests from blktests that flips bus mastering while doing I/O.
> > > 
> > > 
> > > Cheers,
> > > Klaus
> > 
> > So I looked and it does not seem like anyone at all checks the
> > return value. While this makes the patch safe, how come it
> > helps anyone at all?
> 
> I have a series[1] under review for the nvme device (I should have
> mentioned that). There is a certain test (block/011) from blktests[2],
> that disables the PCI device while doing I/O by flipping the bus master
> register. QEMU correctly understands this which causes the dma_memory_rw
> calls to fail while the device is not a bus master. For the NVMe device
> to pass that test, it needs to know that it could not do the DMA,
> otherwise it will just think that a completion queue entry was
> successfully posted or data was correctly read.
> 
> > Maybe this is just infrastructure to allow checks in the
> > future, in this case do we need this for the freeze?
> > Or can it wait until after the release?
> > 
> 
> The series I'm mentioning won't be going into the release, so yeah - it
> can surely wait. I was just pinging to make sure someone would pick it
> up at some point :)
>  
>     [1]: https://patchwork.kernel.org/cover/11190045/
>     [2]: https://github.com/osandov/blktests

It's probably best to just include it in the series.
When you do feel free to include

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

and mention that the return code has no existing
users so it's safe to change.