Re: [Qemu-devel] [PATCH] fw_cfg: fix endianness in fw_cfg_data_mem_read() / _write()

[Laszlo Ersek]

On 12/31/14 14:20, Paolo Bonzini wrote:
> 
> 
> On 31/12/2014 12:21, Laszlo Ersek wrote:
>> Because register emulation in QEMU is integer-preserving, not
>> string-preserving (see (2)), we have to jump through a few hoops.
>>
>> (3a) We defined the memory mapped fw_cfg data register as
>> DEVICE_BIG_ENDIAN.
>>
>> The particular choice is not really relevant -- we picked BE only for
>> consistency with the control register, which *does* transfer integers --
>> but our choice affects how we must host-encode values from fw_cfg strings.
>>
>> (3b) Since we want the fw_cfg string "XY" to appear as the [0x58, 0x59]
>> array on the data register, *and* we picked DEVICE_BIG_ENDIAN, we must
>> compose the host (== C language) value 0x5859 in the read accessor
>> function.
> 
> I'm not sure this is right.
> 
> DEVICE_BIG_ENDIAN means that if we return 0xaabb and the guest is little
> endian, it will return 0xbbaa.

Let me decode that...
- the logical value (which we return from the accessor in host-endian)
is 0xaabb
- the device is big endian, so 0xaabb will mean [0xaa, 0xbb] for it
- you're saying that if the guest is little endian, then it will
  read 0xbbaa.

I agree fully.

> But the value returned by the accessor
> is always in host endianness.

Yes.

> And it makes sense to swap in the guest if the register is big endian
> but the guest is little endian.

Absolutely -- *if* in the guest you care about the integer value that
the accessor function originally returned. (Ie. you want 0xaabb in the
guest.)

But we don't care about that integer value. What we care about is the
fw_cfg string underlying the host value. And the current code will
return a different host value, dependent on host endianness, for the
same underlying string.

> So IMHO your old code is right.

The immediate sign that makes me nervous is the ldX_he_p() calls in the
accessors. "Host endian" means that the same fw_cfg string will cause
different host (== C) integer values to be returned from the accessor if
you change the host from LE to BE. It doesn't seem right.

Obviously this issue could be put to rest if I could test on any big
endian host. Too bad the last sparc64 I used to have access to was
decommissioned years ago. The True64 / alpha box even earlier. :(

Of course this also renders the issue mostly moot -- if none of us can
test the code on a BE host, then that use case simply doesn't exist in
practice.

(Maybe I should build a big endian target of qemu, like mips or powerpc
or sparc, then install a matching Debian distro in this TCG guest, then
build & install qemu-system-aarch64 in it, then test the ARM firmware in
*that*...)

> Either you are overthinking it, or I'm
> underthinking it...  Knowing our respective personalities, either
> possibility is just as likely... ;)

Touché :)

I just wanted to have a clear conscience. The ldX_he_p() thing kept
bugging me, and I would have *hated* to see this patch down the line
from someone else, saying (or not saying, but thinking) "lol, after all
this noise, Laszlo messed it up nonetheless". So, I wrote it up as
precisely as I could for discussion's sake.

You could argue that the commit message is actually the more important
half of the patch, because it tries to codify silent interface
contracts, and I definitely wanted to run those statements by you guys.

If the statements in the commit message are incorrect, then I'm more
than relieved to leave the in-tree code as-is, because then you'll have
investigated my concerns and put them to rest. :) I felt that
*submitting* (but not necessarily committing) this patch was necessary
for keeping my integrity / credibility.

Thanks!
Laszlo