Hi Andre,
On Mon, Feb 8, 2021 at 9:33 PM Andre Przywara <andre.przyw...@arm.com> wrote:
>
> On Sun, 7 Feb 2021 14:13:37 -0500
> Tom Rini <tr...@konsulko.com> wrote:
>
> Hi Tom, Marek,
>
> > On Sun, Feb 07, 2021 at 07:20:14PM +0100, Marek Vasut wrote:
> > > On 2/4/21 5:57 PM, Tom Rini wrote:
> > > [...]
> > >
> > > > > > > > > > > +static void nvme_flush_dcache_range(void *start,
> > > > > > > > > > > unsigned long size)
> > > > > > > > > > > +{
> > > > > > > > > > > + unsigned long s, e;
> > > > > > > > > > > + nvme_align_dcache_range(start, size, &s, &e);
> > > > > > > > > > > + flush_dcache_range(s, e);
> > > > > > > > >
> > > > > > > > > There is no good reason for alignment restrictions when it
> > > > > > > > > comes to
> > > > > > > > > clean (& invalidate), so there is no need for this wrapper.
> > > > > > > >
> > > > > > > > Is that on ARM64-specific or is that applicable in general ?
> > > > > > > > The driver
> > > > > > > > is expected to work on any CPU.
> > > > > > >
> > > > > > > Cache clean (actually: cache clean&invalidate) is what happens on
> > > > > > > evictions
> > > > > > > all of the time, at the cache controller's discretion. So there
> > > > > > > is no
> > > > > > > real harm in that operation per se. When an eviction happens on a
> > > > > > > *clean* cache line, this is basically just an invalidate, which
> > > > > > > is also not
> > > > > > > harmful.
> > > > > > >
> > > > > > > There are harmful cases when buffers sharing a cache line are
> > > > > > > both "just invalidated"
> > > > > > > and "cleaned" at different points in time.
> > > > > >
> > > > > > Is that on ARM64-specific or is that applicable in general ? (the
> > > > > > above
> > > > > > does not answer that question)
> > > > >
> > > > > I would say that's a property of *every* write-back cache
> > > > > implementation:
> > > > > https://en.wikipedia.org/wiki/Cache_(computing)#/media/File:Write-back_with_write-allocation.svg
> > > >
> > > > I've been reading and digesting the thread as it goes, and the only
> > > > thing I do want to chime in on here right now is that yes, U-Boot
> > > > does and will continue to support every CPU that someone wants to run it
> > > > on, and one of the takeaways I see from this thread is we need some
> > > > better documented abstractions around cache, as it's very tricky to get
> > > > right all the time.
> > >
> > > Documenting the u-boot cache function behavior precisely is fine by me,
> > > but
> > > that is somewhat separate topic from this bugfix.
> > >
> > > So I will ask a simple question -- is there anything blocking this bugfix
> > > from being applied ?
> >
> > While I can fix the typo, I'm waiting for an Ack/Reviewed-by from Bin
> > (as he's spent the most time on this driver of late) and I'd really like
> > one from Andre, or at least him agreeing this patch is a step in the
> > right direction.
>
> As I said: I don't see how this patch changes anything on arm64, which
> the commit messages claims to be the reason for this post.
> If someone please can confirm, but invalidate_dcache_range() always
> works on arm64, in fact does the very rounding already that this patch
> introduces here. So what is the actual fix here?
>
> Plus, I consider this blanket rounding more harmful than helpful, since
> it actually just silences the check_cache_range() check.
>
> If we include armv7 here, which in fact would ignore(!) an unaligned
> invalidate, this is my analysis (just for invalidate):
> nvme_read_completion_status(): NEEDS ALIGNMENT
> size smaller than cache line, cqes[1] base address unaligned.
> fix needed, rounding *could* be a temporary fix with comments
> as of why this is legitimate in this case.
> Better alternative sketched in a previous email.
This is exactly what I mentioned before [1]. The only problematic
routine is the nvme_read_completion_status() but I didn't have time to
experiment with a fix.
> nvme_identify(): OK
> struct nvme_id_ctrl is 4K, if I counted correctly, so is fine.
> buffer comes the callers, the in-file users pass an aligned
> address, the only external user I see is in nvme_show.c, which
> also explicitly aligns the buffer.
> nvme_blk_rw(): OK
> total_len seems to be a multiple of block size, so is hopefully
> already cache line aligned.
> buffer comes from the upper layers, I guess it's page
> aligned already (at least 512b sector aligned)?
>
> I turned my sketch for the cqes[] fix into a proper patch and will send
> this in a minute.
Thanks for looking into this.
[1] https://lists.denx.de/pipermail/u-boot/2021-February/439580.html
Regards,
Bin
