[PATCH] nvme: Invalidate dcache before submitting admin cmd
Some architecture like ARM Cortex A53, A72 would need to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory and found the sane nvme_ctrl result.
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
Cc: Andre Przywara andre.przywara@arm.com Reported-by: Suniel Mahesh sunil@amarulasolutions.com Signed-off-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Suniel Mahesh sunil@amarulasolutions.com --- drivers/nvme/nvme.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 0357aba7f1..fc64d93ab8 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -466,6 +466,9 @@ int nvme_identify(struct nvme_dev *dev, unsigned nsid,
c.identify.cns = cpu_to_le32(cns);
+ invalidate_dcache_range(dma_addr, + dma_addr + sizeof(struct nvme_id_ctrl)); + ret = nvme_submit_admin_cmd(dev, &c, NULL); if (!ret) invalidate_dcache_range(dma_addr,
On Sun, Jun 7, 2020 at 7:22 PM Jagan Teki jagan@amarulasolutions.com wrote:
Some architecture like ARM Cortex A53, A72 would need to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory and found the sane nvme_ctrl result.
insane?
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
Cc: Andre Przywara andre.przywara@arm.com Reported-by: Suniel Mahesh sunil@amarulasolutions.com Signed-off-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Suniel Mahesh sunil@amarulasolutions.com
drivers/nvme/nvme.c | 3 +++ 1 file changed, 3 insertions(+)
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On 07/06/2020 12:22, Jagan Teki wrote:
Hi,
(CC: ing Mark)
Without looking to deep, I think invalidating the cache might be the right thing to do, but the rationale or at least the wording of it seems somehow flawed:
Some architecture like ARM Cortex A53, A72 would need
Please don't mix the term "architecture" with actual implementations. And the problem is more general, I would say. This *might* be a case here where this is due to speculation, and then I would expect it to only show on an A72, for instance. I guess this is about NVMe on RK3399? Does U-Boot run on an A53 or an A72 core?
to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
That is somewhat confusing. What does "sync" and "flush" mean here? AFAIK only "clean" and "invalidate" are clearly defined terms. The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is purely about the buffer for the *return* data, in which case I would expect this being a pure invalidation problem.
This issue looks like the case described on slide 30 and 31 here: https://elinux.org/images/6/6d/Rutland2.pdf
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory
This does not sound right: dma_addr is always pointing to the right location in memory. The actual reason this fixes something might be that (some) cache lines of the DMA buffer were dirty and were evicted *before* the existing invalidation, but *after* the DMA transfer. That sounds unlikely in an U-Boot context, though, but would match the case described in Mark's slides.
So there might be more to this. Are we missing a barrier here? Should we use coherent buffers (memory mapped as un-cached) for the return DMA in the first place (but I don't think U-Boot provides those)?
and found the sane nvme_ctrl result.
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
If this shows up already, I think we should address all the comments in the driver where it says we should do cache maintenance. And it makes me wonder how this actually works today ...
Cheers, Andre.
Cc: Andre Przywara andre.przywara@arm.com Reported-by: Suniel Mahesh sunil@amarulasolutions.com Signed-off-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Suniel Mahesh sunil@amarulasolutions.com
drivers/nvme/nvme.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 0357aba7f1..fc64d93ab8 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -466,6 +466,9 @@ int nvme_identify(struct nvme_dev *dev, unsigned nsid,
c.identify.cns = cpu_to_le32(cns);
- invalidate_dcache_range(dma_addr,
dma_addr + sizeof(struct nvme_id_ctrl));- ret = nvme_submit_admin_cmd(dev, &c, NULL); if (!ret) invalidate_dcache_range(dma_addr,
Hi
On Mon, Jun 8, 2020 at 3:52 PM André Przywara andre.przywara@arm.com wrote:
On 07/06/2020 12:22, Jagan Teki wrote:
Hi,
(CC: ing Mark)
Without looking to deep, I think invalidating the cache might be the right thing to do, but the rationale or at least the wording of it seems somehow flawed:
Some architecture like ARM Cortex A53, A72 would need
Please don't mix the term "architecture" with actual implementations. And the problem is more general, I would say. This *might* be a case here where this is due to speculation, and then I would expect it to only show on an A72, for instance. I guess this is about NVMe on RK3399? Does U-Boot run on an A53 or an A72 core?
to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
We will adjust commit message. The idea is that when we invalidate after the DMA transfer, a cache flush happen for the invalidation itself
That is somewhat confusing. What does "sync" and "flush" mean here? AFAIK only "clean" and "invalidate" are clearly defined terms. The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is purely about the buffer for the *return* data, in which case I would expect this being a pure invalidation problem.
Correct
This issue looks like the case described on slide 30 and 31 here: https://elinux.org/images/6/6d/Rutland2.pdf
Thank you for point out to the slide
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory
This does not sound right: dma_addr is always pointing to the right location in memory. The actual reason this fixes something might be that (some) cache lines of the DMA buffer were dirty and were evicted *before* the existing invalidation, but *after* the DMA transfer. That sounds unlikely in an U-Boot context, though, but would match the case described in Mark's slides.
Does it not depend on how invalidation work in the particular CPU?
So there might be more to this. Are we missing a barrier here? Should we use coherent buffers (memory mapped as un-cached) for the return DMA in the first place (but I don't think U-Boot provides those)?
Right, I don't find a better way and yes this is general problem in uboot
Michael
and found the sane nvme_ctrl result.
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
If this shows up already, I think we should address all the comments in the driver where it says we should do cache maintenance. And it makes me wonder how this actually works today ...
Cheers, Andre.
Cc: Andre Przywara andre.przywara@arm.com Reported-by: Suniel Mahesh sunil@amarulasolutions.com Signed-off-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Suniel Mahesh sunil@amarulasolutions.com
drivers/nvme/nvme.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 0357aba7f1..fc64d93ab8 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -466,6 +466,9 @@ int nvme_identify(struct nvme_dev *dev, unsigned nsid,
c.identify.cns = cpu_to_le32(cns);
invalidate_dcache_range(dma_addr,dma_addr + sizeof(struct nvme_id_ctrl));ret = nvme_submit_admin_cmd(dev, &c, NULL); if (!ret) invalidate_dcache_range(dma_addr,
Hi André,
On Mon, Jun 8, 2020 at 9:52 PM André Przywara andre.przywara@arm.com wrote:
On 07/06/2020 12:22, Jagan Teki wrote:
Hi,
(CC: ing Mark)
Without looking to deep, I think invalidating the cache might be the right thing to do, but the rationale or at least the wording of it seems somehow flawed:
Some architecture like ARM Cortex A53, A72 would need
Please don't mix the term "architecture" with actual implementations. And the problem is more general, I would say. This *might* be a case here where this is due to speculation, and then I would expect it to only show on an A72, for instance. I guess this is about NVMe on RK3399? Does U-Boot run on an A53 or an A72 core?
to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
That is somewhat confusing. What does "sync" and "flush" mean here? AFAIK only "clean" and "invalidate" are clearly defined terms. The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is purely about the buffer for the *return* data, in which case I would expect this being a pure invalidation problem.
This issue looks like the case described on slide 30 and 31 here: https://elinux.org/images/6/6d/Rutland2.pdf
Thanks for the slides. This is really helpful.
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory
This does not sound right: dma_addr is always pointing to the right location in memory. The actual reason this fixes something might be that (some) cache lines of the DMA buffer were dirty and were evicted *before* the existing invalidation, but *after* the DMA transfer. That sounds unlikely in an U-Boot context, though, but would match the case described in Mark's slides.
So there might be more to this. Are we missing a barrier here? Should we use coherent buffers (memory mapped as un-cached) for the return DMA in the first place (but I don't think U-Boot provides those)?
and found the sane nvme_ctrl result.
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
If this shows up already, I think we should address all the comments in the driver where it says we should do cache maintenance. And it makes me wonder how this actually works today ...
I believe the driver was only validated on x86 and NXP's PowerPC series where cache coherence is ensured between the DMA master and the system memory. I suspect it was never validated on ARM hence this patch.
I agree we should address all the comments in the driver about cache maintenance.
Regards, Bin
Hi all
On Tue, Jun 9, 2020 at 6:13 AM Bin Meng bmeng.cn@gmail.com wrote:
Hi André,
On Mon, Jun 8, 2020 at 9:52 PM André Przywara andre.przywara@arm.com wrote:
On 07/06/2020 12:22, Jagan Teki wrote:
Hi,
(CC: ing Mark)
Without looking to deep, I think invalidating the cache might be the right thing to do, but the rationale or at least the wording of it seems somehow flawed:
Some architecture like ARM Cortex A53, A72 would need
Please don't mix the term "architecture" with actual implementations. And the problem is more general, I would say. This *might* be a case here where this is due to speculation, and then I would expect it to only show on an A72, for instance. I guess this is about NVMe on RK3399? Does U-Boot run on an A53 or an A72 core?
to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
That is somewhat confusing. What does "sync" and "flush" mean here? AFAIK only "clean" and "invalidate" are clearly defined terms. The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is purely about the buffer for the *return* data, in which case I would expect this being a pure invalidation problem.
This issue looks like the case described on slide 30 and 31 here: https://elinux.org/images/6/6d/Rutland2.pdf
Thanks for the slides. This is really helpful.
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory
This does not sound right: dma_addr is always pointing to the right location in memory. The actual reason this fixes something might be that (some) cache lines of the DMA buffer were dirty and were evicted *before* the existing invalidation, but *after* the DMA transfer. That sounds unlikely in an U-Boot context, though, but would match the case described in Mark's slides.
So there might be more to this. Are we missing a barrier here? Should we use coherent buffers (memory mapped as un-cached) for the return DMA in the first place (but I don't think U-Boot provides those)?
and found the sane nvme_ctrl result.
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
If this shows up already, I think we should address all the comments in the driver where it says we should do cache maintenance. And it makes me wonder how this actually works today ...
I believe the driver was only validated on x86 and NXP's PowerPC series where cache coherence is ensured between the DMA master and the system memory. I suspect it was never validated on ARM hence this patch.
I agree we should address all the comments in the driver about cache maintenance.
What about this description:
nvme: Invalidate dcache before submitting admin cmd
This patch tries to avoid eviction of dirty lines during DMA transfer. The code right now executes the following step:
- allocate the buffer - start a DMA operation using the non-coherent DMA buffer - invalidate cache lines associated with the buffer - read the buffer
This can lead to reading back not valid information. CPU can still read stale data. In order to avoid the eviction of dirty lines, a new invalidation is required before starting the DMA operation. The patch just adds an invalidation before submitting the DMA command.
The example below shows the nvme disk scan result without the following patch
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command, fix the cpu read.
Michael
Regards, Bin
-- | Michael Nazzareno Trimarchi Amarula Solutions BV | | COO - Founder Cruquiuskade 47 | | +31(0)851119172 Amsterdam 1018 AM NL | | [`as] http://www.amarulasolutions.com |
On 09/06/2020 19:47, Michael Nazzareno Trimarchi wrote:
Hi,
that looks much better now, thanks! Some suggestion below.
On Tue, Jun 9, 2020 at 6:13 AM Bin Meng bmeng.cn@gmail.com wrote:
Hi André,
On Mon, Jun 8, 2020 at 9:52 PM André Przywara andre.przywara@arm.com wrote:
On 07/06/2020 12:22, Jagan Teki wrote:
Hi,
(CC: ing Mark)
Without looking to deep, I think invalidating the cache might be the right thing to do, but the rationale or at least the wording of it seems somehow flawed:
Some architecture like ARM Cortex A53, A72 would need
Please don't mix the term "architecture" with actual implementations. And the problem is more general, I would say. This *might* be a case here where this is due to speculation, and then I would expect it to only show on an A72, for instance. I guess this is about NVMe on RK3399? Does U-Boot run on an A53 or an A72 core?
to invalidate dcache to sync the cache with the memory contents before flushing the cache to memory.
That is somewhat confusing. What does "sync" and "flush" mean here? AFAIK only "clean" and "invalidate" are clearly defined terms. The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is purely about the buffer for the *return* data, in which case I would expect this being a pure invalidation problem.
This issue looks like the case described on slide 30 and 31 here: https://elinux.org/images/6/6d/Rutland2.pdf
Thanks for the slides. This is really helpful.
The NVME here submitting the admin command using dma_addr to the memory without prior cache invalidation. This causing dma_addr is pointing to an invalid location in the memory
This does not sound right: dma_addr is always pointing to the right location in memory. The actual reason this fixes something might be that (some) cache lines of the DMA buffer were dirty and were evicted *before* the existing invalidation, but *after* the DMA transfer. That sounds unlikely in an U-Boot context, though, but would match the case described in Mark's slides.
So there might be more to this. Are we missing a barrier here? Should we use coherent buffers (memory mapped as un-cached) for the return DMA in the first place (but I don't think U-Boot provides those)?
and found the sane nvme_ctrl result.
Below example shows the nvme disk scan result improper result
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command makes the dma_addr points to a valid location in the memory.
If this shows up already, I think we should address all the comments in the driver where it says we should do cache maintenance. And it makes me wonder how this actually works today ...
I believe the driver was only validated on x86 and NXP's PowerPC series where cache coherence is ensured between the DMA master and the system memory. I suspect it was never validated on ARM hence this patch.
I agree we should address all the comments in the driver about cache maintenance.
What about this description:
nvme: Invalidate dcache before submitting admin cmd
This patch tries to avoid eviction of dirty lines during DMA transfer. The code right now executes the following step:
- allocate the buffer
- start a DMA operation using the non-coherent DMA buffer
- invalidate cache lines associated with the buffer
- read the buffer
This can lead to reading back not valid information. CPU can still read stale data. In order to avoid the eviction of dirty lines, a new invalidation is required before starting the DMA operation. The patch just adds an invalidation before submitting the DMA command.
As I think this case is not obvious, I would make it more clear what actually happens here, for instance (replace your paragraph above):
====================== This can lead to reading back not valid information, because the cache controller could evict dirty cache lines belonging to the buffer *after* the DMA operation has started to fill the DRAM. In order to avoid this, a new invalidation is required *before* starting the DMA operation. The patch just adds an invalidation before submitting the DMA command. ======================
And your point about NXP PCIe and x86 being cache coherent is a good one: Actually the ARM SBSA specification demands the same, so systems complying with this (servers, typically) would probably also work already. But those smartphone-class SoCs are probably not coherent.
It seems like drivers for PCI devices in U-Boot seem to assume some properties based on the host controllers and systems they have been tested on, as I found other issues lately. Some drivers do cache maintenance, others don't at all.
I will try to test NVMe on some other ARM platforms as soon as I can get a hand on one.
Many thanks! Andre
The example below shows the nvme disk scan result without the following patch
=> nvme scan nvme_get_info_from_identify: nn = 544502629, vwc = 100, sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
So, invalidating the cache before submitting the admin command, fix the cpu read.
Michael
Regards, Bin
-- | Michael Nazzareno Trimarchi Amarula Solutions BV | | COO - Founder Cruquiuskade 47 | | +31(0)851119172 Amsterdam 1018 AM NL | | [`as] http://www.amarulasolutions.com |
participants (4)
-
André Przywara -
Bin Meng -
Jagan Teki -
Michael Nazzareno Trimarchi