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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.11.18 at 00:06, <sstabellini@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -194,7 +194,7 @@ void init_or_livepatch apply_alternatives(struct 
> alt_instr *start,
>       * So be careful if you want to change the scan order to any other
>       * order.
>       */
> -    for ( a = base = start; a < end; a++ )
> +    for ( a = base = start; SYMBOL(a) < SYMBOL(end); a++ )

At this point all is fine: end is allowed to point to the end of start[].
If anything you want to change the invocations (where the
questionable symbols are used). I'm also not convinced you need
to touch both sides of the comparison or subtraction expressions.

In order for people to not start wondering what the purpose of
SYMBOL() is at any of its use sites, you really want to use it on
the problematic symbols themselves, not somewhere on a derived
variable or parameter.

I also think review would be helped if you at least split this patch
into an Arm, and x86, and a common code patch.

> @@ -33,8 +33,8 @@ static int init_percpu_area(unsigned int cpu)
>      if ( (p = alloc_xenheap_pages(PERCPU_ORDER, 0)) == NULL )
>          return -ENOMEM;
>  
> -    memset(p, 0, __per_cpu_data_end - __per_cpu_start);
> -    __per_cpu_offset[cpu] = p - __per_cpu_start;
> +    memset(p, 0, SYMBOL(__per_cpu_data_end) - SYMBOL(__per_cpu_start));
> +    __per_cpu_offset[cpu] = (unsigned long)p - SYMBOL(__per_cpu_start);

Can't you make SYMBOL() retain the original type, such that casts
like this one aren't needed? As soon as the compiler doesn't know
anymore that particular globals (or statics) are used, it can't infer
anymore that two pointers can't possibly point into the same array.

> @@ -1037,7 +1037,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           * Is the region size greater than zero and does it begin
>           * at or above the end of current Xen image placement?
>           */
> -        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >= __pa(_end)) )
> +        if ( (end > s) && (end - reloc_size + XEN_IMG_OFFSET >=
> +             __pa(_end)) )

Only re-flowing? If no change is meant to be done to this use of _end,
please omit the change.

> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -151,7 +151,7 @@ extern vaddr_t xenheap_virt_start;
>  #endif
>  
>  #define is_xen_fixed_mfn(mfn)                                   \
> -    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) &&       \
> +    ((pfn_to_paddr(mfn) >= virt_to_maddr(&_start)) && \
>       (pfn_to_paddr(mfn) <= virt_to_maddr(&_end)))

Unnecessary or incomplete change again?

Jan



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