diff mbox

[Xen-devel,for-4.5,2/4] xen: arm: correct off by one in xgene-storm's map_one_mmio

Message ID 1416329088-23328-2-git-send-email-ian.campbell@citrix.com
State New
Headers show

Commit Message

Ian Campbell Nov. 18, 2014, 4:44 p.m. UTC
The callers pass the end as the pfn immediately *after* the last page to be
mapped, therefore adding one is incorrect and causes an additional page to be
mapped.

At the same time correct the printing of the mfn values, zero-padding them to
16 digits as for a paddr when they are frame numbers is just confusing.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/platforms/xgene-storm.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Julien Grall Nov. 18, 2014, 5:01 p.m. UTC | #1
Hi Ian,

On 11/18/2014 04:44 PM, Ian Campbell wrote:
> The callers pass the end as the pfn immediately *after* the last page to be
> mapped, therefore adding one is incorrect and causes an additional page to be
> mapped.
> 
> At the same time correct the printing of the mfn values, zero-padding them to
> 16 digits as for a paddr when they are frame numbers is just confusing.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/platforms/xgene-storm.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 29c4752..38674cd 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
>  {
>      int ret;
>  
> -    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
> +    printk("Additional MMIO %lx-%lx (%s)\n",
>             start, end, what);
> -    ret = map_mmio_regions(d, start, end - start + 1, start);
> +    ret = map_mmio_regions(d, start, end - start, start);
>      if ( ret )
>          printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
>                 what, start, d->domain_id);

As you fixed the previous printf format. I would fix this one too.

Regards,
Ian Campbell Nov. 19, 2014, 9:54 a.m. UTC | #2
On Tue, 2014-11-18 at 17:01 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 11/18/2014 04:44 PM, Ian Campbell wrote:
> > The callers pass the end as the pfn immediately *after* the last page to be
> > mapped, therefore adding one is incorrect and causes an additional page to be
> > mapped.
> > 
> > At the same time correct the printing of the mfn values, zero-padding them to
> > 16 digits as for a paddr when they are frame numbers is just confusing.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/platforms/xgene-storm.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> > index 29c4752..38674cd 100644
> > --- a/xen/arch/arm/platforms/xgene-storm.c
> > +++ b/xen/arch/arm/platforms/xgene-storm.c
> > @@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
> >  {
> >      int ret;
> >  
> > -    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
> > +    printk("Additional MMIO %lx-%lx (%s)\n",
> >             start, end, what);
> > -    ret = map_mmio_regions(d, start, end - start + 1, start);
> > +    ret = map_mmio_regions(d, start, end - start, start);
> >      if ( ret )
> >          printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
> >                 what, start, d->domain_id);
> 
> As you fixed the previous printf format. I would fix this one too.

Yes, good idea.

Ian.
Konrad Rzeszutek Wilk Nov. 19, 2014, 9:11 p.m. UTC | #3
On Tue, Nov 18, 2014 at 04:44:46PM +0000, Ian Campbell wrote:
> The callers pass the end as the pfn immediately *after* the last page to be
> mapped, therefore adding one is incorrect and causes an additional page to be
> mapped.
> 
> At the same time correct the printing of the mfn values, zero-padding them to
> 16 digits as for a paddr when they are frame numbers is just confusing.

HA! I was just looking at that today and thought it was odd.

Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/platforms/xgene-storm.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 29c4752..38674cd 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -45,9 +45,9 @@ static int map_one_mmio(struct domain *d, const char *what,
>  {
>      int ret;
>  
> -    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
> +    printk("Additional MMIO %lx-%lx (%s)\n",
>             start, end, what);
> -    ret = map_mmio_regions(d, start, end - start + 1, start);
> +    ret = map_mmio_regions(d, start, end - start, start);
>      if ( ret )
>          printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
>                 what, start, d->domain_id);
> -- 
> 1.7.10.4
>
diff mbox

Patch

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 29c4752..38674cd 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -45,9 +45,9 @@  static int map_one_mmio(struct domain *d, const char *what,
 {
     int ret;
 
-    printk("Additional MMIO %"PRIpaddr"-%"PRIpaddr" (%s)\n",
+    printk("Additional MMIO %lx-%lx (%s)\n",
            start, end, what);
-    ret = map_mmio_regions(d, start, end - start + 1, start);
+    ret = map_mmio_regions(d, start, end - start, start);
     if ( ret )
         printk("Failed to map %s @ %"PRIpaddr" to dom%d\n",
                what, start, d->domain_id);