[patch] MCA recovery verify pfn_valid

[patch] MCA recovery verify pfn_valid

[Russ Anderson]

Verify the pfn is valid before calling pfn_to_page().

Signed-off-by: Russ Anderson (rja@sgi.com)

---------------------------------------------------------------------
 arch/ia64/kernel/mca_drv.c |    3 +++
 1 files changed, 3 insertions(+)

Index: linux-2.6.git#test/arch/ia64/kernel/mca_drv.c
=================================--- linux-2.6.git#test.orig/arch/ia64/kernel/mca_drv.c	2005-09-16 16:28:11.857773257 -0500
+++ linux-2.6.git#test/arch/ia64/kernel/mca_drv.c	2005-09-16 16:29:32.119181669 -0500
@@ -87,6 +87,9 @@ mca_page_isolate(unsigned long paddr)
 	if ( !ia64_phys_addr_valid(paddr) ) 
 		return ISOLATE_NG;
 
+	if ( !pfn_valid(paddr))
+		return ISOLATE_OK;
+
 	/* convert physical address to physical page number */
 	p = pfn_to_page(paddr>>PAGE_SHIFT);
 

Re: [patch] MCA recovery verify pfn_valid

[Russ Anderson]

Jack Steiner wrote:
> > On Fri, Sep 16, 2005 at 04:54:53PM -0500, Russ Anderson wrote:
> > 
> > Verify the pfn is valid before calling pfn_to_page().
> > 
> > Signed-off-by: Russ Anderson (rja@sgi.com)
> > 
> > ---------------------------------------------------------------------
> >  arch/ia64/kernel/mca_drv.c |    3 +++
> >  1 files changed, 3 insertions(+)
> > 
> > Index: linux-2.6.git#test/arch/ia64/kernel/mca_drv.c
> > =================================> > --- linux-2.6.git#test.orig/arch/ia64/kernel/mca_drv.c	2005-09-16 16:28:11.857773257 -0500
> > +++ linux-2.6.git#test/arch/ia64/kernel/mca_drv.c	2005-09-16 16:29:32.119181669 -0500
> > @@ -87,6 +87,9 @@ mca_page_isolate(unsigned long paddr)
> >  	if ( !ia64_phys_addr_valid(paddr) ) 
> >  		return ISOLATE_NG;
> >  
> > +	if ( !pfn_valid(paddr))
> > +		return ISOLATE_OK;
> 
> Should this be ISOLATE_OK or ISOLATE_NG? 
> 
> If the page is from another partition, the page has not been isolated? I realize
> that the return status is not used for anything but printing a message, but it
> seems misleading to print that isolation was successful when nothing was actually done.
> 
> Perhaps another status should be returned.

In this case, is any message really needed?  As you say, success is
is misleading because nothing was done.  Failure is not accurate
because it didn't fail.  So perhaps a third status that does not
print any page isolation message is the best solution.

For reference, the code:
----------------------------------------------------------
void
mca_handler_bh(unsigned long paddr)
{
        printk(KERN_DEBUG "OS_MCA: process [pid: %d](%s) encounters MCA.\n",
                current->pid, current->comm);

        spin_lock(&mca_bh_lock);
        if (mca_page_isolate(paddr) = ISOLATE_OK) {
                printk(KERN_DEBUG "Page isolation: ( %lx ) success.\n", paddr);
        } else {
                printk(KERN_DEBUG "Page isolation: ( %lx ) failure.\n", paddr);
        }
        spin_unlock(&mca_bh_lock);

        /* This process is about to be killed itself */
        do_exit(SIGKILL);
}


-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com

Re: [patch] MCA recovery verify pfn_valid

[Hidetoshi Seto]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

How about this?

Thanks,
H.Seto

Verify the pfn is valid before calling pfn_to_page(),
and cut isolation message if nothing was done.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

Russ Anderson wrote:
> Jack Steiner wrote:
>>
>>Perhaps another status should be returned.
> 
> In this case, is any message really needed?  As you say, success is
> is misleading because nothing was done.  Failure is not accurate
> because it didn't fail.  So perhaps a third status that does not
> print any page isolation message is the best solution.

[-- Attachment #2: mca_drv_pfnverify.patch --]
[-- Type: text/plain, Size: 1515 bytes --]

 arch/ia64/kernel/mca_drv.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

Index: build/arch/ia64/kernel/mca_drv.c
===================================================================
--- build.orig/arch/ia64/kernel/mca_drv.c
+++ build/arch/ia64/kernel/mca_drv.c
@@ -56,8 +56,9 @@ static struct page *page_isolate[MAX_PAG
 static int num_page_isolate = 0;
 
 typedef enum {
-	ISOLATE_NG = 0,
-	ISOLATE_OK = 1
+	ISOLATE_NG,
+	ISOLATE_OK,
+	ISOLATE_NONE
 } isolate_status_t;
 
 /*
@@ -74,7 +75,7 @@ static struct {
  * @paddr:	poisoned memory location
  *
  * Return value:
- *	ISOLATE_OK / ISOLATE_NG
+ *	one of isolate_status_t, ISOLATE_OK/NG/NONE.
  */
 
 static isolate_status_t
@@ -85,7 +86,10 @@ mca_page_isolate(unsigned long paddr)
 
 	/* whether physical address is valid or not */
 	if (!ia64_phys_addr_valid(paddr))
-		return ISOLATE_NG;
+		return ISOLATE_NONE;
+
+	if (!pfn_valid(paddr))
+		return ISOLATE_NONE;
 
 	/* convert physical address to physical page number */
 	p = pfn_to_page(paddr>>PAGE_SHIFT);
@@ -122,10 +126,15 @@ mca_handler_bh(unsigned long paddr)
 		current->pid, current->comm);
 
 	spin_lock(&mca_bh_lock);
-	if (mca_page_isolate(paddr) == ISOLATE_OK) {
+	switch (mca_page_isolate(paddr)) {
+	case ISOLATE_OK:
 		printk(KERN_DEBUG "Page isolation: ( %lx ) success.\n", paddr);
-	} else {
+		break;
+	case ISOLATE_NG:
 		printk(KERN_DEBUG "Page isolation: ( %lx ) failure.\n", paddr);
+		break;
+	default:
+		break;
 	}
 	spin_unlock(&mca_bh_lock);
 

Re: [patch] MCA recovery verify pfn_valid

[Russ Anderson]

Hidetoshi Seto wrote:
> 
> How about this?
> 
> Thanks,
> H.Seto
> 
> Verify the pfn is valid before calling pfn_to_page(),
> and cut isolation message if nothing was done.
> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

Looks good to me.

-- 
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          rja@sgi.com