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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 2 Jan 2019, Stefano Stabellini wrote:
> On Wed, 2 Jan 2019, Stefano Stabellini wrote:
> > On Tue, 13 Nov 2018, Jan Beulich wrote:
> > > >>> 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 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 replied too quickly. I agree that changing only the problematic
> symbols, and not the derived variables, is easier to understand from the
> code point of view, and should still be safe and compliant.
> 
> However, I would also prefer to keep the comparisons as unsigned long
> comparisons, like it is done in this patch. I think it is safer and more
> compliant to do unsigned long comparisons to avoid completely the issue
> of having to understand when pointers point to same or different
> objects. It is certainly more in the spirit of the spec. 
> 
> So, in the case you mentioned above (and a bunch of similar cases), to
> do unsigned long comparisons only and apply SYMBOL() only to the
> problematic symbols, we need one more cast to unsigned long at the
> derived variable side:
> 
>   for ( a = base = start; (unsigned long)a < SYMBOL(end); a++ )
> 
> 
> FYI I realized I was using SYMBOL() conversions more than strictly
> necessary at the derived variables side, but I though it would be nicer
> to have:
> 
>   SYMBOL(derived) < SYMBOL(problem_variable)
> 
> rather than
> 
>   (unsigned long)derived < SYMBOL(problem_variable)
> 
> everywhere. I am happy either way as both should be compliant.

With the hope of getting it merge before the code freeze and to save
time, given that I also had other comments to address and a rebase to
do, I'll send a new series update using the latter of these two options.

See:

https://marc.info/?l=xen-devel&m=154654323311302

We can further discuss the issue there -- I am happy to change things
again as necessary.

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