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

[Stefano Stabellini <sstabellini@kernel.org>]

On Tue, 13 Nov 2018, Jan Beulich wrote:
> >>> On 13.11.18 at 00:06, <sstabellini@kernel.org> 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 wasn't sure about what to do about derived variables and decided to
err on the safe side. I am happy to remove those changes, because I
agree that it would be far clearer if SYMBOL() is only used on the
problematic symbols.


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

I'll do


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

I'll reply to your point to later email in the thread.


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

Sorry about the spurious change, I'll remove it.


> > --- 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?

Same again

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