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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 27 Feb 2019, Jan Beulich wrote:
> >>> On 26.02.19 at 20:20, <sstabellini@xxxxxxxxxx> wrote:
> > 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?
> 
> I don't think so, no. In livepatch.c we have
> 
>         end = sec->load_addr + sec->sec->sh_size;
> 
> which (a) is effectively a linker defined symbol, just that we don't
> obtain it through a linker script and it also has no name associated
> with it and (b) will be fine without any casts thanks to load_addr
> being of type void * (i.e. the type of "end" can freely change).

Yes, good suggestion.


> >> > -    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?
> 
> Did you try changing p's type? That said, the calculation isn't going
> to be universally correct (in MISRA's sense), no matter what you do:
> We _deliberately_ subtract two entities here which are _guaranteed_
> not to point to the same object (or one past an array's last element).
> 

No I haven't tried changing p's type -- I have been trying to avoid
using abstract_blah_blah in the code, my assumption is that it was
supposed to be hidden within the depths of the macro, not used
explicitly. But I can do that and it gets rid of these casts.


> >> > @@ -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.
> 
> That's why I'm advocating that free()-style functions should take
> pointers to const. A patch to this effect wasn't liked, though.

Changes like this are very few in this series, only a couple. I would
hope that it won't be a problem. Especially considering that most of the
linker symbols are declared as non-const today. I.e. we are not making
things worse.

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