[Patch 0/2] Kexec/Kdump support POWER6

Here are a set of patches which adds kdump support for Power6.

The first patch reads VRMA page size from device tree.

Second patch adds Kexec/Kdump support for Power6 processor. 
On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte 
hash tables. This patch makes sure these hpte entries are 
left untouched.

Comments are welcome.

Thanks
-Sachin

Signed-off-by : Sachin Sant [off-list ref]
Signed-off-by : Mohan Kumar M [off-list ref]
---

Read supported VRMA page size from device tree.

Thanks
-Sachin

On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte hash
tables. Make sure these hpte entries are left untouched.

Thanks
-Sachin

Hi,

On Tue, May 22, 2007 at 05:56:36PM +0530, Sachin P. Sant wrote:

On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte hash
tables. Make sure these hpte entries are left untouched.

This will break kexec builds on non-pseries. It's referring to platform
code that might not be built.

Does this function find the vrma, or save it away? Seems like the name
is misleading.

+{
+	unsigned int step;
+	unsigned long hash, slot, vaddr;
+	unsigned long dword0, dummy1, rma_size;
+	long lpar_rc;
+	int i;
+	
+	/* Get the RMA size */
+	rma_size = lmb.rmo_size;
+	
+	/* Get the VRMA page size */	
+	step = 1 << ppc64_vrma_page_size;

Is ppc64_vrma_page_size really the size, or the shift? Above would
indicate that it's really a shift value.

+
+	vaddr = HPTE_V_RMA_VPN + rma_size;
+
+	/* Find hpte's with VRMA mappings */
+	for (; vaddr >= HPTE_V_RMA_VPN; vaddr -= step) {
+		hash = hpt_hash(vaddr, mmu_psize_defs[MMU_PAGE_16M].shift);

Why is 16M hardcoded here, when you're taking such great care to read
out the pagesize earlier?

+		slot = ((hash & htab_hash_mask) * HPTES_PER_GROUP);	
+
+		for (i = 0; i < HPTES_PER_GROUP; i++) {
+			lpar_rc = plpar_pte_read(0, slot, 
+						&dword0, &dummy1);
+			if (!lpar_rc && dword0 &&
+			((dword0 & HPTE_V_MASK) == MAGIC_SKIP_HPTE)) {

Indentation

+				/* store the hpte */
+				hpte_vrma_slots[num_hpte_vrma_slots++] = slot;

Here you rely on global exported state (num_hpte_vrma_slots), increasing it without
checking for limits. What happens if this function is ever called twice? Should you
set it to 0 in the beginning of the function and check it against the size of the
hpte_vrma_slots array instead?

Same comment as above: This isn't a kexec function as much as a pseries function, so
it should be defined in some other header instead.

"MAGIC_SKIP_HPTE"? I'm sure there's a proper name for this field in the
PAPR, isn't there? Also, HPTE_V_RMA_NUM isn't a HPTE_V field, it shouldn't
have that prefix. It's not a property of the mmu in the first place.

These should maybe be local defines in the pseries lpar code instead, since it's
more of a lpar<->phyp interface than mmu programming interface.


-Olof

Hi Olaf, thanks for the review. 

quoted

+	if (have_vrma)
+		pSeries_find_hpte_vrma();
+
    

This will break kexec builds on non-pseries. It's referring to platform
code that might not be built.

  

Ok. Will call this function from lpar.c instead of
default_machine_kexec().

Does this function find the vrma, or save it away? Seems like the name
is misleading.

  

Well it finds a vrma entry and saves it. I thought of
pSeries_find_save_hpte_vrma(), but decided against it. I could
change it to pSeries_save_hpte_vrma().

Is ppc64_vrma_page_size really the size, or the shift? Above would
indicate that it's really a shift value.

  

It is a shift. I will change it to ppc64_vrma_page_shift.

Why is 16M hardcoded here, when you're taking such great care to read
out the pagesize earlier?

  

Hrmm. Ok will use the vrma_page_shift value.

quoted

+			((dword0 & HPTE_V_MASK) == MAGIC_SKIP_HPTE)) {
    

Indentation
  

Done.

quoted

+				/* store the hpte */
+				hpte_vrma_slots[num_hpte_vrma_slots++] = slot;
    

Here you rely on global exported state (num_hpte_vrma_slots), increasing it without
checking for limits. What happens if this function is ever called twice? Should you
set it to 0 in the beginning of the function and check it against the size of the
hpte_vrma_slots array instead?
  

Will add proper checks for num_hpte_vrma_slots variable value.

quoted

+extern void pSeries_find_hpte_vrma(void);

    

Same comment as above: This isn't a kexec function as much as a pseries function, so
it should be defined in some other header instead.
  

quoted

 
+#define HPTE_V_RMA_VPN         ASM_CONST(0x001FFFFFF0000000)
+#define HPTE_V_MASK            ASM_CONST(0xc000000000000000)
+#define MAGIC_SKIP_HPTE        ASM_CONST(0x4000000000000000)
+#define HPTE_V_RMA_NUM         16
    

"MAGIC_SKIP_HPTE"? I'm sure there's a proper name for this field in the
PAPR, isn't there? Also, HPTE_V_RMA_NUM isn't a HPTE_V field, it shouldn't
have that prefix. It's not a property of the mmu in the first place.

These should maybe be local defines in the pseries lpar code instead, since it's
more of a lpar<->phyp interface than mmu programming interface.
  

Will move them to pseries lpar code.
Updated patch on its way.

Thanks
-Sachin

Hi Olof, thanks for the review. 

quoted

+	if (have_vrma)
+		pSeries_find_hpte_vrma();
+
    

This will break kexec builds on non-pseries. It's referring to platform
code that might not be built.

  

Ok. Will call this function from lpar.c instead of
default_machine_kexec().

Does this function find the vrma, or save it away? Seems like the name
is misleading.

  

Well it finds a vrma entry and saves it. I thought of
pSeries_find_save_hpte_vrma(), but decided against it. I could
change it to pSeries_save_hpte_vrma().

Is ppc64_vrma_page_size really the size, or the shift? Above would
indicate that it's really a shift value.

  

It is a shift. I will change it to ppc64_vrma_page_shift.

Why is 16M hardcoded here, when you're taking such great care to read
out the pagesize earlier?

  

Hrmm. Ok will use the vrma_page_shift value.

quoted

+			((dword0 & HPTE_V_MASK) == MAGIC_SKIP_HPTE)) {
    

Indentation
  

Done.

quoted

+				/* store the hpte */
+				hpte_vrma_slots[num_hpte_vrma_slots++] = slot;
    

Here you rely on global exported state (num_hpte_vrma_slots), increasing it without
checking for limits. What happens if this function is ever called twice? Should you
set it to 0 in the beginning of the function and check it against the size of the
hpte_vrma_slots array instead?
  

Will add proper checks for num_hpte_vrma_slots variable value.

quoted

+extern void pSeries_find_hpte_vrma(void);

    

Same comment as above: This isn't a kexec function as much as a pseries function, so
it should be defined in some other header instead.
  

quoted

 
+#define HPTE_V_RMA_VPN         ASM_CONST(0x001FFFFFF0000000)
+#define HPTE_V_MASK            ASM_CONST(0xc000000000000000)
+#define MAGIC_SKIP_HPTE        ASM_CONST(0x4000000000000000)
+#define HPTE_V_RMA_NUM         16
    

"MAGIC_SKIP_HPTE"? I'm sure there's a proper name for this field in the
PAPR, isn't there? Also, HPTE_V_RMA_NUM isn't a HPTE_V field, it shouldn't
have that prefix. It's not a property of the mmu in the first place.

These should maybe be local defines in the pseries lpar code instead, since it's
more of a lpar<->phyp interface than mmu programming interface.
  

Will move them to pseries lpar code.
Updated patch on its way.

Thanks
-Sachin

On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte hash
tables. Make sure these hpte entries are left untouched.

This patch also adds plpar_pte_read_raw() on the lines of 
plpar_pte_remove_raw().

Thanks
-Sachin

Sachin P. Sant writes:

On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte hash
tables. Make sure these hpte entries are left untouched.

Surely all we need to do is to avoid clearing the VRMA entries.  We
can do this by not clearing any HPTE where the top 40 bits of the
first dword are 0x4001ffffff (B=1 for a 1TB segment and the
0x0001ffffff special VSID).  In fact we can avoid having to read each
entry by doing the H_REMOVEs with H_ANDCOND and the bolted bit when we
clear the hash table, and only reading the HPTEs for which the
H_REMOVE returns an error.

Paul.

On Wed, May 23, 2007 at 08:55:00PM +1000, Paul Mackerras wrote:

Sachin P. Sant writes:

quoted

On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte hash
tables. Make sure these hpte entries are left untouched.

Surely all we need to do is to avoid clearing the VRMA entries.  We
can do this by not clearing any HPTE where the top 40 bits of the
first dword are 0x4001ffffff (B=1 for a 1TB segment and the
0x0001ffffff special VSID).  In fact we can avoid having to read each
entry by doing the H_REMOVEs with H_ANDCOND and the bolted bit when we
clear the hash table, and only reading the HPTEs for which the
H_REMOVE returns an error.

Paul,

Thanks for your suggestion.

But we can not use 0x4001fffff for H_ANDCOND flag since AND'ing
0x4001ffff value with most of the HPTEs results in non-zero and most of
the HPTE entries are not removed.

From POWER ISA 2.04 document page 425,

"Programming Note:
Software should specify PTE(B) = 0b01 for all Page
Table Entries that map the VRMA in order to be
consistent with the values in Figure 14."

So I tried 0x4000000000000000 as AVPN parameter and using that it
removes all hpte entries other than VRMA(ie non 1TB segment size PTE
entries).

Tested on POWER6/POWER5 machines.

========================
Starting from POWER5+, hypervisor stores VRMA entries in the HPTE tables
and these entries are need by OS and they should not be cleared. These
VRMA entries will be of segment size 1TB. Using H_ANDCOND flag for
H_REMOVE hypervisor call it is made sure that we will not remove any PTE
whose size is 1TB(ie VRMA entries)

Signed-off-by: Sachin Sant <redacted>
Signed-off-by: Mohan Kumar M <redacted>
---
 arch/powerpc/platforms/pseries/lpar.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6.21.1/arch/powerpc/platforms/pseries/lpar.c
===================================================================

--- linux-2.6.21.1.orig/arch/powerpc/platforms/pseries/lpar.c
+++ linux-2.6.21.1/arch/powerpc/platforms/pseries/lpar.c

@@ -369,6 +369,8 @@ static long pSeries_lpar_hpte_remove(uns
 	return -1;
 }
 
+#define VRMA_HPTE_B_1TB ASM_CONST(0x4000000000000000)
+
 static void pSeries_lpar_hptab_clear(void)
 {
 	unsigned long size_bytes = 1UL << ppc64_pft_size;

@@ -378,7 +380,9 @@ static void pSeries_lpar_hptab_clear(voi
 
 	/* TODO: Use bulk call */
 	for (i = 0; i < hpte_count; i++)
-		plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
+		/* dont remove HPTEs of segments size 1TB (VRMA entries) */
+		plpar_pte_remove_raw(H_ANDCOND, i, VRMA_HPTE_B_1TB,
+						&dummy1, &dummy2);
 }
 
 /*

On Thu, May 24, 2007 at 05:47:51PM +0530, Mohan Kumar M wrote:

On Wed, May 23, 2007 at 08:55:00PM +1000, Paul Mackerras wrote:

quoted

Sachin P. Sant writes:

quoted

On Power machines supporting VRMA, Kexec/Kdump does not work.
Hypervisor stores VRMA mapping used by the OS, in the hpte hash
tables. Make sure these hpte entries are left untouched.

Surely all we need to do is to avoid clearing the VRMA entries.  We
can do this by not clearing any HPTE where the top 40 bits of the
first dword are 0x4001ffffff (B=1 for a 1TB segment and the
0x0001ffffff special VSID).  In fact we can avoid having to read each
entry by doing the H_REMOVEs with H_ANDCOND and the bolted bit when we
clear the hash table, and only reading the HPTEs for which the
H_REMOVE returns an error.

So I tried 0x4000000000000000 as AVPN parameter and using that it
removes all hpte entries other than VRMA(ie non 1TB segment size PTE
entries).

Tested on POWER6/POWER5 machines.

Hi,

As Paul says above, you need to check for failures and compare the VSID
and possibly unhash it anyway in case of non-match. Otherwise if the
kernel ever starts using 1TB segments for regular use, those pages will
never be unhashed. I don't see your code doing that now.


-Olof

Olof Johansson wrote:

As Paul says above, you need to check for failures and compare the VSID
and possibly unhash it anyway in case of non-match. Otherwise if the
kernel ever starts using 1TB segments for regular use, those pages will
never be unhashed. I don't see your code doing that now.

  

How about the following patch ?

Thanks
-Sachin

Signed-off-by : Sachin Sant [off-list ref]
Signed-off-by : Mohan Kumar M [off-list ref]
---

 
 	/* TODO: Use bulk call */
-	for (i = 0; i < hpte_count; i++)
-		plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
+	for (i = 0; i < hpte_count; i++) {
+		/* dont remove HPTEs with VRMA mappings */
+		lpar_rc = plpar_pte_remove_raw(H_ANDCOND, i, VRMA_HPTE_B_1TB,
+						&dummy1, &dummy2);

If you're going to use the B (1T segment) bit instead of the bolted bit,
at least define a proper constant in line with the existing naming of
the hash table constants in mmu-hash64.h. I would suggest doing the same
with the VRMA_MASK/VALUE thing and calling it HPTE_V_VRMA_MASK or
something similar.

+		if (lpar_rc == H_NOT_FOUND) {
+			lpar_rc = plpar_pte_read_raw(0, i, &dword0, &dummy1);
+			if (!lpar_rc && 
+				(((dword0 >> 24) & VRMA_MASK) != VRMA_MASK))
+				/* Can be hpte for 1TB Seg. So remove it */
+				plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
+		}
+	}
 }

In addition, I would recommend following Michael's advice and using
using the bulk remove Hcall whenever possible.

Cheers,
Ben.

Benjamin Herrenschmidt wrote:

If you're going to use the B (1T segment) bit instead of the bolted bit,
at least define a proper constant in line with the existing naming of
the hash table constants in mmu-hash64.h. I would suggest doing the same
with the VRMA_MASK/VALUE thing and calling it HPTE_V_VRMA_MASK or
something similar.
  

Well i had used them properly in my previous patches. Don't know why
i changed it in this patch :-( 

Here is the updated patch. 

quoted

+	}
 }
    

In addition, I would recommend following Michael's advice and using
using the bulk remove Hcall whenever possible.
  

Yes will send out a separate patch to use bulk remove Hcall.

Thanks
-Sachin


Signed-off-by : Sachin Sant [off-list ref]
Signed-off-by : Mohan Kumar M [off-list ref]
---

On Mon, 2007-05-28 at 17:10 +0530, Sachin P. Sant wrote:

+#define HPTE_V_1TB_SEG         ASM_CONST(0x4000000000000000)
+#define HPTE_V_VRMA_MASK       ASM_CONST(0x4001ffffff)

Move those to mmu-hash64.h along with the other ones. Also, keep the
mask aligned to the top bits

 static void pSeries_lpar_hptab_clear(void)
 {
        unsigned long size_bytes = 1UL << ppc64_pft_size;
        unsigned long hpte_count = size_bytes >> 4;
-       unsigned long dummy1, dummy2;
+       unsigned long dummy1, dummy2, dword0;
+       long lpar_rc;
        int i;
 
        /* TODO: Use bulk call */
-       for (i = 0; i < hpte_count; i++)
-               plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
+       for (i = 0; i < hpte_count; i++) {
+               /* dont remove HPTEs with VRMA mappings */
+               lpar_rc = plpar_pte_remove_raw(H_ANDCOND, i, HPTE_V_1TB_SEG,
+                                               &dummy1, &dummy2);
+               if (lpar_rc == H_NOT_FOUND) {
+                       lpar_rc = plpar_pte_read_raw(0, i, &dword0, &dummy1);
+                       if (!lpar_rc && (((dword0 >> 24) & HPTE_V_VRMA_MASK) 
+                               != HPTE_V_VRMA_MASK))

No need to shift >> 24, just have the mask left justified in the first
place. No need to have a "magic" shift value in there.

+                               /* Can be hpte for 1TB Seg. So remove it */
+                               plpar_pte_remove_raw(0, i, 0, &dummy1, &dummy2);
+               }
+       }
 }

Appart from that, looks good. Does it actually work ? :-)

Cheers,
Ben.

Benjamin Herrenschmidt wrote:

Move those to mmu-hash64.h along with the other ones. Also, keep the
mask aligned to the top bits
  

In previous patches i had these #defines in mmu-hash64.h. But got
review comments to move them to lpar.c :-)

Oh well i will move them back to mmu-hash64.h.

quoted

+                       if (!lpar_rc && (((dword0 >> 24) & HPTE_V_VRMA_MASK) 
    

No need to shift >> 24, just have the mask left justified in the first
place. No need to have a "magic" shift value in there.
  

Ok.

Appart from that, looks good. Does it actually work ? :-)
  

Yes it does work. It must be my lucky day when i coded the patch :-)

Thanks
-Sachin

Signed-off-by : Sachin Sant [off-list ref]
Signed-off-by : Mohan Kumar M [off-list ref]
---

On Tue, 2007-05-29 at 11:48 +0530, Sachin P. Sant wrote:

In previous patches i had these #defines in mmu-hash64.h. But got
review comments to move them to lpar.c :-)

Hrm... weird. Oh well, _I_ personally prefer them in mmu-hash64.h and
since I'm writing most of the MMU code lately I'd say my opinion makes
rule :-)

Oh well i will move them back to mmu-hash64.h.

Thanks.

Cheers,
Ben.

On Mon, 2007-05-28 at 17:10 +0530, Sachin P. Sant wrote:

You have trailing whitespace on that line.

Add this to your ~/.vimrc, you do use vim right :)

highlight RedundantWhitespace ctermbg=red guibg=red

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

On Tue, 29 May 2007 20:14:44 +1000 Michael Ellerman [off-list ref] wrote:

Add this to your ~/.vimrc, you do use vim right :)

highlight RedundantWhitespace ctermbg=red guibg=red

You need this as well:

match RedundantWhitespace /\s\+$\| \+\ze\t/

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/