[Cbe-oss-dev] [PATCH] 64K support for Kexec

[Benjamin Herrenschmidt]

On Fri, 2007-04-06 at 23:58 -0300, Luke Browning wrote:
> This patch decodes the page size from the pte. This code is used by
> kexec, which walks the hash table and issues a tlbie for each 
> valid entry. The page size is encoded in the virtual address that
> is passed to the tlbie instruction.
> 
> Signed-off-by: Luke Browning <lukebrowning at us.ibm.com>

Patch got wrapped/damaged by your mailer... Also, I haven't looked too
closely but are you sure slot2va is ok for non-4K page sizes ?

It doesn't seem to me ...

	if (! (hpte_v & HPTE_V_LARGE)) {
		unsigned long vpi, pteg;

		pteg = slot / HPTES_PER_GROUP;
		if (hpte_v & HPTE_V_SECONDARY)
			pteg = ~pteg;

		vpi = ((va >> 28) ^ pteg) & htab_hash_mask;

		va |= vpi << PAGE_SHIFT;
	}

The if () will clearly make sure it does nothing for large pages :-)
While shifting avpn by 23 bits might be enough for a 16M page, I think
we need to do something for 64K pages or we'll be missing 8 bits of the
va...
  
> +#define LP_SHIFT	12	
> +#define LP_BITS		8	
> +#define LP_MASK(i)	((((1 << LP_BITS) - 1) >> (i)) << LP_SHIFT)
> +
> +static int hpte_decode_lpsize(unsigned long pa)
> +{
> +	int penc, i;
> +
> +	if (pa & LP_MASK(0)) {
> +		for (i = 0; i < LP_BITS; i++) {
> +			if ((pa & LP_MASK(i+1)) == LP_MASK(i+1))
> +				break;
> +		}
> +		penc = LP_MASK(i+1) >> LP_SHIFT;
> +		for (i = MMU_PAGE_COUNT - 1; i > 0; i--) {
> +			if (!mmu_psize_defs[i].shift)
> +				continue;
> +			if (penc == mmu_psize_defs[i].penc)
> +				break;
> +		}
> +	}

Can't the above be made a tad simpler ? Also, your loop, if I'm correct,
starts with 0xff and goes down to 0, I feel that's the wrong way around.

Why not just something like (off the top of my head, might not be right)

	for (i = MMU_PAGE_64K; i < MMU_PAGE_COUNT - 1; i++) {
		unsigned long relevant;

		if (!mmu_psize_defs[i].shift)
			continue;
		relevant = (1 << mmu_psize_defs[i].shift) = 1;
		if (((pa >> 12) & relevant) == mmu_psize_defs[i].penc)
			return i;
	}
	BUG(); /* Wassup ? */

Or even better, I suppose, have a static variable remembering the last
"hit" value and start the loop at that value though I suppose that
shouldn't be really necessary as we start on 64K which is likely to be
the most common and there aren't that many 16M pages to be expected in
the hash table.

Cheers,
Ben.