Re: [Xen-devel] [PATCH v6 1/4] xen: introduce SYMBOL

[Julien Grall <julien.grall@xxxxxxx>]

Hi Jan,

On 1/15/19 8:21 AM, Jan Beulich wrote:
> > > > On 14.01.19 at 22:18, <sstabellini@xxxxxxxxxx> wrote:
> > Hi Jan,
> >
> > One question below to make a decision on the way forward.
> >
> > On Mon, 14 Jan 2019, Jan Beulich wrote:
> > > > > > On 14.01.19 at 04:45, <Stewart.Hildebrand@xxxxxxxxxxxxxxx> wrote:
> > > > So let's keep the linker-accessible variable as a type that works for the
> > > > linker (which really could be anything as long as you use the address, not
> > > > the value), but name it something else - a name that screams "DON'T USE ME
> > > > UNLESS YOU KNOW WHAT YOU'RE DOING". And then before the first use, copy
> > > > that value to "uintptr_t _start;".
> > > >
> > > > The following is a quick proof of concept for aarch64. I changed the type
> > > > of _start and _end, and added code to copy the linker-assigned value to
> > > > _start and _end. Upon booting, I see the correct values:
> > > Global symbols starting with underscores should already be shouting
> > > enough. But what's worse - the whole idea if using array types is to
> > > avoid the intermediate variables.
> > >
> > > > --- a/xen/arch/arm/setup.c
> > > > +++ b/xen/arch/arm/setup.c
> > > > @@ -726,6 +726,12 @@ static void __init setup_mm(unsigned long dtb_paddr,
> > size_t dtb_size)
> > > >   
> > > >   size_t __read_mostly dcache_line_bytes;
> > > >   
> > > > +typedef char TYPE_DOESNT_MATTER;
> > > > +extern TYPE_DOESNT_MATTER _start_linker_assigned_dont_use_me,
> > > > +                          _end_linker_assigned_dont_use_me;
> > > This and ...
> > >
> > > > @@ -770,10 +776,17 @@ void __init start_xen(unsigned long boot_phys_offset,
> > > >       printk("Command line: %s\n", cmdline);
> > > >       cmdline_parse(cmdline);
> > > >   
> > > > +    _start = (uintptr_t)&_start_linker_assigned_dont_use_me;
> > > > +    _end = (uintptr_t)&_end_linker_assigned_dont_use_me;
> > > ... this violates what the symbol names say. And if you want to
> > > avoid issues, you'd want to keep out of C files uses of those
> > > symbols altogether anyway, and you easily can: In any
> > > assembly file, have
> > >
> > > _start: .long _start_linker_assigned_dont_use_me
> > > _end:   .long _end_linker_assigned_dont_use_me
> > >
> > > In particular, they don't need to be runtime initialized, saving
> > > you from needing to set them before first use. But as said -
> > > things are the way they are precisely to avoid such variables.
> > >
> > > > > But, instead of converting _start to unsigned long via SYMBOL_HIDE, we
> > > > > could convert it to uintptr_t instead, it would be a trivial change on
> > > > > top of the existing unsigned long series. Not sure if it is beneficial.
> > > > The difference would be whether we want to rely on implementation-defined
> > > > behavior or not.
> > > Why not? Simply specify that compilers with implementation defined
> > > behavior not matching our expectations are unsuitable. And btw, I
> > > suppose this is just the tiny tip of the iceberg of our reliance on
> > > implementation defined behavior.
> > The reason is that relying on undefined behavior is not reliable, it is
> > not C compliant, it is not allowed by MISRA-C, and not guaranteed to
> > work with any compiler.
> "undefined behavior" != "implementation defined behavior"
>
> > Yes, this instance is only the tip of the
> > iceberg, we have a long road ahead, but we shouldn't really give up
> > because it is going to be difficult :-) Stewart's approach would
> > actually be compliant and help toward reducing reliance on undefined
> > behavior.
> >
> > Would you be OK if I rework the series to follow his approach using
> > intermediate variables? See the attached patch as a reference, it only
> > "converts" _start and _end as an example. Fortunately, it will be
> > textually similar to the previous SYMBOL returning unsigned long version
> > of the series.
> Well, I've given reasons why I dislike that, and why (I think) it was
> done without such intermediate variables. Nevertheless, if this is
> _the only way_ to achieve compliance, I don't think I could
> reasonably NAK it. >
> The thing that I don't understand though is how the undefined
> behavior (if there really is any) goes away: Even if you compare
> the contents of the variables instead of the original (perhaps
> casted) pointers, in the end you still compare what C would
> consider pointers to different objects. It's merely a different
> way of hiding that fact from C. Undefined behavior would imo
> go away only if those comparisons/subtractions didn't happen
> in C anymore. IOW - see my .startof.() / .sizeof.() proposal.
Do you have a pointer to the series using startof/sizeof?

> > If you are OK with it, do you have any suggestions on how would you like
> > the intermediate variables to be called? I went with _start/start_ and
> > _end/end_ but I am open to suggestions. Also to which assembly file you
> > would like the new variables being added -- I created a new one for the
> > purpose named var.S in the attached example.
> First of all we should explore whether the variables could also be
> linker generated, in particular to avoid the current symbols to be
> global (thus making it impossible to access them from C files in the
> first place). Failing that, I don't think it matters much where these
> helper symbols live, and hence your choice is probably fine (I'd
> prefer though if, just like on Arm, the x86 file didn't live in the
> boot/ subdirectory; in the end it might even be possible to have
> some of them in xen/common/var.S).
From my test [1], I don't think intermediate variables are necessary. 
You could directly define the symbol with uintptr_t.
Cheers,

[1] https://lists.xen.org/archives/html/xen-devel/2019-01/msg01109.html

> Jan
--
Julien Grall

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