[PATCH] arm: add check for global exclusive monitor

[Vladimir Murzin]

On Wed, Feb 20, 2013 at 10:59:57AM +0000, Russell King - ARM Linux wrote:
> On Wed, Feb 20, 2013 at 08:55:34AM +0400, Vladimir Murzin wrote:
> > Thanks for review Russel!
> > 
> > On Mon, Feb 18, 2013 at 04:44:20PM +0000, Russell King - ARM Linux wrote:
> > > On Mon, Feb 18, 2013 at 08:26:50PM +0400, Vladimir Murzin wrote:
> > > > +void __init check_gmonitor_bugs(void)
> > > > +{
> > > > +   struct page *page;
> > > > +   const char *reason;
> > > > +   unsigned long res = 1;
> > > > +
> > > > +   printk(KERN_INFO "CPU: Testing for global monitor: ");
> > > > +
> > > > +   page = alloc_page(GFP_KERNEL);
> > > > +   if (page) {
> > > > +           unsigned long *p;
> > > > +           pgprot_t prot = __pgprot_modify(PAGE_KERNEL,
> > > > +                                   L_PTE_MT_MASK, L_PTE_MT_UNCACHED);
> > > > +
> > > > +           p = vmap(&page, 1, VM_IOREMAP, prot);
> > >
> > > This is bad practise.  Remapping a page of already mapped kernel memory
> > > using different attributes (in this case, strongly ordered) is _definitely_
> > > a violation of the architecture requirements.  The behaviour you will see
> > > from this are in no way guaranteed.
> > DDI0406C_arm_architecture_reference_manual.pdf (A3-131) says:
> > 
> > A memory location can be marked as having different cacheability
> > attributes, for example when using aliases in a
> > virtual to physical address mapping:
> > * if the attributes differ only in the cache allocation hint this does
> >   not affect the behavior of accesses to that location
> > * for other cases see Mismatched memory attributes on page A3-136.
> > 
> > Isn't L_PTE_MT_UNCACHED about cache allocation hint?
> 
> No.  I've told you above what this gets you.
> 
> > > If you want to do this, it must either come from highmem, or not already
> > > be mapped.
> > >
> > > Moreover, this is absolutely silly - the ARM ARM says:
> > >
> > > "LDREX and STREX operations *must* be performed only on memory with the
> > >  Normal memory attribute."
> > 
> > DDI0406C_arm_architecture_reference_manual.pdf (A3-121) says:
> > 
> > It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be
> > performed to a memory region with the Device or Strongly-ordered
> > memory attribute. Unless the implementation documentation explicitly
> > states that LDREX and STREX operations to a memory region with the
> > Device or Strongly-ordered attribute are permitted, the effect of such
> > operations is UNPREDICTABLE.
> > 
> > At least it allows to perform operations on memory region with the
> > Strongly-ordered attribute... but still unpredictable.
> 
> And "unpredictable" means that you can't rely on *any* aspect of its
> behaviour.  You can't test for its behaviour and then rely on the
> result of that test telling you how it will behave in future.
> 
> Even if a "ldrex/strex" sequence to a strongly-ordered region succeeds
> or fails, that's no guarantee with "unpredictable" that it will have
> that behaviour next time.
> 
> > > L_PTE_MT_UNCACHED doesn't get you that.  As I say above, that gets you
> > > strongly ordered memory, not "normal memory" as required by the
> > > architecture for use with exclusive types.
> > >
> > > > +
> > > > +           if (p) {
> > > > +                   int temp;
> > > > +
> > > > +                   __asm__ __volatile__(                   \
> > > > +                           "ldrex   %1, [%2]\n"            \
> > > > +                           "strex   %0, %1, [%2]"          \
> > > > +                           : "=&r" (res), "=&r" (temp)     \
> > > > +                           : "r" (p)                       \
> > >
> > > \ character not required for any of the above.  Neither is the __ version
> > > of "asm" and "volatile".
> > Thanks.
> > >
> > > > +                           : "cc", "memory");
> > > > +
> > > > +                           reason = "n\\a (atomic ops may be faulty)";
> > >
> > > "n\\a" ?
> > "not detected"?
> > > So... at the moment this has me wondering - you're testing atomic
> > > operations with a strongly ordered memory region, which ARM already
> > > define this to be outside of the architecture spec.  The behaviour you
> > > see is not defined architecturally.
> > >
> > > And if you're trying to use LDREX/STREX to a strongly ordered or device
> > > memory region, then you're quite right that it'll be unreliable.  It's
> > > not defined to even work.  That's not because they're faulty, it's because
> > > you're abusing them.
> > However, IRL it is not hard to meet this undefined difference. At
> > least I'm able to see it on Tegra2 Harmony and Pandaboard. Moreover,
> > demand on Normal memory attribute breaks up ability to turn caches
> > off. In this case we are not able to boot the system up (seen on
> > Tegra2 harmony). This patch is aimed to highlight the difference in
> > implementation. That's why it has some softering in guessing about
> > faulty. Might be it worth warning about unpredictable effect instead?
> 
> You're soo busy quoting bits of architecture manual back at me that you
> can't see your own errors - errors which I've pointed out already.
> 
> "normal memory" != "uncached".  Normal memory can be uncacheable.
> Asking the kernel for L_PTE_MT_BUFFERABLE will give you "normal memory,
> uncached".

Sorry for delay.

Thanks for pointing the error. It was bad idea to make assumption
about attribute based on the name. I shoul have checked them first.

I'll update the patch soon.

Best wishes
Vladimir