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
