[RFC PATCH] Revert "arm: Show cache warnings in U-Boot proper only"

[Tom Rini]

On Thu, Dec 19, 2019 at 12:32:09PM +0000, Andre Przywara wrote:
> On Thu, 19 Dec 2019 12:43:35 +0100
> Marek Vasut <marex at denx.de> wrote:
> 
> Hi Marek,
> 
> > On 12/19/19 12:36 PM, Andre Przywara wrote:
> > > On Thu, 19 Dec 2019 09:04:32 +0100
> > > Marek Vasut <marex at denx.de> wrote:
> > >> On 12/19/19 2:23 AM, André Przywara wrote:  
> > >>> On 19/12/2019 00:55, Marek Vasut wrote:
> > >>>> On 12/19/19 1:52 AM, Andre Przywara wrote:    
> > >>>>> According to commit 11aa6a32eb5f ("arm: cache: Implement cache range
> > >>>>> check for v7"), which introduced check_cache_range(), this was meant
> > >>>>> as a pure debugging feature, only to be compiled in when a developer
> > >>>>> explicitly #defined DEBUG in cache.c. Presumably the intention was to
> > >>>>> help with finding *certain* alignment issues with DMA buffers.
> > >>>>>
> > >>>>> Commit bcc53bf09589 ("arm: Show cache warnings in U-Boot proper only")
> > >>>>> compiled this in *unconditionally* into U-Boot proper.
> > >>>>>
> > >>>>> This has the annoying side effect of producing tons of somewhat
> > >>>>> pointless warnings about non-aligned clean&invalidate operations, which
> > >>>>> tend to be appeased by even more pointless rounding operations in many
> > >>>>> drivers (mostly those used on ARM boards).
> > >>>>>
> > >>>>> Bring back the old behaviour, of only compiling this in for DEBUG
> > >>>>> situations, but staying silent otherwise.
> > >>>>>
> > >>>>> This reverts commit bcc53bf095893fbdae531a9a7b5d4ef4a125a7fc.
> > >>>>>
> > >>>>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
> > >>>>> ---
> > >>>>> Hi,
> > >>>>>
> > >>>>> if the intention was indeed to always force cache maintenance range
> > >>>>> alignments, I would like to open a discussion on this, because I believe
> > >>>>> it is not useful, especially in the clean&invalidate case.    
> > >>>>
> > >>>> Why don't you rather fix the cache op alignment bugs ?    
> > >>>
> > >>> Which bugs do you mean?    
> > >>
> > >> Well, the ones reported by this warning.  
> > > 
> > > As mentioned, most of them are just silenced in drivers, though not actually fixed.  
> > 
> > Do you have specific details here ? They should be fixed.
> > 
> > > This patch here was actually triggered by me cleaning up sun8i-emac.c, which then made those warnings appear again.
> > > I will try to send out this series, which might make it more clear what I am after.
> > >   
> > >>> Those that would currently trigger those warnings?
> > >>> I don't think they are actual bugs, besides I don't think they are any
> > >>> cases left for 32-bit ARM boards (leave alone the new RPi4 Ethernet
> > >>> driver in the rpi-4-32b_defconfig).    
> > >>
> > >> If your cache isn't flushed or invalidated properly, it's a bug.
> > >> On ARM32, this is (was, before this warning) a real problem.  
> > > 
> > > The actual cache maintenance is fine in the drivers, it's the over-strict alignment requirements that bothers me and makes things actually worse:
> > > 
> > > Architecturally (ARM ARM) there is no alignment requirement for VA operations. Walking over a range of addresses does get easier if you start on a cache line boundary, so this is what we do in the implementations for v7 and v8.  
> > 
> > U-Boot is not only V7 and V8, but also all the earlier ones (back to V4).
> 
> I checked v6 as well (same), and couldn't find any evidence that this would be different for v5 or v4, although CP15 does not seem to be architecturally defined there. Do you have an example of an older core failing that?
> But anyway this particular check and patch is affecting v7 only.
> 
> > > Alignment gets critical when it comes to a pure invalidate call. If you have other dirty data in this same cache line, you might lose the latest stores, that's why it is important to align those *buffers*. So the strict check we have in v7_dcache_inval_range() is the right thing (at least for the base address). What is not right is to deliberately align just the *parameters* of the invalidate call, as seens in net/sun8i_emac.c or net/dwc_eth_qos.c, for instance. This could actually hide bugs. Drivers should always pass on the actual buffer address to invalidate().  
> > 
> > Sure, but that's a driver bug. That's a separate topic from this patch.
> > 
> > > Then there is no real need to align the range for flush (clean&invalidate) calls. Overshooting here is not critical, and again we do align them anyway in the implementation.  
> > 
> > Wrong, you may interfere with some DMA engine which might be accessing
> > the area which you just overshot.
> 
> Can you give an example on that? Are you thinking about cleaning dirty cache lines to a DRAM region which DMA is just writing data into?
> After all a clean could happen at any time, when a line gets evicted from the cache, without an explicit cache maintenance instruction. So if you have such a case, that's a separate bug.
> 
> > > This
> > > forced check only leads to drivers aligning the *parameters* of these calls, to no actual effect, probably given a wrong impression to driver authors and readers.
> > 
> > Again, driver bug, that's unrelated and must be fixed.
> 
> If you fix them, they will trigger tons of warnings, mostly bogus, about end addresses not being aligned (see below). Been there, done that. That's why this patch as a first step to allow those fixes without spamming the user.
> 
> > > So in preparation for fixing the drivers, I ask for those checks to become pure DEBUG warnings. Another alternative would be to relax those checks, to just require the start address of invalidate to be aligned - but this is what we check separately already.  
> > 
> > See above.
> > 
> > >>> Or those that are currently hidden because we *force* an alignment on
> > >>> the *arguments* passed to invalidate_dcache_range, for instance?
> > >>> These are quite numerous, so I would rather get some input first before
> > >>> spending a lot of time on this.    
> > >>
> > >> Make sure your buffers are properly aligned and you have no cache
> > >> issues. It's really that easy. If you see numerous, then your platform
> > >> has a real problem which must be fixed ; I didn't see any for quite a
> > >> while on platforms I have access to.  
> > > 
> > > Yes, I think the buffers are correctly aligned in the drivers, as least the ones I checked. But then we should always pass this address to invalidate, and not round it just before calling invalidate. Same goes to some degree for the end address, although is a somewhat separate topic.  
> > 
> > Of course the address passed to cache ops should not be rounded up.
> > That's a bug (unless the buffer actually covers that rounded address,
> > which sometimes is the case, and might or might not thus be harmless.
> > That's up to the driver author to determine)
> 
> Yes, and that's why the strict unconditional check we have right now is not helping. We should have them in, and developers could check the warnings for being legit or not.
>  
> > > And that's why I think having check_cache_range() is useful for development and debugging, but only then, because not all warnings are legit (end addresses).  
> > 
> > This implication makes no sense. How do you infer from "drivers have
> > bugs" that a warning about such bugs should be turned into a DEBUG()?
> 
> Most (if not all) of those warnings come from the end address not being aligned. And this is something not easily being checked outside of the driver. What we want to check for invalidate is that we only invalidate a well known region, so:
> 1) The start address of the buffer is aligned.
> 2) The end address, rounded up to the next cache line, is still within our buffer (so nothing else is using the same cacheline).
> 
> We can check 1) easily, but not 2), that would require driver specific code, which knows about the size of the buffer. We could provide something like:
>   bool check_cache_buffer(void *start_address, size_t length, size_t buffer_size);
> which drivers would call explicitly. Or we could include the buffer size into the invalidate call, but that would change the interface.
> 
> That would at least cover the buffers that the driver provides itself (RX buffers for network drivers). Network TX buffers are handed in from the generic network code, and the driver does not know what is there after "length". So the best thing we can do is to call "flush_dcache_range(packet, packet + length);", and then should not receive warnings if the end address is unaligned. That's why I think checks for flush are not useful.

Thanks for having this discussion.  I think it is reasonable at this
point to investigate how we can evolve what we about cache flushing to
be better than what we have now.  Looking at the cases above, it seems
like it might be reasonable to have always enabled checks #1 (that can
easily be disabled) and if we can't easily migrate our callers to handle
#2 (and maybe not have that check be default enabled?) at least provide
the helper for it, for debugging issues.  Thanks all!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200106/1bc26e40/attachment.sig>