Xen devel - Nov 2013 - [PATCH] xen/arm: ioremap_attr: return NULL is __vmap failed

Most of ioremap_* caller check if ioremap returns NULL. Actually, if the
physical address is non-aligned, Xen will return the pointer given by
__vmap plus the offset in the page. So if ioremap_* fails, the caller
will retrieve an non-NULL address and continue as if there was no error.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/mm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 26c6768..5137668 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -742,8 +742,13 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned int
attributes)
     unsigned long pfn = PFN_DOWN(pa);
     unsigned int offs = pa & (PAGE_SIZE - 1);
     unsigned int nr = PFN_UP(offs + len);
+    void *ptr;
 
-    return (__vmap(&pfn, nr, 1, 1, attributes) + offs);
+    ptr = __vmap(&pfn, nr, 1, 1, attributes);
+    if ( ptr == NULL )
+        return NULL;
+
+    return (ptr + offs);
 }
 
 void *ioremap(paddr_t pa, size_t len)
-- 
1.8.3.1

Actually, if an error occurs, Xen will silently ignore it and continue.
Convert platform_init to a void function and panic if we fail to
correctly initialize the platform.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/platform.c        | 5 +++--
 xen/include/asm-arm/platform.h | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
index db135f8..0fbbdc7 100644
--- a/xen/arch/arm/platform.c
+++ b/xen/arch/arm/platform.c
@@ -54,7 +54,7 @@ static void dump_platform_table(void)
         printk("    - %s\n", p->name);
 }
 
-int __init platform_init(void)
+void __init platform_init(void)
 {
     int res = 0;
 
@@ -82,7 +82,8 @@ int __init platform_init(void)
     if ( platform && platform->init )
         res = platform->init();
 
-    return res;
+    if ( res )
+        panic("Unable to initialize the platform\n");
 }
 
 int __init platform_init_time(void)
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index 43afebb..c282b30 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -45,7 +45,7 @@ struct platform_desc {
  */
 #define PLATFORM_QUIRK_DOM0_MAPPING_11 (1 << 0)
 
-int __init platform_init(void);
+void __init platform_init(void);
 int __init platform_init_time(void);
 int __init platform_specific_mapping(struct domain *d);
 #ifdef CONFIG_ARM_32
-- 
1.8.3.1

The caller of xen_init_time, start_xen, doesn''t check the return value
of the function. Xen will silently ignore the error and continue.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index a30d422..938995d 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -132,7 +132,7 @@ int __init init_xen_time(void)
 
     res = platform_init_time();
     if ( res )
-        return res;
+        panic("Timer: Cannot initialize platform timer\n");
 
     /* Check that this CPU supports the Generic Timer interface */
     if ( !cpu_has_gentimer )
-- 
1.8.3.1

On 15/11/13 15:27, Julien Grall wrote:
> Most of ioremap_* caller check if ioremap returns NULL. Actually, if the
> physical address is non-aligned, Xen will return the pointer given by
> __vmap plus the offset in the page. So if ioremap_* fails, the caller
> will retrieve an non-NULL address and continue as if there was no error.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/mm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 26c6768..5137668 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -742,8 +742,13 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned
int attributes)
>      unsigned long pfn = PFN_DOWN(pa);
>      unsigned int offs = pa & (PAGE_SIZE - 1);
>      unsigned int nr = PFN_UP(offs + len);
> +    void *ptr;
>  
> -    return (__vmap(&pfn, nr, 1, 1, attributes) + offs);
> +    ptr = __vmap(&pfn, nr, 1, 1, attributes);
No need to split the declaration of void *ptr and its initialisation.

~Andrew
> +    if ( ptr == NULL )
> +        return NULL;
> +
> +    return (ptr + offs);
>  }
>  
>  void *ioremap(paddr_t pa, size_t len)

On Fri, 15 Nov 2013, Julien Grall wrote:
> The caller of xen_init_time, start_xen, doesn''t check the return
value
> of the function. Xen will silently ignore the error and continue.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/time.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index a30d422..938995d 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -132,7 +132,7 @@ int __init init_xen_time(void)
>  
>      res = platform_init_time();
>      if ( res )
> -        return res;
> +        panic("Timer: Cannot initialize platform timer\n");
>  
>      /* Check that this CPU supports the Generic Timer interface */
>      if ( !cpu_has_gentimer )
> -- 
> 1.8.3.1
>

On Fri, 15 Nov 2013, Julien Grall wrote:
> Actually, if an error occurs, Xen will silently ignore it and continue.
> Convert platform_init to a void function and panic if we fail to
> correctly initialize the platform.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/platform.c        | 5 +++--
>  xen/include/asm-arm/platform.h | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/platform.c b/xen/arch/arm/platform.c
> index db135f8..0fbbdc7 100644
> --- a/xen/arch/arm/platform.c
> +++ b/xen/arch/arm/platform.c
> @@ -54,7 +54,7 @@ static void dump_platform_table(void)
>          printk("    - %s\n", p->name);
>  }
>  
> -int __init platform_init(void)
> +void __init platform_init(void)
>  {
>      int res = 0;
>  
> @@ -82,7 +82,8 @@ int __init platform_init(void)
>      if ( platform && platform->init )
>          res = platform->init();
>  
> -    return res;
> +    if ( res )
> +        panic("Unable to initialize the platform\n");
>  }
>  
>  int __init platform_init_time(void)
> diff --git a/xen/include/asm-arm/platform.h
b/xen/include/asm-arm/platform.h
> index 43afebb..c282b30 100644
> --- a/xen/include/asm-arm/platform.h
> +++ b/xen/include/asm-arm/platform.h
> @@ -45,7 +45,7 @@ struct platform_desc {
>   */
>  #define PLATFORM_QUIRK_DOM0_MAPPING_11 (1 << 0)
>  
> -int __init platform_init(void);
> +void __init platform_init(void);
>  int __init platform_init_time(void);
>  int __init platform_specific_mapping(struct domain *d);
>  #ifdef CONFIG_ARM_32
> -- 
> 1.8.3.1
>

On Fri, 15 Nov 2013, Julien Grall wrote:
> Most of ioremap_* caller check if ioremap returns NULL. Actually, if the
> physical address is non-aligned, Xen will return the pointer given by
> __vmap plus the offset in the page. So if ioremap_* fails, the caller
> will retrieve an non-NULL address and continue as if there was no error.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/mm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 26c6768..5137668 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -742,8 +742,13 @@ void *ioremap_attr(paddr_t pa, size_t len, unsigned
int attributes)
>      unsigned long pfn = PFN_DOWN(pa);
>      unsigned int offs = pa & (PAGE_SIZE - 1);
>      unsigned int nr = PFN_UP(offs + len);
> +    void *ptr;
>  
> -    return (__vmap(&pfn, nr, 1, 1, attributes) + offs);
> +    ptr = __vmap(&pfn, nr, 1, 1, attributes);
> +    if ( ptr == NULL )
> +        return NULL;
> +
> +    return (ptr + offs);
No need for brackets here.

In any case

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

On Fri, 2013-11-15 at 15:58 +0000, Stefano Stabellini
wrote:
> On Fri, 15 Nov 2013, Julien Grall wrote:
> > The caller of xen_init_time, start_xen, doesn''t check the
return value
> > of the function. Xen will silently ignore the error and continue.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

I did wonder if it might be worth pushing the panic down into
platform_time_init, so that the message could be more specific. But that
seems like it relies on platform code to be more aware of when to panic
and to use consistent wording etc. So all in all I think it is better
here.
> 
> 
> >  xen/arch/arm/time.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> > index a30d422..938995d 100644
> > --- a/xen/arch/arm/time.c
> > +++ b/xen/arch/arm/time.c
> > @@ -132,7 +132,7 @@ int __init init_xen_time(void)
> >  
> >      res = platform_init_time();
> >      if ( res )
> > -        return res;
> > +        panic("Timer: Cannot initialize platform timer\n");
> >  
> >      /* Check that this CPU supports the Generic Timer interface */
> >      if ( !cpu_has_gentimer )
> > -- 
> > 1.8.3.1
> >

On Fri, 2013-11-15 at 15:59 +0000, Stefano Stabellini
wrote:
> On Fri, 15 Nov 2013, Julien Grall wrote:
> > Actually, if an error occurs, Xen will silently ignore it and
continue.
> > Convert platform_init to a void function and panic if we fail to
> > correctly initialize the platform.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

/insert similar wanderings about pushing the panic down the stack.

On 11/19/2013 02:47 PM, Ian Campbell wrote:
> On Fri, 2013-11-15 at 15:58 +0000, Stefano Stabellini wrote:
>> On Fri, 15 Nov 2013, Julien Grall wrote:
>>> The caller of xen_init_time, start_xen, doesn''t check the
return value
>>> of the function. Xen will silently ignore the error and continue.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I did wonder if it might be worth pushing the panic down into
> platform_time_init, so that the message could be more specific. But that
> seems like it relies on platform code to be more aware of when to panic
> and to use consistent wording etc. So all in all I think it is better
> here.
I thought about this solution. I didn''t find good argument for one of
these solutions. So, by default, I choose to panic in init_xen_time.

-- 
Julien Grall