On Tue, Apr 11, 2017 at 01:05:51PM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2017-04-10 at 18:14 +1000, David Gibson wrote: > > > + switch (size) { > > > + case 1: > > > + break; > > > + case 2: > > > + val = bswap16(val); > > > > An unconditional bswap() seems unlikely to be correct. What's the > > purpose of thise? > > Though it is :-) I *think* ... I did test with both LE and BE hosts I > believe but Cedric can double check. > > >From memory it comes from the fact that the PHB register space is > declared to be big endian since 99% of it actually is. > > However the CONFIG_DATA register used to issue config space accesses is > "special" and expect little endian accesses. > > However because the whole region is marked BE, qemu will have done > the necessary swapping for BE, which thus needs to be undone for > that specific register unconditionally: > > - BE host will not swap the value, so we get the LE value written by > the guest which needs to be swapped to BE. > > - LE host will have swapped the value as if it was BE except it isn't > so we need to re-swap it. > > IE. Another option would be to create a specific memory region just for > that one register but it's easier to just unconditionally swap.
Ok, that makes sense. A comment saying that this one register is LE whereas most are BE would be helpful. > > > + break; > > > + case 4: > > > + val = bswap32(val); > > > + break; > > > > No 64-bit config registers? > > No, they don't exist. (Which is also why an assert isn't a good > idea as other registers do support 64-bit accesses). Ah, ok. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature