|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 4/4] xen/common: use SYMBOL when required
On Mon, 7 Jan 2019, Jan Beulich wrote:
> >>> On 03.01.19 at 20:19, <sstabellini@xxxxxxxxxx> wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -312,14 +312,18 @@ extern const initcall_t __initcall_start[],
> > __presmp_initcall_end[],
> > void __init do_presmp_initcalls(void)
> > {
> > const initcall_t *call;
> > - for ( call = __initcall_start; call < __presmp_initcall_end; call++ )
> > + for ( call = __initcall_start;
> > + (unsigned long)call < SYMBOL(__presmp_initcall_end);
> > + call++ )
>
> Hard tabs here and ...
I'll fix
> > (*call)();
> > }
> >
> > void __init do_initcalls(void)
> > {
> > const initcall_t *call;
> > - for ( call = __presmp_initcall_end; call < __initcall_end; call++ )
> > + for ( call = __presmp_initcall_end;
> > + (unsigned long)call < SYMBOL(__initcall_end);
> > + call++ )
>
> ... here.
I'll fix
> > --- a/xen/common/lib.c
> > +++ b/xen/common/lib.c
> > @@ -497,7 +497,7 @@ extern const ctor_func_t __ctors_start[], __ctors_end[];
> > void __init init_constructors(void)
> > {
> > const ctor_func_t *f;
> > - for ( f = __ctors_start; f < __ctors_end; ++f )
> > + for ( f = __ctors_start; (unsigned long)f < SYMBOL(__ctors_end); ++f )
> > (*f)();
>
> One of the best examples where SYMBOL() retaining the original type
> would help.
Yes, depending on the result of the discussion on the topic, I'll either
keep it as is, or change all these instances.
> Also please take the opportunity and add the missing blank line.
You mean before the `for', right? I'll add.
> > --- a/xen/common/schedule.c
> > +++ b/xen/common/schedule.c
> > @@ -68,7 +68,7 @@ DEFINE_PER_CPU(struct scheduler *, scheduler);
> > DEFINE_PER_CPU(cpumask_t, cpumask_scratch);
> >
> > extern const struct scheduler *__start_schedulers_array[],
> > *__end_schedulers_array[];
> > -#define NUM_SCHEDULERS (__end_schedulers_array - __start_schedulers_array)
> > +#define NUM_SCHEDULERS (SYMBOL(__end_schedulers_array) -
> > SYMBOL(__start_schedulers_array))
>
> Long line.
I'll fix.
> > --- a/xen/common/spinlock.c
> > +++ b/xen/common/spinlock.c
> > @@ -474,7 +474,9 @@ static int __init lock_prof_init(void)
> > {
> > struct lock_profile **q;
> >
> > - for ( q = &__lock_profile_start; q < &__lock_profile_end; q++ )
> > + for ( q = &__lock_profile_start;
> > + (unsigned long)q < SYMBOL(&__lock_profile_end);
> > + q++ )
>
> Hard tabs again.
I'll fix
> > --- a/xen/common/virtual_region.c
> > +++ b/xen/common/virtual_region.c
> > @@ -119,7 +119,11 @@ void __init setup_virtual_regions(const struct
> > exception_table_entry *start,
> > const struct bug_frame *s;
> >
> > s = bug_frames[i - 1];
> > - sz = bug_frames[i] - s;
> > + /*
> > + * Cast to unsigned long to calculate the size to avoid
> > + * subtractions between pointers pointing to different objects.
> > + */
> > + sz = (unsigned long)bug_frames[i] - (unsigned long)s;
>
> Perhaps better to use SYMBOL() in the definition of bug_frames[]?
That was my initial thought, but then bug_frames cannot be const and
cannot be statically initialized. So the code would become something
like this:
void __init setup_virtual_regions(const struct exception_table_entry *start,
const struct exception_table_entry *end)
{
size_t sz;
unsigned int i = 0;
static unsigned long bug_frames[6];
bug_frames[i++] = SYMBOL(__start_bug_frames);
bug_frames[i++] = SYMBOL(__stop_bug_frames_0),
bug_frames[i++] = SYMBOL(__stop_bug_frames_1),
bug_frames[i++] = SYMBOL(__stop_bug_frames_2),
#ifdef CONFIG_X86
bug_frames[i++] = SYMBOL(__stop_bug_frames_3),
#endif
bug_frames[i++] = 0;
for ( i = 1; bug_frames[i]; i++ )
{
unsigned long s;
s = bug_frames[i - 1];
/*
* Cast to unsigned long to calculate the size to avoid
* subtractions between pointers pointing to different objects.
*/
sz = bug_frames[i] - s;
core.frame[i - 1].n_bugs = sz;
core.frame[i - 1].bugs = (const struct bug_frame *)s;
core_init.frame[i - 1].n_bugs = sz;
core_init.frame[i - 1].bugs = (const struct bug_frame *)s;
}
What do you think?
> > --- 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) ({ \
> > + const unsigned long __p = (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) ({ \
> > + const unsigned long __p = (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 unsigned long __p = (unsigned long)(p); \
> > + (__p >= SYMBOL(_srodata)) && (__p < SYMBOL(_erodata)); \
> > })
> >
> > extern char _sinittext[], _einittext[];
> > -#define is_kernel_inittext(p) ({ \
> > - char *__p = (char *)(unsigned long)(p); \
> > - (__p >= _sinittext) && (__p < _einittext); \
> > +#define is_kernel_inittext(p) ({ \
> > + const unsigned long __p = (unsigned long)(p); \
> > + (__p >= SYMBOL(_sinittext)) && (__p < SYMBOL(_einittext)); \
> > })
>
> If you fully replace the macro bodies anyway, please take the
> opportunity and also do away with the name space violating
> leading underscores (use trailing ones instead, for example).
OK, I can do.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |