Re: [Xen-devel] [PATCH v6 4/4] xen/common: use SYMBOL when required

2019-01-10 Thread Stefano Stabellini
On Thu, 10 Jan 2019, Jan Beulich wrote:
> >>> On 10.01.19 at 00:42,  wrote:
> > --- a/xen/common/version.c
> > +++ b/xen/common/version.c
> > @@ -147,14 +147,14 @@ static int __init xen_build_init(void)
> >  int rc;
> >  
> >  /* --build-id invoked with wrong parameters. */
> > -if ( __note_gnu_build_id_end <= [0] )
> > +if ( SYMBOL(__note_gnu_build_id_end) <= [0] )
> >  return -ENODATA;
> >  
> >  /* Check for full Note header. */
> > -if ( [1] >= __note_gnu_build_id_end )
> > +if ( [1] >= SYMBOL(__note_gnu_build_id_end) )
> >  return -ENODATA;
> >  
> > -sz = (void *)__note_gnu_build_id_end - (void *)n;
> > +sz = (void *)SYMBOL(__note_gnu_build_id_end) - (void *)n;
> 
> Now this is an instance where I wouldn't mind if you switched the
> casts to (unsigned long).

OK


> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -103,13 +103,13 @@ void __init setup_virtual_regions(const struct 
> > exception_table_entry *start,
> >  {
> >  size_t sz;
> >  unsigned int i;
> > -static const struct bug_frame *const __initconstrel bug_frames[] = {
> > -__start_bug_frames,
> > -__stop_bug_frames_0,
> > -__stop_bug_frames_1,
> > -__stop_bug_frames_2,
> > +const struct bug_frame *bug_frames[] = {
> 
> Please don't loose the second const.

OK


> > --- a/xen/include/xen/kernel.h
> > +++ b/xen/include/xen/kernel.h
> > @@ -66,27 +66,27 @@
> >  })
> >  
> >  extern char _start[], _end[], start[];
> > -#define is_kernel(p) ({ \
> > -char *__p = (char *)(unsigned long)(p); \
> > -(__p >= _start) && (__p < _end);\
> > +#define is_kernel(p) ({ \
> > +char *p__ = (char *)(unsigned long)(p); \
> > +(p__ >= SYMBOL(_start)) && (p__ < SYMBOL(_end));\
> >  })
> >  
> >  extern char _stext[], _etext[];
> > -#define is_kernel_text(p) ({\
> > -char *__p = (char *)(unsigned long)(p); \
> > -(__p >= _stext) && (__p < _etext);  \
> > +#define is_kernel_text(p) ({\
> > +char *p__ = (char *)(unsigned long)(p); \
> > +(p__ >= SYMBOL(_stext)) && (p__ < SYMBOL(_etext));  \
> >  })
> >  
> >  extern const char _srodata[], _erodata[];
> > -#define is_kernel_rodata(p) ({  \
> > -const char *__p = (const char *)(unsigned long)(p); \
> > -(__p >= _srodata) && (__p < _erodata);  \
> > +#define is_kernel_rodata(p) ({  \
> > +const char *p__ = (const char *)(unsigned long)(p); \
> 
> Just like here, in all other of the sibling macros you could easily
> have switched p__ to be const char * as well.

OK


> With at least the bug_frames[] remark taken care of
> Reviewed-by: Jan Beulich 
 
I'll make the changes above and add your reviewed-by

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v6 4/4] xen/common: use SYMBOL when required

2019-01-10 Thread Jan Beulich
>>> On 10.01.19 at 00:42,  wrote:
> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -147,14 +147,14 @@ static int __init xen_build_init(void)
>  int rc;
>  
>  /* --build-id invoked with wrong parameters. */
> -if ( __note_gnu_build_id_end <= [0] )
> +if ( SYMBOL(__note_gnu_build_id_end) <= [0] )
>  return -ENODATA;
>  
>  /* Check for full Note header. */
> -if ( [1] >= __note_gnu_build_id_end )
> +if ( [1] >= SYMBOL(__note_gnu_build_id_end) )
>  return -ENODATA;
>  
> -sz = (void *)__note_gnu_build_id_end - (void *)n;
> +sz = (void *)SYMBOL(__note_gnu_build_id_end) - (void *)n;

Now this is an instance where I wouldn't mind if you switched the
casts to (unsigned long).

> --- a/xen/common/virtual_region.c
> +++ b/xen/common/virtual_region.c
> @@ -103,13 +103,13 @@ void __init setup_virtual_regions(const struct 
> exception_table_entry *start,
>  {
>  size_t sz;
>  unsigned int i;
> -static const struct bug_frame *const __initconstrel bug_frames[] = {
> -__start_bug_frames,
> -__stop_bug_frames_0,
> -__stop_bug_frames_1,
> -__stop_bug_frames_2,
> +const struct bug_frame *bug_frames[] = {

Please don't loose the second const.

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -66,27 +66,27 @@
>  })
>  
>  extern char _start[], _end[], start[];
> -#define is_kernel(p) ({ \
> -char *__p = (char *)(unsigned long)(p); \
> -(__p >= _start) && (__p < _end);\
> +#define is_kernel(p) ({ \
> +char *p__ = (char *)(unsigned long)(p); \
> +(p__ >= SYMBOL(_start)) && (p__ < SYMBOL(_end));\
>  })
>  
>  extern char _stext[], _etext[];
> -#define is_kernel_text(p) ({\
> -char *__p = (char *)(unsigned long)(p); \
> -(__p >= _stext) && (__p < _etext);  \
> +#define is_kernel_text(p) ({\
> +char *p__ = (char *)(unsigned long)(p); \
> +(p__ >= SYMBOL(_stext)) && (p__ < SYMBOL(_etext));  \
>  })
>  
>  extern const char _srodata[], _erodata[];
> -#define is_kernel_rodata(p) ({  \
> -const char *__p = (const char *)(unsigned long)(p); \
> -(__p >= _srodata) && (__p < _erodata);  \
> +#define is_kernel_rodata(p) ({  \
> +const char *p__ = (const char *)(unsigned long)(p); \

Just like here, in all other of the sibling macros you could easily
have switched p__ to be const char * as well.

With at least the bug_frames[] remark taken care of
Reviewed-by: Jan Beulich 

Jan



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel