It's possible that the data cache for the buffer still holds data to be flushed to memory, since the buffer was probably used as stack before. Thus we need to make sure to flush it also on reads, since it's possible that the cache is automatically flused to memory after the NVMe DMA transfer happened, thus overwriting the NVMe transfer's data. Also add a missing dcache flush for the prp list.

Signed-off-by: Patrick Wildt patrick@blueri.se --- drivers/nvme/nvme.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index dee92b613d..f915817aaa 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -123,6 +123,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2, } *prp2 = (ulong)dev->prp_pool;

+ flush_dcache_range((ulong)dev->prp_pool, (ulong)dev->prp_pool + + dev->prp_entry_num * sizeof(u64)); + return 0; }

@@ -717,9 +720,8 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); u64 total_lbas = blkcnt;

- if (!read) - flush_dcache_range((unsigned long)buffer, - (unsigned long)buffer + total_len); + flush_dcache_range((unsigned long)buffer, + (unsigned long)buffer + total_len);

c.rw.opcode = read ? nvme_cmd_read : nvme_cmd_write; c.rw.flags = 0;

On Thu, Oct 17, 2019 at 10:55:11AM +0800, Bin Meng wrote:

Hi Patrick,

On Wed, Oct 16, 2019 at 11:35 PM Patrick Wildt patrick@blueri.se wrote:

...

On Wed, Oct 16, 2019 at 06:11:23PM +0800, Bin Meng wrote:

...

On Mon, Oct 14, 2019 at 7:11 PM Patrick Wildt patrick@blueri.se wrote:

...

On an i.MX8MQ our nvme driver doesn't completely work since we are missing a few cache flushes. One is the prp list, which is an extra buffer that we need to flush before handing it to the hardware. Also the block read/write operations needs more cache flushes on this SoC.

Signed-off-by: Patrick Wildt patrick@blueri.se

drivers/nvme/nvme.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 2444e0270f..69d5e3eedc 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -123,6 +123,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2, } *prp2 = (ulong)dev->prp_pool;

•   flush_dcache_range((ulong)dev->prp_pool, (ulong)dev->prp_pool +
  

•                      dev->prp_entry_num * sizeof(u64));
  

•   return 0;
  

}

@@ -717,9 +720,10 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); u64 total_lbas = blkcnt;

•   if (!read)
  

•           flush_dcache_range((unsigned long)buffer,
  

•                              (unsigned long)buffer + total_len);
  

•   flush_dcache_range((unsigned long)buffer,
  

•                      (unsigned long)buffer + total_len);
  

Why we need this for read?

I'm no processor engineer, but I have read (and "seen") the following on ARMs. If I'm wrong. please correct me.

Since the buffer is usually allocated cache-aligned on the stack, it is very possible that this buffer was previously still used as it's supposed to be used: as stack. Thus, the caches can still be filled, and are not yet evicted/flushed to memory. Now it is possible that between the DMA access from the device and our cache invalidation, the CPU cache writes back its contents to memory, overwriting whatever the NVMe just gave us.

OK, this makes sense. So if we allocate the buffer from the heap, we should only care about flush on write, right? Can you test this?

If you're talking about having a bounce buffer: You'd need to flush it once upon allocation, because that part of the heap could have also be used earlier by someone else.

Best regards, Patrick

...

...

...

•   invalidate_dcache_range((unsigned long)buffer,
  

•                           (unsigned long)buffer + total_len);
  
    c.rw.opcode = read ? nvme_cmd_read : nvme_cmd_write;
    c.rw.flags = 0;
  

@@ -755,9 +759,8 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, buffer += lbas << ns->lba_shift; }

•   if (read)
  

•           invalidate_dcache_range((unsigned long)buffer,
  

•                                   (unsigned long)buffer + total_len);
  

•   invalidate_dcache_range((unsigned long)buffer,
  

•                           (unsigned long)buffer + total_len);
  

Why we need this for write?

That's a good point. After the transaction, if it was a read then we need to invalidate it, as we might have speculatively read it. On a write, we don't care about its contents. I will test it w/o this chunk and report back.

Regards, Bin

Hi Patrick,

On Thu, Oct 17, 2019 at 2:44 PM Patrick Wildt patrick@blueri.se wrote:

On Thu, Oct 17, 2019 at 10:55:11AM +0800, Bin Meng wrote:

...

Hi Patrick,

On Wed, Oct 16, 2019 at 11:35 PM Patrick Wildt patrick@blueri.se wrote:

...

On Wed, Oct 16, 2019 at 06:11:23PM +0800, Bin Meng wrote:

...

On Mon, Oct 14, 2019 at 7:11 PM Patrick Wildt patrick@blueri.se wrote:

...

On an i.MX8MQ our nvme driver doesn't completely work since we are missing a few cache flushes. One is the prp list, which is an extra buffer that we need to flush before handing it to the hardware. Also the block read/write operations needs more cache flushes on this SoC.

Signed-off-by: Patrick Wildt patrick@blueri.se

drivers/nvme/nvme.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 2444e0270f..69d5e3eedc 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -123,6 +123,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2, } *prp2 = (ulong)dev->prp_pool;

•   flush_dcache_range((ulong)dev->prp_pool, (ulong)dev->prp_pool +
  

•                      dev->prp_entry_num * sizeof(u64));
  

•   return 0;
  

}

@@ -717,9 +720,10 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); u64 total_lbas = blkcnt;

•   if (!read)
  

•           flush_dcache_range((unsigned long)buffer,
  

•                              (unsigned long)buffer + total_len);
  

•   flush_dcache_range((unsigned long)buffer,
  

•                      (unsigned long)buffer + total_len);
  

Why we need this for read?

I'm no processor engineer, but I have read (and "seen") the following on ARMs. If I'm wrong. please correct me.

Since the buffer is usually allocated cache-aligned on the stack, it is very possible that this buffer was previously still used as it's supposed to be used: as stack. Thus, the caches can still be filled, and are not yet evicted/flushed to memory. Now it is possible that between the DMA access from the device and our cache invalidation, the CPU cache writes back its contents to memory, overwriting whatever the NVMe just gave us.

OK, this makes sense. So if we allocate the buffer from the heap, we should only care about flush on write, right? Can you test this?

If you're talking about having a bounce buffer: You'd need to flush it once upon allocation, because that part of the heap could have also be used earlier by someone else.

I was not talking about bounce buffer. I mean the buffer allocated from help and use that directly for DMA.

Regards, Bin

Hi All, On 10/17/2019 12:12 AM, Patrick Wildt wrote:

On Thu, Oct 17, 2019 at 03:08:59PM +0800, Bin Meng wrote:

...

Hi Patrick,

On Thu, Oct 17, 2019 at 2:44 PM Patrick Wildt patrick@blueri.se wrote:

...

On Thu, Oct 17, 2019 at 10:55:11AM +0800, Bin Meng wrote:

...

Hi Patrick,

On Wed, Oct 16, 2019 at 11:35 PM Patrick Wildt patrick@blueri.se wrote:

...

On Wed, Oct 16, 2019 at 06:11:23PM +0800, Bin Meng wrote:

...

On Mon, Oct 14, 2019 at 7:11 PM Patrick Wildt patrick@blueri.se wrote: > On an i.MX8MQ our nvme driver doesn't completely work since we are > missing a few cache flushes. One is the prp list, which is an extra > buffer that we need to flush before handing it to the hardware. Also > the block read/write operations needs more cache flushes on this SoC. > > Signed-off-by: Patrick Wildt patrick@blueri.se > --- > drivers/nvme/nvme.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > index 2444e0270f..69d5e3eedc 100644 > --- a/drivers/nvme/nvme.c > +++ b/drivers/nvme/nvme.c > @@ -123,6 +123,9 @@ static int nvme_setup_prps(struct nvme_dev *dev, u64 *prp2, > } > *prp2 = (ulong)dev->prp_pool; > > + flush_dcache_range((ulong)dev->prp_pool, (ulong)dev->prp_pool + > + dev->prp_entry_num * sizeof(u64)); > + > return 0; > } > > @@ -717,9 +720,10 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, > u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); > u64 total_lbas = blkcnt; > > - if (!read) > - flush_dcache_range((unsigned long)buffer, > - (unsigned long)buffer + total_len); > + flush_dcache_range((unsigned long)buffer, > + (unsigned long)buffer + total_len); Why we need this for read?

I'm no processor engineer, but I have read (and "seen") the following on ARMs. If I'm wrong. please correct me.

Since the buffer is usually allocated cache-aligned on the stack, it is very possible that this buffer was previously still used as it's supposed to be used: as stack. Thus, the caches can still be filled, and are not yet evicted/flushed to memory. Now it is possible that between the DMA access from the device and our cache invalidation, the CPU cache writes back its contents to memory, overwriting whatever the NVMe just gave us.

OK, this makes sense. So if we allocate the buffer from the heap, we should only care about flush on write, right? Can you test this?

If you're talking about having a bounce buffer: You'd need to flush it once upon allocation, because that part of the heap could have also be used earlier by someone else.

I was not talking about bounce buffer. I mean the buffer allocated from help and use that directly for DMA.

Regards, Bin

If you allocate a buffer from the heap, you still need to make sure to flush it once, since there's still the chance that you have used it shortly earlier. But it's less of an issue as on the stack.

The "rules" for cache management of DMA buffers for non-cache-coherent CPUs are immutable. It doesn't matter where the memory came from (static, heap, or stack). It may be in cache, and it may be dirty. You can't know  it is for sure "clean". It is assumed that the DMA buffers are allocated to begin on a cache line boundary and are a multiple of a cache line in length. If this is not the case, references by the CPU to locations outside the buffer can make the cache state of the buffer dirty, which is an error. It is also required that there be no accesses to the DMA buffer by the CPU while DMA is in progress. This is normally true by default, and if it isn't true, it is an error. The rules are then as follows:

On write, before DMA is started. the cache corresponding to the DMA buffer addresses MUST be flushed to ensure the desired content is transferred from cache to RAM before write DMA begins.

On read, before DMA is started, the cache corresponding to the DMA buffer addresses MUST be either invalidated or flushed to ensure that no cache system initiated writes to RAM will occur while read DMA is in progress. Normally, invalidate is preferred because it is faster. However, for programming simplicity some drivers choose to flush before both read or write DMA is started. If that is the case, it is good practice to note that choice in a comment.

On write, after DMA is completed, no additional cache actions are required.

On read, after DMA is completed, the cache corresponding to the DMA buffer addresses MUST be invalidated. This is necessary to ensure that stale data in the cache will not be used instead of the new read data in RAM. If one elected to invalidate the cache before the read DMA started, one may wonder why a second invalidate is necessary.  Since the buffer is not allowed to be referenced by the program in the interim, the cache should not contain any data from the DMA buffer area. The reason is that modern CPUs, may load data from the DMA buffer into cache while DMA is in progress. This can be the product of hardware "optimizations" such as pre-load. This data may be stale, and shouldn't be used. The invalidate after DMA is complete makes sure that it isn't used. If you absolutely, for sure, know your CPU doesn't do this, you could in theory omit the second invalidate. IMHO, it isn't worth the risk. If the code is re-used on a newer CPU, it may stop working.

These are the things you must do. You don't need to do anything more, and to do less will cause problems. I believe that some CPUs do not provide a cache_invalidate function, but rather a cache_flush_and_invalidate function. That is fine. The flush is gratuitous but not harmful.

From these rules, the code in question would be correct as

if (read) invalidate_dcache_range((unsigned long)buffer, (unsigned long)buffer + total_len); else  flush_dcache_range((unsigned long)buffer, (unsigned long)buffer + total_len);

or equally /* update DMA buffer RAM, ensure no cache system writes to DMA buffer RAM after this point */ flush_dcache_range((unsigned long)buffer, (unsigned long)buffer + total_len);

FWIW, I would also caution that testing is NOT a good way to ensure the cache management is correct. It must be correct by design. Inspect the code to verify it follows the rules. The problems induced by incorrect cache management may only show up for certain memory address reference patterns. Everything may appear to work fine, then somebody makes a change in a totally unrelated area, which moves the DMA buffer address, and suddenly you have problems.

Regards, Bill

Regards, Patrick _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot