Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Compile only once, not for each target

[Peter Maydell]

On 5 October 2018 at 13:17, Laszlo Ersek <address@hidden> wrote:
> On 10/05/18 12:49, Philippe Mathieu-Daudé wrote:
>> Is the comment added in 8e4a424b305 still valid?
>>
>> /*
>>  * A helper function for the _utterly broken_ virtio device model to
>> find out if
>>  * it's running on a big endian machine. Don't do this at home kids!
>>  */
>> bool target_words_bigendian(void);
>
> Yeah, that comment irks me too.
>
> Not that it isn't valid, necessarily. "hw/virtio/virtio.c" still
> features an embedded declaration of this function. Just above
> virtio_default_endian(), which calls it. And the latter function is
> called in several places (mostly from commit 616a655219a92).
>
> So I can't really tell what's "utterly broken" here -- the device model
> (i.e., the virtio implementation in QEMU), or the legacy *spec* that
> required QEMU to jump through such hoops?

AIUI, the brokenness here is that the virtio specification
requires that a device knows the endianness of the CPU,
which is an awkward layering violation [*]. The point of the
comment is really to say "virtio needs this, but that is
a special weird case, think twice or three times before
using this function anywhere else" (which I suspect is also
why the declaration is not in a header file, to make it less
easily accessible).

[*] Actually it's not even the endianness of the CPU, because
bi-endian CPUs like Arm can be in big-endian mode even though
TARGET_WORDS_BIGENDIAN is false...

I entirely agree that we should clean up the comment to be
clearer about what it's trying to say.

> BTW, I don't think angry comments are useful in *code*. They look & feel
> great for about a week. In a year, they just look unprofessional. (NB,
> this is not about "political correctness"; it's just that, when someone
> reads such a comment, they receive the anger *before* they understand,
> or recall, the underlying problem.

Agreed 100%.

thanks
-- PMM