[PATCH v6 00/18] ARM: Add support for the Large Physical Address Extensions

On Mon, Jul 04, 2011 at 06:23:00PM +0100, Catalin Marinas wrote:

On Sat, Jul 02, 2011 at 01:19:28PM +0100, Russell King - ARM Linux wrote:

quoted

On Fri, Jul 01, 2011 at 05:24:19PM +0100, Catalin Marinas wrote:

quoted

On Tue, May 24, 2011 at 10:39:06PM +0100, Catalin Marinas wrote:

quoted

Catalin Marinas (14):
  ARM: LPAE: Use unsigned long for __phys_to_virt and __virt_to_phys

I don't like this; these macros are supposed to take unsigned long
arguments (the hint is in their non-__ versions and __[vp]a, which
contain the explicit casts.)  I'd much rather have their callers
reduced in number, especially as this is a macro which platform classes
may (and do) define.

There are five places which use __phys_to_virt and one place which uses
__virt_to_phys.

Of the __virt_to_phys, this takes a u32 argument anyway, so that won't
cause any concerns.

Of the __phys_to_virt, the L2 cache handing files can use a void * for
vaddr and use the standard phys_to_virt().  That leaves three in the core
arch code, which can be dealt with by local casting.

The error actually comes via the default __bus_to_virt. I can change the
default dma_to_virt() to do the casting (and the other places that you
mentioned).

Is that because of:

static inline void *dma_to_virt(struct device *dev, dma_addr_t addr)
{
        return (void *)__bus_to_virt(addr);
}

?  In theory, we should never be passed a dma_addr_t > 32-bit here, but
we probably want a check in there just in case.  On the other hand, it's
only used (after this merge window) in dma_sync_single_range_for_cpu
and dma_sync_single_range_for_device.  I'm not sure its worth keeping
just for those.

quoted

quoted

quoted

  ARM: LPAE: Use PMD_(SHIFT|SIZE|MASK) instead of PGDIR_*

This one I think is fine, except the dependency on what we do about the
dma coherent code...

Is this in -next already?

Yes it is, but as I've been saying I think I'm dropping it for this merge
window because it causes regressions on existing platforms to the extent
that they fail to boot.  We could move forward by getting everywhere else
fixed up...

quoted

Also, see the patch below which is something I've
been thinking about to avoid the multiple-walking of the page table tree.

I'll comment on the patch below.

quoted

However, can we guarantee what the L1 page table in LPAE will contain?

The L1 can either be empty or point to the L2 page table. What other
guarantees do you mean?

That.  So unused L1 entries won't contain garbage.

quoted

quoted

quoted

  ARM: LPAE: Factor out 2-level page table definitions into separate
    files
  ARM: LPAE: Add (pte|pmd|pgd|pgprot)val_t type definitions as u32

I'm unconvinced that this is the right solution to add all these val_t
types, especially as u32.  

Well, they are u32 in hardware. We can leave them as unsigned long but
it shouldn't make any difference. We introduce the LPAE once as u64, so
it makes sense to have some consistent definition.

And there are places where we want some common type names rather than
using ifdef's.

While I realize the whole point of using the typedefs, I don't think
the conversion is entirely correct - there were certainly some things
that raised an eyebrow here, but at the moment I can't put my finger
on them immediately without going back over the patch.

quoted

It also creates some nonsense like assigning
pmdval_t values to pgprotval_t, and misses converting some other stuff
too.

An inconsistency I noticed is early_pte_alloc(). Are there others?

I couldn't say without looking at the patch again.

quoted

quoted

quoted

  ARM: LPAE: Use a mask for physical addresses in page table entries

[...]

quoted

Will Deacon (4):
  ARM: LPAE: add ISBs around MMU enabling code

I still don't like this one - and I do think there's a solution without
having to resort to isb there.

And without exception return (which had it's own problems with the
execution state bits).

quoted

This also affects the cpu resume code too.

I fixed the sleep.S file but there wasn't a decision on this patch yet.

I didn't notice it as part of this patch set.

quoted

diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index 6e7f67f..1892c3d 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c

@@ -862,6 +862,29 @@ static void __init sanity_check_meminfo(void)
 	meminfo.nr_banks = j;
 }
 
+static inline void pmd_clear_range(pgd_t *pgd, unsigned long addr, unsigned long end)
+{
+	pmd_t *pmd;
+
+	pmd = pmd_offset(pgd, addr);
+	do {
+		addr = pmd_addr_end(addr, end);
+		pmd_clear(pmd);
+	} while (pmd++, addr != end);
+}
+
+static void pgd_clear_range(unsigned long addr, unsigned long end)
+{
+	unsigned long next;
+	pgd_t *pgd;
+
+	pgd = pgd_offset_k(addr);
+	do {
+		next = pgd_addr_end(addr, end);
+		pmd_clear_range(pgd, addr, next);
+	} while (pgd++, addr = next, addr != end);
+}

You need to go through the pud here as pmd_offset takes a pud. We can
assume that there is one pud folded into the pgd and call
pmd_clear_range with a pud directly (similar to the fixup I posted some
time ago to your patch converting to nopud).

Of course, this patch was written pre-PUD and used to be patched by the
PUD stuff.  As I was uncertain about how L1/L2 works on LPAE, it got
dropped out... Hence I've only the pre-PUD version now.