Re: [Xen-devel] [PATCH v8 2/7] xen: use start_, end_, and more

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.01.19 at 00:35, <sstabellini@xxxxxxxxxx> wrote:
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -187,8 +187,8 @@ static int __apply_alternatives_multi_stop(void *unused)
>      {
>          int ret;
>          struct alt_region region;
> -        mfn_t xen_mfn = virt_to_mfn(_start);
> -        paddr_t xen_size = _end - _start;
> +        mfn_t xen_mfn = virt_to_mfn(start_);
> +        paddr_t xen_size = end_ - start_;

I can see why you want to replace the latter, but why also the former?
There are more similar cases elsewhere. One particularly funny one is

> @@ -1871,7 +1870,7 @@ int __hwdom_init xen_in_range(unsigned long mfn)
>           */
>  
>          /* hypervisor .text + .rodata */
> -        xen_regions[region_ro].s = __pa(&_stext);
> +        xen_regions[region_ro].s = __pa(stext_);
>          xen_regions[region_ro].e = __pa(&__2M_rodata_end);
>          /* hypervisor .data + .bss */
>          xen_regions[region_rw].s = __pa(&__2M_rwdata_start);

... this: You replace one both not the other two visible here.

> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -373,9 +373,9 @@ void tboot_shutdown(uint32_t shutdown_type)
>          g_tboot_shared->mac_regions[0].size = bootsym_phys(trampoline_end) -
>                                                bootsym_phys(trampoline_start);
>          /* hypervisor .text + .rodata */
> -        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(&_stext);
> +        g_tboot_shared->mac_regions[1].start = (uint64_t)__pa(stext_);
>          g_tboot_shared->mac_regions[1].size = __pa(&__2M_rodata_end) -
> -                                              __pa(&_stext);
> +                                              __pa(stext_);
>          /* hypervisor .data + .bss */
>          g_tboot_shared->mac_regions[2].start = 
> (uint64_t)__pa(&__2M_rwdata_start);
>          g_tboot_shared->mac_regions[2].size = __pa(&__2M_rwdata_end) -

Same here.

> @@ -114,6 +110,11 @@ void __init setup_virtual_regions(const struct 
> exception_table_entry *start,
>          NULL
>      };
>  
> +    core.start = (char *)start_;
> +    core.end = (char *)end_;
> +    core_init.start = (char *)sinittext_;
> +    core_init.end = (char *)einittext_;

Not sure why it occurred to me when looking at this code, but I
can't resist quoting the standard here:

"An integer may be converted to any pointer type. Except as previously
 specified, the result is implementation-defined, might not be correctly
 aligned, might not point to an entity of the referenced type, and might
 be a trap representation.

 Any pointer type may be converted to an integer type. Except as
 previously specified, the result is implementation-defined. If the result
 cannot be represented in the integer type, the behavior is undefined.
 The result need not be in the range of values of any integer type."

Is this not concerning to MISRA?

> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -65,28 +65,28 @@
>       1;                                      \
>  })
>  
> -extern char _start[], _end[], start[];
> -#define is_kernel(p) ({                         \
> -    char *__p = (char *)(unsigned long)(p);     \
> -    (__p >= _start) && (__p < _end);            \
> +extern uintptr_t start_, end_;
> +#define is_kernel(p) ({                                             \
> +    const uintptr_t p__ = (const uintptr_t)(p);                     \

Without it being a pointer the const in the cast is now pointless.

Jan



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