public inbox for linuxppc-dev@ozlabs.org 
 help / color / mirror / Atom feed
From: olof@lixom•net (Olof Johansson)
To: Luke Browning <lukebr@linux•vnet.ibm.com>
Cc: Paul Mackerras <paulus@samba•org>,
	cbe-oss-dev@ozlabs•org, Arnd Bergmann <arnd@arndb•de>,
	linuxppc-dev@ozlabs•org
Subject: Re: [PATCH v2] powerpc: 64K page support for kexec
Date: Thu, 26 Apr 2007 02:15:32 -0500	[thread overview]
Message-ID: <20070426071532.GA24937@lixom.net> (raw)
In-Reply-To: <1177529739.24866.34.camel@luke-laptop>

Hi,
                                                                                                                                             
Two comments below, besides that it looks good!
                                                                                                                                             
On Wed, Apr 25, 2007 at 04:35:39PM -0300, Luke Browning wrote:

> +#define LP_SHIFT	12
> +#define LP_BITS		8
> +#define LP_MASK(i)	((0xFF >> (i)) << LP_SHIFT)
> +
> +static void hpte_decode(hpte_t *hpte, unsigned long slot, 
> +			int *psize, unsigned long *va)
> +{
> +	unsigned long hpte_r = hpte->r;
> +	unsigned long hpte_v = hpte->v;
> +	unsigned long avpn;
> +	int i, size, shift, penc, avpnm_bits;
> +		
> +	if (!(hpte_v & HPTE_V_LARGE))
> +		size = MMU_PAGE_4K;
> +	else {
> +		for (i = 0; i < LP_BITS; i++) {
> +			if ((hpte_r & LP_MASK(i+1)) == LP_MASK(i+1))
> +				break;
> +		}
> +		penc = LP_MASK(i+1) >> LP_SHIFT;
> +		for (size = 0; size < MMU_PAGE_COUNT - 1; size++) {

This will never consider the last page size. Either <= MMU_PAGE_COUNT-1
or < MMU_PAGE_COUNT. The latter would be more natural.

> -		pteg = slot / HPTES_PER_GROUP;
> -		if (hpte_v & HPTE_V_SECONDARY)
> -			pteg = ~pteg;
> +			/* 4K pages are not represented by LP */
> +			if (size == MMU_PAGE_4K)
> +				continue;
> +
> +			/* valid entries have a shift value */
> +			if (!mmu_psize_defs[size].shift)
> +				continue;
>  
> -		vpi = ((va >> 28) ^ pteg) & htab_hash_mask;
> +			if (penc == mmu_psize_defs[size].penc)
> +				break;
> +		}
> +	}
>  
> -		va |= vpi << PAGE_SHIFT;
> +	/*
> +	 * FIXME, the code below works for 16M, 64K, and 4K pages as these
> +	 * fall under the p<=23 rules for calculating the virtual address.
> +	 * In the case of 16M pages, an extra bit is stolen from the AVPN
> +	 * field to achieve the requisite 24 bits. 
> +	 *
> +	 * 16G pages are not supported by the code below.
> +	 */
> +	BUG_ON(hpte_v & 0x4000000000000000UL);		/* 1T segment */
> +	BUG_ON(size == MMU_PAGE_16G);
> +	BUG_ON(size == MMU_PAGE_64K_AP);

Milton didn't like BUG_ON() here, and I think I agree after hearing his
motivations. I know I was the one suggesting them, but they're better
to keep out at the moment.

[...]



Thanks,

-Olof

  parent reply	other threads:[~2007-04-26  7:15 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-04-24 18:31 [PATCH] 64K page support for kexec Luke Browning
2007-04-24 19:43 ` Olof Johansson
2007-04-24 22:50   ` Benjamin Herrenschmidt
2007-04-24 23:07     ` Olof Johansson
2007-04-25  5:48       ` Milton Miller
2007-04-25 19:35     ` [PATCH v2] powerpc: " Luke Browning
2007-04-25 22:19       ` Benjamin Herrenschmidt
2007-04-26 15:28         ` Luke Browning
2007-04-27  4:36           ` [PATCH v3] " Milton Miller
2007-04-27 14:42             ` Luke Browning
2007-04-27 16:51               ` Milton Miller
2007-04-27 16:22             ` [PATCH v4] " Luke Browning
2007-04-27 16:59               ` Milton Miller
2007-04-27 17:30                 ` Luke Browning
2007-04-27 18:23                   ` Haren Myneni
2007-04-29  5:35                     ` Milton Miller
2007-04-29  8:30                   ` Paul Mackerras
2007-04-29  9:31                     ` Benjamin Herrenschmidt
2007-04-29 13:27                     ` Segher Boessenkool
2007-04-29 22:49                       ` Benjamin Herrenschmidt
2007-04-26  7:15       ` Olof Johansson [this message]
2007-04-24 22:48 ` [PATCH] " Benjamin Herrenschmidt
2007-04-25 13:06   ` Luke Browning
2007-04-25 22:11     ` Benjamin Herrenschmidt
  -- strict thread matches above, loose matches on Subject: below --
2007-04-26 22:23 [PATCH v3] powerpc: " Luke Browning
2007-04-26 22:32 ` Olof Johansson
2007-05-02 14:19   ` [PATCH v4] " Luke Browning
2007-05-03 13:45     ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070426071532.GA24937@lixom.net \
    --to=olof@lixom$(echo .)net \
    --cc=arnd@arndb$(echo .)de \
    --cc=cbe-oss-dev@ozlabs$(echo .)org \
    --cc=linuxppc-dev@ozlabs$(echo .)org \
    --cc=lukebr@linux$(echo .)vnet.ibm.com \
    --cc=paulus@samba$(echo .)org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox