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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan



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