Re: [Xen-devel] [PATCH v5 3/4] xen/x86: use SYMBOL when required

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 7 Jan 2019, Jan Beulich wrote:
> >>> On 03.01.19 at 20:19, <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; (unsigned long)a < SYMBOL(end); a++ )
> 
> As said before, imo SYMBOL() should only eve be applied _directly_
> to one of the (commonly linker script generated) symbols listed above,
> never to any derivatives.

This is the most problematic case for doing that. This issue also
highlights one of the problems mentioned in the thread about returning
unsigned long or native type from SYMBOL.

To address your comment, I could do the following:

        apply_alternatives((struct alt_instr *)SYMBOL(__alt_instructions),
                           (struct alt_instr *)SYMBOL(__alt_instructions_end));

But then, we would still have two pointers pointing to different objects
being compared in the implementation of apply_alternatives: `a' and
`end' would be pointers pointing to __alt_instructions and
__alt_instructions_end respectively. The MISRA-C violation would not be
addressed, I think. The same thing would happen here, and in many other
instances, if we switched SYMBOL to returning the native pointer type.

My preference would be to keep the code as-is in this patch and add an
in-code comment in apply_alternatives to explain the behavior.


> > @@ -1382,7 +1383,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >      }
> >  #endif
> >  
> > -    xen_virt_end = ((unsigned long)_end + (1UL << L2_PAGETABLE_SHIFT) - 1) 
> > &
> > +    xen_virt_end = (SYMBOL(_end) +
> > +                    (1UL << L2_PAGETABLE_SHIFT) - 1) &
> >                     ~((1UL << L2_PAGETABLE_SHIFT) - 1);
> 
> No need for the extra line split.

I'll remove

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