Re: [PATCH v4 2/2] xen: use SYMBOL when required

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.01.19 at 19:08, <sstabellini@kernel.org> wrote:
> On Tue, 8 Jan 2019, Stefano Stabellini wrote:
>> So, this is what I am going to do: I'll send a series update according
>> to your suggestion, with SYMBOL returning the native pointer type. As I
>> wrote earlier, although weaker, it is still an improvement from my point
>> of view.
> 
> There is a problem with this though I didn't foresee :-(
> 
> The native type of _start is not char* -- it is char[]. So I cannot
> actually return the native type from SYMBOL because I cannot cast to
> (char[]). I didn't notice it until I actually tried it.
> 
> See the implementation of RELOC_HIDE:
> 
>   #define RELOC_HIDE(ptr, off)                    \
>     ({ unsigned long __ptr;                       \
>       __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
>       (typeof(ptr)) (__ptr + (off)); })
> 
> It casts to the type at the end, the error is:
> 
>   error: cast specifies array type
>        (typeof(ptr)) (__ptr + (off)); })
> 
> We have a few options:
> 
> 1) use unsigned long as in this version of the series (the Linux kernel
> also uses this technique)
> Sorry if I insist, it is still the best I think :-)
> 
> 2) casts the parameters of SYMBOL to the corresponding pointer type
> For instance:
>   SYMBOL((char *)_start)
>   SYMBOL((struct alt_instr *)__alt_instructions_end)
> This works, but it is ugly, I would say it makes the code worse than
> option 1)
> 
> 2) always return void* from SYMBOL
> I don't think it is a good idea to use void* as a workaround here
> 
> 3) pass the desired return type to SYMBOL
> For instance:
>   SYMBOL(_start, char *)
>   SYMBOL(__alt_instructions_end, struct alt_instr *)
> Then SYMBOL would automatically cast the return type to char * and
> struct alt_instr * according to the second parameter.
> 
> Do you have any other suggestions?

4) 

#define RELOC_HIDE(ptr, off)                    \
    ({ unsigned long ptr_;                       \
      __asm__ ("" : "=r"(ptr_) : "0" (ptr));      \
      (typeof(*(ptr)) *) (ptr_ + (off)); })

or, if not suitable for RELOC_HIDE() itself, clone the macro into one
that fits SYMBOL()'s needs.

Jan



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