[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.