On 04/11/2017 05:05 AM, 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.
Yeah that works. I ran a PowerNV guest under a Pseries BE guest under a PowerNV P8 host. Network was running on the final guest so I suppose that's enough to check the PCI layer. > > 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. No that's fine. I will add some helper routines to explain what is happening. I have had to do some thing similar with the Aspeed flash controller tests when using the Command mode. Thanks, C. >>> + 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). > > Ben. >