[PATCH 0/3] xen: small fixes and cleanups at arch/x86/xen/setup.c

[PATCH 0/3] xen: small fixes and cleanups at arch/x86/xen/setup.c

[Igor Mammedov]

 Some fixes/cleanups as result of backporting xen ballooning to rhel6.
 Thanks to Laszlo Ersek for spotting bugs.

[PATCH 1/3] xen: off by one error in xen/setup.c

[Igor Mammedov]

Do not try to initialize pfn beyond of available address space.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch/x86/xen/setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 60aeeb5..2221b05 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -69,7 +69,7 @@ static void __init xen_add_extra_mem(unsigned long pages)
 
 	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
 
-	for (pfn = PFN_DOWN(extra_start); pfn <= xen_max_p2m_pfn; pfn++)
+	for (pfn = PFN_DOWN(extra_start); pfn < xen_max_p2m_pfn; ++pfn)
 		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 }
 
-- 
1.7.5

Re: [PATCH 1/3] xen: off by one error in xen/setup.c

[Konrad Rzeszutek Wilk]

On Tue, Aug 02, 2011 at 11:45:23AM +0200, Igor Mammedov wrote:
> Do not try to initialize pfn beyond of available address space.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  arch/x86/xen/setup.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index 60aeeb5..2221b05 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -69,7 +69,7 @@ static void __init xen_add_extra_mem(unsigned long pages)
>  
>  	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
>  
> -	for (pfn = PFN_DOWN(extra_start); pfn <= xen_max_p2m_pfn; pfn++)
> +	for (pfn = PFN_DOWN(extra_start); pfn < xen_max_p2m_pfn; ++pfn)
>  		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);

Did this actually break anything?

>  }
>  
> -- 
> 1.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/3] xen: off by one error in xen/setup.c

[Igor Mammedov]

On 08/02/2011 07:07 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 02, 2011 at 11:45:23AM +0200, Igor Mammedov wrote:
>> Do not try to initialize pfn beyond of available address space.
>>
>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>> ---
>>   arch/x86/xen/setup.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
>> index 60aeeb5..2221b05 100644
>> --- a/arch/x86/xen/setup.c
>> +++ b/arch/x86/xen/setup.c
>> @@ -69,7 +69,7 @@ static void __init xen_add_extra_mem(unsigned long pages)
>>
>>   	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
>>
>> -	for (pfn = PFN_DOWN(extra_start); pfn<= xen_max_p2m_pfn; pfn++)
>> +	for (pfn = PFN_DOWN(extra_start); pfn<  xen_max_p2m_pfn; ++pfn)
>>   		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
>
> Did this actually break anything?

Not really, but for the sake of correctness and as cleanup it's good idea.

>
>>   }
>>
>> --
>> 1.7.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Thanks,
  Igor

Re: [PATCH 1/3] xen: off by one error in xen/setup.c

[Jesper Juhl]

On Wed, 3 Aug 2011, Igor Mammedov wrote:

> On 08/02/2011 07:07 PM, Konrad Rzeszutek Wilk wrote:
> > On Tue, Aug 02, 2011 at 11:45:23AM +0200, Igor Mammedov wrote:
> > > Do not try to initialize pfn beyond of available address space.
> > > 
> > > Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> > > ---
> > >   arch/x86/xen/setup.c |    2 +-
> > >   1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> > > index 60aeeb5..2221b05 100644
> > > --- a/arch/x86/xen/setup.c
> > > +++ b/arch/x86/xen/setup.c
> > > @@ -69,7 +69,7 @@ static void __init xen_add_extra_mem(unsigned long
> > > pages)
> > > 
> > >   	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
> > > 
> > > -	for (pfn = PFN_DOWN(extra_start); pfn<= xen_max_p2m_pfn; pfn++)
> > > +	for (pfn = PFN_DOWN(extra_start); pfn<  xen_max_p2m_pfn; ++pfn)
> > >   		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
> > 
> > Did this actually break anything?
> 
> Not really, but for the sake of correctness and as cleanup it's good idea.
> 

Ok I'm really, really nitpicking here, but if it's supposed to "clean up", 
wouldn't this:

  for (pfn = PFN_DOWN(extra_start); pfn < xen_max_p2m_pfn; ++pfn)

be preferable (note the spacing around '<') ?

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH 1/3] xen: off by one error in xen/setup.c

[Igor Mammedov]

On 08/03/2011 08:23 AM, Jesper Juhl wrote:
> On Wed, 3 Aug 2011, Igor Mammedov wrote:
>
>> On 08/02/2011 07:07 PM, Konrad Rzeszutek Wilk wrote:
>>> On Tue, Aug 02, 2011 at 11:45:23AM +0200, Igor Mammedov wrote:
>>>> Do not try to initialize pfn beyond of available address space.
>>>>
>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>> ---
>>>>    arch/x86/xen/setup.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
>>>> index 60aeeb5..2221b05 100644
>>>> --- a/arch/x86/xen/setup.c
>>>> +++ b/arch/x86/xen/setup.c
>>>> @@ -69,7 +69,7 @@ static void __init xen_add_extra_mem(unsigned long
>>>> pages)
>>>>
>>>>    	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
>>>>
>>>> -	for (pfn = PFN_DOWN(extra_start); pfn<= xen_max_p2m_pfn; pfn++)
>>>> +	for (pfn = PFN_DOWN(extra_start); pfn<   xen_max_p2m_pfn; ++pfn)
>>>>    		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
>>>
>>> Did this actually break anything?
>>
>> Not really, but for the sake of correctness and as cleanup it's good idea.
>>
>
> Ok I'm really, really nitpicking here, but if it's supposed to "clean up",
> wouldn't this:
>
>    for (pfn = PFN_DOWN(extra_start); pfn<  xen_max_p2m_pfn; ++pfn)
>
> be preferable (note the spacing around '<') ?

It is correct. Checkout https://lkml.org/lkml/2011/8/2/82

-- 
Thanks,
  Igor

Re: [PATCH 1/3] xen: off by one error in xen/setup.c

[Konrad Rzeszutek Wilk]

On Tue, Aug 02, 2011 at 11:45:23AM +0200, Igor Mammedov wrote:
> Do not try to initialize pfn beyond of available address space.

Actually, thinking about this I don't think this is right.

We want to initialize the P2M space _at_ the xen_max_p2m_pfn. With
your patch we would only initialize it up to xen_max_p2m_pfn-1 - so
the last PFN would not be set to INVALID_P2M_ENTRY.

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  arch/x86/xen/setup.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index 60aeeb5..2221b05 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -69,7 +69,7 @@ static void __init xen_add_extra_mem(unsigned long pages)
>  
>  	xen_max_p2m_pfn = PFN_DOWN(extra_start + size);
>  
> -	for (pfn = PFN_DOWN(extra_start); pfn <= xen_max_p2m_pfn; pfn++)
> +	for (pfn = PFN_DOWN(extra_start); pfn < xen_max_p2m_pfn; ++pfn)
>  		__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
>  }
>  
> -- 
> 1.7.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 2/3] xen: Fix printk() format in xen/setup.c

[Igor Mammedov]

Use correct format specifier for unsigned long.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch/x86/xen/setup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index 2221b05..d9eef1e 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -113,7 +113,7 @@ static unsigned long __init xen_release_chunk(phys_addr_t start_addr,
 			len++;
 		}
 	}
-	printk(KERN_CONT "%ld pages freed\n", len);
+	printk(KERN_CONT "%lu pages freed\n", len);
 
 	return len;
 }
@@ -139,7 +139,7 @@ static unsigned long __init xen_return_unused_memory(unsigned long max_pfn,
 	if (last_end < max_addr)
 		released += xen_release_chunk(last_end, max_addr);
 
-	printk(KERN_INFO "released %ld pages of unused memory\n", released);
+	printk(KERN_INFO "released %lu pages of unused memory\n", released);
 	return released;
 }
 
-- 
1.7.5

[PATCH 3/3] xen: Fix misleading WARN message at xen_release_chunk

[Igor Mammedov]

WARN message should not complain
 "Failed to release memory %lx-%lx err=%d\n"
                           ^^^^^^^
about range when it fails to release just one page,
instead it should say what pfn is not freed.

In addition line:
 printk(KERN_INFO "xen_release_chunk: looking at area pfn %lx-%lx: "
 ...
 printk(KERN_CONT "%lu pages freed\n", len);
will be broken if WARN in between this line is fired. So fix it
by using a single printk for this.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 arch/x86/xen/setup.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index d9eef1e..1ec79b7 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -92,8 +92,6 @@ static unsigned long __init xen_release_chunk(phys_addr_t start_addr,
 	if (end <= start)
 		return 0;
 
-	printk(KERN_INFO "xen_release_chunk: looking at area pfn %lx-%lx: ",
-	       start, end);
 	for(pfn = start; pfn < end; pfn++) {
 		unsigned long mfn = pfn_to_mfn(pfn);
 
@@ -106,14 +104,14 @@ static unsigned long __init xen_release_chunk(phys_addr_t start_addr,
 
 		ret = HYPERVISOR_memory_op(XENMEM_decrease_reservation,
 					   &reservation);
-		WARN(ret != 1, "Failed to release memory %lx-%lx err=%d\n",
-		     start, end, ret);
+		WARN(ret != 1, "Failed to release pfn %lx err=%d\n", pfn, ret);
 		if (ret == 1) {
 			__set_phys_to_machine(pfn, INVALID_P2M_ENTRY);
 			len++;
 		}
 	}
-	printk(KERN_CONT "%lu pages freed\n", len);
+	printk(KERN_INFO "Freeing  %lx-%lx pfn range: %lu pages freed\n",
+	       start, end, len);
 
 	return len;
 }
-- 
1.7.5