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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 26 Feb 2019, Ian Jackson wrote:
> 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.

The in-code comment is great, and I have added it to the right patch.

Let me get a clarification on the rest of the suggestion: I would
have to change the type of region->end to const struct
abstract_alt_instr* (see xen/arch/arm/alternative.c).

If I do that, I get a compilation failure at:

alternative.c:245:16: error: initialization from incompatible pointer type 
[-Werror=incompatible-pointer-types]
         .end = end,

because apply_alternatives currently expects two struct alt_instr* as
parameters. I cannot change the type of the second parameter of
apply_alternatives, because actually it might not be a linker symbol, it
might not be a struct abstract_alt_instr*. So I would still have to cast
to struct abstract_alt_instr* somewhere?

 
> > -    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.

I am using uintptr_t because p is not a abstract_per_cpu type. I could
do:

  __per_cpu_offset[cpu] = per_cpu_diff(__per_cpu_start,
                             (struct abstract_per_cpu *)p);

I didn't think it was better though. What do you think?


> > @@ -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.

That is because __per_cpu_start is now const, it wasn't before.


> The rest in this patch looks fine to me.

Thanks again for your help!

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