[U-Boot] [PATCH v3] Fix board init code to use a valid C runtime environment

[Albert ARIBAUD]

Hello Simon,

On Sat, 14 Nov 2015 08:41:01 -0700, Simon Glass <sjg at chromium.org>
wrote:

> Reading between the lines, there isn't actually a problem with this in
> practice, but it is doing things that C should not really be doing. Is
> that right?

You did not even need to read between the lines :) as the very first
comment to the v1 of this patch was an explicit question whether I had
seen actual failure cause by board_init_f_mem() -- and my explicit
answer was that no, I hadn't, but as an analogy, I hadn't personally
seen any car crash caused by worn tires either, and that did not deter
me from changing worn tires as soon as I noticed them rather than wait
for the tires to fail.

> To me it seems OK provided we can assume that the stack
> needed to call the function is less than 64 bytes. But I'm sure there
> is a spec somewhere that the current function violates. Your idea of
> what constitutes the stack seems slightly different to mine - I think
> of it as the data already written, whereas you think of it as data yet
> to write, There is some associated uncertainty since the compiler is
> free to fiddle with the stack pointer. In practice in a boot loader we
> always make some architectural assumptions based on current operation.
> But I agree we should minimise this.

Ok.

> Anyway, I think your solution only adds one more function and still
> has most of the code in C rather than assembler, so that it is common
> to all archs. That was my main goal with the change.

Just to clarify, keeping as much C as possible is my goal too.
Especially, the patch keeps the size computation in a C function rather
than reverting back to computing it in crt0.S as it was 'in olden
days'; that is because computing the size in a C function is much more
flexible than it would be in assembly language, and accessible to many
more developers.

My general view of assembly vs C language is that assembly language
should only be used to perform things that cannot be done in C language
without violating the C runtime environment, or for which a C runtime
environment is not available, or which may fail depending on the
compiler implementation. This essentially covers:

- the entry point: when it is directly pointed at by the reset vector,
  it obviously does not have a C runtime environment, and it usually
  does not have one either even when it it entered from ROM code. In
  fact, the general goal of the entry point is to set up the C runtime
  environment(s).

- exception vectors: except for rare architectures for which the
  compiler provides a way to directly write interrupt routines in C,
  exception vectors have to be written in assembly language; again,
  their role *is* to set up the C runtime environment for the C
  language exception handler.

- exceptional cases where any implementation of some design in C
  conflicts with the compiler implementation. Of course, these case are
  specific to an architecture and frequently even a compiler version
  (see below for the r9-as-GD issue) and must only resort to assembly
  language for said architecture, and use as little of it as possible.
  Typically, such designs are related to an architecture's system ABI,
  which a compiler does not necessarily handle -- again, r9 as a system
  wide constant register comes to the mind of the ARM custodian -- or
  the need for specific instructions for which the compiler does not
  provide support in C (mcr and mrc are ARM examples).

Also, while the patch does add one C function, it does not change the C
code functionally overall, and adds very little assembly code, thereby
resulting in a negligible global code increase (actually, on ARM, I see
more boards experiencing a slight global code size *decrease* than boards
experiencing an *increase*).

> I prefer to set up gd in the C function if we can, to avoid the
> oddness that gd has to be at the bottom and the associated 16-byte
> alignment issue. But let's see how that goes. If we need to do it in
> assembler, it might be better to require the caller to do it before
> calling board_init_f_reserve().

Right now I only see two architectures which cannot set GD in
arch_setup_gd(): x86 and ARM; my patch takes care not to affect other
arches, nor do I have an intent to do so. They can set GD from C, and
that's fine.

Note about the ARM case:

Regarding ARM, I do change the behaviour for all of ARM, whereas the
issue of arch_setup_gd() failing to set r9 only happens for Thumb-1 code
generation and only with gcc 5.2.1 (and possibly a few other versions).

This is because, as I said elsewhere, I fear that the gcc -ffixed-reg
behaviour change is compliant with a strict interpretation of the
option (all accesses to r9 from user C code will return the same
value) and therefore I expect later gcc versions to not change the
Thumb-1 behaviour; it might even happen that this behaviour change be
'ported over' to Thumb-2 or even ARM code generation.

As the ARM custodian, I do not view tracking gcc behaviour changes as
a rewarding task, nor do I consider it a sane solution to pepper
gcc-version conditionals around some C code because its success happens
to depend on which implementation of gcc it was compiled with.

Plus, I do not know right now how LLVM handles r9 but I do remember
some hours of... fun... with it, and I don't want to discover in later
times that LLVM suddenly changed its mind about r9 handling too.

This is why, for ARM, I prefer cutting the problem short; taking the
strictest option and only trusting -ffixed-reg to provide a constant r9
for reading, not writing, from within C code; therefore, setting r9 to
GD from outside any C context; and doing all of this regardless of ARM
instruction set or compiler version or even compiler choice.

> Regards,
> Simon

Amicalement,
-- 
Albert.