Re: [Xen-devel] [PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as required

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v10 3/6] xen/arm: use DEFINE_SYMBOL as 
required"):
> Use DEFINE_SYMBOL and the two static inline functions that come with it
> for comparisons and subtractions of:
> 
> __init_begin, __init_end, __alt_instructions, __alt_instructions_end,
> __per_cpu_start, __per_cpu_data_end, _splatform, _eplatform, _sdevice,
> _edevice, _asdevice, _aedevice.
> 
> Use explicit casts to uintptr_t when it is not possible to use the
> provided static inline functions.
> 
> M3CM: Rule-18.2: Subtraction between pointers shall only be applied to
> pointers that address elements of the same array
...
> -extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
> +DEFINE_SYMBOL(struct alt_instr, alt_instr, __alt_instructions,
> +              __alt_instructions_end);
>  
>  struct alt_region {
>      const struct alt_instr *begin;

> @@ -204,7 +208,7 @@ static int __apply_alternatives_multi_stop(void *unused)
>          BUG_ON(!xenmap);
>  
>          region.begin = __alt_instructions;
> -        region.end = __alt_instructions_end;
> +        region.end = (struct alt_instr *)__alt_instructions_end;

I disapprove of this.  It is hazardous to convert one of these
linker-generated end pointers to the underlying pointer type, because
that would allow accidental direct pointer comparison.

So, for example, here:

> @@ -131,7 +132,10 @@ static int __apply_alternatives(const struct alt_region 
> *region,
>      printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n",
>             region->begin, region->end);
>  
> -    for ( alt = region->begin; alt < region->end; alt++ )
> +    /* region->begin and region->end might point to different objects. */
> +    for ( alt = region->begin;
> +          (uintptr_t)alt < (uintptr_t)region->end;
> +          alt++ )

If you missed out this hunk, the code would still compile.  Also I
find the comment you have used opaque.

It is, in fact, false: the two pointers always do in fact point to the
same object.  However, we know that compilers erroneously believe that
they do not.  Obviously we don't want to put a complete discussion of
this issue next to each relevant use site.


I think what this demonstrates is that the macros in your patch 2 need
a big doc comment explaining (1) why this exists (2) what the rules
are.  I suggest replacing the doc comment by the macro definition with
something like this:

 /*
  * Declare start and end array variables in C corresponding to existing
  * linker symbols.
  *
  * These macros, or an alternative technique, MUST be used any time
  * linker symbols are imported into C via the `extern []' idiom.
  *
  *    DEFINE_SYMBOL(TYPE, START, START, END)
  *
  *  introduces the following two constant expressions
  *
  *    const TYPE *START;
  *    const struct abstract_NAME *END;
  *
  *  whose values are the linker symbols START and END; these
  *  should be the start and end of a memory region.
  *
  *  You may then use these two inline functions:
  *
  *    bool NAME_lt(const TYPE *s1, const struct abstract_NAME *s2);
  *    ptrdiff_t NAME_diff(const TYPE *s1, const struct abstract_NAME *s2);
  *
  *  lt returns true iff s1 < s2.
  *  diff returns the s2-s1 in units of TYPE.
  *
  *
  * You MUST NOT cast a struct abstract_NAME* to a TYPE*.  Doing so
  * risks miscompilation.  If you need to operate on a struct
  * abstract_NAME* in a way not supported here, you must provide
  * a clear argument explaining why (i) the compiler will not
  * misoptimise your code (ii) future programmers will not
  * accidentally introduce errors.
  *
  * Rationale:
  * 
  * This exists because compilers errnoeously believe that no two
  * external symbols can refer to the same array.  They deem
  * operations (eg comparisons) which mix pointers from different
  * linker symbols illegal and miscompile them.  We consider this a
  * compiler bug (or standards bug) but are not in a position to make
  * the compilers sane; so we must work around things.
  * 
  * The workaround is to do arithmetic and comparions on uintptr_t's
  * derived from the pointers.  Arithmetic on uintptr_t is of course
  * always defined.  The conversion from a pointer is implementation
  * defined, but Xen cannot run on a platform where the conversion is
  * anything other than the usual bit pattern equivalence.
  *
  * Wrapping end in a new type prevents it being accidentally compared
  * to or subtracted from pointers derived from start.
  */


Having written all that down (what a palaver), we can see that your
cast, above, is a breach of the rules.  Instead you can just pass the
struct abstract_alt_instr*, and all is well.  Then you don't need a
comment at the use site, either, since you are doing things which are
entirely supported and explained.


> -    memset(p, 0, __per_cpu_data_end - __per_cpu_start);
> -    __per_cpu_offset[cpu] = p - __per_cpu_start;
> +    memset(p, 0, per_cpu_diff(__per_cpu_start, __per_cpu_data_end));
> +    __per_cpu_offset[cpu] = (uintptr_t)p - (uintptr_t)__per_cpu_start;

If per_cpu_diff(__per_cpu_start, __per_cpu_data_end) gives the right
value for memset, isn't it the right value for offset[cpu] too ?
Ie I don't know why you are using uintptr_t here.

> @@ -37,7 +38,7 @@ static void _free_percpu_area(struct rcu_head *head)
>  {
>      struct free_info *info = container_of(head, struct free_info, rcu);
>      unsigned int cpu = info->cpu;
> -    char *p = __per_cpu_start + __per_cpu_offset[cpu];
> +    char *p = (char *)__per_cpu_start + __per_cpu_offset[cpu];

I also don't know why you are casting to char* here if you didn't need
to do so before.


The rest in this patch looks fine to me.

Thanks,
Ian.

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